From 25309df1c0f05cb9064ec43a6055c0084c175353 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Sat, 2 Mar 2024 13:18:06 -0600 Subject: [PATCH] GH-2125 Vote before applying/validating block if possible --- libraries/chain/controller.cpp | 92 +++++++++++++++----------- libraries/chain/hotstuff/finalizer.cpp | 11 ++- 2 files changed, 60 insertions(+), 43 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 5d79d39b1a..0ff23872a9 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -3172,43 +3172,6 @@ struct controller_impl { }); } - // expected to be called from application thread as it modifies bsp->valid_qc and if_irreversible_block_id - void integrate_received_qc_to_block(const block_state_ptr& bsp_in) { - // extract QC from block extension - const auto& block_exts = bsp_in->block->validate_and_extract_extensions(); - auto qc_ext_id = quorum_certificate_extension::extension_id(); - - if( block_exts.count(qc_ext_id) == 0 ) { - return; - } - const auto& qc_ext = std::get(block_exts.lower_bound(qc_ext_id)->second); - const auto& received_qc = qc_ext.qc.qc; - - const auto claimed = fetch_bsp_on_branch_by_num( bsp_in->previous(), qc_ext.qc.block_num ); - if( !claimed ) { - return; - } - - // Don't save the QC from block extension if the claimed block has a better valid_qc. - if (claimed->valid_qc && (claimed->valid_qc->is_strong() || received_qc.is_weak())) { - return; - } - - // Save the QC. This is safe as the function is called by push_block from application thread. - claimed->valid_qc = received_qc; - - // advance LIB if QC is strong - if( received_qc.is_strong() ) { - // We evaluate a block extension qc and advance lib if strong. - // This is done before evaluating the block. It is possible the block - // will not be valid or forked out. This is safe because the block is - // just acting as a carrier of this info. It doesn't matter if the block - // is actually valid as it simply is used as a network message for this data. - auto& final_on_strong_qc_block_ref = claimed->core.get_block_reference(claimed->core.final_on_strong_qc_block_num); - set_if_irreversible_block_id(final_on_strong_qc_block_ref.block_id); - } - } - // Verify QC claim made by instant_finality_extension in header extension // and quorum_certificate_extension in block extension are valid. // Called from net-threads. It is thread safe as signed_block is never modified @@ -3417,6 +3380,59 @@ struct controller_impl { return fork_db.apply>(f); } + // expected to be called from application thread as it modifies bsp->valid_qc and if_irreversible_block_id + void integrate_received_qc_to_block(const block_state_ptr& bsp_in) { + // extract QC from block extension + const auto& block_exts = bsp_in->block->validate_and_extract_extensions(); + auto qc_ext_id = quorum_certificate_extension::extension_id(); + + if( block_exts.count(qc_ext_id) == 0 ) { + return; + } + const auto& qc_ext = std::get(block_exts.lower_bound(qc_ext_id)->second); + const auto& received_qc = qc_ext.qc.qc; + + const auto claimed = fetch_bsp_on_branch_by_num( bsp_in->previous(), qc_ext.qc.block_num ); + if( !claimed ) { + return; + } + + // Don't save the QC from block extension if the claimed block has a better valid_qc. + if (claimed->valid_qc && (claimed->valid_qc->is_strong() || received_qc.is_weak())) { + return; + } + + // Save the QC. This is safe as the function is called by push_block & accept_block from application thread. + claimed->valid_qc = received_qc; + + // advance LIB if QC is strong + if( received_qc.is_strong() ) { + // We evaluate a block extension qc and advance lib if strong. + // This is done before evaluating the block. It is possible the block + // will not be valid or forked out. This is safe because the block is + // just acting as a carrier of this info. It doesn't matter if the block + // is actually valid as it simply is used as a network message for this data. + const auto& final_on_strong_qc_block_ref = claimed->core.get_block_reference(claimed->core.final_on_strong_qc_block_num); + set_if_irreversible_block_id(final_on_strong_qc_block_ref.block_id); + } + } + + void consider_voting(const block_state_ptr& bsp) { + // 1. Get the `core.final_on_strong_qc_block_num` for the block you are considering to vote on and use that to find the actual block ID + // of the ancestor block that has that block number. + // 2. If that block ID is not an ancestor of the current head block, then do not vote for that block. + // 3. Otherwise, consider voting for that block according to the decide_vote rules. + + if (bsp->core.final_on_strong_qc_block_num > 0) { + const auto& final_on_strong_qc_block_ref = bsp->core.get_block_reference(bsp->core.final_on_strong_qc_block_num); + auto final = fetch_bsp_on_head_branch_by_num(final_on_strong_qc_block_ref.block_num()); + if (final) { + assert(final->is_valid()); // if found on head branch then it must be validated + create_and_send_vote_msg(bsp); + } + } + } + template void accept_block(const BSP& bsp) { assert(bsp && bsp->block); @@ -3424,6 +3440,7 @@ struct controller_impl { // Save the received QC as soon as possible, no matter whether the block itself is valid or not if constexpr (std::is_same_v) { integrate_received_qc_to_block(bsp); + consider_voting(bsp); } auto do_accept_block = [&](auto& forkdb) { @@ -3447,6 +3464,7 @@ struct controller_impl { // Save the received QC as soon as possible, no matter whether the block itself is valid or not if constexpr (std::is_same_v) { integrate_received_qc_to_block(bsp); + consider_voting(bsp); } controller::block_status s = controller::block_status::complete; diff --git a/libraries/chain/hotstuff/finalizer.cpp b/libraries/chain/hotstuff/finalizer.cpp index a0d17dca67..e454cf2cb7 100644 --- a/libraries/chain/hotstuff/finalizer.cpp +++ b/libraries/chain/hotstuff/finalizer.cpp @@ -5,7 +5,7 @@ namespace eosio::chain { // ---------------------------------------------------------------------------------------- -finalizer::vote_result finalizer::decide_vote(const finality_core& core, const block_id_type &proposal_id, +finalizer::vote_result finalizer::decide_vote(const finality_core& core, const block_id_type& proposal_id, const block_timestamp_type proposal_timestamp) { vote_result res; @@ -14,7 +14,7 @@ finalizer::vote_result finalizer::decide_vote(const finality_core& core, const b // just activated and we can proceed if (!res.monotony_check) { - dlog("monotony check failed for proposal ${p}, cannot vote", ("p", proposal_id)); + dlog("monotony check failed for proposal ${bn} ${p}, cannot vote", ("bn", block_header::num_from_id(proposal_id))("p", proposal_id)); return res; } @@ -38,8 +38,6 @@ finalizer::vote_result finalizer::decide_vote(const finality_core& core, const b } bool can_vote = res.liveness_check || res.safety_check; - dlog("liveness_check=${l}, safety_check=${s}, monotony_check=${m}, can vote=${can_vote}", - ("l",res.liveness_check)("s",res.safety_check)("m",res.monotony_check)("can_vote",can_vote)); // Figure out if we can vote and wether our vote will be strong or weak // If we vote, update `fsi.last_vote` and also `fsi.lock` if we have a newer commit qc @@ -64,8 +62,9 @@ finalizer::vote_result finalizer::decide_vote(const finality_core& core, const b res.decision = voting_strong ? vote_decision::strong_vote : vote_decision::weak_vote; } - dlog("liveness_check=${l}, safety_check=${s}, monotony_check=${m}, can vote=${can_vote}, voting=${v}", - ("l",res.liveness_check)("s",res.safety_check)("m",res.monotony_check)("can_vote",can_vote)("v", res.decision)); + dlog("block=${bn}, liveness_check=${l}, safety_check=${s}, monotony_check=${m}, can vote=${can_vote}, voting=${v}", + ("bn", block_header::num_from_id(proposal_id))("l",res.liveness_check)("s",res.safety_check)("m",res.monotony_check) + ("can_vote",can_vote)("v", res.decision)); return res; }