simplify std::visitor uses; cache base_digest; and minor changes for …

…reviewing comments

libraries/chain/block_state.cpp

@@ -16,6 +16,7 @@ block_state::block_state(const block_header_state& prev, signed_block_ptr b, con
    , strong_digest(compute_finality_digest())
    , weak_digest(create_weak_digest(strong_digest))
    , pending_qc(prev.active_finalizer_policy->finalizers.size(), prev.active_finalizer_policy->threshold, prev.active_finalizer_policy->max_weak_sum_before_weak_final())
+   , base_digest(compute_base_digest())
 {
    // ASSUMPTION FROM controller_impl::apply_block = all untrusted blocks will have their signatures pre-validated here
    if( !skip_validate_signee ) {
@@ -42,6 +43,7 @@ block_state::block_state(const block_header_state&                bhs,
    , pub_keys_recovered(true) // called by produce_block so signature recovery of trxs must have been done
    , cached_trxs(std::move(trx_metas))
    , action_mroot(action_mroot)
+   , base_digest(compute_base_digest())
 {
    block->transactions = std::move(trx_receipts);
 
@@ -92,6 +94,8 @@ block_state::block_state(const block_state_legacy& bsp, const digest_type& actio
    validated = bsp.is_valid();
    pub_keys_recovered = bsp._pub_keys_recovered;
    cached_trxs = bsp._cached_trxs;
+   action_mroot = action_mroot_svnn;
+   base_digest = compute_base_digest();
 }
 
 block_state::block_state(snapshot_detail::snapshot_block_state_v7&& sbs)

libraries/chain/include/eosio/chain/block_state.hpp

@@ -78,7 +78,8 @@ struct block_state : public block_header_state {     // block_header_state provi
    // ------ data members caching information available elsewhere ----------------------
    bool                       pub_keys_recovered = false;
    deque<transaction_metadata_ptr> cached_trxs;
-   digest_type                action_mroot;
+   digest_type                action_mroot; // For base_digest sent to SHiP
+   digest_type                base_digest;  // For base_digest sent to SHiP
 
    // ------ private methods -----------------------------------------------------------
    bool                                is_valid() const { return validated; }

plugins/state_history_plugin/include/eosio/state_history_plugin/session.hpp

@@ -169,9 +169,7 @@ class blocks_ack_request_send_queue_entry : public send_queue_entry_base {
       assert(std::holds_alternative<state_history::get_blocks_request_v0>(*session->current_request) ||
              std::holds_alternative<state_history::get_blocks_request_v1>(*session->current_request));
 
-      std::visit(chain::overloaded{
-                    [&](eosio::state_history::get_blocks_request_v0& request) { request.max_messages_in_flight += req.num_messages;},
-                    [&](eosio::state_history::get_blocks_request_v1& request) { request.max_messages_in_flight += req.num_messages;} },
+      std::visit([&](auto& request) { request.max_messages_in_flight += req.num_messages; },
                  *session->current_request);
       session->send_update(false);
    }
@@ -513,15 +511,8 @@ struct session : session_base, std::enable_shared_from_this<session<Plugin, Sock
       assert(std::holds_alternative<state_history::get_blocks_request_v0>(req) ||
              std::holds_alternative<state_history::get_blocks_request_v1>(req));
 
-      std::visit(chain::overloaded{
-                    [&](state_history::get_blocks_request_v0& request) {
-                       update_current_request_impl(request);
-                       current_request = std::move(req);},
-                    [&](state_history::get_blocks_request_v1& request) {
-                       update_current_request_impl(request);
-                       fc_dlog(plugin.get_logger(), "replying get_blocks_request_v1, fetch_finality_data = ${fetch_finality_data}", ("fetch_finality_data", request.fetch_finality_data));
-                       current_request = std::move(req);} },
-                 req);
+      std::visit( [&](auto& request) { update_current_request_impl(request); }, req );
+      current_request = std::move(req);
    }
 
    void send_update(state_history::get_blocks_request_v0& request, bool fetch_finality_data, state_history::get_blocks_result_v0 result, const chain::signed_block_ptr& block, const chain::block_id_type& id) {
@@ -532,7 +523,6 @@ struct session : session_base, std::enable_shared_from_this<session<Plugin, Sock
             request.irreversible_only ? result.last_irreversible.block_num : result.head.block_num;
 
       fc_dlog( plugin.get_logger(), "irreversible_only: ${i}, last_irreversible: ${p}, head.block_num: ${h}", ("i", request.irreversible_only)("p", result.last_irreversible.block_num)("h", result.head.block_num));
-      fc_dlog( plugin.get_logger(), "recved result: ${r}", ("r", result));
       if (to_send_block_num > current || to_send_block_num >= request.end_block_num) {
          fc_dlog( plugin.get_logger(), "Not sending, to_send_block_num: ${s}, current: ${c} request.end_block_num: ${b}",
                   ("s", to_send_block_num)("c", current)("b", request.end_block_num) );
@@ -605,11 +595,7 @@ struct session : session_base, std::enable_shared_from_this<session<Plugin, Sock
          return true;
 
       uint32_t max_messages_in_flight = std::visit(
-         chain::overloaded{
-            [&](state_history::get_blocks_request_v0& request) -> uint32_t {
-               return request.max_messages_in_flight; },
-            [&](state_history::get_blocks_request_v1& request) -> uint32_t {
-               return request.max_messages_in_flight; }},
+         [&](auto& request) -> uint32_t { return request.max_messages_in_flight; },
          *current_request);
 
       return !max_messages_in_flight;

plugins/state_history_plugin/state_history_plugin.cpp

@@ -210,7 +210,7 @@ struct state_history_plugin_impl : std::enable_shared_from_this<state_history_pl
       try {
          store_traces(block, id);
          store_chain_state(id, block->previous, block->block_num());
-         store_finality_data(block, id);
+         store_finality_data(id, block->previous);
       } catch (const fc::exception& e) {
          fc_elog(_log, "fc::exception: ${details}", ("details", e.to_detail_string()));
          // Both app().quit() and exception throwing are required. Without app().quit(),
@@ -275,7 +275,7 @@ struct state_history_plugin_impl : std::enable_shared_from_this<state_history_pl
    } // store_chain_state
 
    // called from main thread
-   void store_finality_data(const signed_block_ptr& block, const block_id_type& id) {
+   void store_finality_data(const block_id_type& id, const block_id_type& previous_id) {
       if (!finality_data_log)
          return;
 
@@ -285,7 +285,7 @@ struct state_history_plugin_impl : std::enable_shared_from_this<state_history_pl
 
       state_history_log_header header{
          .magic = ship_magic(ship_current_version, 0), .block_id = id, .payload_size = 0};
-      finality_data_log->pack_and_write_entry(header, block->previous, [finality_data](auto&& buf) {
+      finality_data_log->pack_and_write_entry(header, previous_id, [finality_data](auto&& buf) {
          fc::datastream<boost::iostreams::filtering_ostreambuf&> ds{buf};
          fc::raw::pack(ds, *finality_data);
       });

tests/CMakeLists.txt

@@ -152,7 +152,7 @@ add_test(NAME ship_streamer_test COMMAND tests/ship_streamer_test.py -v --num-cl
 set_property(TEST ship_streamer_test PROPERTY LABELS long_running_tests)
 add_test(NAME ship_streamer_if_test COMMAND tests/ship_streamer_test.py -v --num-clients 10 --activate-if ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
 set_property(TEST ship_streamer_if_test PROPERTY LABELS long_running_tests)
-add_test(NAME ship_streamer_if_fetch_finality_data_test COMMAND tests/ship_streamer_test.py -v --num-clients 10 --activate-if --finality-data-history --fetch-finality-data ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
+add_test(NAME ship_streamer_if_fetch_finality_data_test COMMAND tests/ship_streamer_test.py -v --num-clients 10 --activate-if --finality-data-history ${UNSHARE} WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
 set_property(TEST ship_streamer_if_fetch_finality_data_test PROPERTY LABELS long_running_tests)
 
 add_test(NAME p2p_dawn515_test COMMAND tests/p2p_tests/dawn_515/test.sh WORKING_DIRECTORY ${CMAKE_BINARY_DIR})

tests/ship_streamer_test.py

@@ -32,12 +32,8 @@
 appArgs = AppArgs()
 extraArgs = appArgs.add(flag="--num-clients", type=int, help="How many ship_streamers should be started", default=1)
 extraArgs = appArgs.add_bool(flag="--finality-data-history", help="Enable finality data history", action='store_true')
-extraArgs = appArgs.add_bool(flag="--fetch-finality-data", help="Fetch finality data", action='store_true')
 args = TestHelper.parse_args({"--activate-if","--dump-error-details","--keep-logs","-v","--leave-running","--unshared"}, applicationSpecificArgs=appArgs)
 
-if args.fetch_finality_data:
-    assert args.finality_data_history is not None and args.finality_data_history is not False, "ERROR: --finality-data-history is required for --fetch-finality-data"
-
 Utils.Debug=args.v
 cluster=Cluster(unshared=args.unshared, keepRunning=args.leave_running, keepLogs=args.keep_logs)
 activateIF=args.activate_if
@@ -142,7 +138,7 @@ def getLatestSnapshot(nodeId):
 
     shipClient = "tests/ship_streamer"
     cmd = f"{shipClient} --start-block-num {start_block_num} --end-block-num {end_block_num} --fetch-block --fetch-traces --fetch-deltas"
-    if args.fetch_finality_data:
+    if args.finality_data_history:
         cmd += "  --fetch-finality-data"
     if Utils.Debug: Utils.Print(f"cmd: {cmd}")
     clients = []
@@ -244,7 +240,7 @@ def getLatestSnapshot(nodeId):
     block_range = 0
     end_block_num = start_block_num + block_range
     cmd = f"{shipClient} --start-block-num {start_block_num} --end-block-num {end_block_num} --fetch-block --fetch-traces --fetch-deltas"
-    if args.fetch_finality_data:
+    if args.finality_data_history:
         cmd += "  --fetch-finality-data"
     if Utils.Debug: Utils.Print(f"cmd: {cmd}")
     clients = []