tests: Add fuzzing harness for versionbits #21380

ajtowns · 2021-03-07T12:38:52Z

Adds a fuzzing harness for versionbits.

ajtowns · 2021-03-07T12:46:41Z

~~Because of the refactoring this will conflict heavily with other changes to versionbits. I've rebased #21377 on top of this and there's a height based variant at #21392.~~ I think it should be possible to adapt to cover the rest of #19573, but haven't tried. It would require some refactoring of the bip8 code in order to be able to catch bugs in the MUST_SIGNAL handling, but maybe that's worthwhile anyway.

Particularly interested in additional checks that could be added, or better ways of converting fuzz data into interesting chains to test. I've just hardcoded the intervals at 10 minutes, ~~and currently the BIP9 timeouts will precisely match some block's time, which could be missing edge cases?~~ So far it seems pretty reasonable though.

DrahtBot · 2021-03-07T17:25:58Z

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

Conflicts

No conflicts as of last run.

harding · 2021-03-07T17:42:28Z

Particularly interested in additional checks that could be added

Maybe some simple reorg testing, e.g. just invalidateblock around various thresholds to ensure we return to the previous state, then add a new block with a different timestamp to ensure we either advance or don't advance to the next state as is appropriate?

practicalswift · 2021-03-07T18:34:27Z

Strong Concept ACK

Very excited to see the versionbits implementation more thoroughly fuzzed :)

Thanks!

ajtowns · 2021-03-07T23:26:33Z

Maybe some simple reorg testing, e.g. just invalidateblock around various thresholds to ensure we return to the previous state, then add a new block with a different timestamp to ensure we either advance or don't advance to the next state as is appropriate?

That would make sense if versionbits knew about tips or cached blocks based on height, but it caches based on block hash and just relies on being able to query ancestors of whatever CBlockIndex* is passed in. Neither the fuzz tester nor the existing unit test set up the scenario sufficiently for invalidateblock to work.

achow101 · 2021-03-09T04:23:39Z

Code Review ACK 9c08af2

The cleanups to versionbits handling are nice. Skimmed over the fuzzer part, but did run it. It did catch a possible corner case in #21392.

Sjors · 2021-03-09T12:21:33Z

ACK 9c08af2

I managed to run the fuzzer (on linux), but haven't studied it in much detail. We should probably improve it in followups, so the PR's that build on top of the refactoring commits here can continue without rebase headaches.

jnewbery · 2021-03-17T10:04:51Z

Code review ACK cf81c65

ajtowns · 2021-03-19T00:36:16Z

src/test/fuzz/versionbits.cpp

+
+        // allow for times to not exactly match a block
+        if (fuzzed_data_provider.ConsumeBool()) start_time += interval / 2;
+        if (fuzzed_data_provider.ConsumeBool()) timeout += interval / 2;


@JeremyRubin on #21377 wrote:

@MarcoFalke's comment is a slight misunderstanding I believe of how the fuzzer simulates time; I recall AJ saying (somewhere) that block times are steady interval in the fuzzing so that it doesn't matter (can't find that comment now tho). Is that correct?

The fuzzer picks the mtp parameters for the deployment by first picking a random block (start_block or end_block), figuring out the timestamp that block will have (same formula as Blocks::mine_block use for nTime), and then randomly bumps one or the other by half the interval, just to check it's not relying on exact equality.

(So switching fuzzing from mtp to height should just be a matter of using start_block and end_block directly)

The steady interval means there's no direct testing of what happens if blocks happen to come really quickly or really slowly, however if the fuzzer picks two blocks in the same period for start/timeout, that should pretty much simulate the same behaviour.

ajtowns · 2021-03-19T01:19:06Z

Changed to only calculate the start/end blocks if they're going to be used

sipa

utACK afa4e12

src/test/fuzz/versionbits.cpp

sipa · 2021-03-19T05:17:24Z

utACK 1639c3b

maflcko · 2021-03-19T08:09:15Z

src/test/fuzz/versionbits.cpp

+    const int64_t m_end = 0;
+    const int m_period = 0;
+    const int m_threshold = 0;
+    const int m_bit = 0;


would be nice to not initialize const members, so that review is easier and the compiler can warn about missing initialization.

Suggested change

const int m_bit = 0;

const int m_bit;

maflcko · 2021-03-19T08:09:40Z

src/test/fuzz/versionbits.cpp

+        }
+
+        // don't risk exceeding max_blocks or times may wrap around
+        if (blocks.size() + period*2 > max_blocks) break;


clang-format ;)

Suggested change

if (blocks.size() + period*2 > max_blocks) break;

if (blocks.size() + period * 2 > max_blocks) break;

maflcko · 2021-03-19T08:41:48Z

src/test/fuzz/versionbits.cpp

+
+void initialize()
+{
+    SelectParams(CBaseChainParams::MAIN);


Would be nice to not use globals in tests, unless necessary.

- -void initialize() -{ - SelectParams(CBaseChainParams::MAIN); -} } // namespace constexpr uint32_t MAX_TIME = 4102444800; // 2100-01-01 -FUZZ_TARGET_INIT(versionbits, initialize) +FUZZ_TARGET(versionbits) { - const CChainParams& params = Params(); + const auto chainparams = CreateChainParams(ArgsManager{} , CBaseChainParams::MAIN); + const CChainParams& params = *chainparams;

maflcko · 2021-03-19T08:43:38Z

src/test/fuzz/versionbits.cpp

+{
+    SelectParams(CBaseChainParams::MAIN);
+}
+} // namespace


nit: Namespace can cover the whole file

maflcko · 2021-03-19T09:09:05Z

src/test/fuzz/versionbits.cpp

+    uint32_t signalling_mask = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
+
+    // mine prior periods
+    while (fuzzed_data_provider.remaining_bytes() > 0) {


I am trying to reproduce with FUZZ=versionbits ./src/test/fuzz/fuzz -use_value_profile=1 -entropic=1 -max_total_time=10.
Looks like I am exhausting line coverage after 10 seconds with ConsumeBool():

#68347 DONE cov: 653 ft: 3266 corp: 557/16057b lim: 122 exec/s: 6213 rss: 46Mb

Whereas remaining_bytes gives lower line coverage after 10 seconds:

#58568 DONE cov: 650 ft: 3262 corp: 530/14599b lim: 128 exec/s: 5324 rss: 46Mb

(Edit: Lower line coverage is probably expected, since remaining_bytes won't be called)

In either case, this seems premature optimization. A time-scale of 10 seconds is nothing compared to the CPU time that is going to be spent on this in our fuzz farms. I think the primary thing to optimize is to write tests in a way they are easy to understand and hard to get wrong when modified/written. For example, in another test (commit fa42da2) a fully consumed buffer has already bitten me.

maflcko

cr ACK 1639c3b

jnewbery · 2021-03-19T09:00:51Z

src/test/fuzz/versionbits.cpp

+}
+} // namespace
+
+constexpr uint32_t MAX_TIME = 4102444800; // 2100-01-01


This is a slightly confusing constant name. MAX_START_TIME would be more precise. It could also be in the unnamed namespace.

jnewbery · 2021-03-19T09:18:54Z

src/test/fuzz/versionbits.cpp

+    // making period/max_periods larger slows these tests down significantly
+    const int period = 32;
+    const size_t max_periods = 16;
+    const size_t max_blocks = 2 * period * max_periods;


I don't understand the factor of 2 here (or lower down in if (blocks.size() + period*2 > max_blocks). What is it needed for?

max_blocks is the max number of blocks "mined"; there shouldn't be any point mining more than period*max_period worth -- things should just be stuck in ACTIVE/FAILED from that point on, but this lets some extra amount be checked just in case.

If blocks.size() + period*2 > max_blocks then an additional round of the loop and then the final period combined would end up mining more than max_blocks in total.

Ah, got it. So max_periods is the max number of periods in the versionbits parameters and max_blocks is the max number of blocks in the test.

src/test/fuzz/versionbits.cpp

maflcko · 2021-03-19T10:22:20Z

src/test/fuzz/versionbits.cpp

+    CBlockIndex* mine_block(bool signal)
+    {
+        CBlockHeader header;
+        header.nVersion = signal ? m_signal : m_no_signal;


Any reason to only allow two different nVersion per test input? On the real network there is more noise. The following diff compiled for me, but feel free to ignore:

diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp index a898e2782d..1724bde3dc 100644 --- a/src/test/fuzz/versionbits.cpp +++ b/src/test/fuzz/versionbits.cpp @@ -64,11 +64,11 @@ private: std::vector<std::unique_ptr<CBlockIndex>> m_blocks; const uint32_t m_start_time; const uint32_t m_interval; - const int32_t m_signal; - const int32_t m_no_signal; + const std::vector<int32_t> m_signal; + const std::vector<int32_t> m_no_signal; public: - Blocks(uint32_t start_time, uint32_t interval, int32_t signal, int32_t no_signal) + Blocks(uint32_t start_time, uint32_t interval, std::vector<int32_t> signal, std::vector<int32_t> no_signal) : m_start_time{start_time}, m_interval{interval}, m_signal{signal}, m_no_signal{no_signal} {} size_t size() const { return m_blocks.size(); } @@ -81,7 +81,7 @@ public: CBlockIndex* mine_block(bool signal) { CBlockHeader header; - header.nVersion = signal ? m_signal : m_no_signal; + header.nVersion = signal ? m_signal.at(m_blocks.size() % m_signal.size()) : m_no_signal.at(m_blocks.size() % m_no_signal.size()); header.nTime = m_start_time + m_blocks.size() * m_interval; header.nBits = 0x1d00ffff; @@ -126,10 +126,6 @@ FUZZ_TARGET_INIT(versionbits, initialize) const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(params.GenesisBlock().nTime, MAX_TIME); - // what values for version will we use to signal / not signal? - const int32_t ver_signal = fuzzed_data_provider.ConsumeIntegral<int32_t>(); - const int32_t ver_nosignal = fuzzed_data_provider.ConsumeIntegral<int32_t>(); - // select deployment parameters: bit, start time, timeout const int bit = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, VERSIONBITS_NUM_BITS - 1); @@ -168,16 +164,25 @@ FUZZ_TARGET_INIT(versionbits, initialize) TestConditionChecker checker(start_time, timeout, period, threshold, bit); - // Early exit if the versions don't signal sensibly for the deployment - if (!checker.Condition(ver_signal)) return; - if (checker.Condition(ver_nosignal)) return; - if (ver_nosignal < 0) return; - - // TOP_BITS should ensure version will be positive - assert(ver_signal > 0); + // what values for version will we use to signal / not signal? + std::vector<int32_t> vver_signal; + std::vector<int32_t> vver_nosignal; + while (vver_signal.size() < 5) { + const auto ver_signal = fuzzed_data_provider.ConsumeIntegral<int32_t>(); + if (!checker.Condition(ver_signal)) return; + // TOP_BITS should ensure version will be positive + assert(ver_signal > 0); + vver_signal.push_back(ver_signal); + } + while (vver_nosignal.size() < 5) { + const auto ver_nosignal = fuzzed_data_provider.ConsumeIntegral<int32_t>(); + if (ver_nosignal < 0) return; + if (checker.Condition(ver_nosignal)) return; + vver_nosignal.push_back(ver_nosignal); + } // Now that we have chosen time and versions, setup to mine blocks - Blocks blocks(block_start_time, interval, ver_signal, ver_nosignal); + Blocks blocks(block_start_time, interval, vver_signal, vver_nosignal); /* Strategy: * * we will mine a final period worth of blocks, with

Oh, I like that!

I don't think it's much benefit for now though -- without some refactoring, I think it's only checking how the Condition() function defined in the fuzzer behaves, not the one that's actually used for consensus.

Yeah, good point. The first thing I did when reviewing was to change the Condition function to something else. And the test still passed, turns out this is expected. Can be revisited later.

maflcko · 2021-03-19T10:31:41Z

src/test/fuzz/versionbits.cpp

+
+    // TOP_BITS should ensure version will be positive
+    assert(ver_signal > 0);
+


nit: Could assert(ver_signal > VERSIONBITS_LAST_OLD_BLOCK_VERSION);?

jnewbery · 2021-03-19T11:44:11Z

utACK 1639c3b

maflcko · 2021-03-19T11:56:00Z

@ajtowns Let me know if you want this merged or address the comments and wait for re-ACKs

ajtowns · 2021-03-19T14:26:05Z

@MarcoFalke merge and followup sounds good to me

amitiuttarwar · 2021-03-19T20:13:56Z

src/test/fuzz/versionbits.cpp

+
+    bool Condition(int64_t version) const
+    {
+        return ((version >> m_bit) & 1) != 0 && (version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;


I'm curious why you decided to right shift the version message instead of matching how VersionBitsConditionChecker left shifts the bit to compare to the block's version.

Also why the version param here is an int64_t when CBlockHeader.nVersion is int32_t.

I don't think (?) any of these cause problems, but I think we want the test checker to match the version bits checker as closely as possible?

~~I don't think there's any good reason for either of those changes.~~ Err, by that I mean: yes, there's no good reason for it to be the way it is rather than the way you suggest.

ajtowns · 2021-03-21T01:31:52Z

Followup in #21489 ; the "use more than just two versions" idea is still waiting for a consensus refactor so that it does something useful.

Github-Pull: bitcoin#21380 Rebased-From: 1639c3b

e775b0a tests: Add fuzzing harness for versionbits (Anthony Towns) 0c471a5 tests: check never active versionbits (Anthony Towns) 3ba9283 tests: more helpful errors for failing versionbits tests (Anthony Towns) Pull request description: Backport of unit test (#21334) and fuzz test (#21380) changes for versionbits. Top commit has no ACKs. Tree-SHA512: b68b570e48e0076bb2ade3b91c59612029235d2c9e39048d548aa141fa0906343fa492e9a981065fbdbbebecbbb3dcbaf39ec69228c7581178fcca567e8201b8

aa7f418 fuzz: cleanups for versionbits fuzzer (Anthony Towns) Pull request description: Followups for #21380, shouldn't change coverage. Marking as draft to avoid introducing conflicts for the speedy trial PRs. ACKs for top commit: MarcoFalke: cr ACK aa7f418 practicalswift: Tested ACK aa7f418 Tree-SHA512: 6792364e3bb036cc903b4a5f5805d00afceeae475ce84660da962d28335bd98e59d5f45e68718657d3aa526123e351edadda39e99e49f1c6cfab629e98df35ed

ffe33df chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns) f054f6b versionbits: simplify state transitions (Anthony Towns) 55ac5f5 versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns) dd07e6d fuzz: test versionbits delayed activation (Anthony Towns) dd85d54 tests: test versionbits delayed activation (Anthony Towns) 73d4a70 versionbits: Add support for delayed activation (Anthony Towns) 9e6b65f tests: clean up versionbits test (Anthony Towns) 5932744 tests: test ComputeBlockVersion for all deployments (Anthony Towns) 63879f0 tests: pull ComputeBlockVersion test into its own function (Anthony Towns) Pull request description: BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html Edge cases are tested by fuzzing added in #21380. ACKs for top commit: instagibbs: tACK ffe33df jnewbery: utACK ffe33df MarcoFalke: review ACK ffe33df 💈 achow101: re-ACK ffe33df gmaxwell: ACK ffe33df benthecarman: ACK ffe33df Sjors: ACK ffe33df jonatack: Initial approach ACK ffe33df after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore. ariard: Code Review ACK ffe33df Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c

ajtowns force-pushed the 202103-versionbits-fuzz branch from 7b9e656 to 87266e4 Compare March 7, 2021 12:47

DrahtBot added Build system Consensus Validation labels Mar 7, 2021

maflcko added Tests and removed Build system labels Mar 7, 2021

This was referenced Mar 7, 2021

Convert taproot to flag day activation #21378

Closed

Speedy trial support for versionbits #21377

Merged

This was referenced Mar 7, 2021

fuzz: Add tx_pool fuzz target #21142

Merged

Introduce deploymentstatus #19438

Merged

ajtowns force-pushed the 202103-versionbits-fuzz branch from 87266e4 to 6bcbaf8 Compare March 7, 2021 23:07

DrahtBot mentioned this pull request Mar 7, 2021

fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) #19259

Merged

ajtowns force-pushed the 202103-versionbits-fuzz branch 2 times, most recently from bcbe9cc to d46fed6 Compare March 8, 2021 01:51

ajtowns mentioned this pull request Mar 8, 2021

Implement BIP 8 based Speedy Trial activation #21392

Closed

ajtowns force-pushed the 202103-versionbits-fuzz branch 3 times, most recently from 77beee6 to 9c08af2 Compare March 9, 2021 03:28

DrahtBot mentioned this pull request Mar 9, 2021

BIP 341: Add Speedy Trial activation parameters #21393

Closed

DrahtBot mentioned this pull request Mar 9, 2021

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

2 tasks

achow101 mentioned this pull request Mar 9, 2021

Refactor versionbits deployments to avoid potential uninitialized variables #21401

Closed

DrahtBot mentioned this pull request Mar 10, 2021

Genericide BIP9 in variable/type names and comments #21399

Closed

maflcko added the Refactoring label Mar 11, 2021

ajtowns force-pushed the 202103-versionbits-fuzz branch from 56f16a0 to cf81c65 Compare March 17, 2021 01:46

ajtowns commented Mar 19, 2021

View reviewed changes

ajtowns force-pushed the 202103-versionbits-fuzz branch from cf81c65 to afa4e12 Compare March 19, 2021 01:18

sipa approved these changes Mar 19, 2021

View reviewed changes

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

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

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

tests: Add fuzzing harness for versionbits

1639c3b

ajtowns force-pushed the 202103-versionbits-fuzz branch from afa4e12 to 1639c3b Compare March 19, 2021 05:06

maflcko reviewed Mar 19, 2021

View reviewed changes

jnewbery reviewed Mar 19, 2021

View reviewed changes

maflcko reviewed Mar 19, 2021

View reviewed changes

src/test/fuzz/versionbits.cpp Show resolved Hide resolved

maflcko reviewed Mar 19, 2021

View reviewed changes

maflcko merged commit c970c14 into bitcoin:master Mar 19, 2021

amitiuttarwar reviewed Mar 19, 2021

View reviewed changes

ajtowns mentioned this pull request Mar 21, 2021

fuzz: cleanups for versionbits fuzzer #21489

Merged

ajtowns added a commit to ajtowns/bitcoin that referenced this pull request Mar 21, 2021

tests: Add fuzzing harness for versionbits

d494650

Github-Pull: bitcoin#21380 Rebased-From: 1639c3b

ajtowns mentioned this pull request Mar 21, 2021

[0.21] Backport versionbits tests #21490

Merged

ajtowns added a commit to ajtowns/bitcoin that referenced this pull request Mar 21, 2021

tests: Add fuzzing harness for versionbits

e775b0a

Github-Pull: bitcoin#21380 Rebased-From: 1639c3b

michaelfolkson mentioned this pull request Apr 6, 2021

tests: Add fuzzing harness for versionbits BitcoinActivation/bitcoin#4

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Add fuzzing harness for versionbits #21380

tests: Add fuzzing harness for versionbits #21380

ajtowns commented Mar 7, 2021 •

edited

Loading

ajtowns commented Mar 7, 2021 •

edited

Loading

DrahtBot commented Mar 7, 2021 •

edited

Loading

harding commented Mar 7, 2021

practicalswift commented Mar 7, 2021

ajtowns commented Mar 7, 2021

achow101 commented Mar 9, 2021

Sjors commented Mar 9, 2021 •

edited

Loading

jnewbery commented Mar 17, 2021

ajtowns Mar 19, 2021

ajtowns commented Mar 19, 2021

sipa left a comment

sipa commented Mar 19, 2021

maflcko Mar 19, 2021

maflcko Mar 19, 2021

maflcko Mar 19, 2021

maflcko Mar 19, 2021

maflcko Mar 19, 2021 •

edited

Loading

maflcko left a comment

jnewbery Mar 19, 2021

jnewbery Mar 19, 2021

ajtowns Mar 19, 2021

jnewbery Mar 19, 2021

maflcko Mar 19, 2021

ajtowns Mar 19, 2021

maflcko Mar 19, 2021

maflcko Mar 19, 2021

jnewbery commented Mar 19, 2021

maflcko commented Mar 19, 2021

ajtowns commented Mar 19, 2021

amitiuttarwar Mar 19, 2021

ajtowns Mar 21, 2021 •

edited

Loading

ajtowns commented Mar 21, 2021

	if (blocks.size() + period*2 > max_blocks) break;
	if (blocks.size() + period * 2 > max_blocks) break;


		// TOP_BITS should ensure version will be positive
		assert(ver_signal > 0);

tests: Add fuzzing harness for versionbits #21380

tests: Add fuzzing harness for versionbits #21380

Conversation

ajtowns commented Mar 7, 2021 • edited Loading

ajtowns commented Mar 7, 2021 • edited Loading

DrahtBot commented Mar 7, 2021 • edited Loading

Conflicts

harding commented Mar 7, 2021

practicalswift commented Mar 7, 2021

ajtowns commented Mar 7, 2021

achow101 commented Mar 9, 2021

Sjors commented Mar 9, 2021 • edited Loading

jnewbery commented Mar 17, 2021

Choose a reason for hiding this comment

ajtowns commented Mar 19, 2021

sipa left a comment

Choose a reason for hiding this comment

sipa commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Mar 19, 2021

maflcko commented Mar 19, 2021

ajtowns commented Mar 19, 2021

Choose a reason for hiding this comment

ajtowns Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

ajtowns commented Mar 21, 2021

ajtowns commented Mar 7, 2021 •

edited

Loading

ajtowns commented Mar 7, 2021 •

edited

Loading

DrahtBot commented Mar 7, 2021 •

edited

Loading

Sjors commented Mar 9, 2021 •

edited

Loading

maflcko Mar 19, 2021 •

edited

Loading

ajtowns Mar 21, 2021 •

edited

Loading