fuzz: improve utxo_snapshot target #30329

mzumsande · 2024-06-24T19:41:25Z

Add the possibility of giving more guidance to the creation of the metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes successfully creates a valid snapshot.

This also changes the asserts for the success case that were outdated (after #29370) and only didn't result in a crash because the fuzzer wasn't able to reach this code before.

DrahtBot · 2024-06-24T19:41:28Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, TheCharlatan, 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:

#29656 (chainparams: Change nChainTx type to uint64_t by fjahr)
#29307 (util: explicitly close all AutoFiles that have been written by vasild)

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 · 2024-06-24T20:51:02Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/26619261177}

fjahr · 2024-06-24T21:47:40Z

Concept ACK

maflcko

lgtm. Thanks!

maflcko · 2024-07-03T14:45:57Z

src/kernel/chainparams.cpp

@@ -495,12 +495,19 @@ class CRegTestParams : public CChainParams
        };

        m_assumeutxo_data = {
-            {
+            {   // For use by unit tests


nit: maybe also list the filename here, like in the other cases.

I didn't do that because there are already multiple files that use it (validation_chainstatemanager_tests, validation_chainstate_tests). Seems less likely that the same would happen for fuzz tests.

maflcko · 2024-07-03T14:46:12Z

src/kernel/chainparams.cpp

+                .height = 200,
+                .hash_serialized = AssumeutxoHash{uint256S("0x998b6a337899f04a1cd298ecf0fdc6ab66bfb428f00e5a48998505e91b3b55e0")},
+                .nChainTx = 201,
+                .blockhash = uint256S("0x5e93653318f294fb5aa339d00bbf8cf1c3515488ad99412c37608b139ea63b27,")


style nit: Missing trailing ,?

fixed, the "," went into the hash by mistake (and was ignored). I also fixed the hash_serialized, which was wrong somehow.

the "," went into the hash by mistake (and was ignored)

ugh. I didn't even notice this

maflcko · 2024-07-03T14:48:44Z

src/test/fuzz/utxo_snapshot.cpp

+            outfile << Span{metadata};
+        } else {
+            DataStream data_stream{};
+            auto msg_start = Params().MessageStart();


style-nit:

Suggested change

auto msg_start = Params().MessageStart();

auto msg_start = chainman.Params().MessageStart();

fixed, here and at one more spot.

src/test/fuzz/utxo_snapshot.cpp

maflcko

ACK b9ba1a7 🎯

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK b9ba1a73094f4ad593b527e23d2681f41c22539 🎯
iTX6VgeBMzgsveHFGIl5Qnsj/cWt0KYcv2B3dvhxmLm6ljOkWyT0bGPRv0zKVRDXMpcDBJcwelxlW8o6WHPEDw==

TheCharlatan · 2024-07-03T20:27:27Z

I checked if there is anything we could do in the testing setup to make the test faster. With this patchset, the test takes around 1:15 min for 10k runs on my machine. Removing the validation interface from the test setup cuts it down to 1:05 min , if I remove the networking setup, it takes 35 sec to complete.

I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?

fjahr · 2024-07-03T22:24:43Z

I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?

That appears to be a bug. We actually require the mempool to be empty so it not being there should not be a problem. I will open a PR.

fjahr · 2024-07-03T23:34:07Z

I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?

That appears to be a bug. We actually require the mempool to be empty so it not being there should not be a problem. I will open a PR.

This should work fjahr@21530b4, but on second thought I am not sure if we need this (yet). Conceptually it makes sense that we change this because, as I said, we don't need the mempool. However, since we don't use the mempool I think this should also not make an impact on the testing speed and even in blocksonly mode there still is a mempool, so this won't be an issue.

This might be getting off-topic so I can open a separate issue to move the discussion.

maflcko · 2024-07-04T06:16:16Z

I checked if there is anything we could do in the testing setup to make the test faster. With this patchset, the test takes around 1:15 min for 10k runs on my machine. Removing the validation interface from the test setup cuts it down to 1:05 min , if I remove the networking setup, it takes 35 sec to complete.

Nice. Would be good to have a pull request with this, so that it can be reviewed and merged.

fjahr · 2024-07-04T12:23:17Z

utACK b9ba1a7

TheCharlatan

ACK b9ba1a7

... but it would be nice to address the nits, will re-ACK quickly :)

Add the possibility of giving more guidance to the creation of the metadata and/or coins, so that the fuzzer gets the chance to reach more error conditions in ActivateSnapshot and sometimes successfully creates a valid snapshot. This also changes the asserts for the success case that were outdated, and only didn't result in a crash because the fuzzer wasn't able to reach this code before.

mzumsande · 2024-07-05T00:36:38Z

Thanks for the reviews! I addressed the feedback by @maflcko and also corrected hash_serialized in the chainparams. Not sure why it was wrong before, I'm pretty sure I was able to activate the snapshot before opening the PR on another computer. Probably just an oversight, but maybe someone can confirm that everything is deterministic and the snapshot can be loaded now (e.g. by adding an assert and setting base_blockheight and m_coins_count to 200 it should crash almost immediately).

I checked if there is anything we could do in the testing setup to make the test faster. (...)

Sound like good ideas for a follow-up!

maflcko · 2024-07-05T06:09:45Z

re-ACK de71d4d 🎆

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb 🎆
jzFq8Tx4Dv2D6Jmy+qeBCaFo50xs+4wd4o3IJT7NvMAv64PiF75BRT4tYBxjuU2NEb6pH6x4uKTzg4Rptjc3Bw==

TheCharlatan

ACK de71d4d

fjahr · 2024-07-05T09:37:49Z

utACK de71d4d

mzumsande · 2024-07-08T17:49:06Z

maybe someone can confirm that everything is deterministic and the snapshot can be loaded now (e.g. by adding an assert and setting base_blockheight and m_coins_count to 200 it should crash almost immediately).

FYI, confirmed for myself on another computer that this was just an error, and the snapshot can be loaded by the fuzzer.

ryanofsky

Post-merge code review ACK de71d4d

This seems like a good change in principle, but I'm not actually sure what additional test coverage it provides. The only valid snapshot that can be loaded seems to be one that is hardcoded for a chain with height 200 and no transactions except for coinbases. After the snapshot is generated there is only a single ConsumeBool call determining whether or not headers for the blocks are added before loading the snapshot.

So basically it creates a hardcoded snapshot, and then tests loading it with and without headers. I could be missing something, but it seems like that is not really taking advantage of the fuzzer or testing something that would be not more convenient to run as a unit test.

It would probably open up a lot more flexibility for the fuzz test if it were able to modify the m_assumeutxo_data vector instead of relying on hardcoded values.

ryanofsky · 2024-07-09T20:01:33Z

src/kernel/chainparams.cpp

+                .height = 200,
+                .hash_serialized = AssumeutxoHash{uint256S("0x4f34d431c3e482f6b0d67b64609ece3964dc8d7976d02ac68dd7c9c1421738f2")},
+                .nChainTx = 201,
+                .blockhash = uint256S("0x5e93653318f294fb5aa339d00bbf8cf1c3515488ad99412c37608b139ea63b27"),


In commit "fuzz: improve utxo_snapshot target" (de71d4d)

Since fuzz test is not deterministic, and doesn't give us an easy way to know hash values here actually allow snapshot to be loaded, it might be good to write a separate regtest generating a an empty chain with height 200 and verifying these hashes.

mzumsande · 2024-07-09T21:12:53Z

This seems like a good change in principle, but I'm not actually sure what additional test coverage it provides. The only valid snapshot that can be loaded seems to be one that is hardcoded for a chain with height 200 and no transactions except for coinbases. After the snapshot is generated there is only a single ConsumeBool call determining whether or not headers for the blocks are added before loading the snapshot.

Note that loading a valid snapshot is not the only goal of this PR. It also allows for a better coverage of ActivateSnapshot() which was previously hard to reach because the fuzzer already had a hard time fuzzing valid metadata (after #29612) and would abort without it. E.g. it has zero coverage in https://maflcko.github.io/b-c-cov/fuzz.coverage/src/validation.cpp.gcov.html though I'm not sure how up-do-date the corpus is, and fuzzing locally I also didn't reach ActivateSnapshot() (maybe I wasn't patient enough).

I think for more interesting fuzzing we'd have to think about allowing dynamic registration of snapshots in tests (or enabling an option to just skip the check for the hash in chainparams).

Some time ago, I thought about this in the context of functional tests (to avoid having to edit chainparams whenever we change something in a functional test), but after realizing that the registered snapshot data from chainparams is not only necessary during registration but also in a restart (nChainTx) it seemed too crude to me and I didn't open a PR. Might make more sense for fuzzing though.

maflcko · 2024-07-23T19:39:46Z

Found #30514

160799d test: refactor: introduce `create_ephemeral_dust_package` helper (Sebastian Falbesoner) 61e18de doc: ephemeral policy: add missing closing double quote (Sebastian Falbesoner) Pull request description: This small PR contains ephemeral dust follow-ups mentioned in #30329 that were not tackled in the first follow-up PR #31279: #30239 (comment) #30239 (comment) Happy to add more if I missed some or anyone has concrete commits to add. ACKs for top commit: rkrux: tACK 160799d instagibbs: ACK 160799d tdb3: Code review ACK 160799d Tree-SHA512: e9a80c6733f1e7fe9e834d81b404f6e8ef7a61fe986f61b3dcdbda1a0bc547145fc279ec02f54361df56cb4e62a6fedaa0f3991c6e084c3a703ed1b1bfbdbe4e

DrahtBot added the Tests label Jun 24, 2024

mzumsande force-pushed the 202406_fuzz_assumeutxo branch from 2e129c4 to a270cb8 Compare June 24, 2024 20:50

DrahtBot added the CI failed label Jun 24, 2024

mzumsande force-pushed the 202406_fuzz_assumeutxo branch from a270cb8 to 65c5321 Compare June 24, 2024 21:04

mzumsande force-pushed the 202406_fuzz_assumeutxo branch from 65c5321 to 3c8d883 Compare June 24, 2024 22:20

DrahtBot removed the CI failed label Jun 24, 2024

DrahtBot added the Needs rebase label Jul 2, 2024

mzumsande force-pushed the 202406_fuzz_assumeutxo branch from 3c8d883 to b9ba1a7 Compare July 2, 2024 22:52

DrahtBot removed the Needs rebase label Jul 3, 2024

maflcko approved these changes Jul 3, 2024

View reviewed changes

maflcko reviewed Jul 3, 2024

View reviewed changes

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

maflcko reviewed Jul 3, 2024

View reviewed changes

DrahtBot requested a review from fjahr July 3, 2024 15:04

fjahr mentioned this pull request Jul 4, 2024

validation: Check if mempool exists before size check in ActivateSnapshot #30388

Merged

TheCharlatan approved these changes Jul 4, 2024

View reviewed changes

mzumsande force-pushed the 202406_fuzz_assumeutxo branch from b9ba1a7 to de71d4d Compare July 5, 2024 00:19

DrahtBot requested a review from TheCharlatan July 5, 2024 06:09

TheCharlatan approved these changes Jul 5, 2024

View reviewed changes

ryanofsky merged commit 5239e93 into bitcoin:master Jul 9, 2024

mzumsande deleted the 202406_fuzz_assumeutxo branch July 9, 2024 20:18

ryanofsky reviewed Jul 9, 2024

View reviewed changes

maflcko mentioned this pull request Aug 5, 2024

refactor: Add consteval uint256 constructor #30560

Merged

theStack mentioned this pull request Nov 25, 2024

doc, test: more ephemeral dust follow-ups #31371

Merged

darosior mentioned this pull request Feb 19, 2025

qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data #31910

Merged

	auto msg_start = Params().MessageStart();
	auto msg_start = chainman.Params().MessageStart();

fuzz: improve utxo_snapshot target #30329

fuzz: improve utxo_snapshot target #30329

Uh oh!

Conversation

mzumsande commented Jun 24, 2024

Uh oh!

DrahtBot commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Jun 24, 2024

Uh oh!

fjahr commented Jun 24, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented Jul 3, 2024

Uh oh!

fjahr commented Jul 3, 2024

Uh oh!

fjahr commented Jul 3, 2024

Uh oh!

maflcko commented Jul 4, 2024

Uh oh!

fjahr commented Jul 4, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jul 5, 2024

Uh oh!

maflcko commented Jul 5, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jul 5, 2024

Uh oh!

mzumsande commented Jul 8, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jul 9, 2024

Uh oh!

maflcko commented Jul 23, 2024

Uh oh!

Uh oh!

DrahtBot commented Jun 24, 2024 •

edited

Loading