KEP-823 Review changes to

node/test/session_test.cpp

@@ -194,7 +194,7 @@ namespace bzn
         io2->shutdown();
     }
 
-    TEST_F(session_test2, DISABLED_additional_shutdown_handlers_can_be_added_to_session)
+    TEST_F(session_test2, additional_shutdown_handlers_can_be_added_to_session)
     {
         auto io2 = std::make_shared<bzn::asio::smart_mock_io>();
 

pbft/pbft.cpp

@@ -353,31 +353,32 @@ pbft::forward_request_to_primary(const bzn_envelope& request_env)
 }
 
 
+
+//void
+//pbft::maybe_record_internal_request(const bzn_envelope &request_env, const std::shared_ptr<pbft_operation> &op)
+//{
+//    pbft_msg msg;
+//    msg.ParseFromString(request_env.pbft());
+//
+//    if (this->crypto->hash(request_env) != msg.request_hash())
+//    {
+//        LOG(info) << "Not recording request because hashes do not match";
+//        return;
+//    }
+//    op->record_request(request_env);
+//
+//}
+
+
 void
-pbft::maybe_record_request(const pbft_msg& msg, const bzn_envelope& env, const std::shared_ptr<pbft_operation>& op)
+pbft::maybe_record_request(const bzn_envelope &request_env, const std::shared_ptr<pbft_operation> &op)
 {
-    if (!op->has_request())
+    if (request_env.payload_case() != bzn_envelope::PayloadCase::PAYLOAD_NOT_SET &&  this->crypto->hash(request_env) != op->get_request_hash())
     {
-        if (msg.request_type() == PBFT_REQUEST_PAYLOAD)
-        {
-            assert(env.piggybacked_requests_size()); // we must have at least 1 request
-            if (this->crypto->hash(env.piggybacked_requests(0)) != msg.request_hash())
-            {
-                LOG(info) << "Not recording request because hashes do not match";
-                return;
-            }
-            op->record_request(env.piggybacked_requests(0));
-        }
-        else
-        {
-            if (this->crypto->hash(msg.request()) != msg.request_hash())
-            {
-                LOG(info) << "Not recording request because hashes do not match";
-                return;
-            }
-            op->record_request(msg.request());
-        }
+        LOG(info) << "Not recording request because hashes do not match";
+        return;
     }
+    op->record_request(request_env);
 }
 
 
@@ -402,7 +403,12 @@ pbft::handle_preprepare(const pbft_msg& msg, const bzn_envelope& original_msg)
         auto env_copy{original_msg};
         env_copy.clear_piggybacked_requests();
         op->record_pbft_msg(msg, env_copy);
-        this->maybe_record_request(msg, original_msg, op);
+
+        this->maybe_record_request(msg.request(), op);
+        if (original_msg.piggybacked_requests_size())
+        {
+            this->maybe_record_request(original_msg.piggybacked_requests(0), op);
+        }
 
         // This assignment will be redundant if we've seen this preprepare before, but that's fine
         accepted_preprepares[log_key] = persistent<bzn::operation_key_t>{this->storage, op->get_operation_key()

pbft/pbft.hpp

@@ -209,7 +209,7 @@ namespace bzn
         bool move_to_new_configuration(const hash_t& config_hash, uint64_t view);
         bool proposed_config_is_acceptable(std::shared_ptr<pbft_configuration> config);
 
-        void maybe_record_request(const pbft_msg& msg, const bzn_envelope& env, const std::shared_ptr<pbft_operation>& op);
+        void maybe_record_request(const bzn_envelope &env, const std::shared_ptr<pbft_operation> &op);
 
         timestamp_t now() const;
         bool already_seen_request(const bzn_envelope& msg, const request_hash_t& hash) const;
@@ -233,6 +233,7 @@ namespace bzn
         std::map<bzn::hash_t, int> map_request_to_hash(const bzn_envelope& original_msg);
         void save_all_requests(const pbft_msg& msg, const bzn_envelope& original_msg);
 
+
         std::shared_ptr<bzn::storage_base> storage;
 
         // Using 1 as first value here to distinguish from default value of 0 in protobuf