Add simulation-based `CCoinsViewCache` fuzzer #27011

sipa · 2023-01-31T23:12:21Z

The fuzzer goes through a sequence of operations that get applied to both a real stack of CCoinsViewCache objects, and to simulation data, comparing the two at the end.

DrahtBot · 2023-01-31T23:12:24Z

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
ACK	jamesob, dergoegge

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:

#25325 (Add pool based memory resource by martinus)

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.

maflcko

Left some style nits. (Can be ignored unless there is other stuff to push)

src/test/fuzz/coinscache_sim.cpp

dergoegge

Concept ACK

src/test/fuzz/coinscache_sim.cpp

jamesob

ACK c7d2009 (jamesob/ackr/27011.2.sipa.add_simulation_based_cco)

Built and reviewed locally. Always happy to see more coins tests.

Just a few questions, but as-is this represents a mergeable improvement.

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK c7d20096b6808b11aadda7a652273298b3dd98a8 ([`jamesob/ackr/27011.2.sipa.add_simulation_based_cco`](https://github.com/jamesob/bitcoin/tree/ackr/27011.2.sipa.add_simulation_based_cco))

Built and reviewed locally. Always happy to see more coins tests. 

Just a few questions, but as-is this represents a mergeable improvement.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmPadUMACgkQepNdrbLE
TwUs4w//V3HDJYwqiLfMmjKTT1Mo80v0cyFG2WQrUlnLIEBOdSZSor1ur2AgzOjk
4paKsSCuMzUVfm+CHVvuIRxpPyg9Q4sEONql979/H1yB8VvK7zbddJJ1nyKuEHJQ
mex99u0TWspnQRst6T1B1STwSX7Sjc6UUAcVG1dkIZxHlaR0ll8ZDaQcHo9vUWyy
gHVRi6V+YsiyVqTJ59hEYgkbHQASjLIFFSGyghMDJTp3jTtOKEDJZzdzpL8KgqSE
1Mj1/IsEasgekgxAWKuAC29PetSC+XktzHVrLGfR1IHnWnMQ1eoe8lLg86FD1byS
U+QpTjb4KMf6zWiflzWcIE0EHgmxaBqug4uAuufPrZKd5r2OQyDxy1LkiawatTgu
iwSvFriNH6X2B7I8J34l+/9g9NxubwxVoJvscuuk14vW80IK6lUyPsxc/ujIqiRl
ganCc9Obon1JViEdh/t/sjmmHzCFpYmftvjNn+Kwl0XVSMa2a1zKIKMDoR18l/8L
v60G6haqyl49H2mmjIPEnomsujwtQQW6mGYolYhAI+LuYOCmkPHQ6Mw8VsGZRmdg
xxoATDjx5wMoGCjg3/p8TWszUTHDo3UxpkcBlR9lteWpOTCi3LtaNIYuv8/aBYJJ
nbtJlxNXUGFoYBbC/RxLhTx6orbkcldElwXPtxhx/CYKmxWXMiA=
=f9q/
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-6.1.6-arch1-3-x86_64-with-glibc2.36

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin2/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin2/db4/include/ 'CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis' --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --disable-module-ecdh --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i

Compiler version: clang version 15.0.7

src/test/fuzz/coinscache_sim.cpp

jamesob · 2023-02-01T14:13:49Z

src/test/fuzz/coinscache_sim.cpp

+            },
+
+            [&]() { // Sync.
+                // Apply to simulation data (note that in our simulation, syncing and flushing is the same thing).


Hm. Can't you emulate Sync() by adding an erase parameter to flush() that just doesn't set the cache entry to EntryType::NONE? If you don't do this, won't the contents of the simulation cache differ from the real caches?

Semantically, Flush() and Sync() have the same effect: they push the changes made in this cache to the parent coinsview. Whether or not entries stay in a cache is an implementation aspect that's not observable (except through CacheSize() etc).

The NONE entry in the simulated cache is also different from "missing" in the real cache, in that lookups in the simulated cache never pull in entries from parent caches, so you can't really compare the actual cache entrytype with anything the real caches do. They're wholly different implementations, but their observable behavior should be identical.

You're right that I could choose to not reset things to NONE in the simulated cache, but that'd in effect be extending the test to test the behavior of the simulation: resetting to NONE is just a non-observable optimization too there.

jamesob

re-ACK fd84485

A small refactoring change and comment updates.

git range-diff upstream/master ackr/27011.2.sipa.add_simulation_based_cco ackr/27011.3.sipa.add_simulation_based_cco

The fuzzer goes through a sequence of operations that get applied to both a real stack of CCoinsViewCache objects, and to simulation data, comparing the two at the end.

src/coins.cpp

dergoegge

Code review ACK 59e6828

maflcko · 2023-02-02T21:48:14Z

https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/59e6828bb5b56a23/fuzz.coverage/index.html

src/test/fuzz/coinscache_sim.cpp

jamesob · 2023-02-07T16:34:24Z

re-ACK 561848a

Sjors · 2023-02-08T14:32:05Z

I ran the fuzzer using the latest corpus for about an hour and it didn't find any crashes, so that's nice.

sipa · 2023-02-08T14:44:48Z

The seeds I've added to qa-assets for this fuzzer (bitcoin-core/qa-assets#103) are the result of a few months of CPU time, FWIW.

dergoegge · 2023-02-13T12:08:54Z

Code review ACK 561848a

561848a Exercise non-DIRTY spent coins in caches in fuzz test (Pieter Wuille) 59e6828 Add deterministic mode to CCoinsViewCache (Pieter Wuille) b0ff310 Add CCoinsViewCache::SanityCheck() and use it in fuzz test (Pieter Wuille) 3c9cea1 Add simulation-based CCoinsViewCache fuzzer (Pieter Wuille) Pull request description: The fuzzer goes through a sequence of operations that get applied to both a real stack of `CCoinsViewCache` objects, and to simulation data, comparing the two at the end. ACKs for top commit: jamesob: re-ACK bitcoin@561848a dergoegge: Code review ACK 561848a Tree-SHA512: 68634f251fdb39436b128ecba093f651bff12ac11508dc9885253e57fd21efd44edf3b22b0f821c228175ec507df7d46c7f9f5404fc1eb8187fdbd136a5d5ee2

martinus · 2023-02-14T06:40:25Z

@sipa While rebasing #25325 I saw that the fuzzer in coins_view.cpp still uses CCoinsMap coins_map; and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?

I can add it while rebasing, I'm touching that line anyways. Unless you want to do this separately

EDIT: I had a look what happens when I remove the default bool deterministic = false in SaltedOutpointHasher, and found another use of the map in the fuzzer, in tx_pool.cpp through const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool};

sipa · 2023-02-15T00:14:38Z

I saw that the fuzzer in coins_view.cpp still uses CCoinsMap coins_map;

I think that should probably be addressed.

and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?

I believe it does?

    CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};

EDIT: I had a look what happens when I remove the default bool deterministic = false in SaltedOutpointHasher, and found another use of the map in the fuzzer, in tx_pool.cpp through const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool};

Makes sense to change that too, I think.

martinus · 2023-02-15T05:15:21Z

I believe it does?

It does in line 49 for CCoinsViewCache, but not in line 118 for the CCoinsMap coins_map;. I've added that in #25325 here

sipa force-pushed the 202301_coinscache_fuzz branch from 1a1bd15 to f87382a Compare February 1, 2023 02:36

maflcko reviewed Feb 1, 2023

View reviewed changes

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

fanquake requested a review from dergoegge February 1, 2023 10:26

dergoegge reviewed Feb 1, 2023

View reviewed changes

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202301_coinscache_fuzz branch from f87382a to c7d2009 Compare February 1, 2023 14:06

jamesob approved these changes Feb 1, 2023

View reviewed changes

sipa force-pushed the 202301_coinscache_fuzz branch from c7d2009 to fd84485 Compare February 1, 2023 15:44

jamesob approved these changes Feb 1, 2023

View reviewed changes

Add simulation-based CCoinsViewCache fuzzer

3c9cea1

The fuzzer goes through a sequence of operations that get applied to both a real stack of CCoinsViewCache objects, and to simulation data, comparing the two at the end.

sipa force-pushed the 202301_coinscache_fuzz branch from fd84485 to bc2cc72 Compare February 1, 2023 23:34

ghost reviewed Feb 1, 2023

View reviewed changes

src/coins.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202301_coinscache_fuzz branch from bc2cc72 to b95982b Compare February 1, 2023 23:57

DrahtBot mentioned this pull request Feb 2, 2023

test: Use std::unique_ptr over manual delete in coins_tests #27004

Merged

Add CCoinsViewCache::SanityCheck() and use it in fuzz test

b0ff310

sipa force-pushed the 202301_coinscache_fuzz branch from b95982b to 883c2c8 Compare February 2, 2023 04:15

dergoegge reviewed Feb 2, 2023

View reviewed changes

src/coins.cpp Outdated Show resolved Hide resolved

Add deterministic mode to CCoinsViewCache

59e6828

sipa force-pushed the 202301_coinscache_fuzz branch from 883c2c8 to 59e6828 Compare February 2, 2023 14:00

dergoegge approved these changes Feb 2, 2023

View reviewed changes

fanquake requested a review from jamesob February 2, 2023 16:56

This was referenced Feb 2, 2023

Add initial corpus for coinscache_sim bitcoin-core/qa-assets#102

Merged

Add pool based memory resource #25325

Merged

maflcko reviewed Feb 2, 2023

View reviewed changes

src/test/fuzz/coinscache_sim.cpp Outdated Show resolved Hide resolved

Exercise non-DIRTY spent coins in caches in fuzz test

561848a

sipa mentioned this pull request Feb 7, 2023

Update coinscache_sim fuzz corpus bitcoin-core/qa-assets#103

Merged

DrahtBot requested a review from dergoegge February 10, 2023 11:54

maflcko merged commit 8126551 into bitcoin:master Feb 13, 2023

bitcoin locked and limited conversation to collaborators Feb 18, 2024

Add simulation-based CCoinsViewCache fuzzer #27011

Add simulation-based CCoinsViewCache fuzzer #27011

Uh oh!

Conversation

sipa commented Jan 31, 2023

Uh oh!

DrahtBot commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Feb 1, 2023 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 2, 2023

Uh oh!

Uh oh!

jamesob commented Feb 7, 2023

Uh oh!

Sjors commented Feb 8, 2023

Uh oh!

sipa commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dergoegge commented Feb 13, 2023

Uh oh!

martinus commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Feb 15, 2023

Uh oh!

martinus commented Feb 15, 2023

Uh oh!

Uh oh!

Add simulation-based `CCoinsViewCache` fuzzer #27011

Add simulation-based `CCoinsViewCache` fuzzer #27011

DrahtBot commented Jan 31, 2023 •

edited

Loading

maflcko left a comment •

edited

Loading

sipa Feb 1, 2023 •

edited

Loading

sipa commented Feb 8, 2023 •

edited

Loading

martinus commented Feb 14, 2023 •

edited

Loading