Skip to content

net: never start header sync with a pre-fork peer while a fork-aware one is available - #418

Closed
LionThunderFingers wants to merge 1 commit into
bitcoinknots:29.x-knotsfrom
LionThunderFingers:prefork-peer-policy
Closed

LionThunderFingers wants to merge 1 commit into
bitcoinknots:29.x-knotsfrom
LionThunderFingers:prefork-peer-policy

Conversation

@LionThunderFingers

Copy link
Copy Markdown

This never syncs headers from a peer lacking NODE_BLAKE2B while at least one fork-aware peer is connected, with one exception: if we have no fork-aware peer connected at all, we fall back to syncing from whoever we do have, rather than getting a new node stuck with zero headers until one shows up. The fallback switches off automatically the moment a fork-aware peer connects, and back on if that peer later disconnects — both directions are covered by tests.

NODE_BLAKE2B (#386) identifies fork-aware peers, but nothing today governs which connected peer gets used as our header-sync source. We only run one active header-sync attempt at a time (nSyncStarted == 0), and a pre-fork peer can dead-end that attempt at the fork height — its headers are perfectly valid up to that point (they're checked against the consensus rules for their height regardless of who sent them, so this is not a correctness concern), but past it there's nothing more it can give us, and we have to notice the dead-end and switch peers before making further progress.

Pure local peer-selection policy — no consensus, protocol, or wire-format changes. Block download from a pre-fork peer is untouched; this only gates the header-sync trigger itself, at every call site that could otherwise leak a getheaders to one (the initial sync trigger, the low-work-headers-sync continuation, the inv-triggered sync path, and the unconnecting-headers/compact-block paths), all routed through one shared predicate so a future call site can't reintroduce the leak by accident.

Complementary to #398, not built on it: #398 governs which peers we choose to dial; this governs which of our already-connected peers we ask for headers, regardless of whether that peer was dialed out or connected in to us. Neither depends on the other.

Does not attempt to disconnect pre-fork peers — that's a separate, actively-discussed question (#412), and conflating the two seemed likely to cost this PR review cycles on a decision it doesn't need to make. A pre-fork peer we exclude from header sync remains connected and can still relay blocks and transactions to us.

test/functional/p2p_blake2b_prefork_peer_policy.py (10 cases) covers: block data still accepted from a pre-fork peer during our IBD; getheaders withheld from a pre-fork peer at every gated call site while a fork-aware peer is connected; the fallback engaging with zero fork-aware peers and disengaging once one connects; the fallback re-engaging if that peer later disconnects; the fork-aware-peer count staying correct across several different disconnect paths including a misbehavior-triggered ban; and the low-work-headers-sync entry point honoring the same rule.

Tested locally on two independent machines/builds.

@LionThunderFingers

Copy link
Copy Markdown
Author

Built this locally and ran the functional test against both the patch and the commit it branches from (8c85b15), same tree, one commit apart, to check the test actually detects the behaviour rather than just passing:

Unpatched (8c85b15) fails:

p2p_blake2b_prefork_peer_policy.py:307
assert_equal(self.getheaders_count(prefork), 0)
AssertionError: not(1 == 0)

So the base does send a getheaders to a pre-fork peer while a fork-aware one is connected.

With the patch — passes, 13s, all cases:

PASS D: manual pre-fork peer never asked for headers, in or out of IBD
PASS F: unconnecting compact block drew 0 getheaders for the pre-fork peer
PASS G: pre-fork peer received 1 getheaders with no fork-aware peer present
PASS H: pre-fork peer stayed at 1 once a fork-aware peer connected
PASS I: pre-fork peer went 0 -> 1 after the fork-aware peer left, no external stimulus
PASS J: the count tracked every connect and every disconnect path exercised
PASS K: low-work sync allowed only under the fallback, refused otherwise

Environment: Debian 13, gcc, cmake --build --target bitcoind bitcoin-cli, both builds v29.4.1.knots20260508.

G, H and I are the ones I'd point at for the "can this strand a node" question the exclusion turns itself off when there's no fork-aware peer, and back on when one appears.

@kwsantiago

Copy link
Copy Markdown

Tested on mainnet: fresh node, pre-fork peers only, so the fallback is active. Result is 0 headers, presync aborts at 961640 before anything is accepted. Am I missing the case where it helps?

@LionThunderFingers

Copy link
Copy Markdown
Author

Your pre-fork only case is right and nothing helps there... presync aborting at 961640 is correct. The fallback isn't meant to make that work... it's there so the pre-fork exclusion can't make things worse than the current default behaviour when there's no fork-aware peer to ask.

The case I had in mind is with mixed peers: header sync takes a slot, and unpatched there's no preference, so it can go to a pre-fork peer that dead-ends at the fork height while a fork-aware peer sits idle.

But I just went and measured that before answering, and it seems weaker than I assumed. Fresh node, default settings, 30 minutes: 10 of 14 peers advertised the bit. DNS seeding pulls in mostly fork-aware peers, so the slot landing on a pre-fork peer is less likely than I'd thought. I also couldn't get a figure for how long a wasted attempt on a pre-fork peer holds the slot... so I have nothing to show there.

So the behaviour change is real and the tests seem to work as intended, but I can't currently show it saves meaningful time on a default node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants