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

@@ -1117,32 +1117,43 @@ 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";
             return false;
         }
 
         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;

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);

pbft/test/pbft_viewchange_test.cpp

@@ -209,15 +209,13 @@ 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)
                 {
                     EXPECT_EQ(this->pbft->get_uuid(), viewchange_env->sender());
                     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<const bzn_envelope&>())).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";
+        }
     }
 }