Skip to content

fuzz: Add utxo_snapshot target #21953

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
May 22, 2021
Merged

fuzz: Add utxo_snapshot target #21953

maflcko merged 1 commit into from
May 22, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 15, 2021

No description provided.

@fanquake fanquake added the Tests label May 15, 2021
@practicalswift
Copy link
Contributor

practicalswift commented May 15, 2021

Concept ACK

Very nice! :)

@maflcko maflcko force-pushed the 2104-assumeutxoCrash02 branch from fab6ed5 to fa91994 Compare May 16, 2021 09:35
@adamjonas
Copy link
Member

adamjonas commented May 20, 2021

Fuzzed utxo_snapshot with fa91994 and did see modest coverage gains. Naively wondering whether if it'd be possible to improve the execution speed.

Log 
#2	pulse  ft: 36401 exec/s: 0 rss: 547Mb
#4	pulse  cov: 36733 ft: 36457 corp: 2/2b exec/s: 0 rss: 561Mb
#8	pulse  cov: 36749 ft: 36458 corp: 3/3b exec/s: 0 rss: 582Mb
#16	pulse  cov: 36750 ft: 36460 corp: 4/4b exec/s: 0 rss: 624Mb
#32	pulse  cov: 36750 ft: 36461 corp: 6/6b exec/s: 1 rss: 695Mb
#64	pulse  cov: 36751 ft: 36472 corp: 10/10b exec/s: 2 rss: 789Mb
#128	pulse  cov: 36751 ft: 36475 corp: 12/12b exec/s: 4 rss: 790Mb
#256	pulse  cov: 36751 ft: 36475 corp: 12/12b exec/s: 6 rss: 790Mb
#512	pulse  cov: 36751 ft: 36475 corp: 12/12b exec/s: 9 rss: 790Mb
#1024	pulse  cov: 36759 ft: 36511 corp: 18/24b exec/s: 13 rss: 790Mb
#2048	pulse  cov: 36761 ft: 36544 corp: 23/36b exec/s: 15 rss: 790Mb
#4096	pulse  cov: 37753 ft: 40612 corp: 56/151b exec/s: 16 rss: 790Mb
#8192	pulse  cov: 37754 ft: 40812 corp: 106/426b exec/s: 16 rss: 790Mb
...

@maflcko
Copy link
Member Author

maflcko commented May 21, 2021

Naively wondering whether if it'd be possible to improve the execution speed.

Sure, just set the tempdir to a ramdisk

@maflcko
Copy link
Member Author

maflcko commented May 21, 2021

For refrerence, I am getting

#3502	NEW    cov: 4861 ft: 5258 corp: 79/859b lim: 17 exec/s: 106 rss: 91Mb L: 17/17 MS: 1 CopyPart-

jamesob
jamesob approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@jamesob jamesob left a comment

Choose a reason for hiding this comment

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

ACK fa91994

I'm in the process of trying to get this to run locally, but have reviewed the code and it looks like a great start. We could maybe save some cycles by conditionally prepending valid snapshot metadata to the file we construct (in the same way that we conditionally have the background chainstate synced to the snapshot base), but in any case this is a good change.

I'll report back with logs when I get this running.

src/test/fuzz/utxo_snapshot.cpp
const auto& coinscache{chainman.ActiveChainstate().CoinsTip()};
int64_t chain_tx{};
for (const auto& block : *g_chain) {
Assert(coinscache.HaveCoin(COutPoint{block->vtx.at(0)->GetHash(), 0}));
Copy link
Contributor

@jamesob jamesob May 20, 2021

Choose a reason for hiding this comment

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

Just out of curiosity - if we've gotten here, that means that the fuzzer has provided random input that has happened to correspond to a utxo snapshot that matches the chain we've generated in setup. This seems extraordinarily unlikely to me; how often do we expect this to actually happen? Am I missing some mechanic about how the fuzzer works that makes this a more frequent thing?

Copy link
Member Author

@maflcko maflcko May 21, 2021

Choose a reason for hiding this comment

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

Right, I expect this to be impossible unless one of the initial seed inputs is a valid snapshot.

Copy link
Member Author

@maflcko maflcko May 22, 2021

Choose a reason for hiding this comment

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

Full coverage (as of now): https://marcofalke.github.io/btc_cov/fuzz.coverage/src/test/fuzz/utxo_snapshot.cpp.gcov.html

@jamesob
Copy link
Contributor

jamesob commented May 21, 2021

Tested ACK

Ran it locally for a while:
#9331 NEW cov: 18641 ft: 21159 corp: 162/2883b lim: 38 exec/s: 17 rss: 549Mb L: 25/36 MS: 2 InsertByte-PersAutoDict- DE: "debuglogf"-

@maflcko maflcko merged commit be41716 into bitcoin:master May 22, 2021
@maflcko maflcko deleted the 2104-assumeutxoCrash02 branch May 22, 2021 08:14
@practicalswift
Copy link
Contributor

practicalswift commented May 22, 2021

Post-merge tested ACK fa91994

$ FUZZ=utxo_snapshot src/test/fuzz/fuzz utxo_snapshot/
…
INFO:      266 files found in utxo_snapshot/
…
#16     pulse  cov: 20394 ft: 20311 corp: 5/6b exec/s: 8 rss: 241Mb
…
#267    INITED cov: 22144 ft: 29197 corp: 145/5368b exec/s: 3 rss: 497Mb

utxo_snapshot/ created from thin air during ~8 hours of fuzzing.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 24, 2021
Merge bitcoin#21953: fuzz: Add utxo_snapshot target
0ade921
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge be41716 into merged_master (Bitcoin PR bitcoin/bitcoin#21953)
2a2bbda
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jamesob jamesob jamesob approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @practicalswift @adamjonas @jamesob @fanquake