CCoinsViewCache code cleanup & deduplication #9384

ryanofsky · 2016-12-20T00:07:34Z

CCoinsViewCache code cleanup & deduplication

The change moves code responsible for updating the cache out of various
CCoinsViewCache methods and into a Modifier class. This way the cache update
code is just written once in a general way instead of being duplicated and
split up to handle various special cases.

This is a refactoring, with changes to cache behavior only in 2 corner cases
(with corresponding tests in coins_test.cpp) which don't affect the meaning of
data stored in the cache:

In BatchWrite, overwriting a non-dirty pruned cache entry with a fresh pruned
cache entry now deletes the cache entry instead of leaving behind a dirty
pruned entry that will trigger an unnecessary database write later.
In BatchWrite, overwriting a dirty pruned fresh cache entry with a nonpruned
entry updates the entry without dropping the fresh flag. There's no reason to
drop the fresh flag in this case because the flag accurately describes the
state of the base view and could prevent unnecessary database writes in the
future if the utxo is spent later.

ryanofsky · 2017-01-11T16:07:07Z

src/test/coins_tests.cpp

@@ -612,7 +612,7 @@ BOOST_AUTO_TEST_CASE(ccoins_access)
    CheckAccessCoins(ABSENT, VALUE2, VALUE2, FRESH      , FRESH      );
    CheckAccessCoins(ABSENT, VALUE2, VALUE2, DIRTY      , DIRTY      );
    CheckAccessCoins(ABSENT, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH);
-    CheckAccessCoins(PRUNED, ABSENT, PRUNED, NO_ENTRY   , FRESH      );
+    CheckAccessCoins(PRUNED, ABSENT, ABSENT, NO_ENTRY   , NO_ENTRY   );


Explanation of this change: Calling CCoinsViewCache::AccessCoins when no cache entry exists and the base entry is pruned no longer creates a cache entry.

The changed behavior in this case is better than the current behavior because now this case is consistent with the case in line 606 above where no cache entry exists and no base entry exists. It doesn't make any sense for the cache to behave differently just because a base entry is pruned instead of nonexistent.

ryanofsky · 2017-01-11T16:10:36Z

src/test/coins_tests.cpp

@@ -826,15 +826,15 @@ BOOST_AUTO_TEST_CASE(ccoins_write)
    CheckWriteCoins(PRUNED, ABSENT, PRUNED, DIRTY      , NO_ENTRY   , DIRTY      );
    CheckWriteCoins(PRUNED, ABSENT, PRUNED, DIRTY|FRESH, NO_ENTRY   , DIRTY|FRESH);
    CheckWriteCoins(PRUNED, PRUNED, PRUNED, 0          , DIRTY      , DIRTY      );
-    CheckWriteCoins(PRUNED, PRUNED, PRUNED, 0          , DIRTY|FRESH, DIRTY      );
+    CheckWriteCoins(PRUNED, PRUNED, ABSENT, 0          , DIRTY|FRESH, NO_ENTRY   );


Explanation of this change: Overwriting a non-dirty pruned cache entry with a fresh pruned cache entry now deletes the cache entry instead of leaving behind a dirty pruned entry that will trigger an unnecessary database write later.

ryanofsky · 2017-01-11T16:11:37Z

src/test/coins_tests.cpp

    CheckWriteCoins(PRUNED, PRUNED, ABSENT, FRESH      , DIRTY      , NO_ENTRY   );
    CheckWriteCoins(PRUNED, PRUNED, ABSENT, FRESH      , DIRTY|FRESH, NO_ENTRY   );
    CheckWriteCoins(PRUNED, PRUNED, PRUNED, DIRTY      , DIRTY      , DIRTY      );
    CheckWriteCoins(PRUNED, PRUNED, PRUNED, DIRTY      , DIRTY|FRESH, DIRTY      );
    CheckWriteCoins(PRUNED, PRUNED, ABSENT, DIRTY|FRESH, DIRTY      , NO_ENTRY   );
    CheckWriteCoins(PRUNED, PRUNED, ABSENT, DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY   );
    CheckWriteCoins(PRUNED, VALUE2, VALUE2, 0          , DIRTY      , DIRTY      );
-    CheckWriteCoins(PRUNED, VALUE2, VALUE2, 0          , DIRTY|FRESH, DIRTY      );
+    CheckWriteCoins(PRUNED, VALUE2, VALUE2, 0          , DIRTY|FRESH, DIRTY|FRESH);


Explanation of this change: Overwriting a dirty pruned fresh cache entry with a nonpruned entry updates the entry without dropping the fresh flag. There's no reason to drop the fresh flag in this case because the flag accurately describes the state of the base view and could prevent unnecessary database writes in the future if the utxo is spent later.

ryanofsky · 2017-01-11T16:26:39Z

@sipa, this is the PR I mentioned other day. It does move code to the modifier object, but the code could be moved if the modifier object is going away.

ryanofsky · 2017-03-01T22:30:12Z

@sipa, I posted a version of your pertxoutcache branch rebased on top of this PR here:
https://github.com/ryanofsky/bitcoin/commits/pr/pertxoutcache

It might be useful to you even if you aren't interested in this PR because it also updates coins_tests to be compatible with your change.

Relevant commits:

6b547c2 WIP per-txout chainstate – rebased version of your main commit on top of this PR.
5853aed Re-enable and fix coins_test.cpp – coins_test fixes that you can incorporate (regardless of the code refactoring in this PR)
1b8425f Fix wallet CCoinsViewCache::GetCoins call – fix for compile error in wallet caused by ccoins changes

ryanofsky · 2017-06-02T20:03:40Z

Rebased c72de76 -> 2f2d3c8 (pr/ccoins-cleanup.6 -> pr/ccoins-cleanup.7) after #10195 merge.

sipa · 2018-03-06T17:15:05Z

Concept ACK

ryanofsky · 2018-09-24T14:15:20Z

src/coins.cpp

    }
-    return true;
+    coin.Clear();
+    return !already_spent;


I think this change (after #10537) would have prevented the inflation bug caused by #9049 + #10195, discussed in
the CVE-2018-17144 full disclosure.

DrahtBot · 2019-03-15T18:10:29Z

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
Concept ACK	sipa, jonatack

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:

#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
#26331 (Implement CCoinsViewErrorCatcher::HaveCoin and check disk space periodically by aureleoules)
#17487 (coins: allow write to disk without cache drop by jamesob)

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 · 2019-04-28T19:12:13Z

The last travis run for this pull request was 281 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

ryanofsky · 2020-04-27T20:33:53Z

Rebased 8c74391 -> b837952 (pr/ccoins-cleanup.13 -> pr/ccoins-cleanup.14, compare) after #18752 to avoid mempool_reorg sync timeout https://travis-ci.org/github/bitcoin/bitcoin/jobs/678462266#L681
Rebased b837952 -> 0c33bd2 (pr/ccoins-cleanup.14 -> pr/ccoins-cleanup.15, compare) due to conflict with #19806
Updated 0c33bd2 -> a40f7e5 (pr/ccoins-cleanup.15 -> pr/ccoins-cleanup.16, compare) to fix fuzz and appveyor CI errors https://cirrus-ci.com/task/5569196852510720?command=ci#L2831 and https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38155594#L31
Rebased a40f7e5 -> ed03785 (pr/ccoins-cleanup.16 -> pr/ccoins-cleanup.17, compare) due to conflict with #22902

The change moves code responsible for updating the cache out of various CCoinsViewCache methods and into a Modifier class. This way the cache update code is just written once in a general way instead of being duplicated and split up to handle various special cases. This is a refactoring, with changes to cache behavior only in 2 corner cases (with corresponding tests in coins_test.cpp) which don't affect the meaning of data stored in the cache: * In BatchWrite, overwriting a non-dirty pruned cache entry with a fresh pruned cache entry now deletes the cache entry instead of leaving behind a dirty pruned entry that will trigger an unnecessary database write later. * In BatchWrite, overwriting a dirty pruned fresh cache entry with a nonpruned entry updates the entry without dropping the fresh flag. There's no reason to drop the fresh flag in this case because the flag accurately describes the state of the base view and could prevent unnecessary database writes in the future if the utxo is spent later.

maflcko · 2022-02-22T12:01:46Z

fuzz: test/fuzz/coins_view.cpp:140: auto coins_view_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `expected_code_path' failed.

DrahtBot · 2023-01-30T17:06:40Z

🐙 This pull request conflicts with the target branch and needs rebase.

jonatack · 2023-01-30T17:16:23Z

Concept ACK, please ping me to review when you rebase.

fanquake added Refactoring UTXO Db and Indexes labels Dec 20, 2016

ryanofsky force-pushed the pr/ccoins-cleanup branch from bf933d7 to 3110285 Compare December 21, 2016 13:41

ryanofsky changed the title ~~CCoinsViewCache code cleanup & deduplication (based on #9107, #9308, #9310)~~ CCoinsViewCache code cleanup & deduplication (based on #9107 & #9310) Dec 21, 2016

ryanofsky force-pushed the pr/ccoins-cleanup branch from 3110285 to e9316f6 Compare January 4, 2017 20:15

ryanofsky changed the title ~~CCoinsViewCache code cleanup & deduplication (based on #9107 & #9310)~~ CCoinsViewCache code cleanup & deduplication (based on #9310) Jan 4, 2017

ryanofsky force-pushed the pr/ccoins-cleanup branch from e9316f6 to c72de76 Compare January 11, 2017 15:56

ryanofsky changed the title ~~CCoinsViewCache code cleanup & deduplication (based on #9310)~~ CCoinsViewCache code cleanup & deduplication Jan 11, 2017

ryanofsky commented Jan 11, 2017

View reviewed changes

ryanofsky force-pushed the pr/ccoins-cleanup branch from c72de76 to 2f2d3c8 Compare June 2, 2017 19:59

ryanofsky force-pushed the pr/ccoins-cleanup branch from 2f2d3c8 to 3debeb1 Compare July 5, 2017 19:13

ryanofsky force-pushed the pr/ccoins-cleanup branch 2 times, most recently from 4badd8b to 8f171dc Compare November 30, 2017 14:34

ryanofsky mentioned this pull request Dec 12, 2017

Coins DB: Improve handling of FRESH child with non-DIRTY parent in CCoinsViewCa… #11354

Closed

ryanofsky force-pushed the pr/ccoins-cleanup branch from 8f171dc to 2279ff5 Compare December 12, 2017 21:03

DrahtBot closed this Jul 20, 2018

DrahtBot reopened this Jul 20, 2018

ryanofsky commented Sep 24, 2018

View reviewed changes

DrahtBot closed this Apr 28, 2019

DrahtBot reopened this Apr 28, 2019

maflcko mentioned this pull request Apr 29, 2019

Feedback for GitHub CEO #15847

Closed

DrahtBot added the Needs rebase label Jul 23, 2019

ryanofsky force-pushed the pr/ccoins-cleanup branch from 2279ff5 to 9661676 Compare August 1, 2019 18:22

DrahtBot removed the Needs rebase label Aug 1, 2019

This was referenced Feb 11, 2020

[WIP] Consensus: Don't allow a coin to be spent and FRESH. #18113

Closed

coins: allow write to disk without cache drop #17487

Merged

DrahtBot mentioned this pull request Mar 23, 2020

Docs: Improve commenting for coins.cpp|h #18410

Merged

DrahtBot mentioned this pull request Apr 14, 2020

assumeutxo #15606

Closed

18 tasks

DrahtBot added the Needs rebase label Apr 22, 2020

ryanofsky force-pushed the pr/ccoins-cleanup branch from 9661676 to 8c74391 Compare April 23, 2020 05:15

DrahtBot removed the Needs rebase label Apr 23, 2020

DrahtBot mentioned this pull request Apr 23, 2020

consensus: Don't add spent coins to the cache #18746

Closed

ryanofsky force-pushed the pr/ccoins-cleanup branch from 8c74391 to b837952 Compare April 24, 2020 17:59

DrahtBot mentioned this pull request Aug 26, 2020

validation: UTXO snapshot activation #19806

Merged

DrahtBot added the Needs rebase label Feb 16, 2021

ryanofsky force-pushed the pr/ccoins-cleanup branch from b837952 to 0c33bd2 Compare March 10, 2021 14:49

DrahtBot removed the Needs rebase label Mar 10, 2021

ryanofsky force-pushed the pr/ccoins-cleanup branch from 0c33bd2 to a40f7e5 Compare March 29, 2021 14:10

DrahtBot mentioned this pull request Sep 7, 2021

tracing: utxocache tracepoints #22902

Merged

DrahtBot added the Needs rebase label Nov 29, 2021

ryanofsky force-pushed the pr/ccoins-cleanup branch from a40f7e5 to ed03785 Compare November 29, 2021 21:01

DrahtBot removed the Needs rebase label Nov 29, 2021

DrahtBot mentioned this pull request Jun 4, 2022

[WIP] add free list to unordered map #25274

Closed

DrahtBot mentioned this pull request Oct 18, 2022

Implement CCoinsViewErrorCatcher::HaveCoin and check disk space periodically #26331

Merged

DrahtBot mentioned this pull request Nov 29, 2022

tracing: Only prepare tracepoint arguments when actually tracing #26593

Open

DrahtBot added the Needs rebase label Jan 30, 2023

achow101 closed this Apr 25, 2023

bitcoin locked and limited conversation to collaborators Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCoinsViewCache code cleanup & deduplication #9384

CCoinsViewCache code cleanup & deduplication #9384

ryanofsky commented Dec 20, 2016 •

edited by DrahtBot

Loading

ryanofsky Jan 11, 2017

ryanofsky Jan 11, 2017

ryanofsky Jan 11, 2017

ryanofsky commented Jan 11, 2017

ryanofsky commented Mar 1, 2017

ryanofsky commented Jun 2, 2017

sipa commented Mar 6, 2018

ryanofsky Sep 24, 2018

DrahtBot commented Mar 15, 2019 •

edited

Loading

DrahtBot commented Apr 28, 2019

ryanofsky commented Apr 27, 2020 •

edited

Loading

maflcko commented Feb 22, 2022

DrahtBot commented Jan 30, 2023

jonatack commented Jan 30, 2023

CCoinsViewCache code cleanup & deduplication #9384

CCoinsViewCache code cleanup & deduplication #9384

Conversation

ryanofsky commented Dec 20, 2016 • edited by DrahtBot Loading

ryanofsky Jan 11, 2017

Choose a reason for hiding this comment

ryanofsky Jan 11, 2017

Choose a reason for hiding this comment

ryanofsky Jan 11, 2017

Choose a reason for hiding this comment

ryanofsky commented Jan 11, 2017

ryanofsky commented Mar 1, 2017

ryanofsky commented Jun 2, 2017

sipa commented Mar 6, 2018

ryanofsky Sep 24, 2018

Choose a reason for hiding this comment

DrahtBot commented Mar 15, 2019 • edited Loading

Reviews

Conflicts

DrahtBot commented Apr 28, 2019

ryanofsky commented Apr 27, 2020 • edited Loading

maflcko commented Feb 22, 2022

DrahtBot commented Jan 30, 2023

jonatack commented Jan 30, 2023

ryanofsky commented Dec 20, 2016 •

edited by DrahtBot

Loading

DrahtBot commented Mar 15, 2019 •

edited

Loading

ryanofsky commented Apr 27, 2020 •

edited

Loading