From 0bb2322f96f194a1b15c444e7d1e98ee47554dd0 Mon Sep 17 00:00:00 2001 From: Paul Archard Date: Thu, 7 Mar 2019 13:38:16 -0800 Subject: [PATCH] KEP-1202: update next_sequence number from newview --- pbft/pbft.cpp | 35 ++++++++++++++++-------------- pbft/pbft.hpp | 2 +- pbft/test/pbft_viewchange_test.cpp | 1 - 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/pbft/pbft.cpp b/pbft/pbft.cpp index ecf9ead3..584ab190 100644 --- a/pbft/pbft.cpp +++ b/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 pbft::build_newview(uint64_t new_view, const std::map& 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& 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& 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; } } diff --git a/pbft/pbft.hpp b/pbft/pbft.hpp index 4de39456..6ac8705d 100644 --- a/pbft/pbft.hpp +++ b/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>& stable_checkpoint_proof, const std::map>& prepared_operations); pbft_msg make_newview(uint64_t new_view_index, const std::map& viewchange_envelopes_from_senders, const std::map& pre_prepare_messages) const; - std::pair build_newview(uint64_t new_view, const std::map& viewchange_envelopes_from_senders) const; + pbft_msg build_newview(uint64_t new_view, const std::map& viewchange_envelopes_from_senders) const; std::map> 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& pre_prepares) const; diff --git a/pbft/test/pbft_viewchange_test.cpp b/pbft/test/pbft_viewchange_test.cpp index 6f1170f1..3c09b955 100644 --- a/pbft/test/pbft_viewchange_test.cpp +++ b/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)