diff options
| -rw-r--r-- | src/Makefile.am | 1 | ||||
| -rw-r--r-- | src/policy/packages.cpp | 62 | ||||
| -rw-r--r-- | src/policy/packages.h | 10 | ||||
| -rw-r--r-- | src/rpc/rawtransaction.cpp | 10 | ||||
| -rw-r--r-- | src/test/miner_tests.cpp | 4 | ||||
| -rw-r--r-- | src/txmempool.cpp | 4 | ||||
| -rw-r--r-- | src/txmempool.h | 3 | ||||
| -rw-r--r-- | src/validation.cpp | 87 | ||||
| -rw-r--r-- | src/validation.h | 20 | ||||
| -rwxr-xr-x | test/functional/rpc_packages.py | 3 |
10 files changed, 120 insertions, 84 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 66cb7cec2a..e2ed70556d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -351,6 +351,7 @@ libbitcoin_server_a_SOURCES = \ node/ui_interface.cpp \ noui.cpp \ policy/fees.cpp \ + policy/packages.cpp \ policy/rbf.cpp \ policy/settings.cpp \ pow.cpp \ diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp new file mode 100644 index 0000000000..cfd0539965 --- /dev/null +++ b/src/policy/packages.cpp @@ -0,0 +1,62 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include <consensus/validation.h> +#include <policy/packages.h> +#include <primitives/transaction.h> +#include <uint256.h> +#include <util/hasher.h> + +#include <numeric> +#include <unordered_set> + +bool CheckPackage(const Package& txns, PackageValidationState& state) +{ + const unsigned int package_count = txns.size(); + + if (package_count > MAX_PACKAGE_COUNT) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); + } + + const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, + [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); + // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. + if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); + } + + // Require the package to be sorted in order of dependency, i.e. parents appear before children. + // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and + // fail on something less ambiguous (missing-inputs could also be an orphan or trying to + // spend nonexistent coins). + std::unordered_set<uint256, SaltedTxidHasher> later_txids; + std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), + [](const auto& tx) { return tx->GetHash(); }); + for (const auto& tx : txns) { + for (const auto& input : tx->vin) { + if (later_txids.find(input.prevout.hash) != later_txids.end()) { + // The parent is a subsequent transaction in the package. + return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); + } + } + later_txids.erase(tx->GetHash()); + } + + // Don't allow any conflicting transactions, i.e. spending the same inputs, in a package. + std::unordered_set<COutPoint, SaltedOutpointHasher> inputs_seen; + for (const auto& tx : txns) { + for (const auto& input : tx->vin) { + if (inputs_seen.find(input.prevout) != inputs_seen.end()) { + // This input is also present in another tx in the package. + return state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); + } + } + // Batch-add all the inputs for a tx at a time. If we added them 1 at a time, we could + // catch duplicate inputs within a single tx. This is a more severe, consensus error, + // and we want to report that from CheckTransaction instead. + std::transform(tx->vin.cbegin(), tx->vin.cend(), std::inserter(inputs_seen, inputs_seen.end()), + [](const auto& input) { return input.prevout; }); + } + return true; +} diff --git a/src/policy/packages.h b/src/policy/packages.h index 4b1463dcb3..6b7ac3e450 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -6,6 +6,7 @@ #define BITCOIN_POLICY_PACKAGES_H #include <consensus/validation.h> +#include <policy/policy.h> #include <primitives/transaction.h> #include <vector> @@ -14,6 +15,7 @@ static constexpr uint32_t MAX_PACKAGE_COUNT{25}; /** Default maximum total virtual size of transactions in a package in KvB. */ static constexpr uint32_t MAX_PACKAGE_SIZE{101}; +static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT); /** A "reason" why a package was invalid. It may be that one or more of the included * transactions is invalid or the package itself violates our rules. @@ -31,4 +33,12 @@ using Package = std::vector<CTransactionRef>; class PackageValidationState : public ValidationState<PackageValidationResult> {}; +/** Context-free package policy checks: + * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT. + * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE. + * 3. If any dependencies exist between transactions, parents must appear before children. + * 4. Transactions cannot conflict, i.e., spend the same inputs. + */ +bool CheckPackage(const Package& txns, PackageValidationState& state); + #endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 339d711ac9..414c6637a5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -889,7 +889,7 @@ static RPCHelpMan testmempoolaccept() "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n" "\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other.\n" "\nIf one transaction fails, other transactions may not be fully validated (the 'allowed' key will be blank).\n" - "\nThe maximum number of transactions allowed is 25 (MAX_PACKAGE_COUNT)\n" + "\nThe maximum number of transactions allowed is " + ToString(MAX_PACKAGE_COUNT) + ".\n" "\nThis checks if transactions violate the consensus or policy rules.\n" "\nSee sendrawtransaction call.\n", { @@ -905,7 +905,7 @@ static RPCHelpMan testmempoolaccept() RPCResult{ RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" "Returns results for each transaction in the same order they were passed in.\n" - "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n", + "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n", { {RPCResult::Type::OBJ, "", "", { @@ -939,7 +939,6 @@ static RPCHelpMan testmempoolaccept() UniValue::VARR, UniValueType(), // VNUM or VSTR, checked inside AmountFromValue() }); - const UniValue raw_transactions = request.params[0].get_array(); if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) { throw JSONRPCError(RPC_INVALID_PARAMETER, @@ -951,6 +950,7 @@ static RPCHelpMan testmempoolaccept() CFeeRate(AmountFromValue(request.params[1])); std::vector<CTransactionRef> txns; + txns.reserve(raw_transactions.size()); for (const auto& rawtx : raw_transactions.getValues()) { CMutableTransaction mtx; if (!DecodeHexTx(mtx, rawtx.get_str())) { @@ -971,8 +971,8 @@ static RPCHelpMan testmempoolaccept() }(); UniValue rpc_result(UniValue::VARR); - // We will check transaction fees we iterate through txns in order. If any transaction fee - // exceeds maxfeerate, we will keave the rest of the validation results blank, because it + // We will check transaction fees while we iterate through txns in order. If any transaction fee + // exceeds maxfeerate, we will leave the rest of the validation results blank, because it // doesn't make sense to return a validation result for a transaction if its ancestor(s) would // not be submitted. bool exit_early{false}; diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index c47d0eae1e..e54948a840 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -28,8 +28,8 @@ struct MinerTestingSetup : public TestingSetup { void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs); bool TestSequenceLocks(const CTransaction& tx, int flags) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs) { - CCoinsViewMemPool viewMempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool); - return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), viewMempool, tx, flags); + CCoinsViewMemPool view_mempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool); + return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), view_mempool, tx, flags); } BlockAssembler AssemblerForTest(const CChainParams& params); }; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4413da7ea7..7c73a2d78d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -515,9 +515,9 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) LockPoints lp = it->GetLockPoints(); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); - CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this); + CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this); if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) - || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), viewMempool, tx, flags, &lp, validLP)) { + || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) { // Note if CheckSequenceLocks fails the LockPoints may still be invalid // So it's critical that we remove the tx and not depend on the LockPoints. txToRemove.insert(it); diff --git a/src/txmempool.h b/src/txmempool.h index 46b89049bb..ae4b16d377 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -874,7 +874,8 @@ protected: public: CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn); bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; - /** Add the coins created by this transaction. */ + /** Add the coins created by this transaction. These coins are only temporarily stored in + * m_temp_added and cannot be flushed to the back end. Only used for package validation. */ void PackageAddTransaction(const CTransactionRef& tx); }; diff --git a/src/validation.cpp b/src/validation.cpp index 5e3d429c2e..d5c4be6811 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -472,8 +472,10 @@ public: */ std::vector<COutPoint>& m_coins_to_uncache; const bool m_test_accept; - /** Disable BIP125 RBFing; disallow all conflicts with mempool transactions. */ - const bool disallow_mempool_conflicts; + /** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false, + * any transaction spending the same inputs as a transaction in the mempool is considered + * a conflict. */ + const bool m_allow_bip125_replacement{true}; }; // Single transaction acceptance @@ -482,7 +484,7 @@ public: /** * Multiple transaction acceptance. Transactions may or may not be interdependent, * but must not conflict with each other. Parents must come before children if any - * dependencies exist, otherwise a TX_MISSING_INPUTS error will be returned. + * dependencies exist. */ PackageMempoolAcceptResult AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -619,6 +621,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) { const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout); if (ptxConflicting) { + if (!args.m_allow_bip125_replacement) { + // Transaction conflicts with a mempool tx, but we're not allowing replacements. + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed"); + } if (!setConflicts.count(ptxConflicting->GetHash())) { // Allow opt-out of transaction replacement by setting @@ -645,7 +651,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) break; } } - if (fReplacementOptOut || args.disallow_mempool_conflicts) { + if (fReplacementOptOut) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } @@ -1080,65 +1086,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: { AssertLockHeld(cs_main); + // These context-free package limits can be done before taking the mempool lock. PackageValidationState package_state; - const unsigned int package_count = txns.size(); - - // These context-free package limits can be checked before taking the mempool lock. - if (package_count > MAX_PACKAGE_COUNT) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); - return PackageMempoolAcceptResult(package_state, {}); - } - - const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, - [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); - // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. - if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); - return PackageMempoolAcceptResult(package_state, {}); - } + if (!CheckPackage(txns, package_state)) return PackageMempoolAcceptResult(package_state, {}); - // Construct workspaces and check package policies. std::vector<Workspace> workspaces{}; - workspaces.reserve(package_count); - { - std::unordered_set<uint256, SaltedTxidHasher> later_txids; - std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), - [](const auto& tx) { return tx->GetHash(); }); - // Require the package to be sorted in order of dependency, i.e. parents appear before children. - // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and - // fail on something less ambiguous (missing-inputs could also be an orphan or trying to - // spend nonexistent coins). - for (const auto& tx : txns) { - for (const auto& input : tx->vin) { - if (later_txids.find(input.prevout.hash) != later_txids.end()) { - // The parent is a subsequent transaction in the package. - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); - return PackageMempoolAcceptResult(package_state, {}); - } - } - later_txids.erase(tx->GetHash()); - workspaces.emplace_back(Workspace(tx)); - } - } + workspaces.reserve(txns.size()); + std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces), + [](const auto& tx) { return Workspace(tx); }); std::map<const uint256, const MempoolAcceptResult> results; - { - // Don't allow any conflicting transactions, i.e. spending the same inputs, in a package. - std::unordered_set<COutPoint, SaltedOutpointHasher> inputs_seen; - for (const auto& tx : txns) { - for (const auto& input : tx->vin) { - if (inputs_seen.find(input.prevout) != inputs_seen.end()) { - // This input is also present in another tx in the package. - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); - return PackageMempoolAcceptResult(package_state, {}); - } - } - // Batch-add all the inputs for a tx at a time. If we added them 1 at a time, we could - // catch duplicate inputs within a single tx. This is a more severe, consensus error, - // and we want to report that from CheckTransaction instead. - std::transform(tx->vin.cbegin(), tx->vin.cend(), std::inserter(inputs_seen, inputs_seen.end()), - [](const auto& input) { return input.prevout; }); - } - } LOCK(m_pool.cs); @@ -1151,10 +1107,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: return PackageMempoolAcceptResult(package_state, std::move(results)); } // Make the coins created by this transaction available for subsequent transactions in the - // package to spend. Since we already checked conflicts in the package and RBFs are - // impossible, we don't need to track the coins spent. Note that this logic will need to be - // updated if RBFs in packages are allowed in the future. - assert(args.disallow_mempool_conflicts); + // package to spend. Since we already checked conflicts in the package and we don't allow + // replacements, we don't need to track the coins spent. Note that this logic will need to be + // updated if package replace-by-fee is allowed in the future. + assert(!args.m_allow_bip125_replacement); m_viewmempool.PackageAddTransaction(ws.m_ptx); } @@ -1188,7 +1144,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp { std::vector<COutPoint> coins_to_uncache; MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, - test_accept, /* disallow_mempool_conflicts */ false }; + test_accept, /* m_allow_bip125_replacement */ true }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); @@ -1225,12 +1181,11 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx std::vector<COutPoint> coins_to_uncache; const CChainParams& chainparams = Params(); MemPoolAccept::ATMPArgs args { chainparams, GetTime(), /* bypass_limits */ false, coins_to_uncache, - test_accept, /* disallow_mempool_conflicts */ true }; + test_accept, /* m_allow_bip125_replacement */ false }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); const PackageMempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args); // Uncache coins pertaining to transactions that were not submitted to the mempool. - // Ensure the cache is still within its size limits. for (const COutPoint& hashTx : coins_to_uncache) { active_chainstate.CoinsTip().Uncache(hashTx); } diff --git a/src/validation.h b/src/validation.h index 359a6c779f..0c396c92ee 100644 --- a/src/validation.h +++ b/src/validation.h @@ -234,11 +234,13 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** -* Atomically test acceptance of a package. If the package only contains one tx, package rules still apply. +* Atomically test acceptance of a package. If the package only contains one tx, package rules still +* apply. Package validation does not allow BIP125 replacements, so the transaction(s) cannot spend +* the same inputs as any transaction in the mempool. * @param[in] txns Group of transactions which may be independent or contain -* parent-child dependencies. The transactions must not conflict, i.e. -* must not spend the same inputs, even if it would be a valid BIP125 -* replace-by-fee. Parents must appear before children. +* parent-child dependencies. The transactions must not conflict +* with each other, i.e., must not spend the same inputs. If any +* dependencies exist, parents must appear before children. * @returns a PackageMempoolAcceptResult which includes a MempoolAcceptResult for each transaction. * If a transaction fails, validation will exit early and some results may be missing. */ @@ -269,9 +271,13 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE * Check if transaction will be BIP68 final in the next block to be created on top of tip. * @param[in] tip Chain tip to check tx sequence locks against. For example, * the tip of the current active chain. - * @param[in] coins_view Any CCoinsView that provides access to the relevant coins - * for checking sequence locks. Any CCoinsView can be passed in; - * it is assumed to be consistent with the tip. + * @param[in] coins_view Any CCoinsView that provides access to the relevant coins for + * checking sequence locks. For example, it can be a CCoinsViewCache + * that isn't connected to anything but contains all the relevant + * coins, or a CCoinsViewMemPool that is connected to the + * mempool and chainstate UTXO set. In the latter case, the caller is + * responsible for holding the appropriate locks to ensure that + * calls to GetCoin() return correct coins. * Simulates calling SequenceLocks() with data from the tip passed in. * Optionally stores in LockPoints the resulting height and time calculated and the hash * of the block needed for calculation or skips the calculation and uses the LockPoints diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 3d8d81d6b8..d0848d9bd6 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -354,7 +354,8 @@ class RPCPackagesTest(BitcoinTestFramework): # This transaction is a valid BIP125 replace-by-fee assert testres_rbf_single[0]["allowed"] testres_rbf_package = self.independent_txns_testres_blank + [{ - "txid": replacement_tx.rehash(), "wtxid": replacement_tx.getwtxid(), "allowed": False, "reject-reason": "txn-mempool-conflict" + "txid": replacement_tx.rehash(), "wtxid": replacement_tx.getwtxid(), "allowed": False, + "reject-reason": "bip125-replacement-disallowed" }] self.assert_testres_equal(self.independent_txns_hex + [signed_replacement_tx["hex"]], testres_rbf_package) |