Merge pull request #2398 from Taraxa-project/votes_broadcast_on_period

chore: rebroadcast votes based on period not round

libraries/core_libs/consensus/include/pbft/pbft_manager.hpp

@@ -274,10 +274,16 @@ class PbftManager : public std::enable_shared_from_this<PbftManager> {
   size_t getPbftCommitteeSize() const { return config_.committee_size; }
 
   /**
-   * @brief Broadcast or rebroadcast current round soft votes, previous round next votes and reward votes
+   * @brief Broadcast or rebroadcast current round soft votes and previous round next votes
    * @param rebroadcast
    */
-  void broadcastVotes(bool rebroadcast);
+  void broadcastSoftAndNextVotes(bool rebroadcast);
+
+  /**
+   * @brief Broadcast or rebroadcast reward votes
+   * @param rebroadcast
+   */
+  void broadcastRewardVotes(bool rebroadcast);
 
  private:
   /**
@@ -554,8 +560,10 @@ class PbftManager : public std::enable_shared_from_this<PbftManager> {
 
   const uint32_t kBroadcastVotesLambdaTime = 20;
   const uint32_t kRebroadcastVotesLambdaTime = 60;
-  uint32_t broadcast_votes_counter_ = 1;
-  uint32_t rebroadcast_votes_counter_ = 1;
+  uint32_t broadcast_soft_next_votes_counter_ = 1;
+  uint32_t rebroadcast_soft_next_votes_counter_ = 1;
+  uint32_t broadcast_reward_votes_counter_ = 1;
+  uint32_t rebroadcast_reward_votes_counter_ = 1;
 
   std::default_random_engine random_engine_{std::random_device{}()};
 
@@ -571,6 +579,7 @@ class PbftManager : public std::enable_shared_from_this<PbftManager> {
   std::map<blk_hash_t, std::vector<PbftStep>> current_round_broadcasted_votes_;
 
   time_point current_round_start_datetime_;
+  time_point current_period_start_datetime_;
   time_point second_finish_step_start_datetime_;
   std::chrono::milliseconds next_step_time_ms_{0};
 

libraries/core_libs/consensus/src/pbft/pbft_manager.cpp

@@ -345,6 +345,9 @@ bool PbftManager::tryPushCertVotesBlock() {
 
 bool PbftManager::advancePeriod() {
   resetPbftConsensus(1 /* round */);
+  broadcast_reward_votes_counter_ = 1;
+  rebroadcast_reward_votes_counter_ = 1;
+  current_period_start_datetime_ = std::chrono::system_clock::now();
 
   const auto new_period = getPbftPeriod();
 
@@ -389,8 +392,8 @@ void PbftManager::resetPbftConsensus(PbftRound round) {
   LOG(log_dg_) << "Reset PBFT consensus to: period " << getPbftPeriod() << ", round " << round << ", step 1";
 
   // Reset broadcast counters
-  broadcast_votes_counter_ = 1;
-  rebroadcast_votes_counter_ = 1;
+  broadcast_soft_next_votes_counter_ = 1;
+  rebroadcast_soft_next_votes_counter_ = 1;
 
   // Update current round and reset step to 1
   round_ = round;
@@ -517,6 +520,7 @@ void PbftManager::initialState() {
   already_next_voted_null_block_hash_ = db_->getPbftMgrStatus(PbftMgrStatus::NextVotedNullBlockHash);
 
   current_round_start_datetime_ = now;
+  current_period_start_datetime_ = now;
   next_step_time_ms_ = std::chrono::milliseconds(0);
 
   // Set current period & round in vote manager
@@ -615,7 +619,7 @@ void PbftManager::loopBackFinishState_() {
   printVotingSummary();
 }
 
-void PbftManager::broadcastVotes(bool rebroadcast) {
+void PbftManager::broadcastSoftAndNextVotes(bool rebroadcast) {
   auto net = network_.lock();
   if (!net) {
     return;
@@ -631,14 +635,6 @@ void PbftManager::broadcastVotes(bool rebroadcast) {
                                                                                              rebroadcast);
   }
 
-  // Broadcast reward votes - previous round 2t+1 cert votes
-  auto reward_votes = vote_mgr_->getProposeRewardVotes();
-  if (!reward_votes.empty()) {
-    LOG(log_dg_) << "Broadcast propose reward votes for period " << period << ", round " << round;
-    net->getSpecificHandler<network::tarcap::VotesSyncPacketHandler>()->onNewPbftVotesBundle(std::move(reward_votes),
-                                                                                             rebroadcast);
-  }
-
   // Broadcast previous round 2t+1 next votes
   if (round > 1) {
     if (auto next_votes = vote_mgr_->getAllTwoTPlusOneNextVotes(period, round - 1); !next_votes.empty()) {
@@ -649,6 +645,23 @@ void PbftManager::broadcastVotes(bool rebroadcast) {
   }
 }
 
+void PbftManager::broadcastRewardVotes(bool rebroadcast) {
+  auto net = network_.lock();
+  if (!net) {
+    return;
+  }
+
+  auto [round, period] = getPbftRoundAndPeriod();
+
+  // Broadcast reward votes - previous round 2t+1 cert votes
+  auto reward_votes = vote_mgr_->getProposeRewardVotes();
+  if (!reward_votes.empty()) {
+    LOG(log_dg_) << "Broadcast propose reward votes for period " << period << ", round " << round;
+    net->getSpecificHandler<network::tarcap::VotesSyncPacketHandler>()->onNewPbftVotesBundle(std::move(reward_votes),
+                                                                                             rebroadcast);
+  }
+}
+
 void PbftManager::printVotingSummary() const {
   const auto [round, period] = getPbftRoundAndPeriod();
   Json::Value json_obj;
@@ -674,15 +687,27 @@ bool PbftManager::stateOperations_() {
   pushSyncedPbftBlocksIntoChain();
 
   const auto round_elapsed_time = elapsedTimeInMs(current_round_start_datetime_);
+  const auto period_elapsed_time = elapsedTimeInMs(current_period_start_datetime_);
+
+  if (round_elapsed_time / LAMBDA_ms_MIN > kRebroadcastVotesLambdaTime * rebroadcast_soft_next_votes_counter_) {
+    broadcastSoftAndNextVotes(true);
+    rebroadcast_soft_next_votes_counter_++;
+    // If there was a rebroadcast no need to do next broadcast either
+    broadcast_soft_next_votes_counter_++;
+  } else if (round_elapsed_time / LAMBDA_ms_MIN > kBroadcastVotesLambdaTime * broadcast_soft_next_votes_counter_) {
+    broadcastSoftAndNextVotes(false);
+    broadcast_soft_next_votes_counter_++;
+  }
 
-  if (round_elapsed_time / LAMBDA_ms_MIN > kRebroadcastVotesLambdaTime * rebroadcast_votes_counter_) {
-    broadcastVotes(true);
-    rebroadcast_votes_counter_++;
+  // Reward votes need to be broadcast even if we are advancing rounds but unable to advance a period
+  if (period_elapsed_time / LAMBDA_ms_MIN > kRebroadcastVotesLambdaTime * rebroadcast_reward_votes_counter_) {
+    broadcastRewardVotes(true);
+    rebroadcast_reward_votes_counter_++;
     // If there was a rebroadcast no need to do next broadcast either
-    broadcast_votes_counter_++;
-  } else if (round_elapsed_time / LAMBDA_ms_MIN > kBroadcastVotesLambdaTime * broadcast_votes_counter_) {
-    broadcastVotes(false);
-    broadcast_votes_counter_++;
+    broadcast_reward_votes_counter_++;
+  } else if (period_elapsed_time / LAMBDA_ms_MIN > kBroadcastVotesLambdaTime * broadcast_reward_votes_counter_) {
+    broadcastRewardVotes(false);
+    broadcast_reward_votes_counter_++;
   }
 
   auto [round, period] = getPbftRoundAndPeriod();

tests/network_test.cpp

@@ -878,7 +878,7 @@ TEST_F(NetworkTest, pbft_next_votes_sync_in_same_round) {
   node2->getPbftManager()->setPbftRound(2);
 
   // Node 1 broadcast his votes
-  node1_pbft_mgr->broadcastVotes(false);
+  node1_pbft_mgr->broadcastSoftAndNextVotes(false);
   // Node 2 should receive votes from node 1, node 1 has its own 2 votes
   EXPECT_EQ(node1_vote_mgr->getVerifiedVotesSize(), 2);
   EXPECT_HAPPENS({5s, 100ms}, [&](auto& ctx) { WAIT_EXPECT_EQ(ctx, node2_vote_mgr->getVerifiedVotesSize(), 3) });