fuzz: Add tx_pool fuzz target #21142

MarcoFalke commented at 5:55 PM on February 10, 2021: member

No description provided.

MarcoFalke force-pushed on Feb 10, 2021

DrahtBot added the label Build system on Feb 10, 2021

DrahtBot added the label P2P on Feb 10, 2021

DrahtBot added the label RPC/REST/ZMQ on Feb 10, 2021

DrahtBot added the label Validation on Feb 10, 2021

DrahtBot commented at 7:09 PM on February 10, 2021: member

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

Conflicts

No conflicts as of last run.

practicalswift commented at 9:06 PM on February 10, 2021: contributor

Concept ACK: very nice to see the mempool logic more thoroughly fuzzed!

MarcoFalke force-pushed on Feb 11, 2021

MarcoFalke removed the label Build system on Feb 11, 2021

MarcoFalke removed the label P2P on Feb 11, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Feb 11, 2021

MarcoFalke removed the label Validation on Feb 11, 2021

MarcoFalke marked this as ready for review on Feb 11, 2021

MarcoFalke force-pushed on Feb 11, 2021

MarcoFalke commented at 1:51 PM on February 11, 2021: member

Rebased

DrahtBot added the label Build system on Feb 11, 2021

MarcoFalke removed the label Build system on Feb 12, 2021

MarcoFalke added the label Tests on Feb 12, 2021

MarcoFalke force-pushed on Feb 19, 2021

practicalswift commented at 8:13 PM on February 28, 2021: contributor

Tested ACK e4e253d73007e0b680d2a473327c6fd66de4d86c

Very nice to see the mempool logic more thoroughly fuzzed!
Achieves good coverage quickly.
Touches only src/test/fuzz/.

MarcoFalke added the label 1 ACK on Mar 10, 2021

MarcoFalke added the label Review club on Mar 10, 2021

MarcoFalke removed the label 1 ACK on Mar 11, 2021

MarcoFalke commented at 8:21 AM on March 12, 2021: member

https://bitcoincore.reviews/21142

DrahtBot added the label Needs rebase on Mar 15, 2021

MarcoFalke force-pushed on Mar 15, 2021

DrahtBot removed the label Needs rebase on Mar 15, 2021

in src/test/fuzz/tx_pool.cpp:178 in faed1f78f1 outdated

 170 | +            Assert(added.size() == 1); // For now, no package acceptance
 171 | +            Assert(tx == *added.begin());
 172 | +        }
 173 | +
 174 | +        // Do not consider rejected transaction removed
 175 | +        removed.erase(tx);

glozow commented at 12:20 AM on March 17, 2021:

I was wondering if RBF'ed transactions would be in removed, but then realized it wouldn't hit RBF stuff since you only use inputs from outpoints. Maybe add Assert(res.m_replaced_transactions.size() == 0)?

MarcoFalke commented at 8:33 PM on March 17, 2021:

Excellent suggestion to add rbf. Done

in src/test/fuzz/tx_pool.cpp:198 in faed1f78f1 outdated

 190 | +                insert_tx(/* tx_outs */ pop, /* tx_ins */ outpoints, /* tx */ r);
 191 | +            }
 192 | +            for (const auto& a : added) {
 193 | +                insert_tx(/* tx_outs */ outpoints, /* tx_ins */ pop, /* tx */ a);
 194 | +            }
 195 | +            for (const auto& p : pop) {

glozow commented at 12:35 AM on March 17, 2021:

Naming suggestions for clarity

        const auto insert_tx = [](auto& created_in_tx, auto& spent_by_tx, const auto& tx) {
            for (size_t i{0}; i < tx->vout.size(); ++i) {
                Assert(created_in_tx.emplace(tx->GetHash(), i).second);
            }
            for (const auto& in : tx->vin) {
                Assert(spent_by_tx.insert(in.prevout).second);
            }
        };
        // Add created outpoints, remove spent outpoints
        {
            std::set<COutPoint> spent_outpoints;
            for (const auto& removed_tx : removed) {
                insert_tx(/* created_in_tx */ spent, /* spent_by_tx */ outpoints, /* tx */ removed_tx);
            }
            for (const auto& added_tx : added) {
                insert_tx(/* created_in_tx */ outpoints, /* spent_by_tx */ spent, /* tx */ added_tx);
            }
            for (const auto& p : spent) {

MarcoFalke commented at 2:50 PM on March 17, 2021:

Thanks, renamed pop to spent. tx_outs to created_by_tx. tx_ins to spent_by_tx.

in src/test/fuzz/tx_pool.cpp:115 in faed1f78f1 outdated

 110 | +        const CTransactionRef tx = [&] {
 111 | +            CMutableTransaction tx_mut;
 112 | +            tx_mut.nVersion = CTransaction::CURRENT_VERSION;
 113 | +            tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint32_t>();
 114 | +            const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, outpoints.size());
 115 | +            const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, outpoints.size());

glozow commented at 12:52 AM on March 17, 2021:

Why does num_out need to be in the range [1, outpoints.size()]?

MarcoFalke commented at 2:50 PM on March 17, 2021:

Thanks. Limited to 2*outputs.size

in src/test/fuzz/tx_pool.cpp:140 in faed1f78f1 outdated

 135 | +                in.scriptSig = script_sig;
 136 | +                in.scriptWitness.stack = script_wit_stack;
 137 | +
 138 | +                tx_mut.vin.push_back(in);
 139 | +            }
 140 | +            const CAmount max_amount_out{amount_in / num_out};

glozow commented at 12:53 AM on March 17, 2021:

Why not have an amount_left and pull amounts from ConsumeIntegralInRange<CAmount>(0, amount_left)?

MarcoFalke commented at 2:51 PM on March 17, 2021:

Made it to pick the fee instead of the output amounts. They are now all constant for each tx

in src/test/fuzz/tx_pool.cpp:105 in faed1f78f1 outdated

 100 | +            node.args->ForceSetArg("-limitdescendantsize",
 101 | +                                   ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200)));
 102 | +            node.args->ForceSetArg("-maxmempool",
 103 | +                                   ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200)));
 104 | +            node.args->ForceSetArg("-mempoolexpiry",
 105 | +                                   ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200)));

glozow commented at 1:10 AM on March 17, 2021:

Question: I know it's a test, but does it make sense for any of these values to be 0? 😮 I'm also wondering why 200 is the max instead of something closer to the default values?

MarcoFalke commented at 2:54 PM on March 17, 2021:

I want to set it small enough so that the fuzz engine can quickly hit the limit without having to produce megabytes of transactions first.
Changed all maxima to be multiples of the default values, except for maxmempool, which is in [0,200]

glozow commented at 1:13 AM on March 17, 2021: member

Concept ACK, very excited to see fuzzing in ATMP :) I have a lot of questions.

Got it running and have reviewed tx_pool.cpp so far. My biggest suggestion is to try to hit the RBF code by keeping track of outpoints in the mempool and potentially pulling input(s) from there as well.

I'm a fuzzing noob, ran into a sanitizer error on my mac so I ran it in docker instead

<details><summary>(Commands if anyone's interested)</summary><p>

docker run -it ubuntu:bionic /bin/bash
apt update
apt install -y git
apt install -y sudo
git clone https://github.com/bitcoin/bitcoin
cd bitcoin
git remote add marco https://github.com/MarcoFalke/bitcoin-core.git
git fetch marco 2102-fuzzPool
git checkout 2102-fuzzPool
sudo apt-get install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3
sudo apt-get install libevent-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev
sudo apt-get install clang
./autogen.sh
CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined --without-gui --disable-wallet
make
FUZZ=tx_pool src/test/fuzz/fuzz

</p> </details>

MarcoFalke force-pushed on Mar 17, 2021

in src/test/fuzz/tx_pool.cpp:26 in bcf96cdf4b outdated

  20 | +void initialize_tx_pool()
  21 | +{
  22 | +    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
  23 | +    g_setup = testing_setup.get();
  24 | +
  25 | +    for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {

jonatack commented at 3:35 PM on March 17, 2021:

    for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {

in src/test/fuzz/tx_pool.cpp:165 in bcf96cdf4b outdated

 151 | +                Assert(outpoints.insert(in.prevout).second);
 152 | +            }
 153 | +            return tx;
 154 | +        }();
 155 | +
 156 | +        // Remember all removed and added transaction

jonatack commented at 3:44 PM on March 17, 2021:

        // Remember all removed and added transactions

MarcoFalke commented at 1:10 PM on March 18, 2021:

thx, done

in src/test/fuzz/tx_pool.cpp:274 in bcf96cdf4b outdated

 218 | +        const auto mut_tx = ConsumeTransaction(fuzzed_data_provider, txids);
 219 | +
 220 | +        const auto tx = MakeTransactionRef(mut_tx);
 221 | +        const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
 222 | +        const bool require_standard = fuzzed_data_provider.ConsumeBool();
 223 | +        ::fRequireStandard = require_standard;

jonatack commented at 3:55 PM on March 17, 2021:

maybe simplify to

@@ -159,8 +159,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
         const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
-        const bool require_standard = fuzzed_data_provider.ConsumeBool();
-        ::fRequireStandard = require_standard;
+        ::fRequireStandard = fuzzed_data_provider.ConsumeBool();
         const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(node.chainman->ActiveChainstate(), tx_pool, tx, bypass_limits));

@@ -219,8 +218,7 @@ FUZZ_TARGET_INIT(tx_pool, initialize_tx_pool)
         const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
-        const bool require_standard = fuzzed_data_provider.ConsumeBool();
-        ::fRequireStandard = require_standard;
+        ::fRequireStandard = fuzzed_data_provider.ConsumeBool();
         const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(node.chainman->ActiveChainstate(), tx_pool, tx, bypass_limits));

MarcoFalke commented at 1:10 PM on March 18, 2021:

done

jonatack commented at 4:17 PM on March 17, 2021: member

Light ACK bcf96cdf4b3df868050b698713dafbd93bcd7add

Good to see added and improved coverage and efficiency. AFAICT after the first minute, the tx_pool fuzzer is doing roughly 2-3 times the exec/s of the process_message_tx and the tx_pool_standard fuzzers.

MarcoFalke force-pushed on Mar 17, 2021

MarcoFalke commented at 8:07 PM on March 17, 2021: member

Only for tx_pool_standard: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/fa5382241f7943a7/fuzz.coverage/index.html

MarcoFalke commented at 8:34 PM on March 17, 2021: member

I pushed an update for RBF. Will push some more stuff tomorrow :sleeping:

ghost commented at 6:01 AM on March 18, 2021: none

ACK fa5382241f7943a7395f60a8916e0d71c10d2932

Reviewed the code and ran the fuzz coverage tests before & after running for a few hours. Definitely getting better coverage with this, especially with the mempool/validation files.

<details> <summary>Before</summary>


              Hit     Total  Coverage      
Lines:       28292    43939   64.4 %
Functions:    7835    11651   67.2 %
Branches:   101626   333283   30.5 %


                             
Filename                     Line Coverage             Functions               Branches
....
txmempool.cpp             35.7 %    262 / 734      50.8 %     31 / 61     15.8 %   782 / 4940
txmempool.h               61.9 %     99 / 160      60.0 %     42 / 70     30.2 %   189 / 626
txorphanage.cpp           61.7 %     74 / 120      66.7 %      6 / 9      25.0 %   205 / 820
txorphanage.h            100.0 %      4 / 4       100.0 %      5 / 5      50.0 %     4 / 8
txrequest.cpp             99.3 %    292 / 294     100.0 %     78 / 78     53.2 %  1330 / 2500
uint256.cpp              100.0 %     32 / 32       60.0 %      6 / 10     32.3 %    62 / 192
uint256.h                100.0 %     54 / 54       92.0 %     46 / 50     41.0 %    77 / 188
undo.h                   100.0 %     22 / 22       59.6 %     28 / 47     30.7 %    54 / 176
validation.cpp            45.4 %   1338 / 2945     59.9 %    115 / 192    20.1 %  3992 / 19900
validation.h              52.0 %     26 / 50       53.8 %     14 / 26     21.4 %    39 / 182
validationinterface.cpp   46.8 %     58 / 124      39.8 %     35 / 88      8.8 %   178 / 2016
validationinterface.h     20.0 %      2 / 10       20.0 %      2 / 10          -     0 / 0
...

</details>

<details> <summary>After</summary>

              Hit     Total  Coverage
Lines:       28933    43936   65.9 %
Functions:    7934    11652   68.1 %
Branches:   104236   333473   31.3 %




Filename                     Line Coverage             Functions               Branches
....
txmempool.cpp             54.9 %    403 / 734      67.2 %     41 / 61     25.6 %  1267 / 4940
txmempool.h               66.2 %    106 / 160      65.7 %     46 / 70     31.6 %   198 / 626
txorphanage.cpp           61.7 %     74 / 120      66.7 %      6 / 9      25.0 %   205 / 820
txorphanage.h            100.0 %      4 / 4       100.0 %      5 / 5      50.0 %     4 / 8
txrequest.cpp             99.3 %    292 / 294     100.0 %     78 / 78     53.2 %  1330 / 2500
uint256.cpp              100.0 %     32 / 32       60.0 %      6 / 10     32.3 %    62 / 192
uint256.h                100.0 %     54 / 54       92.0 %     46 / 50     41.0 %    77 / 188
undo.h                   100.0 %     22 / 22       59.6 %     28 / 47     30.7 %    54 / 176
validation.cpp            53.9 %   1586 / 2945     65.1 %    125 / 192    25.3 %  5044 / 19900
validation.h              66.0 %     33 / 50       69.2 %     18 / 26     27.5 %    50 / 182
validationinterface.cpp   81.5 %    101 / 124      71.6 %     63 / 88     19.9 %   402 / 2016
validationinterface.h     30.0 %      3 / 10       30.0 %      3 / 10          -     0 / 0
...

</details>

MarcoFalke force-pushed on Mar 18, 2021

MarcoFalke commented at 11:23 AM on March 18, 2021: member

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

MarcoFalke force-pushed on Mar 18, 2021

jonatack commented at 3:02 PM on March 18, 2021: member

@MarcoFalke as you requested in the review club yesterday, I ran the tx_pool fuzzer since then for 20+ hours and didn't see an accepted transaction.

         if (accepted) {
             txids.push_back(tx->GetHash());
+            std::cout << "\n\n\n*********************** SUCCESS ***************************\n\n\n";
         }

[#7104574](/bitcoin-bitcoin/7104574/)	NEW    cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 393483/902446 MS: 3 EraseBytes-ChangeByte-CMP- DE: "\x01\x00\x00\x00\x10\x8b\x12\xb3"-
[#7104591](/bitcoin-bitcoin/7104591/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 70/902446 MS: 1 EraseBytes-
[#7104827](/bitcoin-bitcoin/7104827/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 451/902446 MS: 1 EraseBytes-
[#7104855](/bitcoin-bitcoin/7104855/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 118/902446 MS: 3 CMP-CMP-EraseBytes- DE: "\xff\xff\xff\xff\xff\xff\xff\xff"-"\x00\x00\x00\x00\x00\x00\x00\x01"-
[#7105353](/bitcoin-bitcoin/7105353/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 15731/902446 MS: 2 InsertRepeatedBytes-EraseBytes-
[#7107174](/bitcoin-bitcoin/7107174/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 111/902446 MS: 1 EraseBytes-
[#7108811](/bitcoin-bitcoin/7108811/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 160/902446 MS: 1 EraseBytes-
[#7109018](/bitcoin-bitcoin/7109018/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 5278/902446 MS: 2 InsertByte-EraseBytes-
[#7109675](/bitcoin-bitcoin/7109675/)	REDUCE cov: 10002 ft: 58007 corp: 2495/59Mb lim: 907880 exec/s: 93 rss: 1733Mb L: 42/902446 MS: 2 ChangeBinInt-EraseBytes-

fuzz: Add tx_pool fuzz targets faa9ef49d1

MarcoFalke force-pushed on Mar 18, 2021

MarcoFalke commented at 5:57 PM on March 18, 2021: member

I quickly ran 15kk iterations (double the 7kk iterations of yours) and it did hit a valid tx.

[#15766688](/bitcoin-bitcoin/15766688/)	REDUCE cov: 3250 ft: 28506 corp: 8582/1579Kb lim: 4096 exec/s: 2341 rss: 98Mb L: 49/4096 MS: 1 EraseBytes-

However, I pushed a patch, where you should see a valid tx within ~100k iterations.

All feedback has been addressed, this is now ready for re-ACKs.

jonatack commented at 6:59 PM on March 18, 2021: member

Valid tx within a few seconds with latest push.

[#35708](/bitcoin-bitcoin/35708/)	NEW    cov: 5635 ft: 14652 corp: 159/22Kb lim: 163 exec/s: 2975 rss: 587Mb L: 163/163 MS: 3 ShuffleBytes-InsertRepeatedBytes-CrossOver-
[#36093](/bitcoin-bitcoin/36093/)	REDUCE cov: 5635 ft: 14652 corp: 159/22Kb lim: 163 exec/s: 3007 rss: 587Mb L: 136/163 MS: 5 ChangeBinInt-CrossOver-PersAutoDict-ChangeByte-InsertRepeatedBytes- DE: "\x15\x00\x00\x00\x00\x00\x00\x00"-

*********************** SUCCESS ***************************

	NEW_FUNC[1/335]: 0x559b1920a260 in std::vector<int, std::allocator<int> >::~vector() /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:679
	NEW_FUNC[2/335]: 0x559b1920a870 in std::_Vector_base<int, std::allocator<int> >::_Vector_impl::_Vector_impl() /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:134

Edit: and many valid txs.

ghost commented at 2:40 AM on March 19, 2021: none

reACK faa9ef49d18da9223220afcc263ac91a74c291a6

Getting a better overall coverage profile (though I have been running for a lot longer and probably have better seeds by now)

Summary coverage rate:                                              
  lines......: 66.4% (29127 of 43873 lines)                         
  functions..: 68.3% (7960 of 11646 functions)                      
  branches...: 31.5% (104949 of 333689 branches)

Plus got 378 accepted transactions for the tx_pool target between try #65536 & #131072.

practicalswift commented at 11:07 PM on March 20, 2021: contributor

Tested ACK faa9ef49d18da9223220afcc263ac91a74c291a6

Very nice fuzzing harness! Thanks @MarcoFalke!

in src/test/fuzz/tx_pool.cpp:99 in faa9ef49d1

  94 | +
  95 | +    // The sum of the values of all spendable outpoints
  96 | +    constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN};
  97 | +
  98 | +    CTxMemPool tx_pool_{/* estimator */ nullptr, /* check_ratio */ 1};
  99 | +    MockedTxPool& tx_pool = *(MockedTxPool*)&tx_pool_;

glozow commented at 10:50 PM on March 22, 2021:

Question: Why can't you use a static_cast here instead?

MarcoFalke commented at 9:31 AM on March 23, 2021:

Thanks, done in follow-up

in src/test/fuzz/tx_pool.cpp:106 in faa9ef49d1

 101 | +    // Helper to query an amount
 102 | +    const CCoinsViewMemPool amount_view{WITH_LOCK(::cs_main, return &chainstate.CoinsTip()), tx_pool};
 103 | +    const auto GetAmount = [&](const COutPoint& outpoint) {
 104 | +        Coin c;
 105 | +        amount_view.GetCoin(outpoint, c);
 106 | +        Assert(!c.IsSpent());

glozow commented at 10:52 PM on March 22, 2021:

This can just be:

        Assert(amount_view.GetCoin(outpoint, c));

MarcoFalke commented at 9:31 AM on March 23, 2021:

Thanks, done in follow-up

jnewbery commented at 1:14 PM on March 23, 2021:

Isn't it better to not have side-effects from asserts?

in src/test/fuzz/tx_pool.cpp:257 in faa9ef49d1

 252 | +{
 253 | +    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 254 | +    const auto& node = g_setup->m_node;
 255 | +
 256 | +    std::vector<uint256> txids;
 257 | +    for (const auto& outpoint : g_outpoints_coinbase_init) {

glozow commented at 11:31 PM on March 22, 2021:

Question: is the same g_outpoints_coinbase_init used for both the tx_pool and tx_pool_standard targets?

MarcoFalke commented at 9:37 AM on March 23, 2021:

Yes

in src/test/fuzz/tx_pool.cpp:263 in faa9ef49d1

 258 | +        txids.push_back(outpoint.hash);
 259 | +        if (txids.size() >= COINBASE_MATURITY) break;
 260 | +    }
 261 | +    for (int i{0}; i <= 3; ++i) {
 262 | +        // Add some immature and non-existent outpoints
 263 | +        txids.push_back(g_outpoints_coinbase_init.at(i).hash);

glozow commented at 11:32 PM on March 22, 2021:

Aren't the ones at the beginning the mature ones? It could also be more clear to have two sets g_outpoints_coinbases_mature and g_outpoints_coinbases_immature.

        txids.push_back(g_outpoints_coinbase_init.at(COINBASE_MATURITY + i).hash);

MarcoFalke commented at 9:37 AM on March 23, 2021:

Ouch :facepalm: . Good catch.

in src/test/fuzz/tx_pool.cpp:134 in faa9ef49d1

 129 | +            CAmount amount_in{0};
 130 | +            for (int i = 0; i < num_in; ++i) {
 131 | +                // Pop random outpoint
 132 | +                auto pop = outpoints_rbf.begin();
 133 | +                std::advance(pop, fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, outpoints_rbf.size() - 1));
 134 | +                const auto outpoint = *pop;

glozow commented at 11:45 PM on March 22, 2021:

Why not use PickValue for this? It could take a bool to PickAndMaybeDelete?

MarcoFalke commented at 9:31 AM on March 23, 2021:

Wouldn't that make PickValue unsuitable for collections that can't erase elements. E.g. std::array

glozow commented at 1:56 PM on March 23, 2021:

Ahhh true

in src/test/fuzz/util.h:52 in faa9ef49d1

  47 | @@ -48,6 +48,16 @@ void CallOneOf(FuzzedDataProvider& fuzzed_data_provider, Callables... callables)
  48 |      return ((i++ == call_index ? callables() : void()), ...);
  49 |  }
  50 |  
  51 | +template <typename Collection>
  52 | +const auto& PickValue(FuzzedDataProvider& fuzzed_data_provider, const Collection& col)

glozow commented at 11:48 PM on March 22, 2021:

Why not make this a member function of FuzzedDataProvider like PickValueInArray is?

MarcoFalke commented at 9:40 AM on March 23, 2021:

FuzzedDataProvider is taken from upstream, so someone needs to submit this to upstream first.

glozow commented at 1:42 PM on March 23, 2021:

Oh, is it not src/test/fuzz/FuzzedDataProvider.h?

MarcoFalke commented at 1:45 PM on March 23, 2021:

It is, but it is only synced from upstream: https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h

in src/txmempool.h:479 in faa9ef49d1

 475 | @@ -476,7 +476,7 @@ enum class MemPoolRemovalReason {
 476 |   */
 477 |  class CTxMemPool
 478 |  {
 479 | -private:
 480 | +protected:

glozow commented at 12:06 AM on March 23, 2021:

It seems a bit invasive to change all of these mempool members... can you just call something that automatically does RollingFeeUpdate like tx_pool.GetMinFee()? Or just set the members you need to protected?

MarcoFalke commented at 9:45 AM on March 23, 2021:

tx_pool.GetMinFee exists early because of !blockSinceLastRollingFeeBump, so I can't use that. protected as a replacement for private seems fine generally, because it allows tests to mock any private member without having to mark each member individually or add a friend TestClass01 for each class that mocks the mempool. Since mempool isn't derived outside of tests, protected shouldn't come with any risks either.

glozow commented at 1:42 PM on March 23, 2021:

Sure, makes sense 👍

in src/test/fuzz/util.h:144 in faa9ef49d1

 144 | +
 145 | +[[nodiscard]] CScriptWitness ConsumeScriptWitness(FuzzedDataProvider& fuzzed_data_provider, const size_t max_stack_elem_size = 32) noexcept;
 146 | +
 147 | +[[nodiscard]] CScript ConsumeScript(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096, const bool maybe_p2wsh = false) noexcept;
 148 | +
 149 | +[[nodiscard]] uint32_t ConsumeSequence(FuzzedDataProvider& fuzzed_data_provider) noexcept;

glozow commented at 12:11 AM on March 23, 2021:

Add comments for the util functions? e.g. I think it'd be helpful to document that ConsumeTransaction will give a tx that's well-formed but not necessarily valid, and it takes prevout_txids as input but won't necessarily use a txid from there.

MarcoFalke commented at 9:47 AM on March 23, 2021:

If prevout_txids is passed, it should only pick from there. As none of the functions in this file have doxygen comments, so I'll skip them for now. Though, I am more than happy to review a pull adding docs :)

in src/test/fuzz/util.cpp:40 in faa9ef49d1

  35 | +                          CTransaction::CURRENT_VERSION :
  36 | +                          fuzzed_data_provider.ConsumeIntegral<int32_t>();
  37 | +    tx_mut.nLockTime = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
  38 | +    const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, max_num_in);
  39 | +    const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, max_num_out);
  40 | +    for (int i = 0; i < num_in; ++i) {

glozow commented at 12:20 AM on March 23, 2021:

In some places you do braced, and in some places you do = 🤷

    for (int i{0}; i < num_in; ++i) {

MarcoFalke commented at 9:39 AM on March 23, 2021:

Right, this isn't consistent. Though, I'll leave as is for now and remember to write for (int i{0}; i < ... in the future.

jnewbery commented at 1:11 PM on March 23, 2021:

Why not for (auto i{0} ...? :eyes:

MarcoFalke commented at 1:18 PM on March 23, 2021:

int is one character shorter :shorts:

in src/test/fuzz/tx_pool.cpp:88 in faa9ef49d1

  83 | +    SetMempoolConstraints(*node.args, fuzzed_data_provider);
  84 | +
  85 | +    // All RBF-spendable outpoints
  86 | +    std::set<COutPoint> outpoints_rbf;
  87 | +    // All outpoints counting toward the total supply (subset of outpoints_rbf)
  88 | +    std::set<COutPoint> outpoints_supply;

glozow commented at 12:36 AM on March 23, 2021:

These comments are confusing 😢 outpoints_rbf includes all spendable outpoints including ones that would be RBFs, but "RBF-spendable" sounds like they're only the ones in the mempool. outpoints_supply doesn't actually include all the outpoints that count towards total supply. It doesn't include the mempool inputs which is why we need to add mempool GetTotalFee to it...

MarcoFalke commented at 9:36 AM on March 23, 2021:

outpoints_rbf

Happy to rename if you have suggestions.

It doesn't include the mempool inputs which is why we need to add mempool GetTotalFee to it...

I don't understand what you mean with "mempool inputs". outpoints_supply does include all outpoints from the mempool. The reason that GetTotalFee needs to be added is that the fee in the mempool isn't assigned to an outpoint (yet). The fee is collected in the coinbase transaction in a block, which doesn't exist because this fuzz target doesn't mine any blocks with mempool txs.

glozow commented at 1:43 PM on March 23, 2021:

outpoints_supply does include all outpoints from the mempool.

A right 🤦 I was confoozed

glozow commented at 12:39 AM on March 23, 2021: member

code review ACK faa9ef49d18da9223220afcc263ac91a74c291a6, a bunch of comments but non blocking @MarcoFalke why do you call it tx_pool, is it the cool way of saying mempool? I would've called this mempool_validation.cpp and s/tx_pool/mempool

MarcoFalke merged this on Mar 23, 2021

MarcoFalke closed this on Mar 23, 2021

MarcoFalke commented at 9:59 AM on March 23, 2021: member

replied to comments

MarcoFalke deleted the branch on Mar 23, 2021

sidhujag referenced this in commit f9e1e6b586 on Mar 23, 2021

DrahtBot locked this on Aug 16, 2022