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

[heifner]

Two independent problems in the block notice path, both surfaced while investigating the nodeop_chainbase_allocation_test failure addressed by #547, where a node sat one block behind after its sole producer died mid-broadcast.

latest_blk_time was refreshed at the top of the notice handler, before it decided what to do with the notice, so an announcement for a block we do not have counted as block progress on that connection. That is the one timer that recovers a missing block: check_heartbeat sends a handshake once no block has arrived for half the keepalive interval, and refreshing on a notice we could not act on pushes that recovery further out. It now refreshes only when the notice names a block we already hold.

The notice and nack exchange trades a round trip for bandwidth, which is a bad trade for a node that produces. Once a peer has been nacked past the threshold it stops sending blocks and sends announcements instead, so a producer can be left waiting a round trip for a block it needs to build on, and a block that arrives late can cost it its slot. p2p-disable-block-nack now defaults to true when producer-name is configured. An explicit setting still wins in both directions, and nodes that do not produce are unchanged.

producer_plugin is a declared dependency of net_plugin, so its options are already parsed when net_plugin initializes and the producer set can be read there. The default resolution itself is a pure helper in net_utils so the four way table is covered by unit tests.

Note for deployment: a producer that does not set p2p-disable-block-nack explicitly will begin exchanging full blocks after this change, which costs outbound bandwidth on producer nodes in exchange for not being announcement gated.

Still open: a node that lacks a block but holds its parent drops the notice without requesting anything. A fixed retry deadline is not viable because produce-block-offset-ms makes the interval between blocks configurable, so that case is left for a follow up.

[huangminghuang]

One regression-coverage gap found in the block-notice liveness fix.

[huangminghuang]

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]

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.

[huangminghuang]

Approved. The follow-up at 9bb4f1e addresses the regression-coverage concern; I verified both new suites locally (10 cases). Before merging, please update the PR description to include the second commit's block-notice classification/progress refactor, block_notice_unittest.cpp, and the validation performed, per the repository's review-follow-up guidance.