Add headerssync tuning parameters optimization script to repo #25970

1. 
   sipa commented at 10:54 pm on August 31, 2022: member

   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:

   • It would be a bit cleaner to have these parameters be part of 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 to headerssync.cpp. Keeping them as compile-time evaluatable constants also has a (likely negligible) performance impact (avoiding runtime modulo operations).
   • If we want to make sure the chainparams and headerssync params don’t go out of date, it could be possible to run the script in CI, and and possibly even have the parameters be generated automatically at build time. I think that’s overkill for how unfrequently these need to change, and running the script has non-trivial cost (~minutes in the normal python interpreter).
   • A viable alternative is just leaving this out-of-repo entirely, and just do ad-hoc updating from time to time. Having it in the repo and release notes does make sure it’s not forgotten, though adds a cost to contributors/maintainers who follow the process.
2. sipa force-pushed on Aug 31, 2022
3. sipa force-pushed on Aug 31, 2022
4. sipa force-pushed on Sep 1, 2022
5. fanquake added the label P2P on Sep 1, 2022
6. fanquake added the label Scripts and tools on Sep 1, 2022
7. 
   Sjors commented at 9:32 am on September 1, 2022: member

   Concept ACK
8. 
   DrahtBot commented at 10:54 am on September 1, 2022: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ajtowns
   Concept ACK	Sjors, dergoegge, sdaftuar

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

9. sipa force-pushed on Sep 1, 2022
10. 
    sipa commented at 10:01 pm on September 1, 2022: member

    This shows worst-case per-peer memory usage for headers synchronization, assuming the parameters are regularly updated (3 years beforehand), or never updated past this PR (assuming growth at 600 seconds per block, and minchainwork lagging behind a constant time).

    

11. 
    Sjors commented at 11:50 am on September 2, 2022: member

    @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)
12. 
    sipa commented at 0:25 am on September 7, 2022: member

    @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.
13. sipa force-pushed on Sep 7, 2022
14. 
    dergoegge commented at 6:09 pm on September 15, 2022: member

    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.

15. achow101 requested review from sdaftuar on Apr 25, 2023
16. DrahtBot added the label CI failed on May 30, 2023
17. DrahtBot removed the label CI failed on May 31, 2023
18. in doc/release-process.md:49 in edfcb52c99 outdated

    42@@ -43,6 +43,14 @@ Release Process
    43     - On mainnet, the selected value must not be orphaned, so it may be useful to set the height two blocks back from the tip.
    44     - Testnet should be set with a height some tens of thousands back from the tip, due to reorgs there.
    45   - `nMinimumChainWork` with the "chainwork" value of RPC `getblockheader` using the same height as that selected for the previous step.
    46+* Consider updating the headers synchronization tuning parameters to account for the chainparams updates.
    47+  The optimal values change very slowly, so this isn't strictly necessary every release, but doing so doesn't hurt.
    48+  - Update configuration variables in [`contrib/devtools/headerssync-params.py`](contrib/devtools/headerssync-params.py):
    49+    - Set `TIME` to a reasonable time the software is expected to work well, for example the expected release data plus 3 years.
    

    ---

    ---

    

    ajtowns commented at 6:40 am on September 25, 2023:

    “date” not “data”

    (Could perhaps clarify this along the lines of “the software’s expected supported lifetime – after this time, its ability to defend against a high bandwidth timewarp attacker will begin to degrade”. As the graph makes clear though, it doesn’t degrade that much – after 20 years, it’ll just mean an old node uses 1.5MB to track an attacking peer’s claimed PoW, whereas the latest release with updated params will need under 1MB. so it’s not any kind of strict cut off).

    ---

    

    sipa commented at 4:11 pm on September 28, 2023:

    Done.
19. in src/headerssync.cpp:19 in 20e59ad816 outdated

    19+constexpr size_t HEADER_COMMITMENT_PERIOD{589};
    20 
    21 //! Only feed headers to validation once this many headers on top have been
    22 //! received and validated against commitments.
    23-constexpr size_t REDOWNLOAD_BUFFER_SIZE{13959}; // 13959/584 = ~23.9 commitments
    24+constexpr size_t REDOWNLOAD_BUFFER_SIZE{14059}; // 14059/589 = ~23.9 commitments
    

    ---

    ---

    

    ajtowns commented at 6:44 am on September 25, 2023:

    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)

    ---

    

    sipa commented at 4:13 pm on September 28, 2023:

    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.
20. 
    ajtowns commented at 9:17 am on September 25, 2023: contributor

    ACK edfcb52c995bfeec99e892b59ee48bdfb96bcf05
21. 
    ajtowns commented at 6:47 am on September 26, 2023: contributor

    @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.

    Perhaps plot the the two lines based on:

    • using the proposed parameters in this PR indefinitely (ie, showing how badly the security against a worst case low work attacker degrades over time)
    • bumping the parameters every six months, with time being bumped by 182.5 days and chainwork headers increasing by 26280 (ie, what you get if you upgrade to the latest release whenever it comes out)
    • (maybe add another line, that tracks performance if you’re constantly 1/2/3 years behind the current release?)
22. Add headerssync-params.py script to the repository 7899402cff
23. Update parameters in headerssync.cpp 53d7d35b58
24. Add instructions for headerssync-params.py to release-process.md 3d420d8f28
25. sipa force-pushed on Sep 28, 2023
26. 
    sipa commented at 4:13 pm on September 28, 2023: member

    @ajtowns Working on new graphs.
27. 
    sipa commented at 6:50 pm on September 28, 2023: member

    

    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.

28. 
    ajtowns commented at 6:49 am on September 29, 2023: contributor

    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
29. 
    ajtowns commented at 8:39 am on September 29, 2023: contributor

    reACK 3d420d8f28f2d351abf8b0afe90848110e15d50c
30. 
    sdaftuar commented at 11:45 pm on October 3, 2023: member

    ACK.

    I didn’t figure out exactly what’s going on in the optimize function, but I did verify that my dumb brute force search using the functions I did understand (find_bufsize and memory_usage) produced the same results, so that seems pretty good to me.

31. sdaftuar approved
32. fanquake merged this on Oct 5, 2023
33. fanquake closed this on Oct 5, 2023

    ---

34. in doc/release-process.md:48 in 3d420d8f28

    42@@ -43,6 +43,14 @@ Release Process
    43     - On mainnet, the selected value must not be orphaned, so it may be useful to set the height two blocks back from the tip.
    44     - Testnet should be set with a height some tens of thousands back from the tip, due to reorgs there.
    45   - `nMinimumChainWork` with the "chainwork" value of RPC `getblockheader` using the same height as that selected for the previous step.
    46+* Consider updating the headers synchronization tuning parameters to account for the chainparams updates.
    47+  The optimal values change very slowly, so this isn't strictly necessary every release, but doing so doesn't hurt.
    48+  - Update configuration variables in [`contrib/devtools/headerssync-params.py`](contrib/devtools/headerssync-params.py):
    

    ---

    ---

    

    Sjors commented at 3:01 pm on October 5, 2023:

    I think the link needs a / prefix otherwise the link is broken.

    ---

    

    fanquake commented at 3:05 pm on October 5, 2023:

    Fixed in #28591.
35. 
    Sjors commented at 3:02 pm on October 5, 2023: member

    Also the script needs +x
36. Frank-GER referenced this in commit b72b9042d9 on Oct 13, 2023
37. bitcoin locked this on Oct 4, 2024

Contributors
sipa Sjors DrahtBot dergoegge ajtowns sdaftuar fanquake

Labels
P2P Scripts and tools