Skip to content

Conversation

andrewtoth
Copy link
Contributor

@andrewtoth andrewtoth commented Aug 7, 2023

Since #17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the dbcache limit.

Periodic flushes occur every 24 hours, which empties the cache and causes block connection to slow down. By keeping the cache through periodic flushes a node can run for several days with an increasingly hotter cache and connect blocks much more quickly. Now not only can setting a higher dbcache value be beneficial for IBD, it can also be beneficial for connecting blocks faster.

To benchmark in real world usage, I spun up 6 identical t2.small AWS EC2 instances, all running in the same region in the same VPC. I configured 2 instances to run master, 2 instances to run the change in this PR, and 2 instances to run the change in this PR but with dbcache=1000. All instances had prune=5000 and a 20 GB gp2 EBS volume. A 7th EC2 instance in the same VPC ran master and connected only to some trusted nodes in the outside network. Each of the 6 nodes under test only connected directly to this 7th instance. I manually pruned as much as possible and uploaded the same blocks, chainstate and mempool.dat to all instances. I started all 6 peers simultaneously at block height 835245 and ran them for over a week until block 836534.

The results were much faster block connection times for this branch compared to master, and much faster for this branch with dbcache=1000 compared to default dbcache.

branch speed
master 1 1995.49ms/blk
master 2 2129.78ms/blk
branch default dbcache 1 1189.65ms/blk
branch default dbcache 2 1037.74ms/blk
branch dbcache=1000 1 393.69ms/blk
branch dbcache=1000 2 427.77ms/blk

The log files of all 6 instances are here.
There is a lot of noise with the exact times of blocks being connected, so I plotted the rolling 20 block connect time averages. The large dots are the times where the cache is emptied. For the red master nodes, this happens every 24 hours. The blue branch nodes with default dbcache only filled up and emptied the caches once, which is seen in the middle. The green branch nodes with 1000 dbcache never emptied the cache. It is very clear from the chart that whenever the cache is emptied, connect block speed degrades significantly.

plot

Also note that this still clears the cache for pruning flushes. Having frequent pruning flushes with a large cache that doesn't clear is less performant than the status quo #15265 (comment). See #28280.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 7, 2023

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 sipa, brunoerg, achow101
Concept ACK hernanmarino

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
  • #28280 (Don't empty dbcache on prune flushes: >30% faster IBD by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 3, 2024

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

@andrewtoth andrewtoth marked this pull request as ready for review March 27, 2024 18:33
@andrewtoth andrewtoth changed the title validation: don't clear cache on periodic flush validation: don't clear cache on periodic flush - 2x block connection speed Mar 27, 2024
@andrewtoth andrewtoth changed the title validation: don't clear cache on periodic flush - 2x block connection speed validation: don't clear cache on periodic flush - >2x block connection speed Mar 27, 2024
@andrewtoth andrewtoth changed the title validation: don't clear cache on periodic flush - >2x block connection speed validation: don't clear cache on periodic flush: >2x block connection speed Mar 27, 2024
Copy link
Contributor

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

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

Concept ACK. I also agree with the criteria in the code for setting the empty_cache variable to true

@sipa
Copy link
Member

sipa commented Apr 13, 2024

utACK 4a6d1d1

@DrahtBot DrahtBot requested a review from hernanmarino April 13, 2024 12:13
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

crACK 4a6d1d1

@achow101
Copy link
Member

ACK 4a6d1d1

@achow101 achow101 merged commit d6069cb into bitcoin:master May 13, 2024
@andrewtoth andrewtoth deleted the sync-on-periodic branch May 14, 2024 01:24
@andrewtoth andrewtoth mentioned this pull request May 14, 2024
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 1, 2024
achow101 added a commit that referenced this pull request May 1, 2025
e976bd3 validation: add randomness to periodic write interval (Andrew Toth)
2e2f410 refactor: replace m_last_write with m_next_write (Andrew Toth)
b557fa7 refactor: rename fDoFullFlush to should_write (Andrew Toth)
d73bd9f validation: write chainstate to disk every hour (Andrew Toth)
0ad7d7a test: chainstate write test for periodic chainstate flush (Andrew Toth)

Pull request description:

  Since #28233, periodically writing the chainstate to disk every 24 hours does not clear the dbcache. Since #28280, periodically writing the chainstate to disk is proportional only to the amount of dirty entries in the cache. Due to these changes, it is no longer beneficial to only write the chainstate to disk every 24 hours. The periodic flush interval was necessary because every write of the chainstate would clear the dbcache. Now, we can get rid of the periodic flush interval and simply write the chainstate along with blocks and block index at least every hour.

  Three benefits of doing this:
  1. For IBD or reindex-chainstate with a combination of large dbcache setting, slow CPU, slow internet speed/unreliable peers, it could be up to 24 hours until the chainstate is persisted to disk. A power outage or crash could potentially lose up to 24 hours of progress. If there is a very large amount of dirty cache entries, writing to disk when a flush finally does occur will take a very long time. Crashing during this window of writing can cause #11600. By syncing every hour in unison with the block index we avoid this problem. Only a maximum of one hour of progress can be lost, and the window for crashing during writing is much smaller. For IBD with lower dbcache settings, faster CPU, or better internet speed/reliable peers, chainstate writes are already triggered more often than every hour so this change will have no effect on IBD.
  2. Based on discussion in #28280, writing only once every 24 hours during long running operation of a node causes IO spikes. Writing smaller chainstate changes every hour like we do with blocks and block index will reduce IO spikes.
  3. Faster shutdown speeds. All dirty chainstate entries must be persisted to disk on shutdown. If we have a lot of dirty entries, such as when close to 24 hours or if we sync with a large dbcache, it can take a long time to shutdown. By keeping the chainstate clean we avoid this problem.

  Inspired by [this comment](#28280 (comment)).

  Resolves #11600

ACKs for top commit:
  achow101:
    ACK e976bd3
  davidgumberg:
    utACK e976bd3
  sipa:
    utACK e976bd3
  l0rinc:
    ACK  e976bd3

Tree-SHA512: 5bccd8f1dea47f9820a3fd32fe3bb6841c0167b3d6870cc8f3f7e2368f124af1a914bca6acb06889cd7183638a8dbdbace54d3237c3683f2b567eb7355e015ee
@bitcoin bitcoin locked and limited conversation to collaborators May 14, 2025
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.

6 participants