From 0052c0882fe28b96c43e087244cba2029d5a3dbd Mon Sep 17 00:00:00 2001 From: Jakub Fornadel Date: Thu, 11 Jul 2024 12:10:36 -0700 Subject: [PATCH] adjust pillar votes period --- .../consensus/include/pbft/pbft_manager.hpp | 2 +- .../pillar_chain/pillar_chain_manager.hpp | 6 ++-- .../consensus/src/pbft/pbft_manager.cpp | 29 +++++++++++---- .../src/pillar_chain/pillar_chain_manager.cpp | 36 +++++++++---------- ...get_pillar_votes_bundle_packet_handler.cpp | 3 +- 5 files changed, 45 insertions(+), 31 deletions(-) diff --git a/libraries/core_libs/consensus/include/pbft/pbft_manager.hpp b/libraries/core_libs/consensus/include/pbft/pbft_manager.hpp index a93dbdd046..7b607a009c 100644 --- a/libraries/core_libs/consensus/include/pbft/pbft_manager.hpp +++ b/libraries/core_libs/consensus/include/pbft/pbft_manager.hpp @@ -598,7 +598,7 @@ class PbftManager { bool already_next_voted_null_block_hash_ = false; bool go_finish_state_ = false; bool loop_back_finish_state_ = false; - bool already_placed_pillar_vote_ = false; + bool last_placed_pillar_vote_period_ = 0; // Used to avoid cyclic logging in voting steps that are called repeatedly bool printSecondFinishStepInfo_ = true; diff --git a/libraries/core_libs/consensus/include/pillar_chain/pillar_chain_manager.hpp b/libraries/core_libs/consensus/include/pillar_chain/pillar_chain_manager.hpp index fc796f06c9..e99a16ec85 100644 --- a/libraries/core_libs/consensus/include/pillar_chain/pillar_chain_manager.hpp +++ b/libraries/core_libs/consensus/include/pillar_chain/pillar_chain_manager.hpp @@ -57,10 +57,10 @@ class PillarChainManager { * @param pillar_block_hash * @param node_sk * @param broadcast_vote - * @return true if vote placed, otherwise false + * @return vote if it was created, otherwise nullptr */ - bool genAndPlacePillarVote(PbftPeriod period, const blk_hash_t& pillar_block_hash, const secret_t& node_sk, - bool broadcast_vote); + std::shared_ptr genAndPlacePillarVote(PbftPeriod period, const blk_hash_t& pillar_block_hash, + const secret_t& node_sk, bool broadcast_vote); /** * @brief Set network as a weak pointer diff --git a/libraries/core_libs/consensus/src/pbft/pbft_manager.cpp b/libraries/core_libs/consensus/src/pbft/pbft_manager.cpp index 6318734496..743f8e1af9 100644 --- a/libraries/core_libs/consensus/src/pbft/pbft_manager.cpp +++ b/libraries/core_libs/consensus/src/pbft/pbft_manager.cpp @@ -307,7 +307,6 @@ bool PbftManager::advancePeriod() { resetPbftConsensus(1 /* round */); broadcast_reward_votes_counter_ = 1; rebroadcast_reward_votes_counter_ = 1; - already_placed_pillar_vote_ = false; current_period_start_datetime_ = std::chrono::system_clock::now(); const auto new_period = getPbftPeriod(); @@ -738,6 +737,18 @@ bool PbftManager::placeVote(const std::shared_ptr &vote, std::string_v << ", vote weight " << *vote->getWeight() << ", period " << period << ", round " << vote->getRound() << ", step " << vote->getStep(); + // In case it is pbft with pillar block period and we have not voted yet, place a pillar vote + // Pillar vote can be placed during any pbft step + if (kGenesisConfig.state.hardforks.ficus_hf.isPbftWithPillarBlockPeriod(period) && + last_placed_pillar_vote_period_ < period) { + // No need to check presence of extra data and pillar block hash - this was already validated in validatePbftBlock + const auto pillar_block_hash = voted_block->getExtraData()->getPillarBlockHash(); + const auto pillar_vote = pillar_chain_mgr_->genAndPlacePillarVote(period, *pillar_block_hash, node_sk_, true); + if (pillar_vote) { + last_placed_pillar_vote_period_ = pillar_vote->getPeriod(); + } + } + return true; } @@ -1682,7 +1693,13 @@ void PbftManager::finalize_(PeriodData &&period_data, std::vector &&finali // already finalized if (final_chain_->dpos_is_eligible(period, node_addr_)) { if (pillar_block) { - pillar_chain_mgr_->genAndPlacePillarVote(period, pillar_block->getHash(), node_sk_, periodDataQueueEmpty()); + // Pillar votes are created in the next period (thus period + 1), this is optimization to create & broadcast it + // a bit faster + const auto pillar_vote = pillar_chain_mgr_->genAndPlacePillarVote(period + 1, pillar_block->getHash(), node_sk_, + periodDataQueueEmpty()); + if (pillar_vote) { + last_placed_pillar_vote_period_ = pillar_vote->getPeriod(); + } } else { LOG(log_er_) << "Unable to vote for pillar block with period " << period << ". Block was not created"; } @@ -1713,7 +1730,7 @@ bool PbftManager::pushPbftBlock_(PeriodData &&period_data, std::vectorgetBlockHash() << ", period " << pbft_period << ". Not enough pillar votes for pillar block " << *pillar_block_hash << ". Request it"; if (auto net = network_.lock()) { - net->requestPillarBlockVotesBundle(pbft_period - 1, *pillar_block_hash); + net->requestPillarBlockVotesBundle(pbft_period, *pillar_block_hash); } return false; @@ -2015,12 +2032,12 @@ bool PbftManager::validatePbftBlockPillarVotes(const PeriodData &period_data) co } const auto &pbft_block_hash = period_data.pbft_blk->getBlockHash(); - const auto required_votes_period = period_data.pbft_blk->getPeriod() - 1; + const auto required_votes_period = period_data.pbft_blk->getPeriod(); const auto current_pillar_block = pillar_chain_mgr_->getCurrentPillarBlock(); - if (current_pillar_block->getPeriod() != required_votes_period) { + if (current_pillar_block->getPeriod() + 1 != required_votes_period) { LOG(log_er_) << "Sync pillar votes required period " << required_votes_period << " != " - << " current pillar block period " << current_pillar_block->getPeriod(); + << " current pillar block period " << current_pillar_block->getPeriod() << " + 1"; return false; } diff --git a/libraries/core_libs/consensus/src/pillar_chain/pillar_chain_manager.cpp b/libraries/core_libs/consensus/src/pillar_chain/pillar_chain_manager.cpp index a9e51653c9..0301ded0e3 100644 --- a/libraries/core_libs/consensus/src/pillar_chain/pillar_chain_manager.cpp +++ b/libraries/core_libs/consensus/src/pillar_chain/pillar_chain_manager.cpp @@ -117,8 +117,9 @@ void PillarChainManager::saveNewPillarBlock(std::shared_ptr pillar_ current_pillar_block_vote_counts_ = std::move(data.vote_counts); } -bool PillarChainManager::genAndPlacePillarVote(PbftPeriod period, const blk_hash_t& pillar_block_hash, - const secret_t& node_sk, bool broadcast_vote) { +std::shared_ptr PillarChainManager::genAndPlacePillarVote(PbftPeriod period, + const blk_hash_t& pillar_block_hash, + const secret_t& node_sk, bool broadcast_vote) { const auto vote = std::make_shared(node_sk, period, pillar_block_hash); // Broadcasts pillar vote @@ -126,7 +127,7 @@ bool PillarChainManager::genAndPlacePillarVote(PbftPeriod period, const blk_hash if (!vote_weight) { LOG(log_er_) << "Unable to gen pillar vote. Vote was not added to the verified votes. Vote hash " << vote->getHash(); - return false; + return nullptr; } db_->saveOwnPillarBlockVote(vote); @@ -139,7 +140,7 @@ bool PillarChainManager::genAndPlacePillarVote(PbftPeriod period, const blk_hash << vote->getPeriod() << ", weight " << vote_weight; } - return true; + return vote; } bool PillarChainManager::finalizePillarBlock(const std::shared_ptr& pillar_block) { @@ -153,7 +154,8 @@ bool PillarChainManager::finalizePillarBlock(const std::shared_ptr& return false; } - auto above_threshold_votes = pillar_votes_.getVerifiedVotes(pillar_block->getPeriod(), pillar_block->getHash(), true); + auto above_threshold_votes = + pillar_votes_.getVerifiedVotes(pillar_block->getPeriod() + 1, pillar_block->getHash(), true); if (above_threshold_votes.empty()) { LOG(log_tr_) << "Unable to finalize pillar block " << pillar_block->getHash() << ", period: " << pillar_block->getPeriod() << ". Not enough votes"; @@ -171,7 +173,7 @@ bool PillarChainManager::finalizePillarBlock(const std::shared_ptr& last_finalized_pillar_block_ = pillar_block; // Erase votes that are no longer needed - pillar_votes_.eraseVotes(last_finalized_pillar_block_->getPeriod()); + pillar_votes_.eraseVotes(last_finalized_pillar_block_->getPeriod() + 1); } pillar_block_finalized_emitter_.emit(pillar_block_data); @@ -207,19 +209,19 @@ bool PillarChainManager::isRelevantPillarVote(const std::shared_ptr // Empty current_pillar_block_ means there was no pillar block created yet at all if (!current_pillar_block) [[unlikely]] { // It could happen that other nodes created pillar block and voted for it before this node - if (vote->getPeriod() != kFicusHfConfig.firstPillarBlockPeriod()) { + if (vote->getPeriod() != kFicusHfConfig.firstPillarBlockPeriod() + 1) { LOG(log_nf_) << "Received vote's period " << vote_period << ", no pillar block created yet. Accepting votes with " - << kFicusHfConfig.firstPillarBlockPeriod() << " period"; + << kFicusHfConfig.firstPillarBlockPeriod() + 1 << " period"; return false; } - } else if (vote_period == current_pillar_block->getPeriod()) [[likely]] { + } else if (vote_period == current_pillar_block->getPeriod() + 1) [[likely]] { if (vote->getBlockHash() != current_pillar_block->getHash()) { LOG(log_nf_) << "Received vote's block hash " << vote->getBlockHash() << " != current pillar block hash " << current_pillar_block->getHash(); return false; } - } else if (vote_period != - current_pillar_block->getPeriod() + kFicusHfConfig.pillar_blocks_interval /* +1 future pillar block */) { + } else if (vote_period != current_pillar_block->getPeriod() + kFicusHfConfig.pillar_blocks_interval + + 1 /* future pillar block votes */) { LOG(log_nf_) << "Received vote's period " << vote_period << ", current pillar block period " << current_pillar_block->getPeriod(); return false; @@ -245,9 +247,7 @@ bool PillarChainManager::validatePillarVote(const std::shared_ptr vo // Check if signer is eligible validator try { - // Note: period is used instead of period - 1 because pillar votes are created only after pbft block with - // is finalized - if (!final_chain_->dpos_is_eligible(period, validator)) { + if (!final_chain_->dpos_is_eligible(period - 1, validator)) { LOG(log_er_) << "Validator is not eligible. Pillar vote " << vote->getHash(); return false; } @@ -268,9 +268,7 @@ bool PillarChainManager::validatePillarVote(const std::shared_ptr vo uint64_t PillarChainManager::addVerifiedPillarVote(const std::shared_ptr& vote) { uint64_t validator_vote_count = 0; try { - // Note: period is used instead of period - 1 because pillar votes are created only after pbft block with - // is finalized - validator_vote_count = final_chain_->dpos_eligible_vote_count(vote->getPeriod(), vote->getVoterAddr()); + validator_vote_count = final_chain_->dpos_eligible_vote_count(vote->getPeriod() - 1, vote->getVoterAddr()); } catch (state_api::ErrFutureBlock& e) { LOG(log_er_) << "Pillar vote " << vote->getHash() << " with period " << vote->getPeriod() << " is too far ahead of DPOS. " << e.what(); @@ -284,9 +282,7 @@ uint64_t PillarChainManager::addVerifiedPillarVote(const std::shared_ptrgetPeriod())) { - // Note: period is used instead of period - 1 because pillar votes are created only after pbft block with - // is finalized - const auto threshold = getPillarConsensusThreshold(vote->getPeriod()); + const auto threshold = getPillarConsensusThreshold(vote->getPeriod() - 1); if (!threshold) { LOG(log_er_) << "Unable to get pillar consensus threshold for period " << vote->getPeriod(); return 0; diff --git a/libraries/core_libs/network/src/tarcap/packets_handlers/latest/get_pillar_votes_bundle_packet_handler.cpp b/libraries/core_libs/network/src/tarcap/packets_handlers/latest/get_pillar_votes_bundle_packet_handler.cpp index 14a359a413..8c3872e758 100644 --- a/libraries/core_libs/network/src/tarcap/packets_handlers/latest/get_pillar_votes_bundle_packet_handler.cpp +++ b/libraries/core_libs/network/src/tarcap/packets_handlers/latest/get_pillar_votes_bundle_packet_handler.cpp @@ -53,7 +53,8 @@ void GetPillarVotesBundlePacketHandler::process(const threadpool::PacketData &pa peer->markPillarVoteAsKnown(vote->getHash()); } - LOG(log_nf_) << "Pillar votes bundle sent to " << peer->getId(); + LOG(log_nf_) << "Pillar votes bundle for period " << period << " ans hash " << pillar_block_hash << " sent to " + << peer->getId(); } }