KEP-1202: update next_sequence number from newview

pbft/pbft.cpp

@@ -1361,8 +1361,8 @@ pbft::is_valid_newview_message(const pbft_msg& theirs, const bzn_envelope& origi
         viewchange_envelopes_from_senders[viewchange_env.sender()] = viewchange_env;
     }
 
-    pbft_msg ours = this->build_newview(theirs.view(), viewchange_envelopes_from_senders).first;
-    if (ours.pre_prepare_messages_size() != theirs.pre_prepare_messages_size() )
+    pbft_msg ours = this->build_newview(theirs.view(), viewchange_envelopes_from_senders);
+    if (ours.pre_prepare_messages_size() != theirs.pre_prepare_messages_size())
     {
         LOG(error) << "is_valid_newview_message - expected " << ours.pre_prepare_messages_size()
             << " preprepares in new view, found " << theirs.pre_prepare_messages_size();
@@ -1459,7 +1459,7 @@ pbft::make_newview(
     return newview;
 }
 
-std::pair<pbft_msg, uint64_t>
+pbft_msg
 pbft::build_newview(uint64_t new_view, const std::map<uuid_t,bzn_envelope>& viewchange_envelopes_from_senders) const
 {
     //  Computing O (set of new preprepares for new-view message)
@@ -1472,7 +1472,6 @@ pbft::build_newview(uint64_t new_view, const std::map<uuid_t,bzn_envelope>& view
         viewchange.ParseFromString(sender_viewchange_envelope.second.pbft());
         max_checkpoint_sequence = std::max(max_checkpoint_sequence, viewchange.sequence());
     }
-    uint64_t next_seq = max_checkpoint_sequence + 1;
 
     //  - for each of the 2f+1 viewchange messages
     for (const auto& sender_viewchange_envelope : viewchange_envelopes_from_senders)
@@ -1504,12 +1503,11 @@ pbft::build_newview(uint64_t new_view, const std::map<uuid_t,bzn_envelope>& view
                 continue;
             }
             pre_prepares[pre_prepare.sequence()] = this->wrap_message(pre_prepare);
-            next_seq = std::max(next_seq, pre_prepare.sequence() + 1);
         }
     }
     this->fill_in_missing_pre_prepares(max_checkpoint_sequence, new_view, pre_prepares);
 
-    return std::make_pair(this->make_newview(new_view, viewchange_envelopes_from_senders, pre_prepares), next_seq);
+    return this->make_newview(new_view, viewchange_envelopes_from_senders, pre_prepares);
 }
 
 void
@@ -1574,11 +1572,10 @@ pbft::handle_viewchange(const pbft_msg &msg, const bzn_envelope &original_msg)
         viewchange_envelopes_from_senders[sender] = viewchange_envelope;
     }
 
-    auto res = this->build_newview(viewchange->first, viewchange_envelopes_from_senders);
-    this->next_issued_sequence_number = res.second;
-    this->move_to_new_configuration(res.first.config_hash(), this->view.value() + 1);
+    auto newview_msg = this->build_newview(viewchange->first, viewchange_envelopes_from_senders);
+    this->move_to_new_configuration(newview_msg.config_hash(), this->view.value() + 1);
     LOG(debug) << "Sending NEWVIEW for view " << this->view.value() + 1;
-    this->broadcast(this->wrap_message(res.first));
+    this->broadcast(this->wrap_message(newview_msg));
 }
 
 void
@@ -1610,11 +1607,6 @@ pbft::handle_newview(const pbft_msg& msg, const bzn_envelope& original_msg)
             LOG (debug) << "handle_newview - ignoring invalid NEWVIEW message while waiting to join swarm";
             return;
         }
-
-        // adopt checkpoint in newview message
-        pbft_msg viewchange;
-        viewchange.ParseFromString(msg.viewchange_messages(0).pbft());
-        this->save_checkpoint(viewchange);
         this->in_swarm = swarm_status::joined;
     }
     else if (!this->is_valid_newview_message(msg, original_msg))
@@ -1623,7 +1615,16 @@ pbft::handle_newview(const pbft_msg& msg, const bzn_envelope& original_msg)
         return;
     }
 
-    LOG(debug) << "handle_newview - recieved valid NEWVIEW message";
+    pbft_msg viewchange;
+    viewchange.ParseFromString(msg.viewchange_messages(0).pbft());
+
+    // this is redundant (but harmless) unless it's a new node
+    this->save_checkpoint(viewchange);
+
+    // initially set next sequence from viewchange then update from preprepares later
+    this->next_issued_sequence_number = viewchange.sequence() + 1;
+
+    LOG(debug) << "handle_newview - received valid NEWVIEW message";
 
     // validate requested configuration and switch to it
     if (!this->is_configuration_acceptable_in_new_view(msg.config_hash()) ||
@@ -1645,6 +1646,8 @@ pbft::handle_newview(const pbft_msg& msg, const bzn_envelope& original_msg)
         if (msg2.ParseFromString(original_msg2.pbft()))
         {
             this->handle_preprepare(msg2, original_msg2);
+
+            this->next_issued_sequence_number = msg2.sequence() + 1;
         }
     }
 

pbft/pbft.hpp

@@ -229,7 +229,7 @@ namespace bzn
         void initiate_viewchange();
         pbft_msg make_viewchange(uint64_t new_view, uint64_t n, const std::unordered_map<bzn::uuid_t, persistent<std::string>>& stable_checkpoint_proof, const std::map<uint64_t, std::shared_ptr<bzn::pbft_operation>>& prepared_operations);
         pbft_msg make_newview(uint64_t new_view_index,  const std::map<uuid_t,bzn_envelope>& viewchange_envelopes_from_senders, const std::map<uint64_t, bzn_envelope>& pre_prepare_messages) const;
-        std::pair<pbft_msg, uint64_t> build_newview(uint64_t new_view, const std::map<uuid_t,bzn_envelope>& viewchange_envelopes_from_senders) const;
+        pbft_msg build_newview(uint64_t new_view, const std::map<uuid_t,bzn_envelope>& viewchange_envelopes_from_senders) const;
         std::map<bzn::checkpoint_t , std::set<bzn::uuid_t>> validate_and_extract_checkpoint_hashes(const pbft_msg &viewchange_message) const;
         void save_checkpoint(const pbft_msg& msg);
         void fill_in_missing_pre_prepares(uint64_t max_checkpoint_sequence, uint64_t new_view, std::map<uint64_t, bzn_envelope>& pre_prepares) const;

pbft/test/pbft_viewchange_test.cpp

@@ -421,7 +421,6 @@ namespace bzn
         this->pbft->handle_failure();
 
         EXPECT_EQ(this->pbft->view.value(), 2U);
-        EXPECT_EQ(pbft2->next_issued_sequence_number.value(), 103U);
     }
 
     TEST_F(pbft_viewchange_test, is_valid_viewchange_does_not_throw_if_no_checkpoint_yet)