From f761cc10be4ebe3a01cb39273cdebb93fb2ef69d Mon Sep 17 00:00:00 2001 From: Rich Nistuk Date: Fri, 14 Dec 2018 15:07:45 -0800 Subject: [PATCH] KEP-902 Fixed a exception that occurs in is_valid_viewchange_message when no checkpoint has been set - re-did the fix and made the unit test check that the exception is not thrown - snuck in a fix to compare the viewchange_message.sequence to the valid_checkpoint_sequence after discussion with Isabel --- pbft/pbft.cpp | 21 +++++++++++++---- pbft/pbft.hpp | 1 + pbft/test/pbft_viewchange_test.cpp | 38 ++++++++++++++++++++++++------ 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/pbft/pbft.cpp b/pbft/pbft.cpp index d19dbd29..e8829af1 100644 --- a/pbft/pbft.cpp +++ b/pbft/pbft.cpp @@ -1117,24 +1117,35 @@ pbft::is_valid_viewchange_message(const pbft_msg& viewchange_message, const bzn_ if (!(viewchange_message.view() > this->get_view())) { - LOG(error) << "is_valid_viewchange_message - new view " << viewchange_message.view() << " is not greater than current view " << this->get_view(); + LOG(error) << "is_valid_viewchange_message - new view " << viewchange_message.view() << " is not greater than current view " << this->get_view(); return false; } - auto valid_checkpoint_hashes = this->validate_and_extract_checkpoint_hashes(viewchange_message); - if (valid_checkpoint_hashes.empty() || viewchange_message.sequence() == 0) + auto valid_checkpoint_hashes{this->validate_and_extract_checkpoint_hashes(viewchange_message)}; + if (valid_checkpoint_hashes.empty() && viewchange_message.sequence() != 0) { LOG(error) << "is_valid_viewchange_message - the checkpoint is invalid"; return false; } - const auto valid_checkpoint = *(valid_checkpoint_hashes.begin()); + // KEP-902: If we do not have a valid checkpoint hash, then we must set the valid_checkpoint_sequence value to 0. + uint64_t valid_checkpoint_sequence = (valid_checkpoint_hashes.empty() ? 0 : valid_checkpoint_hashes.begin()->first.first); + + // Snuck into KEP-902: Isabel notes that viewchange messages should not have sequences, and this will be refactored + // out in the near future, but since we are using it we must make the comparison to the expected + // valid_checkpoint_sequence + if (viewchange_message.sequence() != valid_checkpoint_sequence) + { + LOG(error) << "is_valid_viewchange_message - the viewchange sequence: " << viewchange_message.sequence() << ", is different from the sequence that is expected: " << valid_checkpoint_sequence; + return false; + } // TODO: (see KEP-882) Refactor this block into a new method - Isabel: should extract a validate_prepared_proof method // all the the prepared proofs are valid (contains a pre prepare and 2f+1 mathcin prepares) for (int i{0}; i < viewchange_message.prepared_proofs_size(); ++i) { const bzn_envelope& pre_prepare_envelope{viewchange_message.prepared_proofs(i).pre_prepare()}; + if (!this->is_peer(pre_prepare_envelope.sender()) || !this->crypto->verify(pre_prepare_envelope)) { LOG(error) << "is_valid_viewchange_message - a pre prepare message has a bad envelope, or the sender is not in the peers list"; @@ -1142,7 +1153,7 @@ pbft::is_valid_viewchange_message(const pbft_msg& viewchange_message, const bzn_ } pbft_msg preprepare_message; - if (!preprepare_message.ParseFromString(pre_prepare_envelope.pbft()) || (preprepare_message.sequence() <= valid_checkpoint.first.first) || preprepare_message.type() != PBFT_MSG_PREPREPARE) + if (!preprepare_message.ParseFromString(pre_prepare_envelope.pbft()) || (preprepare_message.sequence() <= valid_checkpoint_sequence) || preprepare_message.type() != PBFT_MSG_PREPREPARE) { LOG(error) << "is_valid_viewchange_message - a pre prepare message has an invalid sequence number, or is malformed"; return false; diff --git a/pbft/pbft.hpp b/pbft/pbft.hpp index 7866e266..d8712cbb 100644 --- a/pbft/pbft.hpp +++ b/pbft/pbft.hpp @@ -273,6 +273,7 @@ namespace bzn FRIEND_TEST(pbft_viewchange_test, test_fill_in_missing_pre_prepares); FRIEND_TEST(pbft_viewchange_test, test_save_checkpoint); FRIEND_TEST(pbft_viewchange_test, test_handle_viewchange); + FRIEND_TEST(pbft_viewchange_test, is_valid_viewchange_does_not_throw_if_no_checkpoint_yet); FRIEND_TEST(pbft_newview_test, test_pre_prepares_contiguous); FRIEND_TEST(pbft_newview_test, make_newview); diff --git a/pbft/test/pbft_viewchange_test.cpp b/pbft/test/pbft_viewchange_test.cpp index b8e9c527..85c8853b 100644 --- a/pbft/test/pbft_viewchange_test.cpp +++ b/pbft/test/pbft_viewchange_test.cpp @@ -209,7 +209,6 @@ namespace bzn this->run_transaction_through_primary_times(2, current_sequence); - EXPECT_CALL(*mock_node, send_message(_, ResultOf(test::is_viewchange, Eq(true)))) .WillRepeatedly(Invoke([&](const auto & /*endpoint*/, const auto viewchange_env) { @@ -217,7 +216,6 @@ namespace bzn pbft_msg viewchange; EXPECT_TRUE(viewchange.ParseFromString(viewchange_env->pbft())); // this will be valid. EXPECT_TRUE(this->pbft->is_valid_viewchange_message(viewchange, *viewchange_env)); - })); this->pbft->handle_failure(); @@ -428,17 +426,43 @@ namespace bzn EXPECT_EQ(this->pbft->view, 2U); } - TEST_F(pbft_viewchange_test, is_valid_viewchange_fails_if_no_checkpoint_yet) + TEST_F(pbft_viewchange_test, is_valid_viewchange_does_not_throw_if_no_checkpoint_yet) { // This test was written for KEP-902: is_valid_viewchange throws if no checkpoint yet + uint64_t current_sequence{0}; auto mock_crypto = this->build_pft_with_mock_crypto(); bzn_envelope original_message; - EXPECT_CALL(*mock_node, send_message(_, ResultOf(test::is_viewchange, Eq(true)))).WillRepeatedly(Invoke([&](const auto & /*endpoint*/, const auto &viewchange_env) { original_message = *viewchange_env; })); + EXPECT_CALL(*mock_crypto, verify(_)).WillRepeatedly(Invoke([&](const bzn_envelope& /*msg*/) + {return true;})); + EXPECT_CALL(*mock_crypto, hash(An())).WillRepeatedly(Invoke([&](const bzn_envelope& envelope) + {return envelope.sender() + "_" + std::to_string(current_sequence) + "_" + std::to_string(envelope.timestamp());})); + EXPECT_CALL(*mock_node, send_message(_, ResultOf(test::is_viewchange, Eq(true)))).WillRepeatedly(Invoke([&](const auto & /*endpoint*/, const auto &viewchange_env) + { original_message = *viewchange_env; })); + + this->run_transaction_through_primary_times(1, current_sequence); this->pbft->handle_failure(); - pbft_msg msg; - msg.ParseFromString(original_message.pbft()); - EXPECT_FALSE(this->pbft->is_valid_viewchange_message(msg, original_message)); + auto latest_checkpoint{this->pbft->latest_checkpoint()}; + EXPECT_EQ(0U, latest_checkpoint.first); + EXPECT_EQ(INITIAL_CHECKPOINT_HASH, latest_checkpoint.second); + + pbft_msg viewchange_message; + viewchange_message.ParseFromString(original_message.pbft()); + + // It was the dereferencing of an empty valid_checkpoint_hashes that was causing is_valid_viewchange_message to throw an exception + auto valid_checkpoint_hashes{this->pbft->validate_and_extract_checkpoint_hashes(viewchange_message)}; + // ensure that the valid_checkpoint_hashes is indeed empty + EXPECT_TRUE(valid_checkpoint_hashes.empty()); + + try + { + // even though there is no checkpoint, it is still valid to request a view change, and thus possible to have a valid viewchange message + EXPECT_TRUE(this->pbft->is_valid_viewchange_message(viewchange_message, original_message)); + } + catch(...) + { + FAIL() << "is_valid_viewchange_message must not throw and exception simply because there are no valid checkpoints"; + } } }