Merge bitcoin/bitcoin#29974: fuzz: txorphan tests fixups

58594c7040241f2312b0b8735a8baf0412674613 fuzz: txorphan tests fixups (Sergi Delgado Segura)

Pull request description:

  Motivated by https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576401327

  Adds the following fixups in txorphan fuzz tests:

  - Don't bond the output count of the created orphans to the number of available coins
  - Allow duplicate inputs but don't store duplicate outpoints

  Most significantly, this gets rid of the `duplicate_input` flag altogether, making the test easier to reason about. Notice how, under normal conditions, duplicate inputs would be caught by `MemPoolAccept::PreChecks`, however, no validations checks are run on the test before adding data to the orphanage (neither were they before this patch)

  ## Rationale

  The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`). If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not. This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop.

  Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.

ACKs for top commit:
  maflcko:
    utACK 58594c7040241f2312b0b8735a8baf0412674613
  glozow:
    ACK 58594c7
  instagibbs:
    ACK 58594c7040241f2312b0b8735a8baf0412674613

Tree-SHA512: e97cc2a43e388f87b64d2e4e45f929dd5b0dd85d668dd693b75e4c9ceea734cd7645952385d428208d07b70e1aafbec84cc2ec264a2e07d36fc8ba3e97885a8d

1 files changed, 10 insertions, 18 deletions

diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index a44f47b00d..037c7af77f 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -37,13 +37,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     TxOrphanage orphanage;
-    std::vector<COutPoint> outpoints;
+    std::set<COutPoint> outpoints;
+
     // initial outpoints used to construct transactions later
     for (uint8_t i = 0; i < 4; i++) {
-        outpoints.emplace_back(Txid::FromUint256(uint256{i}), 0);
+        outpoints.emplace(Txid::FromUint256(uint256{i}), 0);
     }
-    // if true, allow duplicate input when constructing tx
-    const bool duplicate_input = fuzzed_data_provider.ConsumeBool();
 
     CTransactionRef ptx_potential_parent = nullptr;
 
@@ -53,29 +52,22 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
         const CTransactionRef tx = [&] {
             CMutableTransaction tx_mut;
             const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1, outpoints.size());
-            const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1, outpoints.size());
-            // pick unique outpoints from outpoints as input
+            const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1, 256);
+            // pick outpoints from outpoints as input. We allow input duplicates on purpose, given we are not
+            // running any transaction validation logic before adding transactions to the orphanage
             for (uint32_t i = 0; i < num_in; i++) {
                 auto& prevout = PickValue(fuzzed_data_provider, outpoints);
-                tx_mut.vin.emplace_back(prevout);
-                // pop the picked outpoint if duplicate input is not allowed
-                if (!duplicate_input) {
-                    std::swap(prevout, outpoints.back());
-                    outpoints.pop_back();
-                }
+                // try making transactions unique by setting a random nSequence, but allow duplicate transactions if they happen
+                tx_mut.vin.emplace_back(prevout, CScript{}, fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(0, CTxIn::SEQUENCE_FINAL));
             }
             // output amount will not affect txorphanage
             for (uint32_t i = 0; i < num_out; i++) {
                 tx_mut.vout.emplace_back(CAmount{0}, CScript{});
             }
-            // restore previously popped outpoints
-            for (auto& in : tx_mut.vin) {
-                outpoints.push_back(in.prevout);
-            }
             auto new_tx = MakeTransactionRef(tx_mut);
-            // add newly constructed transaction to outpoints
+            // add newly constructed outpoints to the coin pool
             for (uint32_t i = 0; i < num_out; i++) {
-                outpoints.emplace_back(new_tx->GetHash(), i);
+                outpoints.emplace(new_tx->GetHash(), i);
             }
             return new_tx;
         }();