tests: Add fuzzing harness for versionbits

ajtowns commented at 12:38 pm on March 7, 2021: member

Adds a fuzzing harness for versionbits.

ajtowns commented at 12:46 pm on March 7, 2021: member

~~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.

ajtowns force-pushed on Mar 7, 2021

DrahtBot added the label Build system on Mar 7, 2021

DrahtBot added the label Consensus on Mar 7, 2021

DrahtBot added the label Validation on Mar 7, 2021

MarcoFalke removed the label Build system on Mar 7, 2021

MarcoFalke added the label Tests on Mar 7, 2021

DrahtBot commented at 5:25 pm on March 7, 2021: member

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

Conflicts

No conflicts as of last run.

harding commented at 5:42 pm on March 7, 2021: contributor

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 commented at 6:34 pm on March 7, 2021: contributor

Strong Concept ACK

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

Thanks!

ajtowns force-pushed on Mar 7, 2021

ajtowns commented at 11:26 pm on March 7, 2021: member

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.

ajtowns force-pushed on Mar 7, 2021

ajtowns force-pushed on Mar 8, 2021

ajtowns force-pushed on Mar 9, 2021

achow101 commented at 4:23 am on March 9, 2021: member

Code Review ACK 9c08af24e77980673efa30b5101451c3c8b20b1b

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 commented at 12:21 pm on March 9, 2021: member

ACK 9c08af24e77980673efa30b5101451c3c8b20b1b

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.

jonasschnelli added this to the "Blockers" column in a project

MarcoFalke added the label Refactoring on Mar 11, 2021

MarcoFalke commented at 9:29 am on March 12, 2021: member

I am not sure if it is good to hide validation code refactoring in a “Add fuzzing harness” pull

ajtowns commented at 9:40 pm on March 12, 2021: member

I am not sure if it is good to hide validation code refactoring in a “Add fuzzing harness” pull

It’s not mean to be hidden; the commits aren’t subtle, it’s called out in the description, and the labels are all there… The reason it’s a single PR is that (I don’t think) the fuzzer works without the refactoring, and without the fuzzer, there’s not a lot of other tests to ensure the refactoring is correct…

MarcoFalke commented at 7:44 am on March 13, 2021: member

Maybe the title could be changed to “Refactor versionbits to add fuzzing harness” or so?

ajtowns renamed this:
~~Add fuzzing harness for versionbits~~
versionbits: Refactor and add fuzzing harness
on Mar 13, 2021

jnewbery commented at 12:43 pm on March 15, 2021: member

Concept ACK.

Can we remove nRuleChangeActivationThreshold from Params now that it’s only used in the WarningBitsConditionChecker?

DrahtBot commented at 4:49 pm on March 15, 2021: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

DrahtBot added the label Needs rebase on Mar 15, 2021

ajtowns force-pushed on Mar 15, 2021

DrahtBot removed the label Needs rebase on Mar 16, 2021

ajtowns commented at 3:31 am on March 16, 2021: member

Can we remove nRuleChangeActivationThreshold from Params now that it’s only used in the WarningBitsConditionChecker?

It’s still chain specific, so it could be moved from Consensus::Params to ChainParams (and renamed), but not removed entirely I think.

ajtowns commented at 5:23 am on March 16, 2021: member

The reason it’s a single PR is that (I don’t think) the fuzzer works without the refactoring, […]

This turns out not to be the case. I’m having a go at changing the approach to fuzzing approach, and if that works out will probably split the refactoring out of this PR.

ajtowns force-pushed on Mar 16, 2021

ajtowns renamed this:
~~versionbits: Refactor and add fuzzing harness~~
tests: Add fuzzing harness for versionbits
on Mar 16, 2021

ajtowns removed the label Consensus on Mar 16, 2021

ajtowns removed the label Refactoring on Mar 16, 2021

ajtowns removed the label Validation on Mar 16, 2021

ajtowns commented at 11:31 am on March 16, 2021: member

Rebased and updated to only add the fuzzing harness and not do any refactoring.

This also changes the approach used for fuzzing to only sanity check one period’s worth of blocks, rather than every period – relying instead on different fuzz inputs to check different periods. Seems to be no less thorough, and much faster for fuzzing. Also might be a bit easier to understand.

in src/test/fuzz/versionbits.cpp:34 in e333affe34 outdated

29+    const int64_t m_end = 0;
30+    const int m_period = 0;
31+    const int m_threshold = 0;
32+    const int m_bit = 0;
33+
34+    TestConditionChecker(int64_t begin, int64_t end, int period, int threshold, int bit) : m_begin{begin}, m_end{end}, m_period{period}, m_threshold{threshold}, m_bit{bit}

jnewbery commented at 11:55 am on March 16, 2021:

Would you consider adding a few line breaks to these function declarations?

MarcoFalke commented at 12:03 pm on March 16, 2021:

style-nit: (add new line and clang-format to avoid excessive long lines)

0    TestConditionChecker(int64_t begin, int64_t end, int period, int threshold, int bit)
1    : m_begin{begin}, m_end{end}, m_period{period}, m_threshold{threshold}, m_bit{bit}

in src/test/fuzz/versionbits.cpp:105 in e333affe34 outdated

105+{
106+    return;
107+}
108+} // namespace
109+
110+FUZZ_TARGET_INIT(versionbits, initialize)

jnewbery commented at 12:00 pm on March 16, 2021:

0} // namespace
1
2FUZZ_TARGET(versionbits)

MarcoFalke commented at 12:06 pm on March 16, 2021:

style-nit: Can be shorter

0FUZZ_TARGET(versionbits)

in src/test/fuzz/versionbits.cpp:117 in e333affe34 outdated

112+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
113+
114+    const int period = 32;
115+    const int threshold = 29;
116+
117+    assert(0 < threshold && threshold <= period - 2);  // must be able to not signal!

jnewbery commented at 12:01 pm on March 16, 2021:

0    constexpr int period{32};
1    constexpr int threshold{29};
2
3    static_assert(0 < threshold && threshold <= period - 2);  // must be able to not signal!

jnewbery commented at 12:03 pm on March 16, 2021:

Also consider s/period/PERIOD/ and s/threshold/THRESHOLD/

MarcoFalke commented at 12:08 pm on March 16, 2021:

shouldn’t this be a static assert? (With the constexpr symbols all uppercase)

Or is the goal to have the fuzz engine pick the period length?

ajtowns commented at 1:04 pm on March 16, 2021:

Wanted to leave it open for the threshold could be picked by the fuzzer at least. Changing the period makes the test take longer, so not sure how much sense it makes to fuzz that.

in src/test/fuzz/versionbits.cpp:76 in e333affe34 outdated

71+    ~Blocks()
72+    {
73+        for (auto& v : blocks) {
74+            delete v;
75+        }
76+        blocks.clear();

jnewbery commented at 12:06 pm on March 16, 2021:

Any reason not to make blocks a std::vector<std::unique_ptr<CBlockIndex>>, and let the default destructor take care of this for you?

ajtowns commented at 1:34 pm on March 16, 2021:

Seemed easier to do it this way than to have to manually extract the raw pointers everywhere

jnewbery commented at 2:02 pm on March 16, 2021:

I think you’d only need to change the tip() and mine_block() member functions:

 0diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
 1index ded18d8602..1613f65586 100644
 2--- a/src/test/fuzz/versionbits.cpp
 3+++ b/src/test/fuzz/versionbits.cpp
 4@@ -13,6 +13,7 @@
 5 #include <test/fuzz/util.h>
 6 
 7 #include <cstdint>
 8+#include <memory>
 9 #include <optional>
10 #include <string>
11 #include <vector>
12@@ -60,7 +61,7 @@ public:
13 class Blocks
14 {
15 private:
16-    std::vector<CBlockIndex*> blocks;
17+    std::vector<std::unique_ptr<CBlockIndex>> blocks;
18     const uint32_t m_start_time;
19     const int32_t m_signal;
20     const int32_t m_no_signal;
21@@ -68,19 +69,11 @@ private:
22 public:
23     Blocks(uint32_t start_time, int32_t signal, int32_t no_signal) : m_start_time{start_time}, m_signal{signal}, m_no_signal{no_signal} { }
24 
25-    ~Blocks()
26-    {
27-        for (auto& v : blocks) {
28-            delete v;
29-        }
30-        blocks.clear();
31-    }
32-
33     size_t size() const { return blocks.size(); }
34 
35     CBlockIndex* tip() const
36     {
37-        return blocks.empty() ? nullptr : blocks.back();
38+        return blocks.empty() ? nullptr : blocks.back().get();
39     }
40 
41     CBlockIndex* mine_block(bool signal)
42@@ -90,14 +83,14 @@ public:
43         header.nTime = m_start_time + blocks.size() * 600;
44         header.nBits = 0x1d00ffff;
45 
46-        CBlockIndex* current_block = new CBlockIndex{header};
47+        auto current_block = std::make_unique<CBlockIndex>(header);
48         current_block->pprev = tip();
49         current_block->nHeight = blocks.size();
50         current_block->BuildSkip();
51 
52-        blocks.push_back(current_block);
53+        blocks.push_back(std::move(current_block));
54 
55-        return current_block;
56+        return tip();
57     }
58 };

ajtowns commented at 11:03 pm on March 16, 2021:

Yeah, I think you’re right – was using delete prior to having made the Blocks class so there would have been more duplication then.

ajtowns commented at 1:46 am on March 17, 2021:

Done

jnewbery commented at 9:26 am on March 17, 2021:

Much better. I was going to have to look up what those funny new and delete keywords meant.

in src/test/fuzz/versionbits.cpp:63 in e333affe34 outdated

58+
59+/** Track blocks mined for test */
60+class Blocks
61+{
62+private:
63+    std::vector<CBlockIndex*> blocks;

jnewbery commented at 12:06 pm on March 16, 2021:

0    std::vector<CBlockIndex*> m_blocks;

in src/test/fuzz/versionbits.cpp:132 in e333affe34 outdated

127+    // between genesis and 2100-01-01
128+    const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1231006505, 4102444800);
129+
130+    // too many blocks at 10min each might cause uint32_t time to overflow if
131+    // block_start_time is at the end of the range above
132+    assert(n_blocks < 320000);

jnewbery commented at 12:10 pm on March 16, 2021:

Maybe move this up next to the n_blocks declaration, make those constants constexpr and make this a static_assert.

MarcoFalke commented at 12:12 pm on March 16, 2021:

static_assert (with same comment from above)

in src/test/fuzz/versionbits.cpp:128 in e333affe34 outdated

123+    // note states will change *after* these blocks because mediantime lags
124+    int start_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * (max_periods-3));
125+    int end_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(start_block, period * (max_periods-3));
126+
127+    // between genesis and 2100-01-01
128+    const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1231006505, 4102444800);

MarcoFalke commented at 12:11 pm on March 16, 2021:

would it be possible to use params.genesis.nTime instead of the copied value?

in src/test/fuzz/versionbits.cpp:125 in e333affe34 outdated

120+    const size_t n_blocks = period * max_periods;
121+
122+    // pick the timestamp to switch based on a block
123+    // note states will change *after* these blocks because mediantime lags
124+    int start_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, period * (max_periods-3));
125+    int end_block = fuzzed_data_provider.ConsumeIntegralInRange<int>(start_block, period * (max_periods-3));

MarcoFalke commented at 12:11 pm on March 16, 2021:

Mind doing a clang-format, so that all files in src/test/fuzz are “clean”?

in src/test/fuzz/versionbits.cpp:146 in e333affe34 outdated

141+    bool always_active_test = false;
142+    bool never_active_test = false;
143+    int64_t start_time;
144+    int64_t timeout;
145+    if (fuzzed_data_provider.ConsumeBool()) {
146+        start_time = block_start_time + start_block*600;

MarcoFalke commented at 12:13 pm on March 16, 2021:

would it be possible to use params.consensus.nPowTargetSpacing instead of the hardcoded value?

in src/test/fuzz/versionbits.cpp:198 in e333affe34 outdated

193+
194+    // establish the mask
195+    uint32_t signalling_mask = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
196+
197+    // mine prior periods
198+    while (fuzzed_data_provider.remaining_bytes() > 0) {

MarcoFalke commented at 12:17 pm on March 16, 2021:

Could replace with

0    while (fuzzed_data_provider.ConsumeBool()) {

or mention that the provider is used up after this call and should not be accessed again?

jnewbery commented at 1:21 pm on March 16, 2021:

Couldn’t the subsequent ConsumeBool() fail if this is the last byte in the data provider?

ajtowns commented at 2:19 pm on March 16, 2021:

Added the comment

MarcoFalke commented at 5:11 pm on March 16, 2021:

FuzzedDataProvider itself doesn’t fail if the last byte was consumed. It will simply return constant values. In the case of bool, false. So using ConsumeBool() would be identical to remaining_bytes, with the difference that the fuzz input is one more byte large (to encode the consumed bool) per iteration. Also, ConsumeBool would leave open the possibility to use the data provider afterwards without invalidating the inputs.

jnewbery commented at 9:57 am on March 17, 2021:

Ah, thanks Marco. I agree that this would be better as ConsumeBool (and then we could move the signalling_mask declaration down to where it’s used).

ajtowns commented at 3:41 am on March 19, 2021:

Tried the while (ConsumeBool()) { signal = ConsumeBool(); } mask = ConsumeIntegral(); alternative and it seems like the current approach gets more coverage more quickly (1896 cov 6382 ft after 20s and 1898 cov 7041 ft after ~3m; vs 1893 cov 6111 ft after 20s and 1895 cov 6960 ft after ~3m), so am leaving this as-is.

MarcoFalke commented at 9:09 am on March 19, 2021:

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():

0[#68347](/bitcoin-bitcoin/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:

0[#58568](/bitcoin-bitcoin/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 fa42da2d5424c0aeccfae4b49fde2bea330b63dc) a fully consumed buffer has already bitten me.

ajtowns commented at 2:25 pm on March 19, 2021:

Well, you’re getting about 150x the exec/s I am, so getting thorough coverage in 10s when it takes me 2m makes sense. I don’t follow why your “cov” figures are 650 rather than the 1900 or so I get though?

MarcoFalke commented at 5:43 pm on March 19, 2021:

“cov” and “ft” depend on the instrumentation (version of libFuzzer, sanitizers, run time flags, …), they are not considered a “stable interface”.

Even if it took 2 minutes to saturate line coverage, I’d still consider it more than acceptable. We have many targets that are ground with hundreds of CPU hours and still aren’t close to being saturated in line coverage.

MarcoFalke commented at 12:21 pm on March 16, 2021: member

Concept ACK, left some low-effort drive-by review. Will review properly 🔜™️©️

This fuzz test is based on times, so I presume it will change again once activation is changed to be based on block heights?

in src/test/fuzz/versionbits.cpp:206 in e333affe34 outdated

201+        for (int b = 0; b < period; ++b) {
202+            blocks.mine_block(signal);
203+        }
204+
205+        // don't go too crazy with how many blocks we mine
206+        if (blocks.size() > 2*n_blocks) break;

jnewbery commented at 1:02 pm on March 16, 2021:

0        if (blocks.size() > 2 * n_blocks) break;

in src/test/fuzz/versionbits.cpp:120 in e333affe34 outdated

115+    const int threshold = 29;
116+
117+    assert(0 < threshold && threshold <= period - 2);  // must be able to not signal!
118+
119+    const size_t max_periods = 16;
120+    const size_t n_blocks = period * max_periods;

jnewbery commented at 1:03 pm on March 16, 2021:

s/n_blocks/max_blocks/

in src/test/fuzz/versionbits.cpp:195 in e333affe34 outdated

190+     * we run out of fuzz data to work out how many prior periods
191+     * there are and which ones will signal.
192+     */
193+
194+    // establish the mask
195+    uint32_t signalling_mask = fuzzed_data_provider.ConsumeIntegral<uint32_t>();

jnewbery commented at 1:08 pm on March 16, 2021:

Move this declaration down to where it’s used first.

ajtowns commented at 1:35 pm on March 16, 2021:

I’m deliberately getting the mask beforehand so that adding bytes at the end of the fuzz data just adds blocks and doesn’t change what gets interpreted as a mask and what gets interpreted as a bool

jnewbery commented at 3:26 pm on March 16, 2021:

ah! Makes sense. Thanks.

in src/test/fuzz/versionbits.cpp:17 in e333affe34 outdated

12+#include <test/fuzz/fuzz.h>
13+#include <test/fuzz/util.h>
14+
15+#include <cstdint>
16+#include <optional>
17+#include <string>

jnewbery commented at 1:19 pm on March 16, 2021:

Neither of these are used.

jnewbery commented at 1:20 pm on March 16, 2021: member

Looks good. A few style comments inline.

ajtowns force-pushed on Mar 16, 2021

ajtowns commented at 2:23 pm on March 16, 2021: member

Bunches of nits addressed.

This fuzz test is based on times, so I presume it will change again once activation is changed to be based on block heights?

My theory is having a fuzz test should make it easier to be confident a switch to heights isn’t introducing new edge cases with bogus behaviour.

amitiuttarwar commented at 8:25 pm on March 16, 2021: contributor

concept ACK

ajtowns force-pushed on Mar 17, 2021

jnewbery commented at 10:04 am on March 17, 2021: member

Code review ACK cf81c6533fe26a64bb8c5a5baf5be21367f7eee5

in src/test/fuzz/versionbits.cpp:42 in cf81c6533f outdated

37+        assert(m_period > 0);
38+        assert(0 <= m_threshold && m_threshold <= m_period);
39+        assert(0 <= m_bit && m_bit <= 32 && m_bit < VERSIONBITS_NUM_BITS);
40+    }
41+
42+    virtual bool Condition(const CBlockIndex* pindex, const Consensus::Params& params) const override { return Condition(pindex->nVersion); }

sipa commented at 0:25 am on March 19, 2021:

Nit: you don’t technically need virtual if you’re already specifying override.

ajtowns commented at 5:06 am on March 19, 2021:

And most of the codebase doesn’t specify virtual with override, so changed to do likewise

in src/test/fuzz/versionbits.cpp:153 in cf81c6533f outdated

148+
149+        assert(start_time <= timeout);
150+
151+        // allow for times to not exactly match a block
152+        if (fuzzed_data_provider.ConsumeBool()) start_time += interval / 2;
153+        if (fuzzed_data_provider.ConsumeBool()) timeout += interval / 2;

ajtowns commented at 0:36 am on March 19, 2021:

@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.

in src/test/fuzz/versionbits.cpp:208 in cf81c6533f outdated

203+        }
204+
205+        // don't go too crazy with how many blocks we mine
206+        if (blocks.size() > 2 * max_blocks) break;
207+    }
208+    // NOTE: fuzzed_data_provider is fully consumed at this point and should not be used further

sipa commented at 1:05 am on March 19, 2021:

Nitnit: s/is fully consumed/may be fully consumed/

ajtowns commented at 5:06 am on March 19, 2021:

Donedone(done…)

ajtowns force-pushed on Mar 19, 2021

ajtowns commented at 1:19 am on March 19, 2021: member

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

in src/test/fuzz/versionbits.cpp:115 in afa4e12c57 outdated

110+    assert(interval > 1); // need to be able to halve it
111+    assert(interval < std::numeric_limits<int32_t>::max());
112+
113+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
114+
115+    // these could be changed to be fuzzed inputs if desired

sipa commented at 1:25 am on March 19, 2021:

Any reason to not do that already (at least for the threshold)? It seems like an easy way of increasing potential edge cases.

ajtowns commented at 5:07 am on March 19, 2021:

No, no reason not to. Doing it with the period slows things down unfortunately.

sipa approved

sipa commented at 1:55 am on March 19, 2021: member

utACK afa4e12c57e8cdeec29503e26738863814aede3a

tests: Add fuzzing harness for versionbits 1639c3b76c

ajtowns force-pushed on Mar 19, 2021

sipa commented at 5:17 am on March 19, 2021: member

utACK 1639c3b76c3f2b74606f62ecd3ca725154e27f1b

in src/test/fuzz/versionbits.cpp:32 in 1639c3b76c

27+public:
28+    const int64_t m_begin = 0;
29+    const int64_t m_end = 0;
30+    const int m_period = 0;
31+    const int m_threshold = 0;
32+    const int m_bit = 0;

MarcoFalke commented at 8:09 am on March 19, 2021:

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

0    const int m_bit;

in src/test/fuzz/versionbits.cpp:206 in 1639c3b76c

201+        for (int b = 0; b < period; ++b) {
202+            blocks.mine_block(signal);
203+        }
204+
205+        // don't risk exceeding max_blocks or times may wrap around
206+        if (blocks.size() + period*2 > max_blocks) break;

MarcoFalke commented at 8:09 am on March 19, 2021:

clang-format ;)

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

in src/test/fuzz/versionbits.cpp:99 in 1639c3b76c

94+    }
95+};
96+
97+void initialize()
98+{
99+    SelectParams(CBaseChainParams::MAIN);

MarcoFalke commented at 8:41 am on March 19, 2021:

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

 0-
 1-void initialize()
 2-{
 3-    SelectParams(CBaseChainParams::MAIN);
 4-}
 5 } // namespace
 6 
 7 constexpr uint32_t MAX_TIME = 4102444800; // 2100-01-01
 8 
 9-FUZZ_TARGET_INIT(versionbits, initialize)
10+FUZZ_TARGET(versionbits)
11 {
12-    const CChainParams& params = Params();
13+    const auto chainparams = CreateChainParams(ArgsManager{} , CBaseChainParams::MAIN);
14+    const CChainParams& params = *chainparams;

in src/test/fuzz/versionbits.cpp:101 in 1639c3b76c

 96+
 97+void initialize()
 98+{
 99+    SelectParams(CBaseChainParams::MAIN);
100+}
101+} // namespace

MarcoFalke commented at 8:43 am on March 19, 2021:

nit: Namespace can cover the whole file

in src/test/fuzz/versionbits.cpp:103 in 1639c3b76c

 98+{
 99+    SelectParams(CBaseChainParams::MAIN);
100+}
101+} // namespace
102+
103+constexpr uint32_t MAX_TIME = 4102444800; // 2100-01-01

jnewbery commented at 9:00 am on March 19, 2021:

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

MarcoFalke commented at 9:11 am on March 19, 2021: member

cr ACK 1639c3b76c3f2b74606f62ecd3ca725154e27f1b

in src/test/fuzz/versionbits.cpp:118 in 1639c3b76c

113+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
114+
115+    // making period/max_periods larger slows these tests down significantly
116+    const int period = 32;
117+    const size_t max_periods = 16;
118+    const size_t max_blocks = 2 * period * max_periods;

jnewbery commented at 9:18 am on March 19, 2021:

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?

ajtowns commented at 10:12 am on March 19, 2021:

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.

jnewbery commented at 11:43 am on March 19, 2021:

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.

in src/test/fuzz/versionbits.cpp:221 in 1639c3b76c

216+    CBlockIndex* prev = blocks.tip();
217+    const int exp_since = checker.GetStateSinceHeightFor(prev);
218+    const ThresholdState exp_state = checker.GetStateFor(prev);
219+    BIP9Stats last_stats = checker.GetStateStatisticsFor(prev);
220+
221+    int prev_next_height = (prev == nullptr ? 0 : prev->nHeight + 1);

MarcoFalke commented at 9:47 am on March 19, 2021:

style-nit: Can be const. Also any downside to assert the genesis block is always DEFINED (unless the non-BIP9 compliant special cases never active/always active)

0-    int prev_next_height = (prev == nullptr ? 0 : prev->nHeight + 1);
1+    const int prev_next_height = (prev == nullptr ? 0 : prev->nHeight + 1);
2     assert(exp_since <= prev_next_height);
3 
4+    if (prev == nullptr && !always_active_test && !never_active_test) {
5+        assert(exp_state == ThresholdState::DEFINED);
6+    }
7+

ajtowns commented at 10:15 am on March 19, 2021:

never_active_test also starts in DEFINED with bip9 (though it wouldn’t wth bip8)

If prev == nullptr then prev_net_height == 0 and therefore exp_since <= 0 (from the above code) and at the end there’s assert(exp_since > 0 || exp_state == ThresholdState::DEFINED) so this case should be already covered.

MarcoFalke commented at 10:18 am on March 19, 2021:

Ah, thanks.

in src/test/fuzz/versionbits.cpp:84 in 1639c3b76c

79+    }
80+
81+    CBlockIndex* mine_block(bool signal)
82+    {
83+        CBlockHeader header;
84+        header.nVersion = signal ? m_signal : m_no_signal;

MarcoFalke commented at 10:22 am on March 19, 2021:

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:

 0diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
 1index a898e2782d..1724bde3dc 100644
 2--- a/src/test/fuzz/versionbits.cpp
 3+++ b/src/test/fuzz/versionbits.cpp
 4@@ -64,11 +64,11 @@ private:
 5     std::vector<std::unique_ptr<CBlockIndex>> m_blocks;
 6     const uint32_t m_start_time;
 7     const uint32_t m_interval;
 8-    const int32_t m_signal;
 9-    const int32_t m_no_signal;
10+    const std::vector<int32_t> m_signal;
11+    const std::vector<int32_t> m_no_signal;
12 
13 public:
14-    Blocks(uint32_t start_time, uint32_t interval, int32_t signal, int32_t no_signal)
15+    Blocks(uint32_t start_time, uint32_t interval, std::vector<int32_t> signal, std::vector<int32_t> no_signal)
16         : m_start_time{start_time}, m_interval{interval}, m_signal{signal}, m_no_signal{no_signal} {}
17 
18     size_t size() const { return m_blocks.size(); }
19@@ -81,7 +81,7 @@ public:
20     CBlockIndex* mine_block(bool signal)
21     {
22         CBlockHeader header;
23-        header.nVersion = signal ? m_signal : m_no_signal;
24+        header.nVersion = signal ? m_signal.at(m_blocks.size() % m_signal.size()) : m_no_signal.at(m_blocks.size() % m_no_signal.size());
25         header.nTime = m_start_time + m_blocks.size() * m_interval;
26         header.nBits = 0x1d00ffff;
27 
28@@ -126,10 +126,6 @@ FUZZ_TARGET_INIT(versionbits, initialize)
29 
30     const int64_t block_start_time = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(params.GenesisBlock().nTime, MAX_TIME);
31 
32-    // what values for version will we use to signal / not signal?
33-    const int32_t ver_signal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
34-    const int32_t ver_nosignal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
35-
36     // select deployment parameters: bit, start time, timeout
37     const int bit = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, VERSIONBITS_NUM_BITS - 1);
38 
39@@ -168,16 +164,25 @@ FUZZ_TARGET_INIT(versionbits, initialize)
40 
41     TestConditionChecker checker(start_time, timeout, period, threshold, bit);
42 
43-    // Early exit if the versions don't signal sensibly for the deployment
44-    if (!checker.Condition(ver_signal)) return;
45-    if (checker.Condition(ver_nosignal)) return;
46-    if (ver_nosignal < 0) return;
47-
48-    // TOP_BITS should ensure version will be positive
49-    assert(ver_signal > 0);
50+    // what values for version will we use to signal / not signal?
51+    std::vector<int32_t> vver_signal;
52+    std::vector<int32_t> vver_nosignal;
53+    while (vver_signal.size() < 5) {
54+        const auto ver_signal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
55+        if (!checker.Condition(ver_signal)) return;
56+        // TOP_BITS should ensure version will be positive
57+        assert(ver_signal > 0);
58+        vver_signal.push_back(ver_signal);
59+    }
60+    while (vver_nosignal.size() < 5) {
61+        const auto ver_nosignal = fuzzed_data_provider.ConsumeIntegral<int32_t>();
62+        if (ver_nosignal < 0) return;
63+        if (checker.Condition(ver_nosignal)) return;
64+        vver_nosignal.push_back(ver_nosignal);
65+    }
66 
67     // Now that we have chosen time and versions, setup to mine blocks
68-    Blocks blocks(block_start_time, interval, ver_signal, ver_nosignal);
69+    Blocks blocks(block_start_time, interval, vver_signal, vver_nosignal);
70 
71     /* Strategy:
72      *  * we will mine a final period worth of blocks, with

ajtowns commented at 11:16 am on March 19, 2021:

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.

MarcoFalke commented at 11:42 am on March 19, 2021:

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.

in src/test/fuzz/versionbits.cpp:178 in 1639c3b76c

173+    if (checker.Condition(ver_nosignal)) return;
174+    if (ver_nosignal < 0) return;
175+
176+    // TOP_BITS should ensure version will be positive
177+    assert(ver_signal > 0);
178+

MarcoFalke commented at 10:31 am on March 19, 2021:

nit: Could assert(ver_signal > VERSIONBITS_LAST_OLD_BLOCK_VERSION);?

jnewbery commented at 11:44 am on March 19, 2021: member

utACK 1639c3b76c

MarcoFalke commented at 11:56 am on March 19, 2021: member

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

ajtowns commented at 2:26 pm on March 19, 2021: member

@MarcoFalke merge and followup sounds good to me

MarcoFalke merged this on Mar 19, 2021

MarcoFalke closed this on Mar 19, 2021

in src/test/fuzz/versionbits.cpp:54 in 1639c3b76c

49+    int GetStateSinceHeightFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateSinceHeightFor(pindexPrev, dummy_params, m_cache); }
50+    BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindexPrev) const { return AbstractThresholdConditionChecker::GetStateStatisticsFor(pindexPrev, dummy_params); }
51+
52+    bool Condition(int64_t version) const
53+    {
54+        return ((version >> m_bit) & 1) != 0 && (version & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;

amitiuttarwar commented at 8:13 pm on March 19, 2021:

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?

ajtowns commented at 0:51 am on March 21, 2021:

~~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.

fanquake removed this from the "Blockers" column in a project

ajtowns commented at 1:31 am on March 21, 2021: member

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

ajtowns referenced this in commit d4946505b4 on Mar 21, 2021

ajtowns referenced this in commit e775b0a6dd on Mar 21, 2021

MarcoFalke referenced this in commit 1bad33f952 on Mar 21, 2021

fanquake referenced this in commit f95071a3f5 on Mar 24, 2021

fanquake referenced this in commit 2cd834e6c0 on Apr 15, 2021

DrahtBot locked this on Aug 16, 2022

tests: Add fuzzing harness for versionbits #21380

Conflicts