From 451398c58fe205f6c53908eeb848d25c9494daeb Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Wed, 31 Jan 2024 19:56:41 -0500 Subject: [PATCH 1/5] Net plugin handles invalid QC and invalid vote messages correctly (dropping connection, keeping track, or ignoring) --- libraries/chain/block_header_state.cpp | 2 +- libraries/chain/block_state.cpp | 14 +++--- libraries/chain/controller.cpp | 30 ++++++------ libraries/chain/hotstuff/hotstuff.cpp | 46 +++++++++--------- .../hotstuff/test/finality_misc_tests.cpp | 4 +- .../chain/include/eosio/chain/block_state.hpp | 2 +- .../chain/include/eosio/chain/controller.hpp | 3 +- .../chain/include/eosio/chain/exceptions.hpp | 2 + .../include/eosio/chain/hotstuff/hotstuff.hpp | 44 ++++++++++------- plugins/net_plugin/net_plugin.cpp | 48 +++++++++++++++---- unittests/block_state_tests.cpp | 13 ++--- 11 files changed, 125 insertions(+), 83 deletions(-) diff --git a/libraries/chain/block_header_state.cpp b/libraries/chain/block_header_state.cpp index 3c0da4bbfb..487f796277 100644 --- a/libraries/chain/block_header_state.cpp +++ b/libraries/chain/block_header_state.cpp @@ -218,4 +218,4 @@ block_header_state block_header_state::next(const signed_block_header& h, const return next(bhs_input); } -} // namespace eosio::chain \ No newline at end of file +} // namespace eosio::chain diff --git a/libraries/chain/block_state.cpp b/libraries/chain/block_state.cpp index 74865cabcc..e01a811864 100644 --- a/libraries/chain/block_state.cpp +++ b/libraries/chain/block_state.cpp @@ -69,7 +69,7 @@ void block_state::set_trxs_metas( deque&& trxs_metas, } // Called from net threads -std::pair> block_state::aggregate_vote(const vote_message& vote) { +std::pair> block_state::aggregate_vote(const vote_message& vote) { const auto& finalizers = active_finalizer_policy->finalizers; auto it = std::find_if(finalizers.begin(), finalizers.end(), @@ -78,17 +78,17 @@ std::pair> block_state::aggregate_vote(const vote_ if (it != finalizers.end()) { auto index = std::distance(finalizers.begin(), it); const digest_type& digest = vote.strong ? strong_digest : weak_digest; - auto [valid, strong] = pending_qc.add_vote(vote.strong, + auto [status, strong] = pending_qc.add_vote(vote.strong, #warning TODO change to use std::span if possible std::vector{digest.data(), digest.data() + digest.data_size()}, index, vote.finalizer_key, vote.sig, finalizers[index].weight); - return {valid, strong ? core.final_on_strong_qc_block_num : std::optional{}}; + return {status, strong ? core.final_on_strong_qc_block_num : std::optional{}}; } else { wlog( "finalizer_key (${k}) in vote is not in finalizer policy", ("k", vote.finalizer_key) ); - return {}; + return {vote_status::unknown_public_key, {}}; } } @@ -116,12 +116,12 @@ void block_state::verify_qc(const valid_quorum_certificate& qc) const { // verfify quorum is met if( qc.is_strong() ) { EOS_ASSERT( strong_weights >= active_finalizer_policy->threshold, - block_validate_exception, + invalid_qc_claim, "strong quorum is not met, strong_weights: ${s}, threshold: ${t}", ("s", strong_weights)("t", active_finalizer_policy->threshold) ); } else { EOS_ASSERT( strong_weights + weak_weights >= active_finalizer_policy->threshold, - block_validate_exception, + invalid_qc_claim, "weak quorum is not met, strong_weights: ${s}, weak_weights: ${w}, threshold: ${t}", ("s", strong_weights)("w", weak_weights)("t", active_finalizer_policy->threshold) ); } @@ -155,7 +155,7 @@ void block_state::verify_qc(const valid_quorum_certificate& qc) const { } // validate aggregated signature - EOS_ASSERT( fc::crypto::blslib::aggregate_verify( pubkeys, digests, qc._sig ), block_validate_exception, "signature validation failed" ); + EOS_ASSERT( fc::crypto::blslib::aggregate_verify( pubkeys, digests, qc._sig ), invalid_qc_claim, "signature validation failed" ); } std::optional block_state::get_best_qc() const { diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 344192d7a4..21156a225d 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -3082,12 +3082,12 @@ struct controller_impl { // block doesn't have a header extension either. Then return early. // ------------------------------------------------------------------------------------------ EOS_ASSERT( !qc_extension_present, - block_validate_exception, + invalid_qc_claim, "Block #${b} includes a QC block extension, but doesn't have a finality header extension", ("b", block_num) ); EOS_ASSERT( !prev_header_ext, - block_validate_exception, + invalid_qc_claim, "Block #${b} doesn't have a finality header extension even though its predecessor does.", ("b", block_num) ); return; @@ -3103,7 +3103,7 @@ struct controller_impl { // ------------------------------------------------------------------------------------------------- if (!prev_header_ext) { EOS_ASSERT( !qc_extension_present && qc_claim.last_qc_block_num == block_num && qc_claim.is_last_qc_strong == false, - block_validate_exception, + invalid_qc_claim, "Block #${b}, which is the finality transition block, doesn't have the expected extensions", ("b", block_num) ); return; @@ -3121,7 +3121,7 @@ struct controller_impl { // new claimed QC block number cannot be smaller than previous block's EOS_ASSERT( qc_claim.last_qc_block_num >= prev_qc_claim.last_qc_block_num, - block_validate_exception, + invalid_qc_claim, "Block #${b} claims a last_qc_block_num (${n1}) less than the previous block's (${n2})", ("n1", qc_claim.last_qc_block_num)("n2", prev_qc_claim.last_qc_block_num)("b", block_num) ); @@ -3129,7 +3129,7 @@ struct controller_impl { if( qc_claim.is_last_qc_strong == prev_qc_claim.is_last_qc_strong ) { // QC block extension is redundant EOS_ASSERT( !qc_extension_present, - block_validate_exception, + invalid_qc_claim, "Block #${b} should not provide a QC block extension since its QC claim is the same as the previous block's", ("b", block_num) ); @@ -3140,14 +3140,14 @@ struct controller_impl { // new claimed QC must be stronger than previous if the claimed block number is the same EOS_ASSERT( qc_claim.is_last_qc_strong, - block_validate_exception, + invalid_qc_claim, "claimed QC (${s1}) must be stricter than previous block's (${s2}) if block number is the same. Block number: ${b}", ("s1", qc_claim.is_last_qc_strong)("s2", prev_qc_claim.is_last_qc_strong)("b", block_num) ); } // At this point, we are making a new claim in this block, so it better include a QC to justify this claim. EOS_ASSERT( qc_extension_present, - block_validate_exception, + invalid_qc_claim, "Block #${b} is making a new finality claim, but doesn't include a qc to justify this claim", ("b", block_num) ); const auto& qc_ext = std::get(block_exts.lower_bound(qc_ext_id)->second); @@ -3155,20 +3155,20 @@ struct controller_impl { // Check QC information in header extension and block extension match EOS_ASSERT( qc_proof.block_num == qc_claim.last_qc_block_num, - block_validate_exception, + invalid_qc_claim, "Block #${b}: Mismatch between qc.block_num (${n1}) in block extension and last_qc_block_num (${n2}) in header extension", ("n1", qc_proof.block_num)("n2", qc_claim.last_qc_block_num)("b", block_num) ); // Verify claimed strictness is the same as in proof EOS_ASSERT( qc_proof.qc.is_strong() == qc_claim.is_last_qc_strong, - block_validate_exception, + invalid_qc_claim, "QC is_strong (${s1}) in block extension does not match is_last_qc_strong (${s2}) in header extension. Block number: ${b}", ("s1", qc_proof.qc.is_strong())("s2", qc_claim.is_last_qc_strong)("b", block_num) ); // find the claimed block's block state on branch of id auto bsp = fork_db_fetch_bsp_by_num( prev.id, qc_claim.last_qc_block_num ); EOS_ASSERT( bsp, - block_validate_exception, + invalid_qc_claim, "Block state was not found in forkdb for last_qc_block_num ${q}. Block number: ${b}", ("q", qc_claim.last_qc_block_num)("b", block_num) ); @@ -4386,18 +4386,18 @@ void controller::set_proposed_finalizers( const finalizer_policy& fin_pol ) { } // called from net threads -bool controller::process_vote_message( const vote_message& vote ) { - auto do_vote = [&vote](auto& forkdb) -> std::pair> { +vote_status controller::process_vote_message( const vote_message& vote ) { + auto do_vote = [&vote](auto& forkdb) -> std::pair> { auto bsp = forkdb.get_block(vote.proposal_id); if (bsp) return bsp->aggregate_vote(vote); - return {false, {}}; + return {vote_status::unknown_block, {}}; }; - auto [valid, new_lib] = my->fork_db.apply_if>>(do_vote); + auto [status, new_lib] = my->fork_db.apply_if>>(do_vote); if (new_lib) { my->set_if_irreversible_block_num(*new_lib); } - return valid; + return status; }; const producer_authority_schedule& controller::active_producers()const { diff --git a/libraries/chain/hotstuff/hotstuff.cpp b/libraries/chain/hotstuff/hotstuff.cpp index e2f361ff1e..d932b0e16f 100644 --- a/libraries/chain/hotstuff/hotstuff.cpp +++ b/libraries/chain/hotstuff/hotstuff.cpp @@ -20,18 +20,18 @@ inline std::vector bitset_to_vector(const hs_bitset& bs) { return r; } -bool pending_quorum_certificate::votes_t::add_vote(const std::vector& proposal_digest, size_t index, - const bls_public_key& pubkey, const bls_signature& new_sig) { +vote_status pending_quorum_certificate::votes_t::add_vote(const std::vector& proposal_digest, size_t index, + const bls_public_key& pubkey, const bls_signature& new_sig) { if (_bitset[index]) { - return false; // shouldn't be already present + return vote_status::duplicate; // shouldn't be already present } if (!fc::crypto::blslib::verify(pubkey, proposal_digest, new_sig)) { wlog( "signature from finalizer ${i} cannot be verified", ("i", index) ); - return false; + return vote_status::invalid_signature; } _bitset.set(index); _sig = fc::crypto::blslib::aggregate({_sig, new_sig}); // works even if _sig is default initialized (fp2::zero()) - return true; + return vote_status::succeeded; } void pending_quorum_certificate::votes_t::reset(size_t num_finalizers) { @@ -59,11 +59,11 @@ bool pending_quorum_certificate::is_quorum_met() const { } // called by add_vote, already protected by mutex -bool pending_quorum_certificate::add_strong_vote(const std::vector& proposal_digest, size_t index, - const bls_public_key& pubkey, const bls_signature& sig, - uint64_t weight) { - if (!_strong_votes.add_vote(proposal_digest, index, pubkey, sig)) - return false; +vote_status pending_quorum_certificate::add_strong_vote(const std::vector& 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::succeeded) + return s; _strong_sum += weight; switch (_state) { @@ -86,15 +86,15 @@ bool pending_quorum_certificate::add_strong_vote(const std::vector& pro // getting another strong vote...nothing to do break; } - return true; + return vote_status::succeeded; } // called by add_vote, already protected by mutex -bool pending_quorum_certificate::add_weak_vote(const std::vector& proposal_digest, size_t index, - const bls_public_key& pubkey, const bls_signature& sig, - uint64_t weight) { - if (!_weak_votes.add_vote(proposal_digest, index, pubkey, sig)) - return false; +vote_status pending_quorum_certificate::add_weak_vote(const std::vector& proposal_digest, size_t index, + const bls_public_key& pubkey, const bls_signature& sig, + uint64_t weight) { + if (auto s = _weak_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::succeeded) + return s; _weak_sum += weight; switch (_state) { @@ -121,17 +121,17 @@ bool pending_quorum_certificate::add_weak_vote(const std::vector& propo // getting another weak vote... nothing to do break; } - return true; + return vote_status::succeeded; } // thread safe, -std::pair pending_quorum_certificate::add_vote(bool strong, const std::vector& proposal_digest, size_t index, - const bls_public_key& pubkey, const bls_signature& sig, - uint64_t weight) { +std::pair pending_quorum_certificate::add_vote(bool strong, const std::vector& proposal_digest, size_t index, + const bls_public_key& pubkey, const bls_signature& sig, + uint64_t weight) { std::lock_guard g(*_mtx); - bool valid = strong ? add_strong_vote(proposal_digest, index, pubkey, sig, weight) - : add_weak_vote(proposal_digest, index, pubkey, sig, weight); - return {valid, _state == state_t::strong}; + vote_status s = strong ? add_strong_vote(proposal_digest, index, pubkey, sig, weight) + : add_weak_vote(proposal_digest, index, pubkey, sig, weight); + return {s, _state == state_t::strong}; } // thread safe diff --git a/libraries/chain/hotstuff/test/finality_misc_tests.cpp b/libraries/chain/hotstuff/test/finality_misc_tests.cpp index 9f1726f07b..7425918a35 100644 --- a/libraries/chain/hotstuff/test/finality_misc_tests.cpp +++ b/libraries/chain/hotstuff/test/finality_misc_tests.cpp @@ -74,8 +74,8 @@ BOOST_AUTO_TEST_CASE(qc_state_transitions) try { // add duplicate weak vote // ----------------------- - bool ok = weak_vote(qc, digest, 0, weight); - BOOST_CHECK(!ok); // vote was a duplicate + auto ok = weak_vote(qc, digest, 0, weight); + BOOST_CHECK(ok != vote_status::succeeded); // vote was a duplicate BOOST_CHECK_EQUAL(qc.state(), state_t::weak_achieved); BOOST_CHECK(qc.is_quorum_met()); diff --git a/libraries/chain/include/eosio/chain/block_state.hpp b/libraries/chain/include/eosio/chain/block_state.hpp index 4548c2804f..9492ca0591 100644 --- a/libraries/chain/include/eosio/chain/block_state.hpp +++ b/libraries/chain/include/eosio/chain/block_state.hpp @@ -39,7 +39,7 @@ struct block_state : public block_header_state { // block_header_state provi void set_trxs_metas(deque&& trxs_metas, bool keys_recovered); const deque& trxs_metas() const { return cached_trxs; } - std::pair> aggregate_vote(const vote_message& vote); // aggregate vote into pending_qc + std::pair> aggregate_vote(const vote_message& vote); // aggregate vote into pending_qc void verify_qc(const valid_quorum_certificate& qc) const; // verify given qc is valid with respect block_state using bhs_t = block_header_state; diff --git a/libraries/chain/include/eosio/chain/controller.hpp b/libraries/chain/include/eosio/chain/controller.hpp index 931a3070b1..9334f69fff 100644 --- a/libraries/chain/include/eosio/chain/controller.hpp +++ b/libraries/chain/include/eosio/chain/controller.hpp @@ -8,6 +8,7 @@ #include #include #include +#include #include @@ -326,7 +327,7 @@ namespace eosio::chain { // called by host function set_finalizers void set_proposed_finalizers( const finalizer_policy& fin_set ); // called from net threads - bool process_vote_message( const vote_message& msg ); + vote_status process_vote_message( const vote_message& msg ); bool light_validation_allowed() const; bool skip_auth_check()const; diff --git a/libraries/chain/include/eosio/chain/exceptions.hpp b/libraries/chain/include/eosio/chain/exceptions.hpp index 22aa819574..f7a41b640d 100644 --- a/libraries/chain/include/eosio/chain/exceptions.hpp +++ b/libraries/chain/include/eosio/chain/exceptions.hpp @@ -255,6 +255,8 @@ namespace eosio { namespace chain { 3030012, "Invalid block extension" ) FC_DECLARE_DERIVED_EXCEPTION( ill_formed_additional_block_signatures_extension, block_validate_exception, 3030013, "Block includes an ill-formed additional block signature extension" ) + FC_DECLARE_DERIVED_EXCEPTION( invalid_qc_claim, block_validate_exception, + 3030014, "Block includes an invalid QC claim" ) FC_DECLARE_DERIVED_EXCEPTION( transaction_exception, chain_exception, 3040000, "Transaction exception" ) diff --git a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp index a11e32653d..322a08d2ee 100644 --- a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp +++ b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp @@ -92,6 +92,14 @@ namespace eosio::chain { invalid // invalid message (other reason) }; + enum class vote_status { + succeeded, + duplicate, + unknown_public_key, + invalid_signature, + unknown_block + }; + using bls_public_key = fc::crypto::blslib::bls_public_key; using bls_signature = fc::crypto::blslib::bls_signature; using bls_private_key = fc::crypto::blslib::bls_private_key; @@ -151,8 +159,8 @@ namespace eosio::chain { void resize(size_t num_finalizers) { _bitset.resize(num_finalizers); } size_t count() const { return _bitset.count(); } - bool add_vote(const std::vector& proposal_digest, size_t index, const bls_public_key& pubkey, - const bls_signature& new_sig); + vote_status add_vote(const std::vector& proposal_digest, size_t index, const bls_public_key& pubkey, + const bls_signature& new_sig); void reset(size_t num_finalizers); }; @@ -165,12 +173,12 @@ namespace eosio::chain { bool is_quorum_met() const; // thread safe - std::pair add_vote(bool strong, - const std::vector&proposal_digest, - size_t index, - const bls_public_key&pubkey, - const bls_signature&sig, - uint64_t weight); + std::pair add_vote(bool strong, + const std::vector&proposal_digest, + size_t index, + const bls_public_key&pubkey, + const bls_signature&sig, + uint64_t weight); state_t state() const { std::lock_guard g(*_mtx); return _state; }; valid_quorum_certificate to_valid_quorum_certificate() const; @@ -198,18 +206,18 @@ namespace eosio::chain { votes_t _strong_votes; // called by add_vote, already protected by mutex - bool add_strong_vote(const std::vector& proposal_digest, - size_t index, - const bls_public_key& pubkey, - const bls_signature& sig, - uint64_t weight); + vote_status add_strong_vote(const std::vector& proposal_digest, + size_t index, + const bls_public_key& pubkey, + const bls_signature& sig, + uint64_t weight); // called by add_vote, already protected by mutex - bool add_weak_vote(const std::vector& proposal_digest, - size_t index, - const bls_public_key& pubkey, - const bls_signature& sig, - uint64_t weight); + vote_status add_weak_vote(const std::vector& proposal_digest, + size_t index, + const bls_public_key& pubkey, + const bls_signature& sig, + uint64_t weight); }; } //eosio::chain diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 5403b65866..a7a38ab9f0 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -261,13 +261,17 @@ namespace eosio { bool verify_catchup( const connection_ptr& c, uint32_t num, const block_id_type& id ); // locks mutex public: + enum class closing_mode { + immediately, + back_off + }; explicit sync_manager( uint32_t span, uint32_t sync_peer_limit, uint32_t min_blocks_distance ); static void send_handshakes(); bool syncing_from_peer() const { return sync_state == lib_catchup; } bool is_in_sync() const { return sync_state == in_sync; } void sync_reset_lib_num( const connection_ptr& conn, bool closing ); void sync_reassign_fetch( const connection_ptr& c, go_away_reason reason ); - void rejected_block( const connection_ptr& c, uint32_t blk_num ); + void rejected_block( const connection_ptr& c, uint32_t blk_num, closing_mode mode ); void sync_recv_block( const connection_ptr& c, const block_id_type& blk_id, uint32_t blk_num, bool blk_applied ); void recv_handshake( const connection_ptr& c, const handshake_message& msg, uint32_t nblk_combined_latency ); void sync_recv_notice( const connection_ptr& c, const notice_message& msg ); @@ -910,6 +914,7 @@ namespace eosio { std::atomic block_sync_throttling{false}; std::atomic last_bytes_sent{0ns}; std::atomic remote_endpoint_port{0}; + std::atomic invalid_finality_msg_total{0}; public: boost::asio::io_context::strand strand; @@ -1484,6 +1489,7 @@ namespace eosio { block_sync_send_start = 0ns; block_sync_frame_bytes_sent = 0; block_sync_throttling = false; + invalid_finality_msg_total = 0; if( reconnect && !shutdown ) { my_impl->connections.start_conn_timer( std::chrono::milliseconds( 100 ), @@ -2437,18 +2443,22 @@ namespace eosio { } // called from connection strand - void sync_manager::rejected_block( const connection_ptr& c, uint32_t blk_num ) { + void sync_manager::rejected_block( const connection_ptr& c, uint32_t blk_num, closing_mode mode ) { c->block_status_monitor_.rejected(); fc::unique_lock g( sync_mtx ); sync_last_requested_num = 0; if (blk_num < sync_next_expected_num) { sync_next_expected_num = my_impl->get_chain_lib_num(); } - if( c->block_status_monitor_.max_events_violated()) { + if( mode == closing_mode::immediately || c->block_status_monitor_.max_events_violated()) { peer_wlog( c, "block ${bn} not accepted, closing connection", ("bn", blk_num) ); sync_source.reset(); g.unlock(); - c->close(); + if( mode == closing_mode::immediately ) { + c->close( false ); // do not reconnect + } else { + c->close(); + } } else { g.unlock(); peer_dlog(c, "rejected block ${bn}, sending handshake", ("bn", blk_num)); @@ -3691,8 +3701,22 @@ namespace eosio { ("bn", block_header::num_from_id(msg.proposal_id))("id", msg.proposal_id.str().substr(8,16)) ("t", msg.strong ? "strong" : "weak")("k", msg.finalizer_key.to_string().substr(8, 16))); controller& cc = my_impl->chain_plug->chain(); - if( cc.process_vote_message(msg) ) { - my_impl->bcast_vote_message(connection_id, msg); + + switch( cc.process_vote_message(msg) ) { + case vote_status::succeeded: + my_impl->bcast_vote_message(connection_id, msg); + break; + case vote_status::unknown_public_key: + case vote_status::invalid_signature: // close peer immediately + close( false ); // do not reconnect after closing + break; + case vote_status::unknown_block: // keep tracking + ++invalid_finality_msg_total; // atomic + break; + case vote_status::duplicate: // do nothing + break; + default: + assert(false); // should never happen } } @@ -3746,9 +3770,15 @@ namespace eosio { std::optional obt; bool exception = false; + sync_manager::closing_mode close_mode = sync_manager::closing_mode::back_off; try { // this may return null if block is not immediately ready to be processed obt = cc.create_block_handle( id, ptr ); + } catch( const invalid_qc_claim &ex) { + exception = true; + close_mode = sync_manager::closing_mode::immediately; + fc_wlog( logger, "invalid QC claim exception, connection ${cid}: #${n} ${id}...: ${m}", + ("cid", cid)("n", ptr->block_num())("id", id.str().substr(8,16))("m",ex.to_string())); } catch( const fc::exception& ex ) { exception = true; fc_ilog( logger, "bad block exception connection ${cid}: #${n} ${id}...: ${m}", @@ -3759,8 +3789,8 @@ namespace eosio { ("cid", cid)("n", ptr->block_num())("id", id.str().substr(8,16))); } if( exception ) { - c->strand.post( [c, id, blk_num=ptr->block_num()]() { - my_impl->sync_master->rejected_block( c, blk_num ); + c->strand.post( [c, id, blk_num=ptr->block_num(), close_mode]() { + my_impl->sync_master->rejected_block( c, blk_num, close_mode ); my_impl->dispatcher->rejected_block( id ); }); return; @@ -3873,7 +3903,7 @@ namespace eosio { } // reason==no_reason means accept_block() return false because we are producing, don't call rejected_block which sends handshake if( reason != no_reason ) { - sync_master->rejected_block( c, blk_num ); + sync_master->rejected_block( c, blk_num, sync_manager::closing_mode::back_off ); } dispatcher->rejected_block( blk_id ); }); diff --git a/unittests/block_state_tests.cpp b/unittests/block_state_tests.cpp index eb1295e41a..3cfe859e20 100644 --- a/unittests/block_state_tests.cpp +++ b/unittests/block_state_tests.cpp @@ -49,7 +49,7 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bool strong = (i % 2 == 0); // alternate strong and weak auto sig = strong ? private_key[i].sign(strong_digest_data) : private_key[i].sign(weak_digest_data); vote_message vote{ block_id, strong, public_key[i], sig }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::succeeded); } } @@ -60,7 +60,7 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bsp->pending_qc = pending_quorum_certificate{ num_finalizers, 1, bsp->active_finalizer_policy->max_weak_sum_before_weak_final() }; vote_message vote {block_id, true, public_key[0], private_key[1].sign(strong_digest_data) }; - BOOST_REQUIRE(!bsp->aggregate_vote(vote).first); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::succeeded); } { // duplicate votes @@ -70,8 +70,9 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bsp->pending_qc = pending_quorum_certificate{ num_finalizers, 1, bsp->active_finalizer_policy->max_weak_sum_before_weak_final() }; vote_message vote {block_id, true, public_key[0], private_key[0].sign(strong_digest_data) }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first); - BOOST_REQUIRE(!bsp->aggregate_vote(vote).first); + + BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::succeeded); // first time succeeds + BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::succeeded); // second time failed due to duplicate voting } { // public key does not exit in finalizer set @@ -84,7 +85,7 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bls_public_key new_public_key{ new_private_key.get_public_key() }; vote_message vote {block_id, true, new_public_key, private_key[0].sign(strong_digest_data) }; - BOOST_REQUIRE(!bsp->aggregate_vote(vote).first); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::succeeded); } } FC_LOG_AND_RETHROW(); @@ -129,7 +130,7 @@ void do_quorum_test(const std::vector& weights, if( to_vote[i] ) { auto sig = strong ? private_key[i].sign(strong_digest_data) : private_key[i].sign(weak_digest_data); vote_message vote{ block_id, strong, public_key[i], sig }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::succeeded); } } From f2a5ef427f10420a7cba57bd51cd36183e4e2261 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Wed, 31 Jan 2024 20:12:11 -0500 Subject: [PATCH 2/5] revert non-changed block_header_state.cpp to previous commit --- libraries/chain/block_header_state.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/block_header_state.cpp b/libraries/chain/block_header_state.cpp index 487f796277..3c0da4bbfb 100644 --- a/libraries/chain/block_header_state.cpp +++ b/libraries/chain/block_header_state.cpp @@ -218,4 +218,4 @@ block_header_state block_header_state::next(const signed_block_header& h, const return next(bhs_input); } -} // namespace eosio::chain +} // namespace eosio::chain \ No newline at end of file From f3fe470cacb28d47e988865656c6ba061508140c Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Thu, 1 Feb 2024 09:44:41 -0500 Subject: [PATCH 3/5] change succeeded to success in vote_status --- libraries/chain/hotstuff/hotstuff.cpp | 10 +++++----- .../chain/hotstuff/test/finality_misc_tests.cpp | 2 +- .../chain/include/eosio/chain/hotstuff/hotstuff.hpp | 2 +- plugins/net_plugin/net_plugin.cpp | 2 +- unittests/block_state_tests.cpp | 12 ++++++------ 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/libraries/chain/hotstuff/hotstuff.cpp b/libraries/chain/hotstuff/hotstuff.cpp index d932b0e16f..93642aebf7 100644 --- a/libraries/chain/hotstuff/hotstuff.cpp +++ b/libraries/chain/hotstuff/hotstuff.cpp @@ -31,7 +31,7 @@ vote_status pending_quorum_certificate::votes_t::add_vote(const std::vector& 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::succeeded) + if (auto s = _strong_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::success) return s; _strong_sum += weight; @@ -86,14 +86,14 @@ vote_status pending_quorum_certificate::add_strong_vote(const std::vector& proposal_digest, size_t index, const bls_public_key& pubkey, const bls_signature& sig, uint64_t weight) { - if (auto s = _weak_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::succeeded) + if (auto s = _weak_votes.add_vote(proposal_digest, index, pubkey, sig); s != vote_status::success) return s; _weak_sum += weight; @@ -121,7 +121,7 @@ vote_status pending_quorum_certificate::add_weak_vote(const std::vector // getting another weak vote... nothing to do break; } - return vote_status::succeeded; + return vote_status::success; } // thread safe, diff --git a/libraries/chain/hotstuff/test/finality_misc_tests.cpp b/libraries/chain/hotstuff/test/finality_misc_tests.cpp index 7425918a35..418d82116c 100644 --- a/libraries/chain/hotstuff/test/finality_misc_tests.cpp +++ b/libraries/chain/hotstuff/test/finality_misc_tests.cpp @@ -75,7 +75,7 @@ BOOST_AUTO_TEST_CASE(qc_state_transitions) try { // add duplicate weak vote // ----------------------- auto ok = weak_vote(qc, digest, 0, weight); - BOOST_CHECK(ok != vote_status::succeeded); // vote was a duplicate + BOOST_CHECK(ok != vote_status::success); // vote was a duplicate BOOST_CHECK_EQUAL(qc.state(), state_t::weak_achieved); BOOST_CHECK(qc.is_quorum_met()); diff --git a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp index 322a08d2ee..e2003fa313 100644 --- a/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp +++ b/libraries/chain/include/eosio/chain/hotstuff/hotstuff.hpp @@ -93,7 +93,7 @@ namespace eosio::chain { }; enum class vote_status { - succeeded, + success, duplicate, unknown_public_key, invalid_signature, diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index a7a38ab9f0..ccfa0ec045 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -3703,7 +3703,7 @@ namespace eosio { controller& cc = my_impl->chain_plug->chain(); switch( cc.process_vote_message(msg) ) { - case vote_status::succeeded: + case vote_status::success: my_impl->bcast_vote_message(connection_id, msg); break; case vote_status::unknown_public_key: diff --git a/unittests/block_state_tests.cpp b/unittests/block_state_tests.cpp index 3cfe859e20..dfe074afa4 100644 --- a/unittests/block_state_tests.cpp +++ b/unittests/block_state_tests.cpp @@ -49,7 +49,7 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bool strong = (i % 2 == 0); // alternate strong and weak auto sig = strong ? private_key[i].sign(strong_digest_data) : private_key[i].sign(weak_digest_data); vote_message vote{ block_id, strong, public_key[i], sig }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::succeeded); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::success); } } @@ -60,7 +60,7 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bsp->pending_qc = pending_quorum_certificate{ num_finalizers, 1, bsp->active_finalizer_policy->max_weak_sum_before_weak_final() }; vote_message vote {block_id, true, public_key[0], private_key[1].sign(strong_digest_data) }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::succeeded); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::success); } { // duplicate votes @@ -71,8 +71,8 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { vote_message vote {block_id, true, public_key[0], private_key[0].sign(strong_digest_data) }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::succeeded); // first time succeeds - BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::succeeded); // second time failed due to duplicate voting + BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::success); // first time succeeds + BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::success); // second time failed due to duplicate voting } { // public key does not exit in finalizer set @@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(aggregate_vote_test) try { bls_public_key new_public_key{ new_private_key.get_public_key() }; vote_message vote {block_id, true, new_public_key, private_key[0].sign(strong_digest_data) }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::succeeded); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first != vote_status::success); } } FC_LOG_AND_RETHROW(); @@ -130,7 +130,7 @@ void do_quorum_test(const std::vector& weights, if( to_vote[i] ) { auto sig = strong ? private_key[i].sign(strong_digest_data) : private_key[i].sign(weak_digest_data); vote_message vote{ block_id, strong, public_key[i], sig }; - BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::succeeded); + BOOST_REQUIRE(bsp->aggregate_vote(vote).first == vote_status::success); } } From fd8af15eabd62e9828efbdd11533b0f99eac4b34 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Fri, 2 Feb 2024 15:23:53 -0500 Subject: [PATCH 4/5] use block_status_monitor to track failures; change closing_mode::back_off to closing_mode::handshake --- plugins/net_plugin/net_plugin.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index ccfa0ec045..1b91f21cfe 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -262,8 +262,8 @@ namespace eosio { public: enum class closing_mode { - immediately, - back_off + immediately, // closing connection immediately + handshake // sending handshake message }; explicit sync_manager( uint32_t span, uint32_t sync_peer_limit, uint32_t min_blocks_distance ); static void send_handshakes(); @@ -914,7 +914,6 @@ namespace eosio { std::atomic block_sync_throttling{false}; std::atomic last_bytes_sent{0ns}; std::atomic remote_endpoint_port{0}; - std::atomic invalid_finality_msg_total{0}; public: boost::asio::io_context::strand strand; @@ -1489,7 +1488,6 @@ namespace eosio { block_sync_send_start = 0ns; block_sync_frame_bytes_sent = 0; block_sync_throttling = false; - invalid_finality_msg_total = 0; if( reconnect && !shutdown ) { my_impl->connections.start_conn_timer( std::chrono::milliseconds( 100 ), @@ -3710,8 +3708,8 @@ namespace eosio { case vote_status::invalid_signature: // close peer immediately close( false ); // do not reconnect after closing break; - case vote_status::unknown_block: // keep tracking - ++invalid_finality_msg_total; // atomic + case vote_status::unknown_block: // track the failure + block_status_monitor_.rejected(); break; case vote_status::duplicate: // do nothing break; @@ -3770,7 +3768,7 @@ namespace eosio { std::optional obt; bool exception = false; - sync_manager::closing_mode close_mode = sync_manager::closing_mode::back_off; + sync_manager::closing_mode close_mode = sync_manager::closing_mode::handshake; try { // this may return null if block is not immediately ready to be processed obt = cc.create_block_handle( id, ptr ); @@ -3903,7 +3901,7 @@ namespace eosio { } // reason==no_reason means accept_block() return false because we are producing, don't call rejected_block which sends handshake if( reason != no_reason ) { - sync_master->rejected_block( c, blk_num, sync_manager::closing_mode::back_off ); + sync_master->rejected_block( c, blk_num, sync_manager::closing_mode::handshake ); } dispatcher->rejected_block( blk_id ); }); From 4065984be313e7623082e54ce4eb69720d459347 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Fri, 2 Feb 2024 15:53:21 -0500 Subject: [PATCH 5/5] fix a merging error --- libraries/chain/controller.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index dcb6393358..f5a58e5a19 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -2999,18 +2999,18 @@ struct controller_impl { } /// apply_block // called from net threads and controller's thread pool - bool process_vote_message( const vote_message& vote ) { - auto do_vote = [&vote](auto& forkdb) -> std::pair> { + vote_status process_vote_message( const vote_message& vote ) { + auto do_vote = [&vote](auto& forkdb) -> std::pair> { auto bsp = forkdb.get_block(vote.proposal_id); if (bsp) return bsp->aggregate_vote(vote); - return {false, {}}; + return {vote_status::unknown_block, {}}; }; - auto [valid, new_lib] = fork_db.apply_if>>(do_vote); + auto [status, new_lib] = fork_db.apply_if>>(do_vote); if (new_lib) { set_if_irreversible_block_num(*new_lib); } - return valid; + return status; }; void create_and_send_vote_msg(const block_state_ptr& bsp) { @@ -4402,7 +4402,7 @@ void controller::set_proposed_finalizers( const finalizer_policy& fin_pol ) { } // called from net threads -bool controller::process_vote_message( const vote_message& vote ) { +vote_status controller::process_vote_message( const vote_message& vote ) { return my->process_vote_message( vote ); };