test: cover requested far-ahead cmpctblock getdata retry

shuv-amp commented at 5:34 pm on March 3, 2026: none

This updates coverage in test/functional/p2p_compactblocks.py for the requested far-ahead CMPCTBLOCK path.

The test now targets a discriminating invariant:

Build a 3-block extension from tip.
Send HEADERS for all 3 blocks so block 3 is already requested from the same peer.
Send CMPCTBLOCK for block 3.
Assert the node sends one additional getdata for block 3 (retry path).

The precondition waits on per-hash getdata counters under p2p_lock, so it does not depend on all inventory entries arriving in a single getdata message.

This distinguishes current behavior from the #34618 removal diff:

current code: pass
with #34618 removal diff applied to src/net_processing.cpp: fail at saw_exactly_one_retry timeout

Tested:

python3 test/functional/p2p_compactblocks.py --configfile=build/test/config.ini (pass)
apply local #34618 removal diff in src/net_processing.cpp, rebuild, rerun same test (fails at saw_exactly_one_retry)
revert src/net_processing.cpp, rebuild

Ref #34618

DrahtBot added the label Tests on Mar 3, 2026

DrahtBot commented at 5:34 pm on March 3, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

maflcko commented at 5:38 pm on March 3, 2026: member

Was this LLM generated? What are the steps to test this? What is the output before and after the changes here?

shuv-amp commented at 5:48 pm on March 3, 2026: none

No.

To test: test/functional/p2p_compactblocks.py --configfile=build/test/config.ini

This is a test-only change, so there is no user-facing before/after output.

Before this change, the far-ahead unsolicited CMPCTBLOCK fallback (fRevertToHeaderProcessing -> ProcessHeadersMessage) was not covered by the functional tests.

After this change, p2p_compactblocks.py covers that case and checks that the tip does not advance and that the node sends getdata for the block.

maflcko commented at 5:53 pm on March 3, 2026: member

If you want to show test coverage, you can add an Assert(false) in the line that you want to show covered. It should pass before, and fail after. If you want to show that, you’ll have to provide the diff as well as the output before and after.

shuv-amp commented at 6:34 pm on March 3, 2026: none

I checked this locally with a temporary Assert(false) in the fRevertToHeaderProcessing branch. This was a local-only check to verify coverage and is not part of the PR code.

Temporary Diff

0--- a/src/net_processing.cpp
1+++ b/src/net_processing.cpp
2@@ -4762,6 +4762,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
3         }
4 
5         if (fRevertToHeaderProcessing) {
6+            Assert(false);
7             // Headers received from HB compact block peers are permitted to be
8             // relayed before full validation (see BIP 152), so we don't want to disconnect
9             // the peer if the header turns out to be for an invalid block.

Verification Results

On master branch: With the temporary diff applied, the test still passes because this path is not reached by existing tests.

Command: test/functional/p2p_compactblocks.py --configfile=build-cov/test/config.ini --tmpdir=/tmp/bitcoin-cov-before --loglevel=INFO

Output:

02026-03-03T18:07:11.074310Z TestFramework (INFO): Testing handling of low-work compact blocks...
12026-03-03T18:07:11.134193Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
22026-03-03T18:07:14.674151Z TestFramework (INFO): Tests successful

On this PR branch: With the same temporary diff applied, the test now fails specifically in test_cmpctblock_revert_to_header_processing.

Output:

02026-03-03T18:19:18.592829Z TestFramework (INFO): Testing handling of low-work compact blocks...
12026-03-03T18:19:18.651698Z TestFramework (INFO): Testing cmpctblock revert-to-header processing...
2
3File ".../p2p_compactblocks.py", line 703, in test_cmpctblock_revert_to_header_processing
4    test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
5AssertionError

Node stderr:

0net_processing.cpp:4765 void (anonymous namespace)::PeerManagerImpl::ProcessMessage(...): Assertion `false' failed.

Conclusion: The fact that the test passes before this PR and fails after confirms that the new test logic successfully reaches and exercises the fRevertToHeaderProcessing branch.

davidgumberg commented at 8:08 pm on March 4, 2026: contributor

I checked this locally with a temporary Assert(false) in the fRevertToHeaderProcessing branch. This was a local-only check to verify coverage and is not part of the PR code.

Temporary Diff

0--- a/src/net_processing.cpp
1+++ b/src/net_processing.cpp
2@@ -4762,6 +4762,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
3         }
4 
5         if (fRevertToHeaderProcessing) {
6+            Assert(false);
7             // Headers received from HB compact block peers are permitted to be
8             // relayed before full validation (see BIP 152), so we don't want to disconnect
9             // the peer if the header turns out to be for an invalid block.

There are already plenty of tests that reach this line on master, adding an assert here causes many other tests on master to fail.

I tried to describe in #34618 that the issue is not whether or not this code is ever reached, it’s whether or not it actually does anything meaningful, and I think more conceptual discussion is needed in #34618 about whether or not this code is doing anything before deciding whether a test case should be added for it or it should be deleted.

The interesting thing to see would be a test case that fails with the diff from #34618 applied, but succeeds on master.

shuv-amp commented at 8:19 pm on March 4, 2026: none

I reran locally with rebuilds and tested both variants: current PR branch and a temporary #34618 style removal of the fRevertToHeaderProcessing path. p2p_compactblocks.py passes in both cases, so my current test does not distinguish the behavior. I’ll pause further changes here. If there is a specific invariant we want to preserve (that should fail with the #34618 diff), I can target that directly; otherwise I can close this PR and continue in #34618.

shuv-amp commented at 8:46 pm on March 4, 2026: none

Quick follow up with a discriminating check.

My previous check (running the existing test) did not distinguish behavior. I then ran a local only probe (not in PR) for the requested far-ahead CMPCTBLOCK case:

Build a 3-block extension from tip.
Announce headers so block 3 is already requested through normal headers flow.
Send CMPCTBLOCK for block 3.
Check whether the node sends a second getdata for block 3 (retry path).

Results (same functional test command each run, with rebuilds after net_processing.cpp diff changes):

Current logic (no net_processing changes): PASS 3/3
Temporary #34618 style removal diff in src/net_processing.cpp: FAIL 3/3 (probe times out waiting for the second getdata for block 3)

So this path appears to have observable behavior in the requested far-ahead case.

If this is the invariant we want to preserve, I can update #34722 to target this case directly.

test: cover requested far-ahead cmpctblock getdata retry

Add functional coverage for the requested far-ahead CMPCTBLOCK path in
p2p_compactblocks.py.

The new test builds a 3-block extension, announces all 3 headers so block 3
is already in-flight from the same peer, then sends CMPCTBLOCK for block 3.
It asserts that the node sends one additional getdata request for block 3
(a retry on top of the initial request).

The precondition uses per-hash getdata counters under p2p_lock and waits
for each block hash individually, so it does not depend on all inventory
entries arriving in a single getdata message.

This covers behavior exercised in the requested far-ahead branch and
distinguishes current logic from the #34618 removal diff.

Tested:
- python3 test/functional/p2p_compactblocks.py --configfile=build/test/config.ini (pass)
- With local #34618 removal diff in src/net_processing.cpp + rebuild:
  python3 test/functional/p2p_compactblocks.py --configfile=build/test/config.ini
  (fails at saw_exactly_one_retry timeout)
- Reverted src/net_processing.cpp and rebuilt successfully.

3fdbd8d9db

shuv-amp force-pushed on Mar 4, 2026

shuv-amp renamed this:
~~test: add coverage for cmpctblock revert-to-header processing~~
test: cover requested far-ahead cmpctblock getdata retry
on Mar 4, 2026

test: cover requested far-ahead cmpctblock getdata retry #34722

Reviews

Temporary Diff

Verification Results

Temporary Diff