From 651fa404e454e31f8e9d830aa292eb3b456b54fb Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 16 Oct 2023 12:20:22 -0400 Subject: [PATCH 1/3] fuzz: tx_pool checks ATMP result invariants --- src/test/fuzz/tx_pool.cpp | 53 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 5ec3e89d1ee..66e537a57ba 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/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()); From 34088d6c9ed4ed99bb6b7fc83795da01ec9f3c97 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 8 Aug 2023 18:25:40 +0100 Subject: [PATCH 2/3] [test util] CheckPackageMempoolAcceptResult for sanity-checking results --- src/test/util/txmempool.cpp | 78 +++++++++++++++++++++++++++++++++++++ src/test/util/txmempool.h | 10 +++++ 2 files changed, 88 insertions(+) diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index c945f35d792..c4fbc8dbb38 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -11,6 +11,7 @@ #include #include #include +#include using node::NodeContext; @@ -36,3 +37,80 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const { return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch(time), nHeight, m_sequence, spendsCoinbase, sigOpCost, lp}; } + +std::optional 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; +} diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h index 4b0daf0d422..a866d1ce741 100644 --- a/src/test/util/txmempool.h +++ b/src/test/util/txmempool.h @@ -5,12 +5,14 @@ #ifndef BITCOIN_TEST_UTIL_TXMEMPOOL_H #define BITCOIN_TEST_UTIL_TXMEMPOOL_H +#include #include #include 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 CheckPackageMempoolAcceptResult(const Package& txns, + const PackageMempoolAcceptResult& result, + bool expect_valid, + const CTxMemPool* mempool); #endif // BITCOIN_TEST_UTIL_TXMEMPOOL_H From fcb3069fa307942cf7f3edabcda1be96d615c91f Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Wed, 1 Nov 2023 09:53:05 -0400 Subject: [PATCH 3/3] Use CheckPackageMempoolAcceptResult for package evaluation fuzzing --- src/test/fuzz/package_eval.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 7220c5d9973..8d316134cc7 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/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());