test: keep node 2 in IBD in p2p_node_network_limited

[PastaPastaPasta]

Issue being fixed or feature implemented

Fixes #7288.

p2p_node_network_limited.py --v2transport intermittently fails on the
linux64_tsan-test lane, always with the same signature:

File "test/functional/p2p_node_network_limited.py", line 83, in run_test
    self.connect_nodes(0, 2)
...
File "test/functional/test_framework/test_framework.py", line 732, in check_bytesrecv
    assert peer is not None, "Error: peer disconnected"
AssertionError: Error: peer disconnected

Six CI occurrences are recorded on the issue, most recently 2026-07-29. It is not
a timeout — the failure lands in ~5 s.

Root cause. The test says, in its own comment, that node 2 cannot sync
"because node 2 is in IBD". That premise is true upstream and false in Dash.

The functional test framework pins every node to -mocktime=TIME_GENESIS_BLOCK
(test/functional/test_framework/test_framework.py:899); Bitcoin Core's does
not. So node 2's genesis tip is never older than nMaxTipAge, and
Chainstate::IsInitialBlockDownload() (src/validation.cpp:1665) latches to
false on its very first call:

	tip time	GetTime()	tip age	in IBD?
Bitcoin Core	2011 regtest genesis	wall clock	~15 years	yes
Dash	1417713337	1417713337 (frozen)	0 s	no

Node 2 therefore passes the block-download gate at src/net_processing.cpp:6636,
syncs headers from node 0, and immediately asks it for blocks 1..16. Node 0 runs
-prune=550, signals NODE_NETWORK_LIMITED without NODE_NETWORK, and answers a
request below the threshold by dropping the peer (src/net_processing.cpp:2715).

connect_nodes() cannot survive that. After the handshake it polls
getpeerinfo() until it observes a pong from both sides, asserting
peer is not None on every poll
(test/functional/test_framework/test_framework.py:732). Whenever node 0 drops
node 2 before the next poll lands, the helper asserts instead of returning. From
the CI combined log:

node2 21:08:57.315815  sending getdata (577 bytes) peer=0
node0 21:08:57.318655  Ignore block request below NODE_NETWORK_LIMITED threshold, disconnect peer=2
node0 21:08:57.331313  ThreadRPCServer method=getpeerinfo     <- peer already gone
test  21:08:57.332000  AssertionError: Error: peer disconnected

--v2transport is the variant hit in practice because the v2 handshake's extra
round trip shifts the ping/pong later relative to the header sync that triggers
the disconnect; the race itself is transport-agnostic.

What was done?

Restored the premise rather than working around it: self.disable_mocktime = True
in set_test_params().

With mocktime disabled node 2 is genuinely in IBD, so it never requests those
blocks, node 0 never drops it, and the race cannot occur rather than merely
being unlikely. The body of the test — including the inherited comment, which is
now accurate again — stays identical to upstream, so future backports of this file
apply cleanly.

There is established precedent for exactly this problem class:
p2p_ibd_stalling.py, p2p_ibd_txrelay.py and p2p_initial_headers_sync.py all
set disable_mocktime because the framework's genesis mocktime denies them the
IBD state they are written to exercise.

No coverage is lost. The "limited peer drops whoever requests historical blocks"
behaviour is still tested directly and deterministically by the P2PIgnoreInv
mininode earlier in run_test(). What is gained is the coverage upstream actually
intended here, which Dash was silently missing: a node in IBD does not even
attempt to sync from a NODE_NETWORK_LIMITED peer.

How Has This Been Tested?

Reproduced and verified locally with the --v2transport variant on a 14-core
M-series host, using parallel test_runner.py batches. Patched and unpatched arms
were interleaved and order-swapped between rounds, so neither arm could
systematically draw a busier slot, with host load recorded before and after every
batch.

arm	runs	failures with the CI signature
unpatched	180	31
patched	195	0

The patched figure includes 120 runs at 30-way parallelism (deliberate
self-inflicted contention) and 15 runs of --v1transport. The unpatched control
was re-confirmed at that same 30-way parallelism (8/30 failures), so the clean
patched batches are meaningful rather than an artifact of an easy configuration.

Every unpatched failure was classified against the CI signature — same file:line,
same predicate, and the same Ignore block request below NODE_NETWORK_LIMITED threshold, disconnect peer=2 immediately preceding it in node 0's debug.log.
Failures from unrelated causes (a port held by an unrelated process; RPC timeouts
inside generate(292) when the host was saturated by an unrelated parallel run)
were counted separately and excluded from both arms.

Mechanism confirmed directly in the logs: with the patch, node 2 issues zero
block requests to node 0, and node 0 logs exactly one threshold-disconnect —
the intended one against the mininode.

Also run: test/lint/lint-python.py (no issues in 312 source files).

Test-only change; no rebuild required.

Note on disconnect_all()

This change makes disconnect_all() do slightly more work. Today
disconnect_nodes(0, 2) is a no-op — node 0 has already dropped node 2, and the
run logs disconnect_nodes: 0 and 2 were not connected. With the patch that
connection is alive, so all three pairs are genuinely torn down.

Measured, instrumenting disconnect_all() over 135 runs of each variant at 45-way
parallelism. The budget per wait is 20 s (timeout=5 scaled by
TEST_RUNNER_TIMEOUT_FACTOR=4; see test/functional/test_framework/util.py:272):

	disconnect_all() total	worst single-pair wait
this PR	p50 1.18 s, p90 1.47 s, max 1.54 s	0.55 s
unpatched shape	p50 0.96 s, p90 0.98 s, max 1.08 s	0.63 s

The extra ~0.5 s is the additional pair, not slower teardown — the worst
individual wait is in fact lower than without the patch. Roughly 13x headroom
against the 20 s budget.

Note on runtime

The test gets ~20 s slower per run in CI. That is not overhead this change adds,
it is the test finally performing the wait it was always written to perform:
sync_blocks([node0, node2], timeout=5) (20 s at TEST_RUNNER_TIMEOUT_FACTOR=4).
Today that call aborts after ~1 s, because node 2 has already been disconnected
and sync_blocks bails on its "each peer has at least one connection" assert
(test/functional/test_framework/test_framework.py:841). Upstream pays this wait
too. It could be trimmed later if it matters, but doing so here would mean
touching behaviour this issue is not about.

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

[coderabbitai]

Walkthrough

The functional test now documents that node 2 must remain in initial block download. It disables framework mocktime in set_test_params so the default genesis mocktime does not cause node 2 to appear synchronized prematurely.

Estimated code review effort: 1 (Trivial) | ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Disabling mocktime keeps node 2 in IBD, prevents the premature block request, and addresses the intermittent disconnect in issue #7288.
Out of Scope Changes check	✅ Passed	The changeset contains only the targeted functional-test adjustment and no unrelated code changes.
Title check	✅ Passed	The title clearly and concisely describes the main test change: keeping node 2 in initial block download.
Description check	✅ Passed	The description directly explains the intermittent failure, root cause, implementation, and validation for the test change.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit 69d75bb)

[thepastaclaw]

Final validation — Codex + Sonnet

This single-commit test-only fix replaces a racy connect_nodes()/pong sync point with an addnode("onetry") + headers-only-chaintip wait in p2p_node_network_limited.py, correctly resolving the intermittent 'peer disconnected' failure (dash#7288). Both Codex and Claude reviewer lanes plus this final verification pass independently traced the RPC semantics, confirmed the addnode idiom matches existing precedent (rpc_setban.py), and validated the new wait condition is disconnect-independent and causally sound. No findings from any lane, no CodeRabbit inline comments to react to, and the change compiles/parses cleanly.

Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (completed)

[PastaPastaPasta]

Considering alternative approach of disabling mock time; testing now

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69d75bba0a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid replacing the disconnect race with wall-clock timeouts

Under heavy host contention, setting disable_mocktime globally makes the later disconnect_all() rely on real-time peer teardown while disconnect_nodes() allows only 5 seconds for the remote side to observe each close. The commit's own validation records 6 failures in 45 runs at that later wait for this exact alternative, so the submitted patch trades the original connect_nodes() race for another reproducible CI flake instead of implementing the advertised addnode/headers-only synchronization.

AGENTS.md reference: AGENTS.md:L173-L175

Useful? React with 👍 / 👎.

[PastaPastaPasta]

Thanks — this is a fair thing to flag, and it made me measure the right quantity. Addressing it in three parts, because one part of it is correct.

On the "6 failures in 45 runs" figure. That number is mine, from an earlier revision of this PR description, and I had already withdrawn it. This review appears to have run against an intermediate state: the new commit paired with the previous description (which still proposed the addnode/headers-only approach and listed disable_mocktime as a rejected alternative). The description now matches the commit.

I withdrew the figure because the run that produced it was confounded. Those batches were run in a fixed order within each round while an unrelated job was draining off the host, so load decayed through the round — 40.97 → 35.72 → 23.94 as measured after each batch. The disable_mocktime arm drew the hotter slot and the comparison arm the cooler one, so the difference cannot be attributed to the patch.

Re-run with the arms order-swapped between rounds and host load recorded before and after every batch:

configuration	patched	unpatched control
15-way, quiet host	60 / 60	—
30-way, self-contended	120 / 120	22 / 30 (8 failures)
45-way, self-contended	135 / 135	—

Zero disconnect_all() failures in 315 runs. The unpatched control was re-confirmed at 30-way so the clean batches are not just an easy configuration.

Two specifics in the mechanism don't hold.

1. The budget is not 5 seconds. disconnect_nodes()'s timeout=5 goes through wait_until() → wait_until_helper(), which applies timeout = timeout * timeout_factor (test/functional/test_framework/util.py:272). CI sets TEST_RUNNER_TIMEOUT_FACTOR=4 (ci/test/00_setup_env.sh), so the actual budget is 20 s.

2. Peer teardown was already wall-clock in both variants. disconnect_nodes() polls getpeerinfo() over RPC in real time, and the socket close happens in the net thread's select loop — neither is driven by mocktime, so mocktime was never accelerating it. Nothing here moves from mock time to real time.

The part that is correct. The patch does make disconnect_all() do more work, which I had not called out. Today disconnect_nodes(0, 2) is a no-op — node 0 has already dropped node 2, and the run logs disconnect_nodes: 0 and 2 were not connected. With the patch that connection is alive, so all three pairs are genuinely torn down (0 such warnings vs 1).

So I measured the headroom directly, instrumenting disconnect_all() and running 135 iterations of each variant at 45-way parallelism:

	disconnect_all() total	worst single-pair wait	budget
patched	p50 1.18 s, p90 1.47 s, max 1.54 s	0.55 s	20 s
addnode/headers-only variant	p50 0.96 s, p90 0.98 s, max 1.08 s	0.63 s	20 s

The patched variant costs ~0.5 s more in aggregate, entirely because it tears down three real pairs instead of two. The worst individual wait is lower than in the alternative (0.55 s vs 0.63 s), which is what one would expect if the extra cost is an additional pair rather than slower teardown. Against a 20 s budget that is ~13x headroom, and it would have to regress by ~36x to threaten the wait.

Why this shape over the addnode/headers-only one. Both eliminate the reported flake (0 failures in 195 and 270 runs respectively). This one is 5 added lines instead of 15, leaves the body of the file identical to Bitcoin Core so future backports apply cleanly, makes the race structurally impossible rather than tolerated, and restores the coverage the upstream test intends and Dash silently loses — a node in IBD does not even attempt to sync from a NODE_NETWORK_LIMITED peer. p2p_ibd_stalling.py, p2p_ibd_txrelay.py and p2p_initial_headers_sync.py already set disable_mocktime for exactly this reason.

The NODE_NETWORK_LIMITED disconnect path itself is unaffected: it is still covered directly and deterministically by the P2PIgnoreInv mininode earlier in run_test(), and node 0 still logs exactly one threshold-disconnect per run.

Happy to switch back to the addnode/headers-only variant if reviewers prefer it — it is measured and works — but on this evidence the smaller, upstream-aligned change looks like the better trade.

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Final validation — Codex + Sonnet

At exact head 69d75bb, the five-line test adjustment correctly disables the framework's genesis mocktime before node startup, keeping node 2 in IBD and preventing it from requesting historical blocks from the NODE_NETWORK_LIMITED peer. The framework, validation, networking, and timeout paths confirm the fix is structurally sound, and the rewritten implementation does not invalidate the prior clean review at cc36d47. No in-scope defects remain.

Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — final-verifier (fallback)
• Sonnet reviewers: claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (completed)