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

coins: allow write to disk without cache drop #17487

Open
wants to merge 4 commits into
base: master
from

Conversation

@jamesob
Copy link
Member

jamesob commented Nov 15, 2019

This is part of the assumeutxo project:

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.

See also #15218.

@fanquake fanquake added this to In progress in Assume UTXO Nov 15, 2019
@jamesob jamesob force-pushed the jamesob:2019-11-au-coins-erase branch from d5a0621 to bd44f51 Nov 15, 2019
Copy link
Member

MarcoFalke left a comment

Does this speed up IBD with low dbcache? If so, a benchmark would be nice

src/coins.cpp Outdated Show resolved Hide resolved
@jamesob

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 15, 2019

Does this speed up IBD with low dbcache? If so, a benchmark would be nice

No, this PR doesn't change any existing behavior - it simply introduces the option to avoid erasing the cache (which is later used by assumeutxo's snapshot activation code). This changeset shouldn't cause any sort of performance difference.

Copy link
Contributor

ariard left a comment

Code Review ACK bd44f51

Can you slightly note in commit message than this change is not used right now ?

CCoinsViewCache::Flush is called in FlushStateToDisk, DisconnectTip, ConnectTip, ReplayBlocks but as new default argument is true there shouldn't be behavior change.

src/coins.h Outdated
@@ -275,7 +275,7 @@ class CCoinsViewCache : public CCoinsViewBacked
* Failure to call this method before destruction will cause the changes to be forgotten.
* If false is returned, the state of this cache (and its backing view) will be undefined.
*/
bool Flush();
bool Flush(bool clear_cache = true);

This comment has been minimized.

Copy link
@ariard

ariard Nov 15, 2019

Contributor

Isn't passing the argument explicitly better than with a default one? Also it's worthy to extend method comment a bit with effect of argument on cache.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 15, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #18113 (coins: Don't allow a coin to be spent and FRESH. Improve commenting. by jnewbery)
  • #17669 (tests: have coins simulation test also use CCoinsViewDB by jamesob)
  • #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

This comment has been minimized.

Copy link
Contributor

dongcarl commented Nov 15, 2019

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.

@MarcoFalke MarcoFalke removed the Tests label Nov 15, 2019
@jamesob jamesob force-pushed the jamesob:2019-11-au-coins-erase branch from bd44f51 to a22b1fb Nov 15, 2019
@jamesob

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 15, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 19, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 19, 2019

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 19, 2019

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 #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

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 19, 2019

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

  1. a reindex to 550,000 with and without an in-memory UTXO cache, and then
  2. 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

This comment has been minimized.

Copy link
Member

laanwj commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 21, 2019

@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

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 21, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 22, 2019

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

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 22, 2019

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

This comment has been minimized.

Copy link
Contributor

andrewtoth commented Nov 22, 2019

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 jamesob force-pushed the jamesob:2019-11-au-coins-erase branch from a22b1fb to 1e68aad Dec 3, 2019
@jamesob

This comment has been minimized.

Copy link
Member Author

jamesob commented Dec 3, 2019

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.

} 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(); ) {

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 3, 2019

Member

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.

This comment has been minimized.

Copy link
@jamesob

jamesob Dec 4, 2019

Author Member

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

This comment has been minimized.

Copy link
Member Author

jamesob commented Dec 4, 2019

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
jamesob and others added 4 commits Dec 3, 2019
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 and this commit does not
change any behavior.

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
Adds comments, slight refactor clarifications to make the code
easier to follow.
@jamesob jamesob force-pushed the jamesob:2019-11-au-coins-erase branch from 1e68aad to eebaca7 Dec 5, 2019
@jamesob jamesob changed the title coins: add `erase` parameter to control cacheCoins drop on flush coins: allow Flush() without cache drop Dec 5, 2019
@jamesob

This comment has been minimized.

Copy link
Member Author

jamesob commented Dec 5, 2019

I've split Flush(bool erase) into Sync() and Flush() as requested by @ryanofsky @MarcoFalke. (changes)

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jan 27, 2020

Review club notes: https://bitcoincore.reviews/17487.html

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 29, 2020

Review club notes:

Preparation notes; it's scheduled for Wednesday the 29th tonight, which explains why I couldn't find the minutes :-)

Copy link
Member

Sjors left a comment

I left a few comments mid-review to help with tonights review club. So far eebaca7 looks good, but I need to stare at it a bit more. The new (clarification of) tests are great.

You may want to rebase, because without that on macOS I get errors during make: (this was an issue with my machine)

Undefined symbols for architecture x86_64:
  "_CRYPTO_num_locks", referenced from:
      (anonymous namespace)::RNGState::RNGState() in libbitcoin_util.a(libbitcoin_util_a-random.o)
  "_RAND_cleanup", referenced from:
const Coin& entry = (InsecureRandRange(500) == 0) ? AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0));

// Infrequently, test usage of AccessByTxid instead of AccessCoin - the
// former just delegates to the latter and returns the first unspent in a txn.

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2020

Member

If COutPoint(txid, 0) doesn't exist and COutPoint(txid, 0) does, then AccessByTxid will randomly behave different from AccessCoin. But the coins are random anyway.

This comment has been minimized.

Copy link
@andrewtoth

andrewtoth Jan 29, 2020

Contributor

Schrödinger's COutPoint(txid, 0)? :P

}
}
if (InsecureRandRange(100) == 0) {
// Every 100 iterations, change the cache stack.
if (stack.size() > 0 && InsecureRandBool() == 0) {
//Remove the top cache
BOOST_CHECK(stack.back()->Flush());
bool should_erase = InsecureRandRange(4) < 3;

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2020

Member

Comment above should be "Flush or sync the top cache". Are you sure this test is working as advertised? What does delete stack.back(); and stack.pop_back(); do here? Making them conditional doesn't break the test.

}


//! For CCoinsViewCache instances backed by either another cache instance and

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 29, 2020

Member

nit: "either ... and"?

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Jan 29, 2020

You may want to rebase, because without that on macOS I get errors during make:

With gcc (Debian 8.3.0-6) and --enable-debug --enable-bench eebaca7 builds and all tests are passing for me. Starting code review.

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Jan 29, 2020

Concept ACK. Code review ACK of commit b2abb39. The changes are straightforward and do not appear to change behavior. My only nit would be to mention in the commit message the addition of the bool erase parameter and the added behavior in the false case. Will review the test commits.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jan 29, 2020

PR title "coins: allow Flush() without cache drop" seems out of date. Would suggest something like "coins: Add new unused CCoinsViewCache::Sync() method"

@jamesob jamesob changed the title coins: allow Flush() without cache drop coins: allow write to disk without cache drop Jan 29, 2020
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Feb 2, 2020

Concept ACK

Copy link
Member

jnewbery left a comment

tested ACK eebaca7

I've added a few nits, but I think this is ready for merge now, and those nits could be cleaned up later.

cacheCoins.clear();
cachedCoinsUsage = 0;
return fOk;
}

bool CCoinsViewCache::Sync()
{
bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase*/ false);

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 10, 2020

Member

This is simply copying the pattern of Flush(), but BatchWrite() never returns false, so this parameter does nothing. I think all three functions should be cleaned up to not return anything so that the calling pattern is clear, but that can be done in a follow-up PR.

// Flush in reverse order to ensure that flushes happen from children up.
for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) {
auto cache = *i;
cache->SetBestBlock(InsecureRand256()); // Hack to allow flushing.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 10, 2020

Member

nit: "Hack to allow flushing" isn't very illuminating. Perhaps "A cache's hashBlock must be filled before flushing to disk. That's normally done in connect/disconnect block. Do it manually here"

//
BOOST_CHECK(view->SpendCoin(outp));

// The coin should be in the cache, but pruned and marked dirty.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 10, 2020

Member

nit: I don't think pruned is the right terminolgy, but this can be cleaned up with all the rest in a future PR.


flush_all(/*erase*/ true); // Erase all cache content.

// --- Bonus check 2: ensure that a FRESH coin is deleted by Sync()

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 10, 2020

Member

I think this should say "ensure that a FRESH spent coin is deleted by Sync()". If the coin is FRESH but not spent, then a Sync() won't delete it.

GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp);
BOOST_CHECK_EQUAL(value, ABSENT);
BOOST_CHECK_EQUAL(flags, NO_ENTRY);
Comment on lines +1058 to +1060

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 10, 2020

Member

nit: I don't think we need these three lines. Just checking that HaveCoinInCache() fails is enough to show that the coin doesn't exist in the cache.

@fjahr
fjahr approved these changes Feb 14, 2020
Copy link
Contributor

fjahr left a comment

ACK eebaca7

Reviewed code and tested locally.

for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) {
auto cache = *i;
cache->SetBestBlock(InsecureRand256()); // Hack to allow flushing.
erase ? cache->Flush() : cache->Sync();

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 14, 2020

Contributor

nit: I like the naming destinction between Sync() and Flush() in the code because it made it easier to parse and that naming could have been used more here in the test as well. Now the destinction it is kind of hidden behind this do_erasing_flush parameter. There could have been a sync_all helper method for example. But it's probably not worth the effort of changing now.

bool CCoinsViewCache::Sync()
{
bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase*/ false);
// Instead of clearing the cache, just clear the FRESH/DIRTY

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 14, 2020

Contributor

nit: If I look at this comment outside of the context of this PR "Instead of clearing the cache" might not make much sense because there is no cache clearing going on inside of this function and the relation to Flush() may not be immediately clear.

@jamesob

This comment has been minimized.

Copy link
Member Author

jamesob commented Feb 17, 2020

Thanks for the reviews, all. I'll address some of the nits if there's a need to rebase, otherwise I'm going for ACK-preservation at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Assume UTXO
  
In progress
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.