-
Notifications
You must be signed in to change notification settings - Fork 37.3k
Add headerssync tuning parameters optimization script to repo #25970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0fd8a02
to
1cce8ca
Compare
Concept ACK |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
1cce8ca
to
7b0741e
Compare
@sipa why does the chart start three years in the future? In case you remake it (not really necessary), can you extrapolate to 2106? (users will have to download some sort of update before then anyway, so it's a natural ending point for the green line) |
@Sjors This graph is showing the result of showing effect of, and optionally optimizing for, a state 3 years in the future. When I'm back from travel next week I'll redo the graphs in a more intuitive way. |
7b0741e
to
edfcb52
Compare
Concept ACK on having this in the repo. I have partially reviewed previous versions of this script and will fully review the current version soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK edfcb52
src/headerssync.cpp
Outdated
|
||
//! Only feed headers to validation once this many headers on top have been | ||
//! received and validated against commitments. | ||
constexpr size_t REDOWNLOAD_BUFFER_SIZE{13959}; // 13959/584 = ~23.9 commitments | ||
constexpr size_t REDOWNLOAD_BUFFER_SIZE{14059}; // 14059/589 = ~23.9 commitments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script now suggests 608 and 14487 (TIME=2026/11/15, MCWH=809261)
(Bumping TIME
by six months seems to have a much bigger impact than bumping MCWH by 26k blocks; but if we're going to optimise things, seems silly to do anything other than optimise for the actual value of MCWH)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased the PR, and updated the numbers to roughly match the code currently in the repository (with dates/chainwork corresponding to the 25.0 release). Updating for 26.0 would be done when updating the chainparams for 26.
Perhaps plot the the two lines based on:
|
edfcb52
to
3d420d8
Compare
@ajtowns Working on new graphs. |
This shows the worst-case memory over time (maximum of normal sync memory usage and timewarp attack scenario), for 6 different configurations (created 50 weeks apart) with the minchainwork_headers value set at the time of configuration, and the time value set 3 years later than the configuration time. |
So the 2023-05 params which are set to work well until 2026-05, continue working better than the 2024-04 params until about 2026-10; by 2027-05 they're performing perhaps ~15kB (?) worse than the 2024-05 params. Interesting that for the normal lifetime of a release (eg 2024-05 through 2024-11) that release will use ~20kB more memory than earlier releases that are still supported, due to the bump in bufsize. Seems fine to me |
reACK 3d420d8 |
ACK. I didn't figure out exactly what's going on in the |
Also the script needs |
Builds upon #25946, as it incorporates changes based on the selected values there.
This adds the headerssync tuning parameters optimization script from https://gist.github.com/sipa/016ae445c132cdf65a2791534dfb7ae1 to the repository, updates the parameters based on its output, and adds release process instructions for doing this update in the future.
A few considerations:
CChainParams
, but due to the nature of the approach, it really only applies to chains with unforgeable proof-of-work, which we really can only reasonably expect from mainnet, so I think it's fine to keep them local toheaderssync.cpp
. Keeping them as compile-time evaluatable constants also has a (likely negligible) performance impact (avoiding runtime modulo operations).