Net: keep block progress honest and stop producers from being nacked

plugins/net_plugin/include/sysio/net_plugin/net_utils.hpp

@@ -157,6 +157,48 @@ namespace detail {
       blocks_only
    };
 
+   /// What a received block notice calls for.
+   enum class block_notice_action {
+      record_peer_has_block, ///< the announced block is already held, so only record that the peer has it
+      request_blocks,        ///< neither the announced block nor its parent is held, so ask for the branch
+      ignore                 ///< the parent is held but the announced block is not, so there is nothing to do here
+   };
+
+   /// Classify a block notice from what the dispatcher already holds.
+   ///
+   /// @param have_announced_block whether the announced block is already held
+   /// @param have_parent_block whether the announced block's parent is already held
+   inline block_notice_action classify_block_notice(bool have_announced_block, bool have_parent_block) {
+      if( have_announced_block )
+         return block_notice_action::record_peer_has_block;
+      return have_parent_block ? block_notice_action::ignore : block_notice_action::request_blocks;
+   }
+
+   /// Whether a notice counts as block progress on the connection it arrived on.
+   ///
+   /// Only an announcement of a block we already hold does. A notice for a block we are missing leaves us
+   /// behind, and treating it as progress defers the handshake in check_heartbeat that recovers the block,
+   /// which is the only thing that recovers it when no further block is produced.
+   inline bool block_notice_marks_progress(block_notice_action action) {
+      return action == block_notice_action::record_peer_has_block;
+   }
+
+   /// Resolve whether block notice and block nack should be disabled for this node.
+   ///
+   /// The block notice and block nack exchange trades a round trip for bandwidth: a peer that is
+   /// repeatedly told "already have it" stops sending blocks and sends announcements instead. A node
+   /// that produces cannot pay that round trip, because a block reaching it late can cost it its own
+   /// production slot, so a configured producer exchanges full blocks by default. An explicit setting
+   /// always wins over that default.
+   ///
+   /// @param configured value supplied for p2p-disable-block-nack, meaningful only when explicitly set
+   /// @param explicitly_set whether @p configured came from configuration rather than the built-in default
+   /// @param configured_producer whether this node has at least one producer configured
+   /// @return true when block notice and block nack should be disabled
+   inline bool resolve_disable_block_nack(bool configured, bool explicitly_set, bool configured_producer) {
+      return explicitly_set ? configured : configured_producer;
+   }
+
    /// De-duplicate listen endpoints while preserving first-seen order so positionally paired
    /// p2p-server-address values remain aligned with their p2p-listen-endpoint.
    inline std::vector<std::string> dedupe_preserve_order(const std::vector<std::string>& addresses) {

plugins/net_plugin/src/net_plugin.cpp

@@ -4021,10 +4021,22 @@ namespace sysio {
       if (block_header::num_from_id(msg.id) <= fork_db_root_num)
          return;
 
-      latest_blk_time = std::chrono::steady_clock::now();
-      if (my_impl->dispatcher.have_block(msg.id)) {
+      const bool have_announced_block = my_impl->dispatcher.have_block(msg.id);
+      // the parent only matters when the announced block is missing, so skip that lookup otherwise
+      const bool have_parent_block = have_announced_block || my_impl->dispatcher.have_block(msg.previous);
+      const auto action = net_utils::classify_block_notice(have_announced_block, have_parent_block);
+
+      // Refreshing on a notice for a block we are missing would hide the fact that we are behind and defer
+      // the handshake in check_heartbeat that recovers it.
+      if (net_utils::block_notice_marks_progress(action)) {
+         latest_blk_time = std::chrono::steady_clock::now();
+      }
+
+      switch (action) {
+      case net_utils::block_notice_action::record_peer_has_block:
          my_impl->dispatcher.add_peer_block(msg.id, connection_id);
-      } else if (!my_impl->dispatcher.have_block(msg.previous)) { // still don't have previous block
+         break;
+      case net_utils::block_notice_action::request_blocks: { // still don't have previous block
          peer_dlog(p2p_blk_log, this, "Received unknown block notice, checking already requested");
          const block_id_type& target = msg.previous;
          bool already_requested = my_impl->connections.any_of_block_connections([&target](const auto& c) {
@@ -4044,6 +4056,10 @@ namespace sysio {
             }
             enqueue(req);
          }
+         break;
+      }
+      case net_utils::block_notice_action::ignore:
+         break;
       }
    }
 
@@ -4543,7 +4559,8 @@ namespace sysio {
          ( "p2p-max-nodes-per-host", bpo::value<int>()->default_value(def_max_nodes_per_host), "Maximum number of client nodes from any single /24 (IPv4) or /48 (IPv6) subnet")
          ( "p2p-accept-transactions", bpo::value<bool>()->default_value(true), "Allow transactions received over p2p network to be evaluated and relayed if valid.")
          ( "p2p-disable-block-nack", bpo::value<bool>()->default_value(false),
-            "Disable block notice and block nack. All blocks received will be broadcast to all peers unless already received.")
+            "Disable block notice and block nack. All blocks received will be broadcast to all peers unless already received.\n"
+            "Defaults to true when producer-name is configured, so a producing node always exchanges full blocks.")
          ( "p2p-auto-bp-peer", bpo::value< vector<string> >()->composing(),
            "The account and public p2p endpoint of a block producer node to automatically connect to when it is in producer schedule. Not gossipped.\n"
            "  Syntax: bp_account,host:port\n"
@@ -4615,7 +4632,15 @@ namespace sysio {
          resp_expected_period = def_resp_expected_wait;
          max_nodes_per_host = options.at( "p2p-max-nodes-per-host" ).as<int>();
          p2p_accept_transactions = options.at( "p2p-accept-transactions" ).as<bool>();
-         p2p_disable_block_nack = options.at( "p2p-disable-block-nack" ).as<bool>();
+         // producer_plugin is a declared dependency, so its options are already parsed here.
+         const auto& block_nack_opt = options.at( "p2p-disable-block-nack" );
+         const producer_plugin* prod_plug = app().find_plugin<producer_plugin>();
+         const bool configured_producer = prod_plug != nullptr && !prod_plug->producer_accounts().empty();
+         p2p_disable_block_nack = net_utils::resolve_disable_block_nack(
+               block_nack_opt.as<bool>(), !block_nack_opt.defaulted(), configured_producer );
+         if( p2p_disable_block_nack && block_nack_opt.defaulted() ) {
+            fc_ilog( p2p_blk_log, "block notice and block nack disabled by default, this node is configured to produce blocks" );
+         }
 
          keepalive_interval = std::chrono::milliseconds( options.at( "p2p-keepalive-interval-ms" ).as<int>() );
          SYS_ASSERT( keepalive_interval.count() > 0, chain::plugin_config_exception,

plugins/net_plugin/test/CMakeLists.txt

@@ -1,5 +1,7 @@
 add_executable( test_net_plugin
         auto_bp_peering_unittest.cpp
+        block_nack_default_unittest.cpp
+        block_notice_unittest.cpp
         connection_type_unittest.cpp
         local_txn_cache_unittest.cpp
         rate_limit_parse_unittest.cpp

plugins/net_plugin/test/block_nack_default_unittest.cpp

@@ -0,0 +1,43 @@
+#include <boost/test/unit_test.hpp>
+#include <sysio/net_plugin/net_utils.hpp>
+
+using namespace sysio::net_utils;
+
+BOOST_AUTO_TEST_SUITE(block_nack_default)
+
+// A node with no producer configured keeps the bandwidth optimization.
+BOOST_AUTO_TEST_CASE(default_enabled_for_non_producer) {
+   constexpr bool explicitly_set = false;
+   constexpr bool configured_producer = false;
+   BOOST_CHECK_EQUAL(resolve_disable_block_nack(false, explicitly_set, configured_producer), false);
+}
+
+// A configured producer must never receive a notice in place of a block.
+BOOST_AUTO_TEST_CASE(default_disabled_for_producer) {
+   constexpr bool explicitly_set = false;
+   constexpr bool configured_producer = true;
+   BOOST_CHECK_EQUAL(resolve_disable_block_nack(false, explicitly_set, configured_producer), true);
+}
+
+// An operator turning it off on a producer must win over the producer default.
+BOOST_AUTO_TEST_CASE(explicit_false_overrides_producer_default) {
+   constexpr bool explicitly_set = true;
+   constexpr bool configured_producer = true;
+   BOOST_CHECK_EQUAL(resolve_disable_block_nack(false, explicitly_set, configured_producer), false);
+}
+
+// An operator turning it on for a non-producer must win over the non-producer default.
+BOOST_AUTO_TEST_CASE(explicit_true_overrides_non_producer_default) {
+   constexpr bool explicitly_set = true;
+   constexpr bool configured_producer = false;
+   BOOST_CHECK_EQUAL(resolve_disable_block_nack(true, explicitly_set, configured_producer), true);
+}
+
+// An explicit setting that matches the default is still honoured as explicit.
+BOOST_AUTO_TEST_CASE(explicit_value_is_used_when_it_matches_the_default) {
+   constexpr bool explicitly_set = true;
+   BOOST_CHECK_EQUAL(resolve_disable_block_nack(true, explicitly_set, true), true);
+   BOOST_CHECK_EQUAL(resolve_disable_block_nack(false, explicitly_set, false), false);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

plugins/net_plugin/test/block_notice_unittest.cpp

@@ -0,0 +1,40 @@
+#include <boost/test/unit_test.hpp>
+#include <sysio/net_plugin/net_utils.hpp>
+
+using namespace sysio::net_utils;
+
+BOOST_AUTO_TEST_SUITE(block_notice_handling)
+
+// A notice for a block we already hold is the only case that records peer knowledge.
+BOOST_AUTO_TEST_CASE(announced_block_already_held_records_peer) {
+   BOOST_CHECK(classify_block_notice(true, true) == block_notice_action::record_peer_has_block);
+   BOOST_CHECK(classify_block_notice(true, false) == block_notice_action::record_peer_has_block);
+}
+
+// Missing both the block and its parent means we are two or more behind, so ask for the branch.
+BOOST_AUTO_TEST_CASE(missing_block_and_parent_requests_blocks) {
+   BOOST_CHECK(classify_block_notice(false, false) == block_notice_action::request_blocks);
+}
+
+// Holding the parent but not the block leaves nothing to do in the handler itself.
+BOOST_AUTO_TEST_CASE(missing_block_with_parent_held_is_ignored) {
+   BOOST_CHECK(classify_block_notice(false, true) == block_notice_action::ignore);
+}
+
+// The liveness property: only a notice naming a block we already hold may refresh latest_blk_time.
+// Marking either missing-block case as progress would defer the check_heartbeat handshake that
+// recovers the block when no further block is produced.
+BOOST_AUTO_TEST_CASE(only_a_held_block_marks_progress) {
+   BOOST_CHECK_EQUAL(block_notice_marks_progress(block_notice_action::record_peer_has_block), true);
+   BOOST_CHECK_EQUAL(block_notice_marks_progress(block_notice_action::request_blocks), false);
+   BOOST_CHECK_EQUAL(block_notice_marks_progress(block_notice_action::ignore), false);
+}
+
+// Stated over the dispatcher inputs rather than the action, so the guarantee survives a reclassification.
+BOOST_AUTO_TEST_CASE(a_notice_for_a_missing_block_never_marks_progress) {
+   BOOST_CHECK_EQUAL(block_notice_marks_progress(classify_block_notice(false, true)), false);
+   BOOST_CHECK_EQUAL(block_notice_marks_progress(classify_block_notice(false, false)), false);
+   BOOST_CHECK_EQUAL(block_notice_marks_progress(classify_block_notice(true, true)), true);
+}
+
+BOOST_AUTO_TEST_SUITE_END()