diff --git a/plugins/net_plugin/include/sysio/net_plugin/net_utils.hpp b/plugins/net_plugin/include/sysio/net_plugin/net_utils.hpp index 0aa70c9b51..778e116136 100644 --- a/plugins/net_plugin/include/sysio/net_plugin/net_utils.hpp +++ b/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 dedupe_preserve_order(const std::vector& addresses) { diff --git a/plugins/net_plugin/src/net_plugin.cpp b/plugins/net_plugin/src/net_plugin.cpp index 9358f4b1ad..6b9d60b37a 100644 --- a/plugins/net_plugin/src/net_plugin.cpp +++ b/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()->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()->default_value(true), "Allow transactions received over p2p network to be evaluated and relayed if valid.") ( "p2p-disable-block-nack", bpo::value()->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 >()->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(); p2p_accept_transactions = options.at( "p2p-accept-transactions" ).as(); - p2p_disable_block_nack = options.at( "p2p-disable-block-nack" ).as(); + // 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(); + 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(), !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() ); SYS_ASSERT( keepalive_interval.count() > 0, chain::plugin_config_exception, diff --git a/plugins/net_plugin/test/CMakeLists.txt b/plugins/net_plugin/test/CMakeLists.txt index 5db7d62de8..abd74a11ff 100644 --- a/plugins/net_plugin/test/CMakeLists.txt +++ b/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 diff --git a/plugins/net_plugin/test/block_nack_default_unittest.cpp b/plugins/net_plugin/test/block_nack_default_unittest.cpp new file mode 100644 index 0000000000..b2951a2481 --- /dev/null +++ b/plugins/net_plugin/test/block_nack_default_unittest.cpp @@ -0,0 +1,43 @@ +#include +#include + +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() diff --git a/plugins/net_plugin/test/block_notice_unittest.cpp b/plugins/net_plugin/test/block_notice_unittest.cpp new file mode 100644 index 0000000000..e8de88a374 --- /dev/null +++ b/plugins/net_plugin/test/block_notice_unittest.cpp @@ -0,0 +1,40 @@ +#include +#include + +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()