Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
heifner merged 2 commits into from
Aug 7, 2026
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
926571f
net: keep block progress honest and stop producers from being nacked
heifner Aug 6, 2026
9bb4f1e
net: cover the block notice liveness rule with a regression test
heifner Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions plugins/net_plugin/include/sysio/net_plugin/net_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,22 @@ namespace detail {
blocks_only
};

/// 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) {
Expand Down
18 changes: 15 additions & 3 deletions plugins/net_plugin/src/net_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4021,8 +4021,11 @@ 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)) {
// Only an announcement of a block we already hold counts as block progress on this connection.
// Refreshing on a notice for a block we are missing hides the fact that we are behind and defers
// the handshake in check_heartbeat that would otherwise recover it.
latest_blk_time = std::chrono::steady_clock::now();

@huangminghuang huangminghuang Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a regression test that delivers a notice for a block absent from the dispatcher and verifies it does not refresh latest_blk_time—or equivalently that the half-heartbeat handshake recovery still fires. The new tests cover only option-default resolution, so this liveness fix could currently regress without failing CI. PR #547 also avoids the original race by waiting for LIB rather than exercising this recovery path.

@heifner heifner Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the tests covered only option-default resolution, so the latest_blk_time change could have regressed without CI noticing.

handle_message(const block_notice_message&) is a connection method that reaches through my_impl->dispatcher, get_fork_db_root_num(), and the connection list, and there is no harness for that; the existing net_plugin tests all work against header-only components. Rather than build one, I lifted the decision into two pure functions in net_utils:

enum class block_notice_action { record_peer_has_block, request_blocks, ignore };
block_notice_action classify_block_notice(bool have_announced_block, bool have_parent_block);
bool block_notice_marks_progress(block_notice_action action);

The handler now has a single site that touches latest_blk_time, gated by block_notice_marks_progress. The added cases pin the three-way classification and, separately, that neither missing-block case counts as progress — the last one stated over the dispatcher inputs rather than the action, so the guarantee survives a reclassification.

I checked that it actually guards rather than just describes: mutating block_notice_marks_progress to return true, the previous behaviour, fails 4 checks across both new cases.

To be clear about the limit, this covers the input to the recovery, not the recovery itself. Asserting that the half-heartbeat handshake fires needs a live connection plus the heartbeat timer, which is the harness that does not exist here. And you are right that #547 avoids this path deliberately — it waits for LIB so the test is deterministic rather than depending on a 5 to 15 second recovery — so no test in either PR exercises the end-to-end path.

The short-circuit is preserved: the parent lookup is still skipped when the announced block is already held, so the common path costs no extra dispatcher lookup.

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
peer_dlog(p2p_blk_log, this, "Received unknown block notice, checking already requested");
Expand Down Expand Up @@ -4543,7 +4546,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"
Expand Down Expand Up @@ -4615,7 +4619,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,
Expand Down
1 change: 1 addition & 0 deletions plugins/net_plugin/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
add_executable( test_net_plugin
auto_bp_peering_unittest.cpp
block_nack_default_unittest.cpp
connection_type_unittest.cpp
local_txn_cache_unittest.cpp
rate_limit_parse_unittest.cpp
Expand Down
43 changes: 43 additions & 0 deletions plugins/net_plugin/test/block_nack_default_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
Loading