diff --git a/doc/release-notes-5834.md b/doc/release-notes-5834.md new file mode 100644 index 000000000000..b405071e8039 --- /dev/null +++ b/doc/release-notes-5834.md @@ -0,0 +1,13 @@ +Updated RPCs +------------ + +- The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee + if the transaction passes validation. +- The `testmempoolaccept` RPC now accepts multiple transactions (still experimental at the moment, + API may be unstable). This is intended for testing transaction packages with dependency + relationships; it is not recommended for batch-validating independent transactions. In addition to + mempool policy, package policies apply: the list cannot contain more than 25 transactions or have a + total size exceeding 101K virtual bytes, and cannot conflict with (spend the same inputs as) each other or + the mempool. There are some known limitations to the accuracy of the test accept: it's possible for + `testmempoolaccept` to return "allowed"=True for a group of transactions, but "too-long-mempool-chain" + if they are actually submitted. diff --git a/src/Makefile.am b/src/Makefile.am index 97abc0ec9066..28aa3bad7186 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -275,6 +275,7 @@ BITCOIN_CORE_H = \ outputtype.h \ policy/feerate.h \ policy/fees.h \ + policy/packages.h \ policy/policy.h \ policy/settings.h \ pow.h \ @@ -483,6 +484,7 @@ libbitcoin_server_a_SOURCES = \ node/ui_interface.cpp \ noui.cpp \ policy/fees.cpp \ + policy/packages.cpp \ policy/policy.cpp \ policy/settings.cpp \ pow.cpp \ diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index d0307a094a47..2e0530c33780 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -47,9 +47,8 @@ static void AssembleBlock(benchmark::Bench& bench) LOCK(::cs_main); // Required for ::AcceptToMemoryPool. for (const auto& txr : txs) { - TxValidationState state; - bool ret{::AcceptToMemoryPool(test_setup.m_node.chainman->ActiveChainstate(), *test_setup.m_node.mempool, state, txr, false /* bypass_limits */, /* nAbsurdFee */ 0)}; - assert(ret); + const MempoolAcceptResult res = ::AcceptToMemoryPool(test_setup.m_node.chainman->ActiveChainstate(), *test_setup.m_node.mempool, txr, false /* bypass_limits */); + assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID); } } diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index 81b5c46a9244..3a9c59f2ab4d 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/src/coinjoin/coinjoin.cpp @@ -306,6 +306,25 @@ bool CCoinJoinBaseSession::IsValidInOuts(const CTxMemPool& mempool, const std::v return true; } +// Responsibility for checking fee sanity is moved from the mempool to the client (BroadcastTransaction) +// but CoinJoin still requires ATMP with fee sanity checks so we need to implement them separately +bool ATMPIfSaneFee(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef &tx, bool test_accept) { + AssertLockHeld(cs_main); + + const MempoolAcceptResult result = AcceptToMemoryPool(active_chainstate, pool, tx, /* bypass_limits */ false, /* test_accept */ true); + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { + /* Fetch fee and fast-fail if ATMP fails regardless */ + return false; + } else if (result.m_base_fees.value() > DEFAULT_MAX_RAW_TX_FEE) { + /* Check fee against fixed upper limit */ + return false; + } else if (test_accept) { + /* Don't re-run ATMP if only doing test run */ + return true; + } + return AcceptToMemoryPool(active_chainstate, pool, tx, /* bypass_limits */ false, test_accept).m_result_type == MempoolAcceptResult::ResultType::VALID; +} + // check to make sure the collateral provided by the client is valid bool CoinJoin::IsCollateralValid(CTxMemPool& mempool, const CTransaction& txCollateral) { @@ -351,8 +370,7 @@ bool CoinJoin::IsCollateralValid(CTxMemPool& mempool, const CTransaction& txColl { LOCK(cs_main); - TxValidationState validationState; - if (!AcceptToMemoryPool(::ChainstateActive(), mempool, validationState, MakeTransactionRef(txCollateral), /*bypass_limits=*/false, /*nAbsurdFee=*/DEFAULT_MAX_RAW_TX_FEE, /*test_accept=*/true)) { + if (!ATMPIfSaneFee(::ChainstateActive(), mempool, MakeTransactionRef(txCollateral), /*test_accept=*/true)) { LogPrint(BCLog::COINJOIN, "CoinJoin::IsCollateralValid -- didn't pass AcceptToMemoryPool()\n"); return false; } diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index c010fac9e690..747997258cc6 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -22,16 +22,20 @@ #include #include +class CChainState; class CConnman; class CBLSPublicKey; class CBlockIndex; class CMasternodeSync; class CTxMemPool; +class TxValidationState; namespace llmq { class CChainLocksHandler; } // namespace llmq +extern RecursiveMutex cs_main; + // timeouts static constexpr int COINJOIN_AUTO_TIMEOUT_MIN = 5; static constexpr int COINJOIN_AUTO_TIMEOUT_MAX = 15; @@ -377,6 +381,8 @@ class CDSTXManager }; +bool ATMPIfSaneFee(CChainState& active_chainstate, CTxMemPool& pool, + const CTransactionRef &tx, bool test_accept = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); extern std::unique_ptr dstxManager; diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index 85fd26443f7c..0322eee4eff1 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -24,8 +24,6 @@ #include -constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10}; - PeerMsgRet CCoinJoinServer::ProcessMessage(CNode& peer, std::string_view msg_type, CDataStream& vRecv) { if (!fMasternodeMode) return {}; @@ -319,9 +317,8 @@ void CCoinJoinServer::CommitFinalTransaction() { // See if the transaction is valid TRY_LOCK(cs_main, lockMain); - TxValidationState validationState; mempool.PrioritiseTransaction(hashTx, 0.1 * COIN); - if (!lockMain || !AcceptToMemoryPool(m_chainstate, mempool, validationState, finalTransaction, false /* bypass_limits */, DEFAULT_MAX_RAW_TX_FEE /* nAbsurdFee */)) { + if (!lockMain || !ATMPIfSaneFee(m_chainstate, mempool, finalTransaction)) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CommitFinalTransaction -- AcceptToMemoryPool() error: Transaction not valid\n"); WITH_LOCK(cs_coinjoin, SetNull()); // not much we can do in this case, just notify clients @@ -453,8 +450,7 @@ void CCoinJoinServer::ChargeRandomFees() const void CCoinJoinServer::ConsumeCollateral(const CTransactionRef& txref) const { LOCK(cs_main); - TxValidationState validationState; - if (!AcceptToMemoryPool(m_chainstate, mempool, validationState, txref, false /* bypass_limits */, 0 /* nAbsurdFee */)) { + if (!ATMPIfSaneFee(m_chainstate, mempool, txref, false /* bypass_limits */)) { LogPrint(BCLog::COINJOIN, "%s -- AcceptToMemoryPool failed\n", __func__); } else { connman.RelayTransaction(*txref); diff --git a/src/llmq/ehf_signals.cpp b/src/llmq/ehf_signals.cpp index 343020c389ae..3bbd801309d2 100644 --- a/src/llmq/ehf_signals.cpp +++ b/src/llmq/ehf_signals.cpp @@ -131,11 +131,11 @@ void CEHFSignalsHandler::HandleNewRecoveredSig(const CRecoveredSig& recoveredSig CTransactionRef tx_to_sent = MakeTransactionRef(std::move(tx)); LogPrintf("CEHFSignalsHandler::HandleNewRecoveredSig Special EHF TX is created hash=%s\n", tx_to_sent->GetHash().ToString()); LOCK(cs_main); - TxValidationState state; - if (AcceptToMemoryPool(chainstate, mempool, state, tx_to_sent, /* bypass_limits=*/ false, /* nAbsurdFee=*/ 0)) { + const MempoolAcceptResult result = AcceptToMemoryPool(chainstate, mempool, tx_to_sent, /* bypass_limits */ false); + if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { connman.RelayTransaction(*tx_to_sent); } else { - LogPrintf("CEHFSignalsHandler::HandleNewRecoveredSig -- AcceptToMemoryPool failed: %s\n", state.ToString()); + LogPrintf("CEHFSignalsHandler::HandleNewRecoveredSig -- AcceptToMemoryPool failed: %s\n", result.m_state.ToString()); } } } diff --git a/src/logging.cpp b/src/logging.cpp index fbb2f99a4ba5..22acb12534a9 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -235,9 +235,9 @@ std::string BCLog::Logger::LogTimestampStr(const std::string& str) strStamped.pop_back(); strStamped += strprintf(".%06dZ", nTimeMicros%1000000); } - int64_t mocktime = GetMockTime(); - if (mocktime) { - strStamped += " (mocktime: " + FormatISO8601DateTime(mocktime) + ")"; + std::chrono::seconds mocktime = GetMockTime(); + if (mocktime > 0s) { + strStamped += " (mocktime: " + FormatISO8601DateTime(count_seconds(mocktime)) + ")"; } strStamped += ' ' + str; } else diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 06185e0bef39..98cb54d55aba 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -179,7 +179,11 @@ struct COrphanTx { size_t list_pos; size_t nTxSize; }; + +/** Guards orphan transactions and extra txs for compact blocks */ RecursiveMutex g_cs_orphans; +/** Map from txid to orphan transaction record. Limited by + * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ std::map mapOrphanTransactions GUARDED_BY(g_cs_orphans); size_t nMapOrphanTransactionsSize = 0; @@ -548,12 +552,19 @@ namespace { return &(*a) < &(*b); } }; - std::map::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); - std::vector::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); //! For random eviction + /** Index from the parents' COutPoint into the mapOrphanTransactions. Used + * to remove orphan transactions from the mapOrphanTransactions */ + std::map::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); + /** Orphan transactions in vector for quick random eviction */ + std::vector::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); - static size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0; + /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction. + * The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of + * these are kept in a ring buffer */ static std::vector> vExtraTxnForCompact GUARDED_BY(g_cs_orphans); + /** Offset into vExtraTxnForCompact to insert the next tx */ + static size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0; } // namespace namespace { @@ -837,13 +848,12 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) } } m_connman.ForNode(nodeid, [this](CNode* pfrom){ - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); uint64_t nCMPCTBLOCKVersion = 1; if (lNodesAnnouncingHeaderAndIDs.size() >= 3) { // As per BIP152, we only get 3 of our peers to announce // blocks using compact encodings. m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this, nCMPCTBLOCKVersion](CNode* pnodeStop){ - AssertLockHeld(cs_main); m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion)); return true; }); @@ -1716,7 +1726,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha } m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, &hashBlock](CNode* pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); // TODO: Avoid the repeated-serialization here if (pnode->fDisconnect) return; @@ -2533,13 +2543,20 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const std::vector& orphan_work_set) { AssertLockHeld(cs_main); AssertLockHeld(g_cs_orphans); - std::set setMisbehaving; - bool done = false; - while (!done && !orphan_work_set.empty()) { + + while (!orphan_work_set.empty()) { const uint256 orphanHash = *orphan_work_set.begin(); orphan_work_set.erase(orphan_work_set.begin()); @@ -2547,19 +2564,13 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) if (orphan_it == mapOrphanTransactions.end()) continue; const CTransactionRef porphanTx = orphan_it->second.tx; - const CTransaction& orphanTx = *porphanTx; - NodeId fromPeer = orphan_it->second.fromPeer; - // Use a new TxValidationState because orphans come from different peers (and we call - // MaybePunishNodeForTx based on the source peer from the orphan map, not based on the peer - // that relayed the previous transaction). - TxValidationState orphan_state; - - if (setMisbehaving.count(fromPeer)) continue; - if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, orphan_state, porphanTx, - false /* bypass_limits */, 0 /* nAbsurdFee */)) { + const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, porphanTx, false /* bypass_limits */); + const TxValidationState& state = result.m_state; + + if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); - RelayTransaction(orphanTx.GetHash()); - for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { + RelayTransaction(porphanTx->GetHash()); + for (unsigned int i = 0; i < porphanTx->vout.size(); i++) { auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(orphanHash, i)); if (it_by_prev != mapOrphanTransactionsByPrev.end()) { for (const auto& elem : it_by_prev->second) { @@ -2568,24 +2579,22 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) } } EraseOrphanTx(orphanHash); - done = true; - } else if (orphan_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { - if (orphan_state.IsInvalid()) { - // Punish peer that gave us an invalid orphan tx - if (MaybePunishNodeForTx(fromPeer, orphan_state)) { - setMisbehaving.insert(fromPeer); - } + break; + } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { + if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s\n", orphanHash.ToString()); + // Maybe punish peer that gave us an invalid orphan tx + MaybePunishNodeForTx(orphan_it->second.fromPeer, state); } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); m_recent_rejects.insert(orphanHash); EraseOrphanTx(orphanHash); - done = true; + break; } - m_mempool.check(m_chainman.ActiveChainstate()); } + m_mempool.check(m_chainman.ActiveChainstate()); } bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, const CChainParams& chain_params, @@ -3616,10 +3625,26 @@ void PeerManagerImpl::ProcessMessage( LOCK2(cs_main, g_cs_orphans); - TxValidationState state; + if (AlreadyHave(inv)) { + if (pfrom.HasPermission(PF_FORCERELAY)) { + // Always relay transactions received from peers with forcerelay permission, even + // if they were already in the mempool, + // allowing the node to function as a gateway for + // nodes hidden behind it. + if (!m_mempool.exists(tx.GetHash())) { + LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); + } else { + LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); + RelayTransaction(tx.GetHash()); + } + } + return; + } + + const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, ptx, false /* bypass_limits */); + const TxValidationState& state = result.m_state; - if (!AlreadyHave(inv) && AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, ptx, - false /* bypass_limits */, 0 /* nAbsurdFee */)) { + if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { // Process custom txes, this changes AlreadyHave to "true" if (nInvType == MSG_DSTX) { LogPrint(BCLog::COINJOIN, "DSTX -- Masternode transaction accepted, txid=%s, peer=%d\n", @@ -3690,19 +3715,6 @@ void PeerManagerImpl::ProcessMessage( if (RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } - - if (pfrom.HasPermission(PF_FORCERELAY)) { - // Always relay transactions received from peers with forcerelay permission, even - // if they were already in the mempool, - // allowing the node to function as a gateway for - // nodes hidden behind it. - if (!m_mempool.exists(tx.GetHash())) { - LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); - } else { - LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); - RelayTransaction(tx.GetHash()); - } - } } // If a tx has been detected by m_recent_rejects, we will have reached @@ -4594,7 +4606,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) int64_t oldest_block_announcement = std::numeric_limits::max(); m_connman.ForEachNode([&](CNode* pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); // Don't disconnect masternodes just because they were slow in block announcement if (pnode->m_masternode_connection) return; @@ -4613,7 +4625,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) }); if (worst_peer != -1) { bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); // Only disconnect a peer that has been connected to us for // some reasonable fraction of our check-frequency, to give diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 8685f48d4460..4461b405c266 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -16,6 +16,18 @@ #include +static TransactionError HandleATMPError(const TxValidationState& state, std::string& err_string_out) { + err_string_out = state.ToString(); + if (state.IsInvalid()) { + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { + return TransactionError::MISSING_INPUTS; + } + return TransactionError::MEMPOOL_REJECTED; + } else { + return TransactionError::MEMPOOL_ERROR; + } +} + TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits) { // BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs. @@ -41,20 +53,24 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; } if (!node.mempool->exists(hashTx)) { - // Transaction is not already in the mempool. Submit it. - TxValidationState state; - if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, state, tx, - bypass_limits, max_tx_fee)) { - err_string = state.ToString(); - if (state.IsInvalid()) { - if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { - return TransactionError::MISSING_INPUTS; - } - return TransactionError::MEMPOOL_REJECTED; - } else { - return TransactionError::MEMPOOL_ERROR; + // Transaction is not already in the mempool. + if (max_tx_fee > 0) { + // First, call ATMP with test_accept and check the fee. If ATMP + // fails here, return error immediately. + const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, + bypass_limits, true /* test_accept */); + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { + return HandleATMPError(result.m_state, err_string); + } else if (result.m_base_fees.value() > max_tx_fee) { + return TransactionError::MAX_FEE_EXCEEDED; } } + // Try to submit the transaction to the mempool. + const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, + bypass_limits, false /* test_accept */); + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { + return HandleATMPError(result.m_state, err_string); + } // Transaction was accepted to the mempool. diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp new file mode 100644 index 000000000000..cfd05399655d --- /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 +#include +#include +#include +#include + +#include +#include + +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 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 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 new file mode 100644 index 000000000000..bb2e4b3b4706 --- /dev/null +++ b/src/policy/packages.h @@ -0,0 +1,44 @@ +// 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. + +#ifndef BITCOIN_POLICY_PACKAGES_H +#define BITCOIN_POLICY_PACKAGES_H + +#include +#include +#include + +#include + +/** Default maximum number of transactions in a package. */ +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 * 1000 >= MAX_STANDARD_TX_SIZE); + +/** 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. + * We don't distinguish between consensus and policy violations right now. + */ +enum class PackageValidationResult { + PCKG_RESULT_UNSET = 0, //!< Initial value. The package has not yet been rejected. + PCKG_POLICY, //!< The package itself is invalid (e.g. too many transactions). + PCKG_TX, //!< At least one tx is invalid. +}; + +/** A package is an ordered list of transactions. The transactions cannot conflict with (spend the + * same inputs as) one another. */ +using Package = std::vector; + +class PackageValidationState : public ValidationState {}; + +/** 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 9f4779186e78..b879e8b4455b 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -1156,7 +1157,10 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) { RPCHelpMan{"testmempoolaccept", "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n" - "\nThis checks if the transaction violates the consensus or policy rules.\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 " + ToString(MAX_PACKAGE_COUNT) + ".\n" + "\nThis checks if transactions violate the consensus or policy rules.\n" "\nSee sendrawtransaction call.\n", { {"rawtxs", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of hex strings of raw transactions.", @@ -1164,16 +1168,25 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""}, }, }, - {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kB\n"}, + {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), + "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kB\n"}, }, RPCResult{ RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" - "Length is exactly one for now.", + "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 another transaction failed.\n", { {RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, - {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"}, + {RPCResult::Type::STR, "package-error", "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."}, + {RPCResult::Type::BOOL, "allowed", "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate." + "If not present, the tx was not fully validated due to a failure in another tx in the list."}, + {RPCResult::Type::NUM, "vsize", "Virtual transaction size."}, + {RPCResult::Type::OBJ, "fees", "Transaction fees (only present if 'allowed' is true)", + { + {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, + }}, {RPCResult::Type::STR, "reject-reason", "Rejection string (only present when 'allowed' is false)"}, }}, } @@ -1194,55 +1207,86 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) UniValue::VARR, UniValueType(), // VNUM or VSTR, checked inside AmountFromValue() }); - - if (request.params[0].get_array().size() != 1) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Array must contain exactly one raw transaction for now"); + 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, + "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions."); } - CMutableTransaction mtx; - if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); - } - CTransactionRef tx(MakeTransactionRef(std::move(mtx))); - const uint256& tx_hash = tx->GetHash(); - const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? DEFAULT_MAX_RAW_TX_FEE_RATE : CFeeRate(AmountFromValue(request.params[1])); - const NodeContext& node = EnsureAnyNodeContext(request.context); + std::vector txns; + txns.reserve(raw_transactions.size()); + for (const auto& rawtx : raw_transactions.getValues()) { + CMutableTransaction mtx; + if (!DecodeHexTx(mtx, rawtx.get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, + "TX decode failed: " + rawtx.get_str() + " Make sure the tx has at least one input."); + } + txns.emplace_back(MakeTransactionRef(std::move(mtx))); + } + NodeContext& node = EnsureAnyNodeContext(request.context); CTxMemPool& mempool = EnsureMemPool(node); - int64_t virtual_size = GetVirtualTransactionSize(*tx); - CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); - - UniValue result(UniValue::VARR); - UniValue result_0(UniValue::VOBJ); - result_0.pushKV("txid", tx_hash.GetHex()); - - TxValidationState state; - bool test_accept_res; - { - ChainstateManager& chainman = EnsureChainman(node); - LOCK(cs_main); - test_accept_res = AcceptToMemoryPool(chainman.ActiveChainstate(), mempool, state, std::move(tx), - false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true); - } - result_0.pushKV("allowed", test_accept_res); - if (!test_accept_res) { - if (state.IsInvalid()) { - if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { - result_0.pushKV("reject-reason", "missing-inputs"); + CChainState& chainstate = EnsureChainman(node).ActiveChainstate(); + const PackageMempoolAcceptResult package_result = [&] { + LOCK(::cs_main); + if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true); + return PackageMempoolAcceptResult(txns[0]->GetHash(), + AcceptToMemoryPool(chainstate, mempool, txns[0], /* bypass_limits */ false, /* test_accept*/ true)); + }(); + + UniValue rpc_result(UniValue::VARR); + // 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}; + for (const auto& tx : txns) { + UniValue result_inner(UniValue::VOBJ); + result_inner.pushKV("txid", tx->GetHash().GetHex()); + if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY) { + result_inner.pushKV("package-error", package_result.m_state.GetRejectReason()); + } + auto it = package_result.m_tx_results.find(tx->GetHash()); + if (exit_early || it == package_result.m_tx_results.end()) { + // Validation unfinished. Just return the txid. + rpc_result.push_back(result_inner); + continue; + } + const auto& tx_result = it->second; + if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + const CAmount fee = tx_result.m_base_fees.value(); + // Check that fee does not exceed maximum fee + const int64_t virtual_size = GetVirtualTransactionSize(*tx); + const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); + if (max_raw_tx_fee && fee > max_raw_tx_fee) { + result_inner.pushKV("allowed", false); + result_inner.pushKV("reject-reason", "max-fee-exceeded"); + exit_early = true; } else { - result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason())); + // Only return the fee and vsize if the transaction would pass ATMP. + // These can be used to calculate the feerate. + result_inner.pushKV("allowed", true); + result_inner.pushKV("vsize", virtual_size); + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(fee)); + result_inner.pushKV("fees", fees); } } else { - result_0.pushKV("reject-reason", state.GetRejectReason()); + result_inner.pushKV("allowed", false); + const TxValidationState state = tx_result.m_state; + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { + result_inner.pushKV("reject-reason", "missing-inputs"); + } else { + result_inner.pushKV("reject-reason", state.GetRejectReason()); + } } + rpc_result.push_back(result_inner); } - - result.push_back(std::move(result_0)); - return result; + return rpc_result; } UniValue decodepsbt(const JSONRPCRequest& request) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index d78aead71977..be5a682931af 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -35,7 +35,8 @@ struct MinerTestingSetup : public TestingSetup { void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector& 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) { - return CheckSequenceLocks(::ChainstateActive(), *m_node.mempool, 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); }; @@ -136,6 +137,7 @@ void MinerTestingSetup::TestPackageSelection(const CChainParams& chainparams, co m_node.mempool->addUnchecked(entry.Fee(50000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); std::unique_ptr pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey); + BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4); BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashParentTx); BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashHighFeeTx); BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashMediumFeeTx); @@ -170,6 +172,7 @@ void MinerTestingSetup::TestPackageSelection(const CChainParams& chainparams, co hashLowFeeTx = tx.GetHash(); m_node.mempool->addUnchecked(entry.Fee(feeToUse+2).FromTx(tx)); pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey); + BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6); BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashFreeTx); BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashLowFeeTx); @@ -204,6 +207,7 @@ void MinerTestingSetup::TestPackageSelection(const CChainParams& chainparams, co tx.vout[0].nValue = 100000000 - 10000; // 10k satoshi fee m_node.mempool->addUnchecked(entry.Fee(10000).FromTx(tx)); pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey); + BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 9); BOOST_CHECK(pblocktemplate->block.vtx[8]->GetHash() == hashLowFeeTx2); } diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 7a7784efb080..ac6f8b91a7d7 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -3,8 +3,12 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include +#include +#include #include #include