Merge bitcoin/bitcoin#28764: Fuzz: Check individual and package transaction invariants

fcb3069 Use CheckPackageMempoolAcceptResult for package evaluation fuzzing (Greg Sanders)
34088d6 [test util] CheckPackageMempoolAcceptResult for sanity-checking results (glozow)
651fa40 fuzz: tx_pool checks ATMP result invariants (Greg Sanders)

Pull request description:

  Poached from bitcoin/bitcoin#26711 since that PR is being split apart, and modified to match current behavior.

ACKs for top commit:
  glozow:
    reACK fcb3069, only whitespace changes
  dergoegge:
    ACK fcb3069

Tree-SHA512: abd687e526d8dfc8d65b3a873ece8ca35fdcbd6b0f7b93da6a723ef4e47cf85612de819e6f2b8631bdf897e1aba27cdd86f89b7bd85fc3356e74be275dcdf8cc

Author: glozow

src/test/fuzz/package_eval.cpp

@@ -257,15 +257,6 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
 
         const auto result_package = WITH_LOCK(::cs_main,
                                     return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit));
-        // If something went wrong due to a package-specific policy, it might not return a
-        // validation result for the transaction.
-        if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) {
-            auto it = result_package.m_tx_results.find(txs.back()->GetWitnessHash());
-            Assert(it != result_package.m_tx_results.end());
-            Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
-                   it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID ||
-                   it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
-        }
 
         const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(), bypass_limits, /*test_accept=*/!single_submit));
         const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
@@ -281,6 +272,12 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
                 Assert(added.size() == 1);
                 Assert(txs.back() == *added.begin());
             }
+        } else if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) {
+            // We don't know anything about the validity since transactions were randomly generated, so
+            // just use result_package.m_state here. This makes the expect_valid check meaningless, but
+            // we can still verify that the contents of m_tx_results are consistent with m_state.
+            const bool expect_valid{result_package.m_state.IsValid()};
+            Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, nullptr));
         } else {
             // This is empty if it fails early checks, or "full" if transactions are looked at deeper
             Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty());

src/test/fuzz/tx_pool.cpp

@@ -131,6 +131,53 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte
     return CTxMemPool{mempool_opts};
 }
 
+void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, bool wtxid_in_mempool)
+{
+
+    switch (res.m_result_type) {
+    case MempoolAcceptResult::ResultType::VALID:
+    {
+        Assert(txid_in_mempool);
+        Assert(wtxid_in_mempool);
+        Assert(res.m_state.IsValid());
+        Assert(!res.m_state.IsInvalid());
+        Assert(res.m_replaced_transactions);
+        Assert(res.m_vsize);
+        Assert(res.m_base_fees);
+        Assert(res.m_effective_feerate);
+        Assert(res.m_wtxids_fee_calculations);
+        Assert(!res.m_other_wtxid);
+        break;
+    }
+    case MempoolAcceptResult::ResultType::INVALID:
+    {
+        // It may be already in the mempool since in ATMP cases we don't set MEMPOOL_ENTRY or DIFFERENT_WITNESS
+        Assert(!res.m_state.IsValid());
+        Assert(res.m_state.IsInvalid());
+        Assert(!res.m_replaced_transactions);
+        Assert(!res.m_vsize);
+        Assert(!res.m_base_fees);
+        // Unable or unwilling to calculate fees
+        Assert(!res.m_effective_feerate);
+        Assert(!res.m_wtxids_fee_calculations);
+        Assert(!res.m_other_wtxid);
+        break;
+    }
+    case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY:
+    {
+        // ATMP never sets this; only set in package settings
+        Assert(false);
+        break;
+    }
+    case MempoolAcceptResult::ResultType::DIFFERENT_WITNESS:
+    {
+        // ATMP never sets this; only set in package settings
+        Assert(false);
+        break;
+    }
+    }
+}
+
 FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
@@ -258,9 +305,11 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
         SyncWithValidationInterfaceQueue();
         UnregisterSharedValidationInterface(txr);
 
+        bool txid_in_mempool = tx_pool.exists(GenTxid::Txid(tx->GetHash()));
+        bool wtxid_in_mempool = tx_pool.exists(GenTxid::Wtxid(tx->GetWitnessHash()));
+        CheckATMPInvariants(res, txid_in_mempool, wtxid_in_mempool);
+
         Assert(accepted != added.empty());
-        Assert(accepted == res.m_state.IsValid());
-        Assert(accepted != res.m_state.IsInvalid());
         if (accepted) {
             Assert(added.size() == 1); // For now, no package acceptance
             Assert(tx == *added.begin());

src/test/util/txmempool.cpp

@@ -11,6 +11,7 @@
 #include <util/check.h>
 #include <util/time.h>
 #include <util/translation.h>
+#include <validation.h>
 
 using node::NodeContext;
 
@@ -36,3 +37,80 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const
 {
     return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight, m_sequence, spendsCoinbase, sigOpCost, lp};
 }
+
+std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
+                                                           const PackageMempoolAcceptResult& result,
+                                                           bool expect_valid,
+                                                           const CTxMemPool* mempool)
+{
+    if (expect_valid) {
+        if (result.m_state.IsInvalid()) {
+            return strprintf("Package validation unexpectedly failed: %s", result.m_state.ToString());
+        }
+    } else {
+        if (result.m_state.IsValid()) {
+            strprintf("Package validation unexpectedly succeeded. %s", result.m_state.ToString());
+        }
+    }
+    if (result.m_state.GetResult() != PackageValidationResult::PCKG_POLICY && txns.size() != result.m_tx_results.size()) {
+        strprintf("txns size %u does not match tx results size %u", txns.size(), result.m_tx_results.size());
+    }
+    for (const auto& tx : txns) {
+        const auto& wtxid = tx->GetWitnessHash();
+        if (result.m_tx_results.count(wtxid) == 0) {
+            return strprintf("result not found for tx %s", wtxid.ToString());
+        }
+
+        const auto& atmp_result = result.m_tx_results.at(wtxid);
+        const bool valid{atmp_result.m_result_type == MempoolAcceptResult::ResultType::VALID};
+        if (expect_valid && atmp_result.m_state.IsInvalid()) {
+            return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString());
+        }
+
+        //m_replaced_transactions should exist iff the result was VALID
+        if (atmp_result.m_replaced_transactions.has_value() != valid) {
+            return strprintf("tx %s result should %shave m_replaced_transactions",
+                                    wtxid.ToString(), valid ? "" : "not ");
+        }
+
+        // m_vsize and m_base_fees should exist iff the result was VALID or MEMPOOL_ENTRY
+        const bool mempool_entry{atmp_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY};
+        if (atmp_result.m_base_fees.has_value() != (valid || mempool_entry)) {
+            return strprintf("tx %s result should %shave m_base_fees", wtxid.ToString(), valid || mempool_entry ? "" : "not ");
+        }
+        if (atmp_result.m_vsize.has_value() != (valid || mempool_entry)) {
+            return strprintf("tx %s result should %shave m_vsize", wtxid.ToString(), valid || mempool_entry ? "" : "not ");
+        }
+
+        // m_other_wtxid should exist iff the result was DIFFERENT_WITNESS
+        const bool diff_witness{atmp_result.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS};
+        if (atmp_result.m_other_wtxid.has_value() != diff_witness) {
+            return strprintf("tx %s result should %shave m_other_wtxid", wtxid.ToString(), diff_witness ? "" : "not ");
+        }
+
+        // m_effective_feerate and m_wtxids_fee_calculations should exist iff the result was valid
+        if (atmp_result.m_effective_feerate.has_value() != valid) {
+            return strprintf("tx %s result should %shave m_effective_feerate",
+                                    wtxid.ToString(), valid ? "" : "not ");
+        }
+        if (atmp_result.m_wtxids_fee_calculations.has_value() != valid) {
+            return strprintf("tx %s result should %shave m_effective_feerate",
+                                    wtxid.ToString(), valid ? "" : "not ");
+        }
+
+        if (mempool) {
+            // The tx by txid should be in the mempool iff the result was not INVALID.
+            const bool txid_in_mempool{atmp_result.m_result_type != MempoolAcceptResult::ResultType::INVALID};
+            if (mempool->exists(GenTxid::Txid(tx->GetHash())) != txid_in_mempool) {
+                strprintf("tx %s should %sbe in mempool", wtxid.ToString(), txid_in_mempool ? "" : "not ");
+            }
+            // Additionally, if the result was DIFFERENT_WITNESS, we shouldn't be able to find the tx in mempool by wtxid.
+            if (tx->HasWitness() && atmp_result.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) {
+                if (mempool->exists(GenTxid::Wtxid(wtxid))) {
+                    strprintf("wtxid %s should not be in mempool", wtxid.ToString());
+                }
+            }
+        }
+    }
+    return std::nullopt;
+}

src/test/util/txmempool.h

@@ -5,12 +5,14 @@
 #ifndef BITCOIN_TEST_UTIL_TXMEMPOOL_H
 #define BITCOIN_TEST_UTIL_TXMEMPOOL_H
 
+#include <policy/packages.h>
 #include <txmempool.h>
 #include <util/time.h>
 
 namespace node {
 struct NodeContext;
 }
+struct PackageMempoolAcceptResult;
 
 CTxMemPool::Options MemPoolOptionsForTest(const node::NodeContext& node);
 
@@ -36,4 +38,12 @@ struct TestMemPoolEntryHelper {
     TestMemPoolEntryHelper& SigOpsCost(unsigned int _sigopsCost) { sigOpCost = _sigopsCost; return *this; }
 };
 
+/** Check expected properties for every PackageMempoolAcceptResult, regardless of value. Returns
+ * a string if an error occurs with error populated, nullopt otherwise. If mempool is provided,
+ * checks that the expected transactions are in mempool (this should be set to nullptr for a test_accept).
+*/
+std::optional<std::string>  CheckPackageMempoolAcceptResult(const Package& txns,
+                                                            const PackageMempoolAcceptResult& result,
+                                                            bool expect_valid,
+                                                            const CTxMemPool* mempool);
 #endif // BITCOIN_TEST_UTIL_TXMEMPOOL_H