#35642 headersync: do parameter search at runtime
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35642 headersync: do parameter search at runtime | Ready | 2 (+1) | Strong | 346 + 310 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Computes headers presync DoS-protection parameters (commitment period and redownload buffer size) at runtime in C++ whenever a headers sync starts, replacing static precomputed parameters stored in chainparams. Adds a closed-form optimization and bisection search algorithm in headerssync, stores the minimum chainwork height in chainparams consensus parameters, removes the offline python generation script, and adds fuzz and unit test suites.
Problem: Release maintainers had to manually run an offline Python script during each release and estimate the software's active lifespan to precompute static sync parameters. Hardcoded parameters degrade from optimality as time advances, the old python model had a bug overlooking attackers stopping mid-batch, and sizing parameters statically or once at startup risked skewing memory limits if the system clock was corrected after boot.
Evidence of importance:
- PR description: 'Less work for maintainers that need to occasionally run the current contrib/devtools/headerssync-params.py script. Less room for mistakes in that process. Automatically adapts based on time (as opposed to needing to predict the lifetime of the software).'
- Commit 3ddc510850: 'The old Python code did that, but actually got it (slightly) wrong by missing the fact that attackers are capable of strategically deciding to stop the attack in the middle of a batch (found by darosior).'
- Commit 990b3fa6d0: 'A node which starts up with a badly wrong clock and has it corrected before syncing would otherwise use a period sized for a much shorter chain than a peer can then feed it, inflating that bound (by a factor 50 for a clock which starts out before genesis).'
Scope: Implements mathematical optimization and continuous relaxation bisection in src/headerssync.cpp alongside extensive brute-force and fuzz testing.
Risk of a mistake: medium (p2p). Incorrectly computed parameters could degrade node DoS resistance against malicious peer headers chains or cause unexpected memory growth during headers sync.
Changed since the previous assessment: Parameters are now recomputed on demand each time a headers sync begins in PeerManagerImpl::TryLowWorkHeadersSync rather than once at node startup, protecting against subsequent clock adjustments, and exhaustive brute-force verification was added to the test suite.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: hodlinator to follow up on author's answers regarding parallel bandwidth and fractional attack_headers 2026-10-24
Agreement
Strong
substantive support, no open objection (darosior, l0rinc, l0rinc, fametrano)
Thread notes: Added fametrano's Tested ACK (c:6394267241). All claims remain settled with no pending action.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns (member) | question, safety | whether NodeClock::now() influencing the parameters causes an anonymity break | answered · author showed parameters change only roughly every 22 days and quantizing would not gain much | answered | yes | 2026-07-02: 'Is there any anonymity breaking via `NodeClock::now()` influencing these parameters?' Status from: 2026-07-02: sipa: 'It looks like the output parameters only change roughly every 22 days. I don't think quantizing further gains us much' |
| shuv-amp (contributor) | objection, correctness | presync height can exceed INT_MAX and overflow on long timewarp chains | fixed · fix pushed · author added a commit to use int64_t | changed | yes | 2026-08-12: 'For the long timewarp chains you mention, the presync height can exceed `INT_MAX` and overflow.' Status from: 2026-08-14: sipa: 'That's mostly independent of this PR, but added a commit to address that.' |
| shuv-amp (contributor) | objection, correctness | ComputeHeadersSyncParams() hangs indefinitely for small minchainwork heights and short timespans | fixed · fix pushed · fixed with bounded loop iterations and assumptions | changed | yes | 2026-08-14: 'ComputeHeadersSyncParams() doesn't terminate for a small minchainwork_headers combined with a short timespan' Status from: 2026-08-19: sipa: 'Fixed, and added some extra `Assume()`s in the code.' |
| darosior (member) | suggestion, maintenance | move the HeadersSyncParams definition from chainparams to the headersync module | fixed · fix pushed · moved to headerssync.h in a later commit | answered | yes | 2026-09-04: 'The `HeadersSyncParams` definition can now be moved from chainparams to the headersync module.' Status from: 2026-10-08: sipa: 'Done, in a later commit.' |
| darosior (member) | suggestion, approach | avoid square root indirection by using gamma and memory_factor directly | accepted · superseded when author rewrote the algorithm to drop batch assumptions | answered | yes | 2026-09-14: 'Why is this square root "indirection" necessary? Using `gamma = bufsize / period` (intuitively "bits of security") and `memory_factor = (max_headers - mcw_headers) / COMPACT_HEADER_SIZE` directly seem' Status from: 2026-09-18: darosior: 'ACK 526db3911d400b793e5715c0f989f1d3b66ac12e' |
| darosior (member) | objection, correctness | the parameter search underestimates attack rate by assuming attackers cannot abort mid-batch | fixed · fix pushed · rewritten with single-header arrival model | changed | yes | 2026-09-14: 'this routine underestimates the rate, because an incomplete batch sent by an attacker may be detected, but will still lead to processing as many headers from the buffer' Status from: 2026-09-15: sipa: 'Rewritten to take that approach. The commit history is simpler now too' |
| darosior (member) | suggestion, usefulness | asks whether Step 2 is still useful under the new approach and suggests dropping or simplifying it | accepted · fix pushed · author simplified Step 2 to check only floor and ceil of optimal period, which reviewer accepted | answered | yes | 2026-09-18: 'Is Step 2 really useful anymore with the new approach?' Status from: 2026-09-18: darosior: 'Neat.' |
| hodlinator (member) | suggestion, maintenance | avoid magic number 81 by computing sizeof(CBlockHeader) or improving the docstring | fixed · fix pushed · author adopted (sizeof(CBlockHeader) + 1) * 8 in NET_HEADER_SIZE | answered | yes | 2026-10-01: 'Rather than magic numbers, could we use `sizeof(CBlockHeader)` or compute it somehow, or improve the doc-string?' Status from: 2026-10-20: sipa: 'Done.' |
| l0rinc (member) | question, safety | asks why Assume is used rather than Assert for preconditions, and whether zero inputs could cause division by zero or infinity-to-integer conversion in production | accepted · fix pushed · author changed to Assert in code and reviewer accepted | answered | no | 2026-10-08: 'is there a reason to use `Assume` (rather than `Assert`) for these preconditions? Shouldn't these abort in production? It seems to me that zero inputs could theoretically still lead to division by zer' Status from: 2026-10-09: l0rinc: 'Thanks, please resolve' |
| l0rinc (member) | objection, correctness | the floating-point bisection loop can infinite-loop due to x87 excess precision | fixed · fix pushed · author switched to integer bisection over bufsize | changed | yes | 2026-10-08: 'Digging further with AI pointed to x87 excess precision: a midpoint can compare between the endpoints, then round back to one when stored, leaving the loop with no progress.' Status from: 2026-10-08: sipa: 'Fixed, but with a much better alternative: do the bisection over integer `bufsize` values' |
| l0rinc (member) | suggestion, maintenance | move HeadersSyncParams and return the struct directly from ComputeHeadersSyncParams instead of std::pair | fixed · fix pushed · moved HeadersSyncParams to headerssync.h in a later commit | answered | yes | 2026-10-08: 'Since we already have a `HeadersSyncParams` type could we move it and use it here as well (as @darosior also hinted I think)?' Status from: 2026-10-08: sipa: 'Done, in a later commit.' |
| l0rinc (member) | suggestion, maintenance | drop the headers_sync_params test override and use the existing fixed fuzz clocks | fixed · fix pushed · author removed the fuzz override and attributed the change | answered | yes | 2026-10-08: 'With the rewrite down to about 3 µs per call and fixed clocks already in these fuzz targets, is the override still worth keeping?' Status from: 2026-10-08: sipa: 'Done, and attributed.' |
| l0rinc (member) | suggestion, approach | track only fail_buf while growing the search range and derive succ_buf at the end | fixed · fix pushed · author adopted the single-bound growth simplification | answered | yes | 2026-10-08: 'Since `fail_buf = 2 * succ_buf + 1` throughout the growth loop, we can grow just `fail_buf` internally and derive `succ_buf` at the end.' Status from: 2026-10-08: sipa: 'Done.' |
| l0rinc (member) | suggestion, style | simplify the remainder calculation using the modulo operator | fixed · fix pushed · author adopted the modulo operator | answered | yes | 2026-10-08: 'Unless I'm missing something, this could be simplified to modulo, see https://godbolt.org/z/53PfMqxnj.' Status from: 2026-10-08: sipa: 'Done.' |
| l0rinc (member) | suggestion, maintenance | drop the unnecessary BasicTestingSetup fixture in headerssync_tests | fixed · fix pushed · author dropped the fixture | answered | yes | 2026-10-08: 'Nit: this only tests `ComputeHeadersSyncParamsInner` (which doesn’t need the node setup), could we drop the fixture?' Status from: 2026-10-08: sipa: 'Done.' |
| l0rinc (member) | suggestion, maintenance | keep the COMPACT_HEADER_SIZE assertion in the test rather than the implementation | answered · author declined to move the assert to avoid risk of disconnect | answered | yes | 2026-10-08: 'Since this assertion protects the test vectors (and since `ComputeHeadersSyncParamsInner` is already very complicated), I’d keep it in the test.' Status from: 2026-10-08: sipa: 'I didn't take this, as it risks a disconnect between the two' |
| l0rinc (member) | suggestion, style | separate expected headers from probability calculation in attack_rate | fixed · fix pushed · author adopted the separation | answered | yes | 2026-10-08: 'Nit: I find the preceding `2^-K * ...` form easier to follow. Could we separate the expected headers from the probability of passing the first `K` checks here?' Status from: 2026-10-08: sipa: 'Done.' |
| l0rinc (member) | suggestion, approach | explore whether restricting the buffer to whole commitment periods could eliminate both searches | answered · author prefers finding the exact optimum and added a comparison commit with a naive search | answered | yes | 2026-10-08: 'I wonder whether we need to find the optimum this precisely. If we restricted the buffer to whole commitment periods, we could calculate its size directly, and a logarithmic estimate might replace the' Status from: 2026-10-08: sipa: 'I still like the fact that it actually finds *the* optimal solution (within rounding precision) to a well-defined (but w' |
| l0rinc (member) | suggestion, approach | precompute std::log2(attack_headers) once in the lambda capture | fixed · fix pushed · author precomputed the logarithm in the capture | answered | yes | 2026-10-08: 'Nit: The attack budget stays fixed during this search, so we could calculate its logarithm once in the capture.' Status from: 2026-10-08: sipa: 'Done.' |
| l0rinc (member) | suggestion, style | describe the timespan parameter as genesis-to-now to match its caller | fixed · fix pushed · author updated the comment description | answered | yes | 2026-10-09: 'The caller uses the current time, even when the local tip is still genesis. I’d describe this as genesis-to-now.' Status from: 2026-10-09: sipa: 'Done.' |
| l0rinc (member) | suggestion, style | use std::midpoint for bisection midpoint calculations in both searches | fixed · fix pushed · author adopted std::midpoint | answered | yes | 2026-10-09: 'Now that both searches use integer bounds, we could use `std::midpoint` in both places.' Status from: 2026-10-09: sipa: 'Done. Cool, I didn't know about `std::midpoint`.' |
| l0rinc (member) | question, approach | asks whether a slightly less memory-efficient result within the model could be worth a simpler calculation, preserving the same security bound | answered · author explained that bufsize has a closed-form formula and further simplifying period search would sacrifice too much accuracy | answered | yes | 2026-10-09: 'Could a slightly less memory-efficient result within that model be worth a simpler calculation, while preserving the same security bound?' Status from: 2026-10-09: sipa: 'It turns out that no simplification is needed, since we already have a closed-form function that finds the optimal bufsi' |
| l0rinc (member) | suggestion, style | refresh comments after search rewrite to explain clamp bounds and real-valued formulas | fixed · fix pushed · author refreshed comments | answered | yes | 2026-10-09: 'With the integer search, a zero height would give Step 2 invalid clamp bounds, so I would update this explanation.' Status from: 2026-10-09: sipa: 'Done.' |
| l0rinc (member) | suggestion, maintenance | report the failing period at boundary failure and initialize best memory to maximum in brute force test | answered · author dropped the test commit because attack_rate was removed entirely | answered | yes | 2026-10-09: 'I would find the failing period more useful than a failure count here.' Status from: 2026-10-09: sipa: 'I have dropped this commit, because `attack_rate` is now gone entirely' |
| l0rinc (member) | suggestion, approach | encapsulate legal period bounds in the search so Step 2 can use its two candidates directly | fixed · fix pushed · author encapsulated legal bounds in the search | answered | yes | 2026-10-09: 'Would it make sense to encapsulate the bounds on legal periods in the search, so Step 2 can use its two candidates directly?' Status from: 2026-10-09: sipa: 'Done.' |
| l0rinc (member) | suggestion, safety | defer the headers sync parameter calculation until after clock checks to prevent inflated commitment allowance if the startup clock was wrong | accepted · fix pushed · author changed calculation to runtime per connection, which reviewer confirmed addressed the clock correction concern | answered | yes | 2026-10-10: 'could we defer the estimate after the clock checks?' Status from: 2026-10-20: l0rinc: 'addressing my clock-correction concern' |
| hodlinator (member) | suggestion, maintenance | reduce duplication with the header comments by referring to them, boiling down, or merging | fixed · fix pushed · author compacted the comment text | answered | yes | 2026-10-13: 'Could we refer to that instead, boil this down further, or merge them?' Status from: 2026-10-20: sipa: 'I've compacted the new text a bit.' |
| hodlinator (member) | suggestion, maintenance | include the code for generating and filtering test record values | fixed · fix pushed · author included the generator code in a comment in headerssync_tests.cpp | answered | yes | 2026-10-13: 'including the code for generating and filtering would be helpful for the next human needing update the record values' Status from: 2026-10-20: sipa: 'I've included the code in a comment, so it doesn't affect code coverage stats, but still allows re-running' |
| l0rinc (member) | suggestion, maintenance | validate expected test record values against the attack-rate formula | fixed · fix pushed · author added optimality checks verifying the test vectors | answered | yes | 2026-10-20: 'Could we also validate these expected values against the attack-rate formula?' Status from: 2026-10-21: sipa: 'Done, and went further by implementing more checks to verify optimality of the test vectors.' |
| l0rinc (member) | suggestion, maintenance | update release-process.md and description to state parameters are computed at runtime when headers sync begins | fixed · fix pushed · author updated release-process.md | answered | yes | 2026-10-20: 'The release-process note and PR description could say that the parameters are computed at runtime whenever a headers sync begins' Status from: 2026-10-21: sipa: 'Done.' |
| l0rinc (member) | suggestion, maintenance | move test vector generator script from comments to contrib/testgen/ | accepted · author declined to move script and reviewer dropped the suggestion | answered | yes | 2026-10-20: 'I'd find this easier to rerun as a script in `contrib/testgen/`, with a reference here.' Status from: 2026-10-21: l0rinc: 'Thanks for considering, please resolve the suggestion.' |
| hodlinator (member) | suggestion, maintenance | clarify comment on timewarp chains minimizing difficulty to allow faster hashing | answered · author explained why starting at genesis is optimal for the attack and noted it will be addressed if retouched | answered | yes | 2026-10-23: 'nit: Decreasing the difficulty allows even faster hashing?' Status from: 2026-10-24: sipa: 'If they fork off after genesis, indeed, but in this attack scenario... I believe it is always better to start at genesis' |
| hodlinator (member) | question, approach | asks why attackers' bandwidth is modeled jointly rather than allowing multiple peers to send headers in parallel | answered · author explained that victim network connection and hash rate bound total incoming attacker bandwidth | answered | yes | 2026-10-23: 'Why jointly, isn't it more correct that multiple headers sync peers could be sending you these low-difficulty headers in parallel?' Status from: 2026-10-24: sipa: 'their joint bandwidth to the victim is what matters, not their individual bandwidth' |
| hodlinator (member) | question, approach | asks how fractional attack_headers (< 1) should be interpreted and whether zero attack headers are accepted | answered · author clarified the value represents expected headers accepted per attack attempt | answered | yes | 2026-10-23: 'Does that mean zero attack headers are accepted? Or do we round it up to only 1 attack header? Would it be possible to better explain this in the comment?' Status from: 2026-10-24: sipa: 'The number is correct, in expectation. We permit 0.00003478 headers to be accepted per attack.' |
Support:
- sedited (member) (Concept ACK, 2026-07-24): no reason given [not substantive]
- ajtowns (member) (Approach ACK, 2026-08-06): Approach ACK ; needs rebase if wanted for 32.0 I guess? [not substantive]
- darosior (member) (Concept ACK, 2026-08-06): no reason given [not substantive]
- fjahr (member) (Concept ACK, 2026-08-21): no reason given [not substantive]
- darosior (member) (ACK, 2026-09-18) [read]: reviewed calculation from scratch; the latest approach is easier to grasp and the looser upper bound is the right tradeoff
- darosior (member) (ACK, 2026-09-18): re-ACK 0e473d175224746c13678db5106f9d4ab1086b9d [not substantive]
- darosior (member) (ACK, 2026-09-30): re-ACK 9f7bcfe8747 [not substantive]
- l0rinc (member) (Concept ACK, 2026-10-08) [read]: calculating the parameters at startup from elapsed time since genesis, instead of predicting the release's lifetime
- l0rinc (member) (ACK, 2026-10-09): Only had time for a quick pass and left a few questions. I'll review more carefully later. [not substantive]
- l0rinc (member) (ACK, 2026-10-10): The clock correction case still seems worth addressing here or in a follow-up, but isn't blocking for me, it's not systemic. [not substantive]
- hodlinator (member) (Concept ACK, 2026-10-13): Will see if I find time to dig into the math. [not substantive]
- l0rinc (member) (Tested ACK, 2026-10-20) [both]: parameters are computed whenever a headers sync starts, addressing clock-correction concern, and test-vector generation script is included
- l0rinc (member) (ACK, 2026-10-21): reACK 0316d0dc9ec6a1abbe84489c44a47a2dd94c79ee [not substantive]
- l0rinc (member) (Tested ACK, 2026-10-21) [tested]: Lightly tested diff re-ACK 0457cec22a273eba31e92950bd485bf72c864ab0 [not substantive]
- fametrano (contributor) (Tested ACK, 2026-10-25) [both: p2p_headers_sync_with_minchainwork.py, HeadersSyncState]: verified the functions return the exact optimum across chains, heights, and dates; ran unit and p2p tests and compared parameters against a Python port and brute-force search
Participants:
- ajtowns (member, support, 2 statements 2026-07-02 to 2026-08-06): Approach ACK; asked about potential anonymity impact of NodeClock::now()
- sedited (member, support, 1 statements 2026-07-24 to 2026-07-24): Concept ACK
- darosior (member, objection, 13 statements 2026-08-06 to 2026-09-30): ACK; found mid-batch abort rate underestimation; suggested simplifying Step 2 and moving param struct
- shuv-amp (contributor, objection, 2 statements 2026-08-12 to 2026-08-14): flagged potential int overflow in presync height and solver infinite loops on edge-case inputs
- fjahr (member, support, 1 statements 2026-08-21 to 2026-08-21): Concept ACK
- hodlinator (member, support, 19 statements 2026-10-01 to 2026-10-24): Concept ACK; suggested sizeof(CBlockHeader), doc compaction, testgen inclusion; asked about parallel attack bandwidth and fractional attack_headers
- l0rinc (member, objection, 31 statements 2026-10-08 to 2026-10-21): Tested ACK after author addressed clock-correction concern; suggested testgen script placement and attack-rate validation; re-ACKed
- fametrano (contributor, support, 1 statements 2026-10-25 to 2026-10-25): Tested ACK; verified the model optimum across chains, heights, and dates, and confirmed unit and p2p tests pass
Files
679 lines under test/bench/ci.
- contrib/devtools/headerssync-params.py +0/-357
- src/headerssync.cpp +278/-4
- src/test/headerssync_tests.cpp +246/-0
- src/test/fuzz/headerssync.cpp +63/-2
- src/kernel/chainparams.cpp +11/-30
- src/headerssync.h +32/-0
- src/net_processing.cpp +19/-1
- src/kernel/chainparams.h +0/-12
- contrib/devtools/README.md +0/-10
- doc/release-process.md +1/-8
- src/consensus/params.h +4/-0
- src/net_processing.h +1/-0
- src/test/CMakeLists.txt +1/-0
Card
The short text the ranking pass compares across PRs.
This pull request calculates headers presync DoS-protection parameters (commitment period and redownload buffer size) at runtime whenever a headers sync starts, removing static precomputed values from chainparams. This eliminates an error-prone release step where maintainers had to predict the release's lifespan using an offline script, while automatically adapting memory bounds as real time progresses. The offline Python tool is deleted and replaced with a closed-form C++ optimization algorithm, backed by unit tests verifying against brute-force search optima and dedicated fuzz targets. No dependencies.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-26T01:21, confidence high, input hash 332512762efab0d5
thread read through 2026-10-25 (119 statements), head 0316d0dc9e
labels: none
thread read through 2026-10-25 (119 statements), head 0316d0dc9e
labels: none
Categories
P2P
P2 · DoS protection
Calculates headers presync parameters dynamically at runtime to ensure optimal DoS resistance and memory bounds indefinitely. Hardcoded parameters degraded over time and could inflate peer memory bounds by a factor of 50 if the system clock was corrected after boot, while also resolving an attack model flaw where an attacker halts mid-batch.
Membership: Modifies src/headerssync.cpp, src/headerssync.h, and src/net_processing.cpp to dynamically calculate DoS-protection parameters when headers sync starts.
Factors: security/stability 2, bug 1, performance 0, user value 0, leverage 1