IF:Unification: Integrate vote message signatures into new fork database #2067

linh2931 · 2024-01-10T15:30:15Z

Process vote messages received from net plugin:

added a new finality_controller.cpp within hotstuff directory (which will be renamed to finality directory) to contain control related code
aggregated the vote signature into the pending QC for the block the vote corresponds to
added tests

Resolved #2046

linh2931 · 2024-01-10T15:37:50Z

Please hold on reviewing this. Need to resolve merge conflicts.

greg7mdp

I'm not understanding why we need finality_controller? It holds no data.
Couldn't we just have a function in block_state, for example block_state::aggregate_vote(const hs_vote_message& vote)?

greg7mdp · 2024-01-10T16:02:28Z

libraries/chain/hotstuff/hotstuff.cpp

-   if (!fc::crypto::blslib::verify(pubkey, proposal_digest, new_sig))
+   }
+   if (!fc::crypto::blslib::verify(pubkey, proposal_digest, new_sig)) {
+      wlog( "sinature from finalizer ${i} cannot be verified", ("i", index) );


Suggested change

wlog( "sinature from finalizer ${i} cannot be verified", ("i", index) );

wlog( "signature from finalizer ${i} cannot be verified", ("i", index) );

greg7mdp · 2024-01-10T16:28:25Z

libraries/chain/controller.cpp

+   auto bsp = fetch_block_state_by_id(vote.proposal_id);
+   if( bsp ) {
+      my->finality_control.aggregate_vote(bsp, vote);
+   } else {


Instead of adding fetch_block_state_by_id and apply_block_data_new, I suggest you add a new method in block_data_t named:

void aggregate_vote(const hs_vote_message& vote) which would do nothing for the dpos case, and would fetch the block_state and apply the vote in the if case.

Is block_data_t a general class and its main purpose to service fork_db APIs? aggregate_vote seems too specific for block_data_t.

Block data and the other structs with variants should contain, as much as possible, any code that does something different for dpos and if, or accesses the block_state_* structs, so that the rest of controller doesn't see these differences.

It is perfectly fine to add new members like aggregate_vote which behave differently for dpos and if.

The apply functions were mostly present to make controller work in the dpos mode while changes were not completed, ideally they should go away.

linh2931 · 2024-01-10T18:34:49Z

I'm not understanding why we need finality_controller? It holds no data. Couldn't we just have a function in block_state, for example block_state::aggregate_vote(const hs_vote_message& vote)?

We were talking about consolidate hoststuff directory into a new finality directory. I created finality_controller as a placeholder for that purpose. But we could do that in the future if needed. Will move to block_state for simplicity.

…remove apply_block_data_new

greg7mdp · 2024-01-10T21:52:02Z

libraries/chain/controller.cpp

@@ -3889,7 +3902,7 @@ std::optional<signed_block_header> controller::fetch_block_header_by_number( uin


 block_state_legacy_ptr controller::fetch_block_state_by_id( block_id_type id )const {
-   // returns nullptr when in IF mode 
+   // returns nullptr when in in mode


greg7mdp · 2024-01-10T22:56:51Z

libraries/chain/controller.cpp

+   bool aggregate_vote(const hs_vote_message& vote) {
+      EOS_ASSERT(std::holds_alternative<block_data_new_t>(v), misc_exception, "attempting to call aggregate_vote in legacy mode");
+
+      block_data_new_t& bd = std::get<block_data_new_t>(v);
+      auto bsp = bd.fork_db.get_block(vote.proposal_id);
+      if( bsp ) {
+         return bsp->aggregate_vote(vote);
+      } else {
+         wlog( "no block exists for the vote (proposal_id: ${id}", ("id", vote.proposal_id) );
+         return false;
+      }
+   }


It is better to avoid holds_alternative and std::get, please use std::visit instead.

Suggested change

bool aggregate_vote(const hs_vote_message& vote) {

EOS_ASSERT(std::holds_alternative<block_data_new_t>(v), misc_exception, "attempting to call aggregate_vote in legacy mode");

block_data_new_t& bd = std::get<block_data_new_t>(v);

auto bsp = bd.fork_db.get_block(vote.proposal_id);

if( bsp ) {

return bsp->aggregate_vote(vote);

} else {

wlog( "no block exists for the vote (proposal_id: ${id}", ("id", vote.proposal_id) );

return false;

}

}

bool aggregate_vote(const hs_vote_message& vote) {

return std::visit(

overloaded{[](const block_data_legacy_t&) {

FC_THROW_EXCEPTION(misc_exception, "attempting to call aggregate_vote in legacy mode");

return false;

},

[&](const block_data_new_t& bd) {

auto bsp = bd.fork_db.get_block(vote.proposal_id);

if (bsp) {

return bsp->aggregate_vote(vote);

} else {

wlog("no block exists for the vote (proposal_id: ${id}", ("id", vote.proposal_id));

return false;

}

}},

v);

}

greg7mdp · 2024-01-10T22:58:01Z

libraries/chain/controller.cpp

@@ -4007,8 +4020,8 @@ void controller::get_finalizer_state( finalizer_state& fs ) const {
 }

 // called from net threads
-void controller::notify_hs_message( const uint32_t connection_id, const hs_message& msg ) {
-   my->pacemaker->on_hs_msg(connection_id, msg);
+void controller::notify_vote_message( const hs_vote_message& vote ) {


Please change the function name to process_vote_message or aggregate_vote_message or something like that.

greg7mdp · 2024-01-10T23:01:23Z

libraries/chain/hotstuff/hotstuff.cpp

+bool pending_quorum_certificate::add_vote(bool strong, size_t index,
                                          const bls_public_key& pubkey, const bls_signature& sig) {
-   return strong ? add_strong_vote(proposal_digest, index, pubkey, sig)
-                 : add_weak_vote(proposal_digest, index, pubkey, sig);
+   return strong ? add_strong_vote(index, pubkey, sig)
+                 : add_weak_vote(index, pubkey, sig);


as discussed in the meeting, you'll have to pass the finalizer_digest from the block_state, and we can remove the _proposal_digest member.

linh2931 · 2024-01-11T14:14:00Z

Replaced by simpler #2073

linh2931 added 5 commits January 10, 2024 10:12

simplify pending_quorum_certificate::add_vote interface

ab8fcc2

update test for new add_vote

8a67d01

add finality_controller.hpp/.cpp implementing aggregate_vote

ee7f45f

add tests for aggregate_vote

b7265db

integrate aggregating vote into controller and net_plugin

2545523

linh2931 requested review from heifner and greg7mdp January 10, 2024 15:30

linh2931 marked this pull request as draft January 10, 2024 15:36

Merge branch 'hotstuff_integration' into vote_integration

dc83521

linh2931 marked this pull request as ready for review January 10, 2024 16:16

greg7mdp reviewed Jan 10, 2024

View reviewed changes

greg7mdp requested changes Jan 10, 2024

View reviewed changes

linh2931 added 5 commits January 10, 2024 15:32

move aggregate_vote to block_state

03cc7b1

implement aggregate_vote inside block_data_t in controller

cb6fd13

restore fetch_block_state_by_id to return block_state_legacy_ptr and …

2c3e163

…remove apply_block_data_new

remove finality_controller.hpp/.cpp

c474e03

mv test_finality_controller.cpp to block_state_tests.cpp

d6fba94

linh2931 linked an issue Jan 10, 2024 that may be closed by this pull request

IF: Unification: Integrate vote message signatures into new fork database #2046

Closed

correct a typo in warning message

0e49022

linh2931 requested a review from greg7mdp January 10, 2024 21:00

greg7mdp reviewed Jan 10, 2024

View reviewed changes

greg7mdp requested changes Jan 10, 2024

View reviewed changes

linh2931 mentioned this pull request Jan 11, 2024

IF:Unification: Process received Vote message #2073

Merged

linh2931 closed this Jan 11, 2024

linh2931 removed a link to an issue Jan 11, 2024

IF: Unification: Integrate vote message signatures into new fork database #2046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IF:Unification: Integrate vote message signatures into new fork database #2067

IF:Unification: Integrate vote message signatures into new fork database #2067

linh2931 commented Jan 10, 2024

linh2931 commented Jan 10, 2024

greg7mdp left a comment

greg7mdp Jan 10, 2024

greg7mdp Jan 10, 2024

linh2931 Jan 10, 2024

greg7mdp Jan 10, 2024

linh2931 commented Jan 10, 2024

greg7mdp Jan 10, 2024

greg7mdp Jan 10, 2024 •

edited

Loading

greg7mdp Jan 10, 2024

greg7mdp Jan 10, 2024 •

edited

Loading

linh2931 commented Jan 11, 2024

	wlog( "sinature from finalizer ${i} cannot be verified", ("i", index) );
	wlog( "signature from finalizer ${i} cannot be verified", ("i", index) );

IF:Unification: Integrate vote message signatures into new fork database #2067

IF:Unification: Integrate vote message signatures into new fork database #2067

Conversation

linh2931 commented Jan 10, 2024

linh2931 commented Jan 10, 2024

greg7mdp left a comment

Choose a reason for hiding this comment

greg7mdp Jan 10, 2024

Choose a reason for hiding this comment

greg7mdp Jan 10, 2024

Choose a reason for hiding this comment

linh2931 Jan 10, 2024

Choose a reason for hiding this comment

greg7mdp Jan 10, 2024

Choose a reason for hiding this comment

linh2931 commented Jan 10, 2024

greg7mdp Jan 10, 2024

Choose a reason for hiding this comment

greg7mdp Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

greg7mdp Jan 10, 2024

Choose a reason for hiding this comment

greg7mdp Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

linh2931 commented Jan 11, 2024

greg7mdp Jan 10, 2024 •

edited

Loading

greg7mdp Jan 10, 2024 •

edited

Loading