Skip to content
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

During IBD, prune as much as possible until we get close to where we will eventually keep blocks #20827

Merged
merged 1 commit into from Jan 25, 2024

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Jan 2, 2021

This should reduce pruning flushes even more, speeding up IBD with pruning on systems that have a sufficient dbcache.

Assumes 1 MB per block between tip and best header chain. Simply adds this to the buffer pruning is trying to leave available, which results in pruning almost everything up until we get close to where we need to be keeping blocks.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 2, 2021

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK andrewtoth, fjahr, achow101
Concept ACK jonasschnelli, theStack, 0xB10C, jonatack, kristapsk

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.

@jonatack
Copy link
Contributor

jonatack commented Jan 4, 2021

Interesting, will have a look.

@jonasschnelli
Copy link
Contributor

Nice!
Concept ACK.
Could we further reduce flushes by allowing a user parameter (-pruneflushbuffer [or similar])? Some users are probably okay with providing a few GBs of extra space for speeding up pruning (assume hight dbcache) (but wan't the extra space be freed after IBD).
Or we could automatically use he dbcache size as the buffer size.

@Sjors
Copy link
Member

Sjors commented Jan 8, 2021

I also like the idea of temporally assigning more RAM for IBD. This could even be done by default for GUI users (with opt-out in the intro screen).

@luke-jr
Copy link
Member Author

luke-jr commented Jan 8, 2021

dbcache adjustments are ultimately an unrelated feature. I see #19873 as the next step in that area.

@DrahtBot
Copy link
Contributor

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

// So when pruning in IBD, increase the buffer to avoid a re-prune too soon.
if (is_ibd && target_sync_height > (uint64_t)chain_tip_height) {
// Since this is only relevant during IBD, we assume blocks are at least 1 MB on average
static constexpr uint64_t average_block_size = 1000000; /* 1 MB */
Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK we don't have access to future block sizes at this point.

@stickies-v
Copy link
Contributor

Without rebase, compiling fails for me:

$ make clean && ./configure CXXFLAGS="-O0 -g" CFLAGS="-O0 -g" && make -j 9
...
touch src/config/bitcoin-config.h.in
  CXX      util/libbitcoin_util_a-moneystr.o
  CXX      util/libbitcoin_util_a-readwritefile.o
  CXX      util/libbitcoin_util_a-settings.o
  CXX      util/libbitcoin_util_a-serfloat.o
  CXX      util/libbitcoin_util_a-spanparsing.o
  CXX      util/libbitcoin_util_a-strencodings.o
  CXX      util/libbitcoin_util_a-string.o
  CXX      util/libbitcoin_util_a-url.o
make[3]: *** No rule to make target `libunivalue.la'.  Stop.
make[2]: *** [univalue/libunivalue.la] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

After git rebase master, everything runs smoothly again. Will report back shortly with my review results!

@maflcko
Copy link
Member

maflcko commented Dec 16, 2021

@stickies-v If you want to compile older commits of master, you'll need to make distclean first.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 16, 2021

Without rebase, compiling fails for me:

Note: It's usually best to merge PRs into master for testing. Sometimes (bugfixes) they can be based on very old commits (when the bug was introduced); and if there's a silent problem with the merge, you'd want to notice that too.

@stickies-v If you want to compile older commits of master, you'll need to make distclean first.

Can we fix this build system bug?

@theStack
Copy link
Contributor

Concept ACK

@0xB10C
Copy link
Contributor

0xB10C commented Dec 20, 2021

Concept ACK

We'll be doing a Bitcoin Core PR Review club covering this PR on the 29th: bitcoincore.reviews/20827

@0xB10C
Copy link
Contributor

0xB10C commented Dec 26, 2021

#12404 (not merged) may also be interesting to reviewers. @Sjors did run a few benchmarks back then and found #11658 (which this PR overrides) to be the better option at that time.

I'm planing on benchmarking this change too.

fanquake added a commit that referenced this pull request Jan 2, 2022
2e42050 doc: fix undo data filename (s/undo???.dat/rev???.dat/) (Sebastian Falbesoner)

Pull request description:

  This typo was discovered in the course of a review club to #20827, see https://bitcoincore.reviews/20827#l-31.

ACKs for top commit:
  shaavan:
    ACK 2e42050

Tree-SHA512: 0c7a00dce24c03bee6d37265d5b4bc97e976c3f3910af1113f967f6298940f892d6fb517f7b154f32ccedb365060314d4d78d5eb2a9c68b25f0859a628209cd3
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 2, 2022
…?.dat/)

2e42050 doc: fix undo data filename (s/undo???.dat/rev???.dat/) (Sebastian Falbesoner)

Pull request description:

  This typo was discovered in the course of a review club to bitcoin#20827, see https://bitcoincore.reviews/20827#l-31.

ACKs for top commit:
  shaavan:
    ACK 2e42050

Tree-SHA512: 0c7a00dce24c03bee6d37265d5b4bc97e976c3f3910af1113f967f6298940f892d6fb517f7b154f32ccedb365060314d4d78d5eb2a9c68b25f0859a628209cd3
@fjahr
Copy link
Contributor

fjahr commented Jan 9, 2022

Concept ACK, I read the PR Review Club and will review after rebase.

@0xB10C
Copy link
Contributor

0xB10C commented Jan 10, 2022

fwiw: #23581 moved BlockManager to node/blockstorage, that's where the conflict with master happens.

I've rebased this PR in https://github.com/0xB10C/bitcoin/tree/2022-01-lukejr-ibd-prune-max-rebased and started benchmarking 0xB10C@db2a9e7 (PR) vs 0xB10C@2e01b69 (mergebase; MB).

I have 8 different prune and dbcache configurations set-up. Each configuration runs three times per binary (PR and MB).
That's 8 configs * 3 runs * 2 binaries = 48. I expect this to take about 6 days. Configurations:

1. -dbcache=300 -prune=550
2. -dbcache=4000 -prune=550

3. -dbcache=300 -prune=1100
4. -dbcache=4000 -prune=1100

5. -dbcache=300 -prune=2200
6. -dbcache=4000 -prune=2200

7. -dbcache=300 -prune=4400
8. -dbcache=4000 -prune=4400

Benchmarks run on a dedicated, properly cooled, and otherwise idle machine with two HDDs in a RAID0. The node syncs from a local node on the same machine and disks (connected via addnode and otherwise connect=0). The sync starts at block height 500_000 and ends at height 600_000. Between each run, the machine idles 5 minutes. The debug.log, with extra debug=coindb debug=prune logging, of each run is saved. I'll post results and graphs next week.

@0xB10C
Copy link
Contributor

0xB10C commented Jan 15, 2022

With a prune just before IBD, there is more blk and undo data available when running master than with this change. One potential issue that was mentioned during the PR review club:

On non-mainnet networks, the 1MB per block assumption doesn't hold. When we prune with a large nBuffer just before IBD ends, the prune might cause problems on larger reorgs as not enough undo data is present to revert back to the last shared block. Testnet has seen large reorgs in the past.

Might be worth thinking about potential alternatives to increasing the nBuffer by 1MB * remaining_blocks. There might be cleaner ways to implement "prune everything as long as we are not close to IBD being done". Haven't come up with a practical threshold for "IBD being close to done" yet that works for all networks.

@luke-jr
Copy link
Member Author

luke-jr commented Jan 15, 2022

The 1 MB per block assumption would mean at least 550 blocks. Testnet has deeper reorgs than that?

@luke-jr luke-jr marked this pull request as ready for review July 19, 2023 01:25
@luke-jr
Copy link
Member Author

luke-jr commented Jul 19, 2023

Rebased

@jonatack
Copy link
Contributor

Concept ACK

@kristapsk
Copy link
Contributor

Concept ACK

@BenWestgate
Copy link
Contributor

I have a project that syncs bitcoin core on an external drive and this would really help a lot of people testing it with smaller flash drives and higher RAM. So much so that I've made scripts to do manual pruning (back to 550mib) only when disk space is getting low to emulate this.

@achow101
Copy link
Member

Are you still working on this?

@@ -298,6 +298,7 @@ void BlockManager::FindFilesToPrune(
// Distribute our -prune budget over all chainstates.
const auto target = std::max(
MIN_DISK_SPACE_FOR_BLOCK_FILES, GetPruneTarget() / chainman.GetAll().size());
const uint64_t target_sync_height = chainman.m_best_header->nHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Any reason this declaration is up here and not directly above where it is used below?

// Since this is only relevant during IBD, we use a fixed 10%
nBuffer += target / 10;
// So when pruning in IBD, increase the buffer to avoid a re-prune too soon.
const auto chain_tip_height = chain.m_chain.Height();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could remove the cast in the next line with

Suggested change
const auto chain_tip_height = chain.m_chain.Height();
const uint64_t chain_tip_height = chain.m_chain.Height();

@andrewtoth
Copy link
Contributor

ACK d298ff8

Ran benchmark with hyperfine:

hyperfine --show-output --parameter-list commit 96ec3b67a7a7f968d002e13d6fc227f69b7f07d7,d298ff8b62b2624ed390c8a2f905c888ffc956ff --setup 'git checkout {commit} && make -j$(nproc) src/bitcoind' --prepare 'sync; sudo /sbin/sysctl vm.drop_caches=3; rm -r /home/user/.bitcoin/blocks /home/user/.bitcoin/chainstate' -M 3 './src/bitcoind -dbcache=2000 -prune=2000 -printtoconsole=0 -stopatheight=800000'

Results were 21% faster on this branch 🚀

Summary
  ./src/bitcoind -dbcache=2000 -prune=2000 -printtoconsole=0 -stopatheight=800000 (commit = d298ff8b62b2624ed390c8a2f905c888ffc956ff) ran
    1.21 ± 0.00 times faster than ./src/bitcoind -dbcache=2000 -prune=2000 -printtoconsole=0 -stopatheight=800000 (commit = 96ec3b67a7a7f968d002e13d6fc227f69b7f07d7)

@fjahr
Copy link
Contributor

fjahr commented Jan 12, 2024

utACK d298ff8

While there may be slightly better configurations possible, as @0xB10C pointed out above, I think the 1MB assumption is alright and this is a clear improvement already, so this can be merged and potentially improved later.

@DrahtBot DrahtBot removed the request for review from fjahr January 12, 2024 20:20
@achow101
Copy link
Member

ACK d298ff8

@achow101 achow101 merged commit 3672099 into bitcoin:master Jan 25, 2024
15 of 16 checks passed
@vostrnad
Copy link

15% improvement in IBD performance in some configurations seems significant enough for a release note, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet