From d03d750ececb1b7b610dbd138d0af818de18fcfc Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 23 Jul 2024 14:56:23 -0500 Subject: [PATCH] GH-376 Set better received qc if a better qc is received then currently available. --- libraries/chain/controller.cpp | 13 ++++------ libraries/chain/finality/qc.cpp | 26 +++++++++++++++---- .../chain/include/eosio/chain/block_state.hpp | 5 ++-- .../chain/include/eosio/chain/finality/qc.hpp | 12 ++++----- libraries/libfc/include/fc/bitutil.hpp | 5 ++++ 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 88ec8fc6f1..d7e75e7d03 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -3972,18 +3972,15 @@ struct controller_impl { return; } - // Don't save the QC from block extension if the claimed block has a better or same received_qc_sig. - // claimed->received_qc_sig_is_strong() acquires a mutex. - if (received_qc.is_weak() || claimed->valid_qc_is_strong()) { - dlog("qc not better, claimed->valid: ${qbn} ${qid}, strong=${s}, received: ${rqc}, for block ${bn} ${id}", - ("qbn", claimed->block_num())("qid", claimed->id())("s", !received_qc.is_weak()) // use is_weak() to avoid mutex on received_qc_sig_is_strong() + // Don't save the QC from block extension if the claimed block has a better or same received_qc + if (!claimed->set_received_qc(received_qc)) { + dlog("qc not better, claimed->received: ${qbn} ${qid}, strong=${s}, received: ${rqc}, for block ${bn} ${id}", + ("qbn", claimed->block_num())("qid", claimed->id())("s", !received_qc.is_weak()) // use is_weak() to avoid mutex on received_qc_is_strong() ("rqc", qc_ext.qc.to_qc_claim())("bn", bsp_in->block_num())("id", bsp_in->id())); return; } - // Save the QC. - dlog("setting valid qc: ${rqc} into claimed block ${bn} ${id}", ("rqc", qc_ext.qc.to_qc_claim())("bn", claimed->block_num())("id", claimed->id())); - claimed->set_valid_qc(received_qc); + dlog("set received qc: ${rqc} into claimed block ${bn} ${id}", ("rqc", qc_ext.qc.to_qc_claim())("bn", claimed->block_num())("id", claimed->id())); if( received_qc.is_strong() ) { // Update finalizer safety information based on vote evidence diff --git a/libraries/chain/finality/qc.cpp b/libraries/chain/finality/qc.cpp index cf4473e98c..72d0b38566 100644 --- a/libraries/chain/finality/qc.cpp +++ b/libraries/chain/finality/qc.cpp @@ -21,6 +21,11 @@ inline std::vector bitset_to_vector(const vote_bitset& bs) { return r; } +size_t qc_sig_t::vote_count() const { + return (strong_votes ? strong_votes->count() : 0u) + + (weak_votes ? weak_votes->count() : 0u); +} + void qc_sig_t::verify(const finalizer_policy_ptr& fin_policy, const digest_type& strong_digest, const weak_digest_t& weak_digest) const { @@ -277,9 +282,18 @@ std::optional open_qc_sig_t::get_best_qc() const { return std::optional{qc_sig_t{ std::move(qc_sig_from_open) }}; } -void open_qc_sig_t::set_received_qc_sig(const qc_sig_t& qc) { +bool open_qc_sig_t::set_received_qc_sig(const qc_sig_t& qc) { std::lock_guard g(*_mtx); - received_qc_sig = qc; + const bool qc_is_better = + !received_qc_sig + || (received_qc_sig->is_weak() && qc.is_strong()) + || received_qc_sig->vote_count() < qc.vote_count(); + + if (qc_is_better) { + received_qc_sig = qc; + return true; + } + return false; } bool open_qc_sig_t::received_qc_sig_is_strong() const { @@ -322,12 +336,14 @@ void open_qc_t::verify_qc(const qc_t& qc, const digest_type& strong_digest, cons } } -void open_qc_t::set_received_qc(const qc_t& qc) { - active_policy_sig.set_received_qc_sig(qc.active_policy_sig); +bool open_qc_t::set_received_qc(const qc_t& qc) { + bool active_better = active_policy_sig.set_received_qc_sig(qc.active_policy_sig); + bool pending_better = false; if (qc.pending_policy_sig) { assert(pending_policy_sig); - pending_policy_sig->set_received_qc_sig(*qc.pending_policy_sig); + pending_better = pending_policy_sig->set_received_qc_sig(*qc.pending_policy_sig); } + return active_better || pending_better; } bool open_qc_t::received_qc_is_strong() const { diff --git a/libraries/chain/include/eosio/chain/block_state.hpp b/libraries/chain/include/eosio/chain/block_state.hpp index a691b3d866..3ecb6efb1d 100644 --- a/libraries/chain/include/eosio/chain/block_state.hpp +++ b/libraries/chain/include/eosio/chain/block_state.hpp @@ -111,8 +111,9 @@ struct block_state : public block_header_state { // block_header_state provi uint32_t final_on_strong_qc_block_num() const { return core.final_on_strong_qc_block_num; } std::optional get_best_qc() const { return open_qc.get_best_qc(block_num()); } // thread safe - bool valid_qc_is_strong() const { return open_qc.received_qc_is_strong(); } // thread safe - void set_valid_qc(const qc_t& qc) { open_qc.set_received_qc(qc); } + bool received_qc_is_strong() const { return open_qc.received_qc_is_strong(); } // thread safe + // return true if better qc, thread safe + bool set_received_qc(const qc_t& qc) { return open_qc.set_received_qc(qc); } // extract the qc_claim from block header finality_extension qc_claim_t extract_qc_claim() const; diff --git a/libraries/chain/include/eosio/chain/finality/qc.hpp b/libraries/chain/include/eosio/chain/finality/qc.hpp index 8e58f85d8c..27a6e24ed5 100644 --- a/libraries/chain/include/eosio/chain/finality/qc.hpp +++ b/libraries/chain/include/eosio/chain/finality/qc.hpp @@ -53,6 +53,7 @@ namespace eosio::chain { struct qc_sig_t { bool is_weak() const { return !!weak_votes; } bool is_strong() const { return !weak_votes; } + size_t vote_count() const; std::optional strong_votes; std::optional weak_votes; @@ -72,10 +73,7 @@ namespace eosio::chain { bool is_strong() const { return active_policy_sig.is_strong() && (!pending_policy_sig || pending_policy_sig->is_strong()); } - - bool is_weak() const { - return active_policy_sig.is_weak() || (pending_policy_sig && pending_policy_sig->is_weak()); - } + bool is_weak() const { return !is_strong(); } qc_claim_t to_qc_claim() const { return { .block_num = block_num, .is_strong_qc = is_strong() }; @@ -170,7 +168,8 @@ namespace eosio::chain { state_t state() const { std::lock_guard g(*_mtx); return pending_state; }; std::optional get_best_qc() const; - void set_received_qc_sig(const qc_sig_t& qc); + // return true if better qc + bool set_received_qc_sig(const qc_sig_t& qc); bool received_qc_sig_is_strong() const; private: friend struct fc::reflector; @@ -229,7 +228,8 @@ namespace eosio::chain { qc_vote_metrics_t vote_metrics(const qc_t& qc) const; // return qc missing vote's finalizers std::set missing_votes(const qc_t& qc) const; - void set_received_qc(const qc_t& qc); + // return true if better qc + bool set_received_qc(const qc_t& qc); bool received_qc_is_strong() const; vote_status aggregate_vote(uint32_t connection_id, const vote_message& vote, block_num_type block_num, std::span finalizer_digest); diff --git a/libraries/libfc/include/fc/bitutil.hpp b/libraries/libfc/include/fc/bitutil.hpp index 067594ac31..f4bbceabe8 100644 --- a/libraries/libfc/include/fc/bitutil.hpp +++ b/libraries/libfc/include/fc/bitutil.hpp @@ -1,6 +1,11 @@ #pragma once +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wshift-count-overflow" +// boost dynamic_bitset count() generates warning #include +#pragma GCC diagnostic pop + #include namespace fc {