Skip to content

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

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

sipa
Copy link
Member

@sipa sipa commented Aug 31, 2022

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.

@sipa sipa force-pushed the 202208_headerssync_script branch 3 times, most recently from 0fd8a02 to 1cce8ca Compare September 1, 2022 02:25
@Sjors
Copy link
Member

Sjors commented Sep 1, 2022

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 1, 2022

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.

@sipa sipa force-pushed the 202208_headerssync_script branch from 1cce8ca to 7b0741e Compare September 1, 2022 14:22
@sipa
Copy link
Member Author

sipa commented Sep 1, 2022

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

params

@Sjors
Copy link
Member

Sjors commented Sep 2, 2022

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

@sipa
Copy link
Member Author

sipa commented Sep 7, 2022

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

@dergoegge
Copy link
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.

@DrahtBot DrahtBot mentioned this pull request Nov 14, 2022
18 tasks
@achow101 achow101 requested a review from sdaftuar April 25, 2023 15:16
@DrahtBot DrahtBot mentioned this pull request May 8, 2023
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK edfcb52


//! 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
Copy link
Contributor

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)

Copy link
Member Author

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.

@ajtowns
Copy link
Contributor

ajtowns commented Sep 26, 2023

@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?)

@sipa sipa force-pushed the 202208_headerssync_script branch from edfcb52 to 3d420d8 Compare September 28, 2023 16:11
@sipa
Copy link
Member Author

sipa commented Sep 28, 2023

@ajtowns Working on new graphs.

@sipa
Copy link
Member Author

sipa commented Sep 28, 2023

headerssync

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.

@ajtowns
Copy link
Contributor

ajtowns commented Sep 29, 2023

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

@ajtowns
Copy link
Contributor

ajtowns commented Sep 29, 2023

reACK 3d420d8

@sdaftuar
Copy link
Member

sdaftuar commented Oct 3, 2023

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.

@fanquake fanquake merged commit 2eacc61 into bitcoin:master Oct 5, 2023
@Sjors
Copy link
Member

Sjors commented Oct 5, 2023

Also the script needs +x

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants