coins: allow write to disk without cache drop #17487

jamesob · 2019-11-15T16:17:46Z

Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

In certain circumstances, we may want to flush chainstate data to disk without
emptying cacheCoins, which affects performance. UTXO snapshot
activation is one such case, as we populate cacheCoins with the snapshot
contents and want to persist immediately afterwards but also enter IBD.

See also #15265, which makes the case that under normal operation a
flush-without-erase doesn't necessarily add much benefit. I open this PR
even in light of the previous discussion because (i) flush-without-erase
almost certainly provides benefit in the case of snapshot activation (especially
on spinning disk hardware) and (ii) this diff is fairly small and gives us convenient
options for more granular cache management without changing existing policy.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, achow101, Sjors
Concept ACK	ryanofsky
Stale ACK	ariard, dongcarl, jnewbery, jonatack, andrewtoth, fjahr

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:

#25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
#25325 (Add pool based memory resource by martinus)
#9384 (CCoinsViewCache code cleanup & deduplication by ryanofsky)

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.

dongcarl · 2019-11-15T21:25:33Z

Code Review ACK bd44f51
Checked that it doesn't change any behavior, if the compiler is smart enough about detecting unused codepaths/function signatures, it will probably even emit an unchanged binary.

jamesob · 2019-11-15T21:38:41Z

Thanks for the looks, everyone. I've incorporated feedback from @MarcoFalke and @ariard. (diff)

Sjors · 2019-11-19T16:31:20Z

Why is it that you don't need this bit in your followup PR? https://github.com/bitcoin/bitcoin/pull/15265/files#diff-cd7b305fd4b4280f22ae88960e60398eR210-R222

maflcko · 2019-11-19T16:57:43Z

Given that this does not improve performance (see #15265 (comment)), is this needed at all? If so, sharing a benchmark with steps to reproduce would be helpful.

Anyway, I'd like to see the tests in src/test/coins_tests.cpp updated to cover the newly added paths.

ryanofsky · 2019-11-19T17:08:45Z

Concept ACK, but:

I think the PR description should be clearer that this change by itself doesn't change behavior.
I don't understand how this speeds up UTXO snapshot activation if conclusion from Flush without erasing cache during periodic and pruning flushes #15265 is that the cache is only really functioning as a write cache and not a read cache.
I agree with Marco about testing. This definitely needs unit tests or some coverage to unsure this isn't adding broken functionality, or code that could easily be broken in the future with no one noticing.
I agree with Antoine about the Flush method signature. Using a default boolean argument seems error prone. Explicit parameter would be ok, but better would just be to have separate methods like Flush() and Sync() or Flush() and Write()

jamesob · 2019-11-19T17:18:37Z

Alright, given the controversy here now is probably as good a time as any to back up and figure out exactly what cacheCoins' impact is on performance, for both this PR and posterity, so I'm going to benchmark

a reindex to 550,000 with and without an in-memory UTXO cache, and then
UTXO snapshot sync-to-tip with and without this changeset.

I'll get back with some results in the next few days, and hopefully will write up an elucidation on where exactly CCoinsViewCache does or doesn't provide benefit.

(Though I wonder how cross-platform differences beyond SSD-or-not play into this, e.g. the cache may or may not show importance based on leveldb's use of mmap & max_open_files, which is particularly relevant on Win32.)

laanwj · 2019-11-20T11:42:31Z

the cache may or may not show importance based on leveldb's use of mmap & max_open_files, which is particularly relevant on Win32

FWIW, #17398 adds leveldb mmap support on Windows, might want to try with and without that PR when doing benchmarking on Windows.

jamesob · 2019-11-21T19:04:15Z

After benching, I stand by this change as being significantly useful. I compare HEAD of the assumeutxo parent PR (utxo-dumpload-compressed) to a branch of it which drops use of the erase=false parameter, erasing the coins after snapshot load (bench/au.no-erase, changes). The benchmark compares the time taken to sync to block 604,667 after loading a snapshot taken at height 600,000.

The result is a significant degradation in performance on spinning disk hosts (4.5 hours vs. 1.5 hours) - see the bench-hdd-3 results. There is a modest but probably negligible degradation on SSD hosts.

host         tag                      time       time% maxmem  cpu%  dbcache
bench-ssd-4  bench/au.no-erase        47:42.80   1.00  6689.85MB 125%  5000MB
bench-ssd-4  bench/au.no-erase        44:15.66   0.93  6560.45MB 136%  5000MB
bench-ssd-4  utxo-dumpload-compressed 40:13.30   0.84  6662.75MB 149%  5000MB
bench-ssd-4  utxo-dumpload-compressed 41:36.01   0.87  6652.87MB 143%  5000MB

bench-ssd-5  bench/au.no-erase        41:33.65   0.95  6754.54MB 144%  5000MB
bench-ssd-5  bench/au.no-erase        41:32.42   0.95  6561.39MB 145%  5000MB
bench-ssd-5  utxo-dumpload-compressed 43:51.50   1.00  6758.98MB 135%  5000MB
bench-ssd-5  utxo-dumpload-compressed 36:39.94   0.84  6649.31MB 162%  5000MB

bench-hdd-3  utxo-dumpload-compressed 1:47:08    0.38  6654.23MB 57%   5000MB
bench-hdd-3  utxo-dumpload-compressed 1:30:48    0.32  6612.49MB 66%   5000MB
bench-hdd-3  bench/au.no-erase        4:24:53    0.93  6578.77MB 23%   5000MB
bench-hdd-3  bench/au.no-erase        4:45:29    1.00  6583.10MB 21%   5000MB

This benchmark is reproducible by running this script:

./bin/run_remote.py au --hosts [hostnames ...]

I'll add tests to make sure this change works as-advertised, but I think it's pretty clearly a useful option for us to have.

jamesob · 2019-11-21T19:37:10Z

@sdaftuar just pointed out to me that this code is very wrong - because I don't wipe the flags of flushed coins, this code ends up with an incorrect on-disk UTXO set (since a coin with the FRESH flag will only be removed from the in-memory cache and the delete will not propagate to leveldb). Going to fix this bug and reevaluate and pledge to never doubt Suhas' benchmarks again.

jamesob · 2019-11-21T19:38:38Z

(FWIW @Sjors wins Employee of the Month for spotting this bug in a previous comment.)

Sjors · 2019-11-22T11:59:21Z

Took me a while to understand what you're doing here. To rephrase in my own words: when you load the snapshot from disk you end up with a coin cache at e.g. 600,000. Depending on the users dbcache setting, part of this is already in RAM, which is potentially nice when continuing IBD.

jamesob@85a73a0#diff-24efdb00bfbe56b140fb006b562cc70bL5514-L5530

If -dbcache is less than the size of the snapshot, the only the most recent (?) coins of the snapshot will in RAM, the rest would already have been flushed. This is probably similar to what happens during a normal IBD when dbcache overflows. (we could use some heuristics to sort coins by life expectancy)

For some reason (why?) you need to flush at the end of loading the snapshot, which normally means no coins are in RAM. This PR changes that last flush to keep stuff around.

IIUC the main benefit of this cache is to reduce the number of unnecessary writes, i.e. when a coin is created and then destroyed we save 2 disk writes. But when we flush, even without deleting the coins from RAM, we expect 1 write if the coin is spent before the tip, otherwise no write.

Looking forward to the new benchmark. I suggest doing this with a ~1 month old snapshot (realistic for users who download immediately after a 1 month release candidates) and a 4 month old snapshot (the average age if we ship every 6 months).

jamesob · 2019-11-22T16:16:53Z

I'm surprised to report that even after fixing the (consensus!) bug that @Sjors and @sdaftuar pointed out, this change still shows considerable improvement for the assumeutxo snapshot-load use. We're still seeing 3x reduction in time with this change applied for spinning disks while doing a from-600k snapshot IBD.

host         tag                      time       time% maxmem  cpu%  dbcache
bench-ssd-4  utxo-dumpload-compressed 31:48.95   0.70  6787.93MB 179%  5000MB
bench-ssd-4  utxo-dumpload-compressed 36:06.14   0.80  6689.20MB 157%  5000MB
bench-ssd-4  bench/au.no-erase.1      43:21.22   0.96  6689.82MB 137%  5000MB
bench-ssd-4  bench/au.no-erase.1      45:14.75   1.00  6739.20MB 131%  5000MB

bench-ssd-5  utxo-dumpload-compressed 33:08.03   0.77  6787.14MB 172%  5000MB
bench-ssd-5  utxo-dumpload-compressed 31:13.55   0.73  6731.26MB 182%  5000MB
bench-ssd-5  bench/au.no-erase.1      42:49.74   1.00  6754.30MB 139%  5000MB
bench-ssd-5  bench/au.no-erase.1      41:22.71   0.97  6557.05MB 144%  5000MB

bench-hdd-3  bench/au.no-erase.1      4:34:00    0.98  6530.24MB 22%   5000MB
bench-hdd-3  bench/au.no-erase.1      4:39:21    1.00  6562.12MB 21%   5000MB
bench-hdd-3  utxo-dumpload-compressed 1:30:11    0.32  6736.32MB 63%   5000MB
bench-hdd-3  utxo-dumpload-compressed 1:33:19    0.33  6702.03MB 61%   5000MB

(host specs here)

Applying @sdaftuar's fix (here in bench/au.no-erase.1, and here in utxo-dumpload-compressed) I think actually improves runtime for the single Flush(erase=false) call that we make after finishing the snapshot load because now we're actually removing spent coins from cacheCoins, thus freeing up more cache space going into the IBD.

For some reason (why?) you need to flush at the end of loading the snapshot, which normally means no coins are in RAM. This PR changes that last flush to keep stuff around.

Thanks for the nice summary, @Sjors. I'm not sure that we necessarily need to flush after loading the snapshot, but I figured it was prudent to ensure that we'd resume the snapshot-based sync even after an unclean shutdown - maybe it's better to just omit the Flush() call altogether after loading the snapshot?

Tangentially, @sdaftuar mentioned the other day a potential technique for partial flushes that may accomplish the kind of optimization that this branch and #15265 are aiming for. The idea would be to allocate some of the cacheCoins memory to a secondary data structure that would serve as an index into cacheCoins, allowing fast lookup by spentness and height of the in-memory coins. That way, on FlushStateToDisk() we could do a partial flush, only making disk writes for those coins which are either unspent and of a certain age or spent and on-disk. This would make use of the fact that most coins are very short-lived (see figure 2 in @adiabat's excellent utreexo paper).

I think this is a promising idea and I'll be experimenting with it soon - though of course that's outside the scope of this PR.

andrewtoth · 2019-11-22T20:22:55Z

What about using Flush(erase=false) for all periodic flushes after IBD? That way when running with a high enough dbcache a flush would never empty cacheCoins, which would surely improve performance. That's also the only real issue with #15218 .

jamesob · 2019-12-03T20:42:54Z

I've pushed some test coverage for the new erase parameter (and more generally some explicit tests for coins cache behavior). Notably, the new test cases use CCoinsViewDB at the base of the cache structure, which isn't currently tested explicitly anywhere else in the unittest suite - though of course an instance lives at the heart of every CChainState.

Throughout the course of writing (and debugging) the tests, I found yet another bug that had to do with std::moveing the coins in CCoinsView::BatchWrite even when erase=false, which was causing the coin.out.scriptPubKey to appear null in the child-most cache. This is just another testament to how tricky the coins cache code is to get right (as I was warned months ago by @sdaftuar). But that said, I feel pretty confident that this change is now correct.

maflcko · 2019-12-03T21:00:29Z

src/coins.cpp

+    } else {
+        // Instead of clearing the cache, just clear the FRESH/DIRTY
+        // flags, and erase any spent coins
+        for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) {


Is it true that this loop is very slow with high dbcache? If so, a comment would be appropriate.

Other than that I also agree with @ryanofsky comment to have "separate methods like Flush() and Sync() or Flush() and Write()". As a rule of thumb, if two functions have a completely different implementation, they should not be bundled in one and switched by a boolean.

In my own benchmarking, I haven't seen anything to support the notion that this loop takes a long time. In the newest benchmarks (to be posted), calling Flush(erase=true) on a cache of size 3826 MB takes 7:17 and calling Flush(erase=false) on a cache of the same size takes 6:03 so the difference is either negligible or favorable.

I got the times by subtracting timestamps from the last and second-to-last loglines below for each branch during snapshot activation.

Before this change

2019-12-03T23:18:26Z [snapshot] 63000000 coins loaded (99.39%, 3826 MB) 2019-12-03T23:18:26Z [snapshot] loaded 63389760 (3877 MB) coins from snapshot 00000000000000000007316856900e76b4f7a9139cfbfba89842c8d196cd5f91 2019-12-03T23:18:26Z [snapshot] flushing snapshot chainstate to disk 2019-12-03T23:25:43Z [snapshot] validated snapshot (592 MB)

After this change

2019-12-03T23:48:09Z [snapshot] 63000000 coins loaded (99.39%, 3826 MB) 2019-12-03T23:48:10Z [snapshot] loaded 63389760 (3877 MB) coins from snapshot 00000000000000000007316856900e76b4f7a9139cfbfba89842c8d196cd5f91 2019-12-03T23:48:10Z [snapshot] flushing snapshot chainstate to disk 2019-12-03T23:54:13Z [snapshot] validated snapshot (3877 MB)

I'll make the Flush() + Sync() change.

jamesob · 2019-12-04T17:12:01Z

Latest round of benchmarks for the current tip of this branch continues to show good performance improvements for UTXO snapshot load, even on SSD. (same setup as #17487 (comment))

host         tag                      time       time% maxmem  cpu%  dbcache
bench-ssd-4  bench/au.no-erase.1      43:27.79   0.98  6572.45MB 137%  5000MB
bench-ssd-4  bench/au.no-erase.1      44:10.18   1.00  6690.36MB 135%  5000MB
bench-ssd-4  utxo-dumpload.54         32:59.07   0.75  6733.55MB 173%  5000MB
bench-ssd-4  utxo-dumpload.54         33:19.00   0.75  6725.27MB 171%  5000MB

bench-ssd-5  utxo-dumpload.54         31:32.39   0.60  6769.57MB 182%  5000MB
bench-ssd-5  utxo-dumpload.54         31:11.39   0.60  6699.07MB 183%  5000MB
bench-ssd-5  bench/au.no-erase.1      41:41.13   0.80  6772.39MB 142%  5000MB
bench-ssd-5  bench/au.no-erase.1      52:16.76   1.00  6567.82MB 114%  5000MB

bench-hdd-3  bench/au.no-erase.1      4:37:10    1.00  6569.48MB 21%   5000MB
bench-hdd-3  bench/au.no-erase.1      4:36:03    1.00  6523.61MB 21%   5000MB
bench-hdd-3  utxo-dumpload.54         1:30:22    0.33  6705.20MB 63%   5000MB
bench-hdd-3  utxo-dumpload.54         1:36:20    0.35  6735.74MB 59%   5000MB

Benchmark results available here: bitcoin#17487 (comment)

… >2x block connection speed 4a6d1d1 validation: don't clear cache on periodic flush (Andrew Toth) Pull request description: Since bitcoin#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](https://gist.github.com/andrewtoth/03c95033e7581d5dbc5be028639a1a91). 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](https://github.com/bitcoin/bitcoin/assets/237213/802cb28d-1ad4-47c3-a886-c5366b423eca) 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 bitcoin#15265 (comment). See bitcoin#28280. ACKs for top commit: sipa: utACK 4a6d1d1 achow101: ACK 4a6d1d1 brunoerg: crACK 4a6d1d1 Tree-SHA512: 05dbc677bc309bbcf89c52a6c5e853e2816b0ef0b5ee3719b30696df315a0427e244bb82da9ad828ec0e7ea8764552f8affe14c0184b52adf1909f5d8c1b4f9e

589db87 validation: don't erase coins cache on prune flushes (Andrew Toth) 0e89187 Add linked-list test to CCoinsViewCache::SanityCheck (Pieter Wuille) 05cf4e1 coins: move Sync logic to CoinsViewCacheCursor (Andrew Toth) 7825b8b coins: pass linked list of flagged entries to BatchWrite (Andrew Toth) a14edad test: add cache entry linked list tests (Andrew Toth) 24ce37c coins: track flagged cache entries in linked list (Andrew Toth) 58b7ed1 coins: call ClearFlags in CCoinsCacheEntry destructor (Andrew Toth) 8bd3959 refactor: require self and sentinel parameters for AddFlags (Andrew Toth) 75f36d2 refactor: add CoinsCachePair alias (Andrew Toth) f08faea refactor: move flags to private uint8_t and rename to m_flags (Andrew Toth) 4e4fb4c refactor: disallow setting flags in CCoinsCacheEntry constructors (Andrew Toth) 8737c0c refactor: encapsulate flags setting with AddFlags and ClearFlags (Andrew Toth) 9715d3b refactor: encapsulate flags get access for all other checks (Andrew Toth) df34a94 refactor: encapsulate flags access for dirty and fresh checks (Andrew Toth) Pull request description: 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. For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using `Sync` in place of `Flush` actually slows down a pruned full IBD with a high `dbcache` value. This is because as the cache grows, sync takes longer since every coin in the cache is scanned to check if it's dirty. For frequent prune flushes and a large cache this constant scanning starts to really slow IBD down, and just emptying the cache on every prune becomes faster. To fix this, we can add two pointers to each cache entry and construct a doubly linked list of dirty entries. We can then only iterate through all dirty entries on each `Sync`, and simply clear the pointers after. With this approach a full IBD with `dbcache=16384` and `prune=550` was 32% faster than master. For default `dbcache=450` speedup was ~9%. All benchmarks were run with `stopatheight=800000`. | | prune | dbcache | time | max RSS | speedup | |-----------:|----------:|------------:|--------:|-------------:|--------------:| | master | 550 | 16384 | 8:52:57 | 2,417,464k | - | | branch | 550 | 16384 | 6:01:00 | 16,216,736k | 32% | | branch | 550 | 450 | 8:05:08 | 2,818,072k | 8.8% | | master | 10000 | 5000 | 8:19:59 | 2,962,752k | - | | branch | 10000 | 5000| 5:56:39 | 6,179,764k | 28.8% | | master | 0 | 16384 | 4:51:53 | 14,726,408k | - | | branch | 0 | 16384 | 4:43:11 | 16,526,348k | 2.7% | | master | 0 | 450 | 7:08:07 | 3,005,892k | - | | branch | 0 | 450 | 6:57:24 | 3,013,556k |2.6%| While the 2 pointers add memory to each cache entry, it did not slow down IBD. For non-pruned IBD results were similar for this branch and master. When I performed the initial IBD, the full UTXO set could be held in memory when using the max `dbcache` value. For non-pruned IBD with max `dbcache` to tip ended up using 12% more memory, but it was also 2.7% faster somehow. For smaller `dbcache` values the `dbcache` limit is respected so does not consume more memory, and the potentially more frequent flushes were not significant enough to cause any slowdown. For reviewers, the commits in order do the following: First 4 commits encapsulate all accesses to `flags` on cache entries, and then the 5th makes `flags` private. Commits `refactor: add CoinsCachePair alias` to `coins: call ClearFlags in CCoinsCacheEntry destructor` create the linked list head nodes and cache entry self references and pass them into `AddFlags`. Commit `coins: track flagged cache entries in linked list` actually adds the entries into a linked list when they are flagged DIRTY or FRESH and removes them from the linked list when they are destroyed or the flags are cleared manually. However, the linked list is not yet used anywhere. Commit `test: add cache entry linked list tests` adds unit tests for the linked list. Commit `coins: pass linked list of flagged entries to BatchWrite` uses the linked list to iterate through DIRTY entries instead of using the entire coins cache. Commit `validation: don't erase coins cache on prune flushes` uses `Sync` instead of `Flush` for pruning flushes, so the cache is no longer cleared. Inspired by [this comment](#15265 (comment)). Fixes #11315. ACKs for top commit: paplorinc: ACK 589db87 sipa: reACK 589db87 achow101: ACK 589db87 mzumsande: re-ACK 589db87 Tree-SHA512: 23b2bc01c83edacb5b39aa60bb0b766de9a74ce17f0c59bf13b97b4328a7b758ad9aff6581c3ca88e2973f7658380651530d497444f48d6e22ea0bfc51cc921d

See related discussions: * bitcoin#28280 (comment) * bitcoin#28280 (comment) * bitcoin#17487 (comment) And reproducer that demonstrates the behavior of all 3 cases: * https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717

, bitcoin#24177, bitcoin#24299, bitcoin#24917, bitcoin#22564, bitcoin#25349, bitcoin#25571, bitcoin#17487, bitcoin#26999, bitcoin#27011 (blockstorage backports: part 2) b658637 merge bitcoin#27011: Add simulation-based `CCoinsViewCache` fuzzer (Kittywhiskers Van Gogh) 1d0e410 merge bitcoin#26999: A few follow-ups to bitcoin#17487 (Kittywhiskers Van Gogh) 7d837ea merge bitcoin#17487: allow write to disk without cache drop (Kittywhiskers Van Gogh) 2c758f4 merge bitcoin#25571: Make mapBlocksUnknownParent local, and rename it (Kittywhiskers Van Gogh) 70a91e1 merge bitcoin#25349: CBlockIndex/CDiskBlockIndex improvements for safety, consistent behavior (Kittywhiskers Van Gogh) eca0a64 merge bitcoin#22564: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager (Kittywhiskers Van Gogh) 916b3f0 merge bitcoin#24917: Make BlockManager::LoadBlockIndex private (Kittywhiskers Van Gogh) e10ca27 merge bitcoin#24299: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups (Kittywhiskers Van Gogh) 18aa55b merge bitcoin#24177: add missing thread safety lock assertions (Kittywhiskers Van Gogh) 678e67c merge bitcoin#24235: use stronger EXCLUSIVE_LOCKS_REQUIRED() (Kittywhiskers Van Gogh) edc665c merge bitcoin#24103: Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it (Kittywhiskers Van Gogh) d19ffd6 merge bitcoin#22278: Add LIFETIMEBOUND to CScript where needed (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6097 * Dependency for #6067 * Test failures in `linux64_multiprocess-build` ([build](https://gitlab.com/dashpay/dash/-/jobs/7550924662)) and `linux64_tsan-test` ([build](https://gitlab.com/dashpay/dash/-/jobs/7550924666)) do not stem from this PR but are pre-existing failures in `develop` ([build](https://gitlab.com/dashpay/dash/-/jobs/7550859495), [build](https://gitlab.com/dashpay/dash/-/jobs/7550859499)). A fix for the build failures has been opened as a separate PR. ## Breaking Changes None observed. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: LGTM, utACK b658637 PastaPastaPasta: utACK b658637 Tree-SHA512: 1a9c3a41617af274db169db47a9c9fce7ba7ce0b2d68aa75617640a55da11a0fa095cb25ce6a2d38f06d3f6a6cc4c08cb4cf82dca4bdc74192e8882fd5f7052f

Summary: Adds comments, slight refactor clarifications to make the code easier to follow. This is a partial backport of [[bitcoin/bitcoin#17487 | core#17487]] bitcoin/bitcoin@6d8affc Depends on D16156 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D16161

Summary: In certain circumstances, we may want to flush to disk without emptying `cacheCoins`, which affects performance. UTXO snapshot activation is one such case. This method is currently unused except in unit tests, and this commit does not change any behavior. See also [[bitcoin/bitcoin#15265 | core#15265]], which makes the case that under normal operation a flush-without-erase doesn't necessarily add much benefit. I open this PR even in light of the previous discussion because (i) flush-without-erase almost certainly provides benefit in the case of snapshot activation (especially on spinning disk hardware) and (ii) this diff is fairly small and gives us convenient options for more granular cache management without changing existing policy. Incorporates feedback from John Newbery. Co-authored-by: Suhas Daftuar <[email protected]> Thanks to Marco Falke for help with move semantics. This concludes backport of [[bitcoin/bitcoin#17487 | core#17487]] and [[bitcoin/bitcoin#26999 | core#26999]] bitcoin/bitcoin@79cedc3 - coins: add Sync() method to allow flush without cacheCoins drop bitcoin/bitcoin@2c3cbd6 - test: add use of Sync() to coins tests bitcoin/bitcoin@1d7935b - test: add test for coins view flush behavior using Sync() bitcoin/bitcoin@bb00357 - Make test/fuzz/coins_view exercise CCoinsViewCache::Sync() bitcoin/bitcoin@941feb6 - Avoid unclear {it = ++it;} bitcoin/bitcoin@2e16054 - Add assertions that BatchWrite(erase=true) erases Depends on D16161 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D16162

jamesob force-pushed the 2019-11-au-coins-erase branch from d5a0621 to bd44f51 Compare November 15, 2019 16:26

DrahtBot added Tests UTXO Db and Indexes labels Nov 15, 2019

maflcko reviewed Nov 15, 2019

View reviewed changes

src/coins.cpp Outdated Show resolved Hide resolved

ariard reviewed Nov 15, 2019

View reviewed changes

maflcko removed the Tests label Nov 15, 2019

jamesob force-pushed the 2019-11-au-coins-erase branch from bd44f51 to a22b1fb Compare November 15, 2019 21:37

maflcko mentioned this pull request Nov 15, 2019

validation: sync chainstate to disk after syncing to tip #15218

Closed

jamesob mentioned this pull request Nov 18, 2019

assumeutxo #15606

Closed

18 tasks

Sjors mentioned this pull request Nov 19, 2019

Flush without erasing cache during periodic and pruning flushes #15265

Closed

jamesob force-pushed the 2019-11-au-coins-erase branch from a22b1fb to 1e68aad Compare December 3, 2019 18:34

maflcko reviewed Dec 3, 2019

View reviewed changes

jamesob force-pushed the 2019-11-au-coins-erase branch from 1e68aad to eebaca7 Compare December 5, 2019 16:50

DrahtBot mentioned this pull request Feb 6, 2023

rpc: Optimize serialization disk space of dumptxoutset #26045

Closed

jamesob added a commit to jamesob/bitcoin that referenced this pull request Feb 22, 2023

validation: optimization: use Sync() when flushing snapshot to disk

129d8f5

Benchmark results available here: bitcoin#17487 (comment)

LarryRuane mentioned this pull request Apr 21, 2023

speed up IBD by not storing unspent coins in UTXO memory cache LarryRuane/bitcoin#7

Open

jamesob added a commit to jamesob/bitcoin that referenced this pull request May 5, 2023

validation: optimization: use Sync() when flushing snapshot to disk

17f6368

Benchmark results available here: bitcoin#17487 (comment)

andrewtoth mentioned this pull request Aug 7, 2023

validation: don't clear cache on periodic flush: >2x block connection speed #28233

Merged

This was referenced Aug 16, 2023

Don't empty dbcache on prune flushes: >30% faster IBD andrewtoth/bitcoin#6

Closed

Don't empty dbcache on prune flushes: >30% faster IBD #28280

Merged

bitcoin locked and limited conversation to collaborators Jan 30, 2024

bitcoin unlocked this conversation Jul 16, 2024

PastaPastaPasta pushed a commit to kwvg/dash that referenced this pull request Aug 13, 2024

merge bitcoin#17487: allow write to disk without cache drop

5d852c3

PastaPastaPasta pushed a commit to kwvg/dash that referenced this pull request Aug 13, 2024

merge bitcoin#26999: A few follow-ups to bitcoin#17487

b119bd4

l0rinc mentioned this pull request Aug 14, 2024

coins: Simplify std::move to ternary in coins.cpp #30656

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Aug 23, 2024

merge bitcoin#17487: allow write to disk without cache drop

7d837ea

kwvg added a commit to kwvg/dash that referenced this pull request Aug 23, 2024

merge bitcoin#26999: A few follow-ups to bitcoin#17487

1d0e410

tomt1664 pushed a commit to tomt1664/elements that referenced this pull request Apr 4, 2025

Merge 82903a7 into merged_master (Bitcoin PR bitcoin/bitcoin#17487)

829623e

tomt1664 pushed a commit to tomt1664/elements that referenced this pull request Apr 4, 2025

Merge 82903a7 into merged_master (Bitcoin PR bitcoin/bitcoin#17487)

393a957

delta1 added a commit to delta1/elements that referenced this pull request Apr 6, 2025

Merge 82903a7 into merged_master (Bitcoin PR bitcoin/bitcoin#17487)

5c25e88

tomt1664 pushed a commit to tomt1664/elements that referenced this pull request Apr 7, 2025

Merge 82902a7a8d into merged_master (Bitcoin PR bitcoin/bitcoin#17487)

7d6dc29

bitcoin locked and limited conversation to collaborators Aug 15, 2025

                    * If false is returned, the state of this cache (and its backing view) will be undefined.
                    */
-                  bool Flush();
+                  bool Flush(bool clear_cache = true);

coins: allow write to disk without cache drop #17487

coins: allow write to disk without cache drop #17487

Uh oh!

Conversation

jamesob commented Nov 15, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Nov 15, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

dongcarl commented Nov 15, 2019

Uh oh!

jamesob commented Nov 15, 2019

Uh oh!

Sjors commented Nov 19, 2019

Uh oh!

maflcko commented Nov 19, 2019

Uh oh!

ryanofsky commented Nov 19, 2019

Uh oh!

jamesob commented Nov 19, 2019

Uh oh!

laanwj commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Nov 21, 2019

Uh oh!

jamesob commented Nov 21, 2019

Uh oh!

jamesob commented Nov 21, 2019

Uh oh!

Sjors commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Nov 22, 2019

Uh oh!

andrewtoth commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Dec 3, 2019

Uh oh!

maflcko Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob Dec 4, 2019

Choose a reason for hiding this comment

Before this change

After this change

Uh oh!

jamesob commented Dec 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

DrahtBot commented Nov 15, 2019 •

edited

Loading

laanwj commented Nov 20, 2019 •

edited

Loading

Sjors commented Nov 22, 2019 •

edited

Loading

andrewtoth commented Nov 22, 2019 •

edited

Loading

maflcko Dec 3, 2019 •

edited

Loading