Repository navigation
net: prefer NODE_BLAKE2B for every outbound slot, and stop churning - #398
kwsantiago wants to merge 4 commits into
Conversation
CheckStaleOutboundRoom answers whether one more automatic outbound peer lacking NODE_BLAKE2B would be tolerated as an additional stale connection. DemoteToStaleOutbound now asks it instead of counting inline, so the same question can be asked before a connection is opened. No behavior change: the counts, the order of the three conditions and the log messages are unchanged.
22c6788 to
e0b27f3
Compare
kwsantiago
changed the title
jasonsopko
left a comment
jasonsopko
left a comment
There was a problem hiding this comment.
Tested ACK e0b27f3, with one gap the same accounting leaves open and a rebase needed.
What I ran. denialofservice_tests passes. p2p_blake2b_outbound_slots.py, p2p_blake2b_outbound_preference.py, p2p_stale_outbound.py and p2p_handshake.py pass, the last two with --v2transport as well. On this build the four cases come out as: A, 8 dials then nothing further; B, all eight full-relay and both block-relay slots NODE_BLAKE2B with no refused handshake; C, 40 lying addresses worked through once each; D, 5 dead addresses dialed once each. Against a 29.x-knots build (8c85b15, whose net.cpp differs from this branch's base only by the v2-only and GetnScore changes) the same test fails at case A's quiet window, 47 dials against 18 with 12 refused handshakes in the 25 s it ran. CheckStaleOutboundRoom with exclude gives the same answer the old same_target > max form did, and the Assume on the connection type holds because the only caller demotes full-relay and block-relay peers.
Mainnet. One run of this build from an empty datadir, DNS seeds only, -listen=0, cut short at 4.5 minutes by the box it shares with a live node: all eight full-relay slots were fork-capable peers by the 90 s sample, both block-relay slots by 90 s as well, 5 stale peers tolerated and 1 refused in that time. What churned after that was 26 pre-fork peers banned for serving the SHA256d chain's headers and IBD stalling, which this PR does not touch. No base run to set against it; your 8-minute numbers stand.
The extra peers. prefer_blake2b is false whenever the target is already full, which is exactly the state of the three branches that open a peer past it: the stale-tip GetTryNewOutboundPeer() peer, the periodic extra block-relay peer and the extra network peer. A peer without the bit is refused there anyway (the outbound target is already full), so those dials can only churn. I filled every slot with fork-capable peers, left IBD, aged the tip past 30 minutes and watched what the stale-tip peer dialed: 30 addresses, 29 of them pre-fork, 28 handshakes refused, before one fork-capable peer landed. That is the moment the node most needs a peer with a better chain. Preferring the bit for both persistent types regardless of the counts, with blake2b_required left as it is, brings that to 2 dials and no refusal, and the three slot tests still pass:
- const bool prefer_blake2b{(conn_type == ConnectionType::OUTBOUND_FULL_RELAY && nOutboundFullRelay < m_max_outbound_full_relay) ||
- (conn_type == ConnectionType::BLOCK_RELAY && nOutboundBlockRelay < m_max_outbound_block_relay)};
+ const bool prefer_blake2b{conn_type == ConnectionType::OUTBOUND_FULL_RELAY || conn_type == ConnectionType::BLOCK_RELAY};The test for it is a fifth case on top of your harness (peers announce the tip after generate, or ConsiderEviction drops them all at 22 minutes); it is at https://gist.github.com/jasonsopko/a5c5e9b2b290a79cf60f1b116b7ca4fd with the diff as a commit.
Rebase. The branch sits on 1cfb2db, 1737 commits behind 29.x-knots. A mainnet node built from it refuses to start without -blake2b_headline, and its test framework still passes that retired argument, which is why the new test cannot run against a release binary as-is. Nothing in the net code moved underneath it, so the review stands, but it should come forward before merge.
The preference only guarded the first two outbound-full-relay slots. Once two fork-capable peers connected it switched off and the remaining six were drawn at random from an addrman that is almost entirely pre-fork, so they filled with peers that are demoted to stale and never count towards the target. Block-relay slots were not covered, and neither were the extra peers opened once a target is already full, where a peer without the bit is refused outright and the dial can only churn. Prefer a NODE_BLAKE2B peer for both persistent outbound types. With 1000 pre-fork and 13 fork-capable addresses, a 1.28% share taken from a mainnet node, filling all eight slots goes from 269 seconds and 537 connections to 18 seconds and 15. The old shape is visible in the per-slot timings: the first two arrive after 2 and 3 seconds, the remaining six only at 56, 99, 108, 169, 191 and 269. On the stale-tip peer, where the node most needs a better chain, it goes from 8 dials and 6 refused handshakes to 2 and none.
A peer without NODE_BLAKE2B is demoted to stale on VERSION, and refused outright once -maxstaleoutbound are already tolerated, its outbound target is occupied or the inbound budget is full. Because a demoted peer stops counting towards the outbound target, ThreadOpenConnections never reaches that target and kept falling back to such a peer every 500 ms: connect, handshake, disconnect, forever. Ask the same question before dialing and skip the fallback when the answer is no. On a network with no fork-capable peers a node now opens its eight outbound connections, keeps them as stale peers and stops, instead of opening and dropping one every 500 ms indefinitely. A node that can find fork-capable peers is unaffected: it still dials them, including when its own configuration leaves no room for a stale peer at all. The budget that answer depends on is -maxstaleoutbound, which lives in PeerManager::Options, so the question goes out through NetEventsInterface like HasAllDesirableServiceFlags rather than keeping a second copy of the option in CConnman::Options. It is asked once per pass of the connection loop, not once per candidate address.
e0b27f3 to
7b22201
Compare
|
@jasonsopko Taken, folded into commit 2 with your case. Base stays, since Knots assembles by merging each spec entry's branch, so this sits on k386's tip. See https://github.com/luke-jr/tmp/blob/knots-spec/knots-29.4.1.knots20260508.spec#L1749 and https://github.com/bitcoinknots/assemble-deriv |
jasonsopko
left a comment
jasonsopko
left a comment
There was a problem hiding this comment.
Tested ACK 7b22201.
The fold is my line verbatim with the comment extended, and case E is the same test with a steadier wait: it stops on a dial-free window rather than on a count, which also removes the flake the count had when addrman drops an address to a bucket collision. On this head denialofservice_tests passes, p2p_blake2b_outbound_slots.py passes all five cases in 2m38s, case E ending on the stale-tip peer dialing only fork-capable addresses with one feeler untouched, and p2p_blake2b_outbound_preference.py, p2p_stale_outbound.py and p2p_handshake.py pass, the last two with --v2transport as well.
I was wrong about the base. I had not looked at how the release is assembled; with each spec entry merged in from its own branch, this one belongs on top of the commit k386 ends on, which is where it is. Withdrawn. The one practical note that survives is that a node built from this branch alone still needs -blake2b_headline to start on mainnet, so anyone testing it there wants the assembled tree.
|
The |
A SOCKS5 proxy stands in for the network so the node dials addresses out of addrman for real and a python peer answers with chosen service flags, which is what it takes to observe the outbound target being filled. Case B keeps some of those peers pre-fork so that dialing one is observable rather than quietly refused by the proxy, and case C has them all answer without the bit their address claimed, which is what a fixed seed looks like: net.cpp stamps those with SeedsServiceFlags() without knowing. Cases A, C and D fail on the parent of this series: A and C each see 30 further dials in their quiet window instead of none, and D sees 29. Case B is covered by the preference rather than the fallback, so it is measured against that change alone: narrowing the preference back to the first two slots makes it see 166 peers without NODE_BLAKE2B connected and dropped at the handshake, instead of none.
7b22201 to
b947864
Compare
jasonsopko
left a comment
jasonsopko
left a comment
There was a problem hiding this comment.
Tested ACK b947864.
Test-only change: the three code commits are the same SHAs I reviewed on 7b22201, and git diff between the two heads touches nothing under src/. The rewrite is 23 lines in cases D and E: case D has the proxy close the dial instead of pointing it at a dead port, and case E takes the log offset before clearing the dial list and waits for the extra peer to land instead of sampling a fixed window.
On this head denialofservice_tests passes; p2p_blake2b_outbound_slots.py passes three runs in a row, 2m23s each, with the same counts every time (A stops after 8 dials, B fills all 8 full-relay and 2 block-relay slots with NODE_BLAKE2B peers, C works through 40 and stops, D tries each of 5 once and backs off, E dials only fork-capable addresses for the stale-tip peer with one feeler untouched); p2p_blake2b_outbound_preference.py, p2p_stale_outbound.py and p2p_handshake.py pass, the last two with --v2transport as well.
The rewritten test still catches what it is for. Against a binary built from the 29.x-knots tip (8c85b15, plus #400, which touches only script.cpp), the four cases the commit message says should fail without the change do fail: A sees 30 further dials in its quiet window (47 against 17) with 12 refusals; B connects and refuses 127 peers without the bit; C and D never reach a quiet window at all, 471 dials in four minutes for C and 450 for D's five dead addresses, so both time out in the wait rather than failing on the count. Case E was not run on the base binary.
The proxy logs a ConnectionResetError at the start of case C in every run, from a connection the node still had open when case B stopped it. Harmless; noting it so nobody chases it.
|
Two of the nine mainnet seeds answer the Addrman is the weaker source: of 120 entries claiming NODE_BLAKE2B, 38 answered and only 8 still advertise it, since the flag comes from ADDR gossip and is only corrected on VERSION. Measured against such an addrman today, same peers.dat for both, no DNS or fixed seeds, 8 minutes each: base made 103 dials, this branch 32, both ending with 1 fork-capable outbound-full-relay peer. |
|
On whether this trusts addrman's service bits: it does, and that bit is gossip. Of 120 entries claiming NODE_BLAKE2B, 38 answered and 8 had it.
So a wrong claim costs one dial and is then disproved, and the cost returns only when something re-asserts it: 50 claimed addresses cost 50 dials to work through, then 43 more in the hour after the claims were re-added. Even with the bits that wrong, the preference is what decides the outcome. Same real addrman copied to both, 600s each: 29.4.1 ended with 0 of its 8 outbound-full-relay slots fork-capable after 414 dials, this branch with 6 of 8 after 341. |
|
Re-measured with matched runs and updated the description: from one addrman, 600s each, this branch ends with 6 of 8 outbound-full-relay slots fork-capable against 0 of 8 on 29.4.1. |
3 participants
The NODE_BLAKE2B preference added in #386 guarded only the first two outbound-full-relay slots. Once two fork-capable peers connected it switched off, and the remaining six were drawn from an addrman that is almost entirely pre-fork. Such a peer is demoted to stale and stops counting towards the outbound target, so
ThreadOpenConnectionsnever reached that target and kept falling back to one every 500 ms: connect, handshake, disconnect.Three changes:
SetServicesonly corrects addrman on a received VERSION and an address that claims the bit but never answers would otherwise be redialed every pass.A node that can find fork-capable peers still dials them; one that cannot still bootstraps, keeps them as stale peers and then stops.
One addrman (5511 entries, 3.08% claiming the bit) copied to both, mainnet, 600s each:
On the
p2p_blake2b_outbound_slots.pyscaffolding with 13 fork-capable addresses among 1013, filling all eight slots goes from 311s and 621 dials to 24s and 20. With none reachable at all, base works through 178 dials and holds 9 connections, this branch holds 8 after 8 dials and stops.Those bits are gossip and mostly wrong: of 120 entries claiming NODE_BLAKE2B, 38 answered and 8 still advertised it, since
AddSingleORs a claim back in (addrman.cpp:574) after a handshake has corrected it. A wrong claim costs one dial and is then disproved, and returns only if re-asserted: 50 claimed addresses cost 50 dials to work through, then 43 more in the hour after they were re-added.The first commit is a pure refactor, covered by existing tests unchanged.
p2p_blake2b_outbound_slots.pycovers five cases and fails on the parent of this series. #399 is the same accounting seen from the other side.test each commitfails at the base commit, 1cfb2db, ontool_cli_completion.py, which knots#365 skips in PR CI.