IF: Implement capabilities to modify, reorder, drop, and duplicate votes and add comprehensive vote processing Boost unit tests #2234

linh2931 · 2024-02-12T18:16:49Z

Implemented a test cluster consisting of 3 nodes (controllers) with capabilities to modify, reorder, drop, and duplicate votes:

Added 18 tests to cover vote processing, including IF transitions, long run of 1000 blocks, conflicting votes, delayed votes, dropped votes, out of order votes.
Fixed a mutex lockup issue.
Fixed a missed voting status check issue.

Resolved #2193 and #2147

libraries/chain/include/eosio/chain/controller.hpp

unittests/finality_tests.cpp

… repeats time; add more verification

unittests/finality_tests.cpp

…e1/node2/node3

…s; change clusetr to tester which makes more sense

ericpassmore · 2024-02-13T18:22:50Z

Note:start
group: IF
category: TEST
summary: Test cluster with additional coverage for vote processing, including IF transitions, long run of 1000 blocks, conflicting votes, delayed votes, dropped votes, and out of order votes.
Note:end

…tiple tester instances

greg7mdp · 2024-02-14T00:08:47Z

libraries/chain/block_state.cpp

-      return {status, strong ? core.final_on_strong_qc_block_num : std::optional<uint32_t>{}};
+      return {status,
+              (status == vote_status::success && strong) ?
+                 core.final_on_strong_qc_block_num : std::optional<uint32_t>{}};


Why this change? It is not needed imo.

When it returns a non-null-optional, the caller will go ahead to call set_if_irreversible_block_num which is a waste. I also think it is cleaner: if a vote is bad, both return values (status and strong) should be set properly so that they cannot be used. @heifner ?

In my branch I have this as:

std::optional<uint32_t> new_lib{}; if (status == vote_status::success && state == pending_quorum_certificate::state_t::strong) new_lib = core.final_on_strong_qc_block_num; return {status, state, new_lib};

I'm fine with Kevin's version.

It occurs to me that this is not correct. It assumes only votes for this->core comes in. This really should be updated_core. I will make that change in my PR.

Honestly, I liked it better before any change. The meaning was clear. We're asking block_state to aggregate a vote into its pending_qc, and it returns a pair where:

pair.first is the status of applying the vote (so we know whether to propagate the vote)
pair.second is the new lib that pending_qc certifies (if it achieved strong status). The caller decides what to do with this info.

I'm not understanding why it helps to make it more complicated than that.

This is changing in my PR. We have to let fork_db tell us new lib.

Isn't still fine to let aggregate_vote tell you which block its pending_qc is making final, even if that's not the lib that the controller will end up storing?

Potentially, but as is it is not telling you the correct answer because it is not correctly taking previous votes into account. It is only looking at core not updated_core.

I'm not understanding what updated_core is about? I thought you may need to update the core of other block state (the descendants), but why the one we are voting on?

greg7mdp · 2024-02-14T01:27:52Z

unittests/finality_tests.hpp

+   eosio::testing::tester   node1;
+   eosio::testing::tester   node2;
+   eosio::testing::tester   node3;
+   uint32_t                 node1_prev_lib_bum {0};
+   uint32_t                 node2_prev_lib_bum {0};
+   uint32_t                 node3_prev_lib_bum {0};


Having a std::array<node_info, 3> with

struct node_info { eosio::testing::tester node; uint32_t node_prev_lib_bum; };

would remove code duplication (in member functions and elsewhere).

Thanks. Done

Also move the implementation out of .hpp file.

Somewhat minor, but I wonder if you shouldn't name them node0, node1, node2 if you use the array approach so they match index.

Yes, I will make the change. That reduces confusion about which node is which. Thanks.

…ion; use array to represent nodes to reduce duplicate code

…and reduce confusion; refactor return code handling in aggregate_vote

greg7mdp · 2024-02-14T20:34:12Z

libraries/chain/hotstuff/hotstuff.cpp

 }

 // called by add_vote, already protected by mutex
 vote_status pending_quorum_certificate::add_strong_vote(const std::vector<uint8_t>& proposal_digest, size_t index,
                                                        const bls_public_key& pubkey, const bls_signature& sig,
                                                        uint64_t weight) {
-   if (auto s = _strong_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::success)
+   if (auto s = _strong_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::success) {
+      dlog("add_strong_vote returned failure");


why is this dlog useful since we have a non-conditional dlog in add_vote?

This is only happening when duplicate occurs; useful for debugging.

greg7mdp · 2024-02-14T20:49:54Z

unittests/finality_test_cluster.hpp

+   static constexpr size_t node0 = 0;  // node0 index in nodes array
+   static constexpr size_t node1 = 1;  // node1 index in nodes array
+   static constexpr size_t node2 = 2;  // node2 index in nodes array


Code will be cleaner if you define these instead:

node_info& node0 = node[0]; node_info& node1 = node[1]; node_info& node2 = node[2];

and:

void setup_node(node_info& node, eosio::chain::account_name local_finalizer); bool lib_advancing(node_info& node); eosio::chain::vote_status process_vote(node_info& node, size_t vote_index, vote_mode mode); eosio::chain::vote_status process_vote(node_info& node, vote_mode mode);

Code will look like this:

finality_test_cluster::finality_test_cluster() { using namespace eosio::testing; setup_node(node0, "node0"_n); setup_node(node1, "node1"_n); setup_node(node2, "node2"_n); // collect node1's votes node1.node.control->voted_block().connect( [&]( const eosio::chain::vote_message& vote ) { node1.votes.emplace_back(vote); }); // collect node2's votes node2.node.control->voted_block().connect( [&]( const eosio::chain::vote_message& vote ) { node2.votes.emplace_back(vote); }); ...

greg7mdp · 2024-02-14T20:55:13Z

unittests/finality_test_cluster.cpp

+   for (size_t i = 0; i < node.size(); ++i) {
+      node[i].votes.clear();
+      node[i].prev_lib_num = node[i].node.control->if_irreversible_block_num();
+   }


Suggested change

for (size_t i = 0; i < node.size(); ++i) {

node[i].votes.clear();

node[i].prev_lib_num = node[i].node.control->if_irreversible_block_num();

}

for (auto& n : nodes {

n.votes.clear();

n.prev_lib_num = n.node.control->if_irreversible_block_num();

}

unittests/finality_test_cluster.hpp

add comprehensive Boost unit tests for vote handling

bb22edf

linh2931 requested review from greg7mdp, heifner and arhag February 12, 2024 18:16

heifner requested changes Feb 12, 2024

View reviewed changes

This was linked to issues Feb 12, 2024

IF: add more flexibility for IF transition to Boost libtester #2193

Closed

IF: port tests using qc_chain and chain_pacemaker to work with the new implementation #2147

Closed

linh2931 and others added 2 commits February 12, 2024 16:53

change if_lib_num to if_irreversible_block_num; reduce number of test…

0aa6aea

… repeats time; add more verification

Merge branch 'hotstuff_integration' into vote_tests

16d423c

linh2931 requested a review from heifner February 12, 2024 21:56

linh2931 mentioned this pull request Feb 12, 2024

IF: port tests using qc_chain and chain_pacemaker to work with the new implementation #2147

Closed

heifner approved these changes Feb 12, 2024

View reviewed changes

heifner requested changes Feb 13, 2024

View reviewed changes

unittests/finality_tests.cpp Outdated Show resolved Hide resolved

unittests/finality_tests.cpp Outdated Show resolved Hide resolved

linh2931 added 2 commits February 13, 2024 11:34

do not create accounts; change finalizer names alice/bob/carol to nod…

7ba11f5

…e1/node2/node3

move finality_tester to a header file so it can be used by other test…

56d1448

…s; change clusetr to tester which makes more sense

linh2931 requested a review from heifner February 13, 2024 17:52

change finality_tester back to finality_cluster as it consists of mul…

b77710a

…tiple tester instances

heifner approved these changes Feb 13, 2024

View reviewed changes

greg7mdp requested changes Feb 14, 2024

View reviewed changes

Move finality_test_cluster implementation to .cpp for better abstract…

24d771a

…ion; use array to represent nodes to reduce duplicate code

linh2931 requested review from heifner and greg7mdp February 14, 2024 16:37

rename node1/node2/node3 to node0/node1/node2 to match their indexes …

ff68524

…and reduce confusion; refactor return code handling in aggregate_vote

greg7mdp requested changes Feb 14, 2024

View reviewed changes

greg7mdp reviewed Feb 14, 2024

View reviewed changes

unittests/finality_test_cluster.hpp Show resolved Hide resolved

greg7mdp reviewed Feb 14, 2024

View reviewed changes

unittests/finality_test_cluster.hpp Show resolved Hide resolved

do not use indexes for node names

0233504

greg7mdp approved these changes Feb 14, 2024

View reviewed changes

heifner approved these changes Feb 15, 2024

View reviewed changes

linh2931 merged commit 2c77713 into hotstuff_integration Feb 15, 2024
26 checks passed

linh2931 deleted the vote_tests branch February 15, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IF: Implement capabilities to modify, reorder, drop, and duplicate votes and add comprehensive vote processing Boost unit tests #2234

IF: Implement capabilities to modify, reorder, drop, and duplicate votes and add comprehensive vote processing Boost unit tests #2234

linh2931 commented Feb 12, 2024

ericpassmore commented Feb 13, 2024

greg7mdp Feb 14, 2024

linh2931 Feb 14, 2024

heifner Feb 14, 2024

greg7mdp Feb 14, 2024

heifner Feb 14, 2024

greg7mdp Feb 14, 2024

heifner Feb 14, 2024

greg7mdp Feb 14, 2024

heifner Feb 15, 2024

greg7mdp Feb 15, 2024

greg7mdp Feb 14, 2024

linh2931 Feb 14, 2024

linh2931 Feb 14, 2024

heifner Feb 14, 2024

linh2931 Feb 14, 2024

greg7mdp Feb 14, 2024

linh2931 Feb 14, 2024

greg7mdp Feb 14, 2024

greg7mdp Feb 14, 2024

IF: Implement capabilities to modify, reorder, drop, and duplicate votes and add comprehensive vote processing Boost unit tests #2234

IF: Implement capabilities to modify, reorder, drop, and duplicate votes and add comprehensive vote processing Boost unit tests #2234

Conversation

linh2931 commented Feb 12, 2024

ericpassmore commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment