From 9c6c82b7d3da902504a60c002128532a81a732f0 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 30 Jan 2024 08:55:27 +0000 Subject: [PATCH 01/12] merge bitcoin#19940: Return fee and vsize from testmempoolaccept --- doc/release-notes-5834.md | 5 +++++ src/rpc/rawtransaction.cpp | 18 ++++++++++++++++-- src/validation.cpp | 14 ++++++++++---- src/validation.h | 6 ++++-- test/functional/feature_asset_locks.py | 12 ++++++------ test/functional/mempool_accept.py | 10 ++++++---- test/functional/test_framework/messages.py | 5 +++++ 7 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 doc/release-notes-5834.md diff --git a/doc/release-notes-5834.md b/doc/release-notes-5834.md new file mode 100644 index 000000000000..fb5f652de683 --- /dev/null +++ b/doc/release-notes-5834.md @@ -0,0 +1,5 @@ +Updated RPCs +------------ + +- The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee + if the transaction passes validation. diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 9f4779186e78..8943b4fdd88c 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1174,6 +1174,11 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) { {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::NUM, "vsize", "Virtual transaction size as defined in BIP 141"}, + {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)"}, }}, } @@ -1222,14 +1227,23 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) TxValidationState state; bool test_accept_res; + CAmount fee; { 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); + false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true, &fee); } result_0.pushKV("allowed", test_accept_res); - if (!test_accept_res) { + + // Only return the fee and vsize if the transaction would pass ATMP. + // These can be used to calculate the feerate. + if (test_accept_res) { + result_0.pushKV("vsize", virtual_size); + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(fee)); + result_0.pushKV("fees", fees); + } else { if (state.IsInvalid()) { if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { result_0.pushKV("reject-reason", "missing-inputs"); diff --git a/src/validation.cpp b/src/validation.cpp index ff8560e14a82..8e814bbe00d0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -558,6 +558,7 @@ class MemPoolAccept */ std::vector& m_coins_to_uncache; const bool m_test_accept; + CAmount* m_fee_out; }; // Single transaction acceptance @@ -764,6 +765,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), state.ToString()); } + // If fee_out is passed, return the fee to the caller + if (args.m_fee_out) { + *args.m_fee_out = nFees; + } + // Check for non-standard pay-to-script-hash in inputs if (fRequireStandard && !AreInputsStandard(tx, m_view)) { return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs"); @@ -986,10 +992,10 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs /** (try to) add transaction to memory pool with a specified acceptance time **/ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CChainState& active_chainstate, TxValidationState &state, const CTransactionRef &tx, int64_t nAcceptTime, bool bypass_limits, - const CAmount nAbsurdFee, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) + const CAmount nAbsurdFee, bool test_accept, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::vector coins_to_uncache; - MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept }; + MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept, fee_out }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); bool res = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); @@ -1011,11 +1017,11 @@ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPo } bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool bypass_limits, const CAmount nAbsurdFee, bool test_accept) + bool bypass_limits, const CAmount nAbsurdFee, bool test_accept, CAmount* fee_out) { const CChainParams& chainparams = Params(); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); - return AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, GetTime(), bypass_limits, nAbsurdFee, test_accept); + return AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, GetTime(), bypass_limits, nAbsurdFee, test_accept, fee_out); } bool GetTimestampIndex(const unsigned int &high, const unsigned int &low, std::vector &hashes) diff --git a/src/validation.h b/src/validation.h index ec003974ffce..cf19f71593a8 100644 --- a/src/validation.h +++ b/src/validation.h @@ -221,10 +221,12 @@ void UnlinkPrunedFiles(const std::set& setFilesToPrune); /** Prune block files up to a given height */ void PruneBlockFilesManual(CChainState& active_chainstate, int nManualPruneHeight); -/** (try to) add transaction to memory pool */ +/** (try to) add transaction to memory pool + * plTxnReplaced will be appended to with all transactions replaced from mempool + * @param[out] fee_out optional argument to return tx fee to the caller **/ bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, bool bypass_limits, - const CAmount nAbsurdFee, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const CAmount nAbsurdFee, bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool GetUTXOCoin(const COutPoint& outpoint, Coin& coin); int GetUTXOHeight(const COutPoint& outpoint); diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index 71d87233676c..d93cf74494f0 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -272,7 +272,7 @@ def run_test(self): asset_lock_tx = self.create_assetlock(coin, locked_1, pubkey) - self.check_mempool_result(tx=asset_lock_tx, result_expected={'allowed': True}) + self.check_mempool_result(tx=asset_lock_tx, result_expected={'allowed': True, 'vsize': asset_lock_tx.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) self.validate_credit_pool_balance(0) txid_in_block = self.send_tx(asset_lock_tx) assert "assetLockTx" in node.getrawtransaction(txid_in_block, 1) @@ -331,7 +331,7 @@ def run_test(self): asset_unlock_tx_duplicate_index.vout[0].nValue += COIN too_late_height = node.getblockcount() + 48 - self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True}) + self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'vsize': asset_unlock_tx.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) self.check_mempool_result(tx=asset_unlock_tx_too_big_fee, result_expected={'allowed': False, 'reject-reason' : 'absurdly-high-fee'}) self.check_mempool_result(tx=asset_unlock_tx_zero_fee, @@ -374,7 +374,7 @@ def run_test(self): self.mine_quorum(llmq_type_name="llmq_test_platform", llmq_type=106) self.log.info("Checking credit pool amount is same...") self.validate_credit_pool_balance(locked_1 - 1 * COIN) - self.check_mempool_result(tx=asset_unlock_tx_late, result_expected={'allowed': True}) + self.check_mempool_result(tx=asset_unlock_tx_late, result_expected={'allowed': True, 'vsize': asset_unlock_tx_late.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) self.log.info("Checking credit pool amount still is same...") self.validate_credit_pool_balance(locked_1 - 1 * COIN) self.send_tx(asset_unlock_tx_late) @@ -384,7 +384,7 @@ def run_test(self): self.log.info("Generating many blocks to make quorum far behind (even still active)...") self.slowly_generate_batch(too_late_height - node.getblockcount() - 1) - self.check_mempool_result(tx=asset_unlock_tx_too_late, result_expected={'allowed': True}) + self.check_mempool_result(tx=asset_unlock_tx_too_late, result_expected={'allowed': True, 'vsize': asset_unlock_tx_too_late.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) node.generate(1) self.sync_all() self.check_mempool_result(tx=asset_unlock_tx_too_late, @@ -421,7 +421,7 @@ def run_test(self): self.log.info("Checking that transaction with exceeding amount accepted by mempool...") # Mempool doesn't know about the size of the credit pool - self.check_mempool_result(tx=asset_unlock_tx_full, result_expected={'allowed': True }) + self.check_mempool_result(tx=asset_unlock_tx_full, result_expected={'allowed': True, 'vsize': asset_unlock_tx_full.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) txid_in_block = self.send_tx(asset_unlock_tx_full) node.generate(1) @@ -434,7 +434,7 @@ def run_test(self): self.mempool_size += 1 asset_unlock_tx_full = self.create_assetunlock(301, self.get_credit_pool_balance(), pubkey) - self.check_mempool_result(tx=asset_unlock_tx_full, result_expected={'allowed': True }) + self.check_mempool_result(tx=asset_unlock_tx_full, result_expected={'allowed': True, 'vsize': asset_unlock_tx_full.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) txid_in_block = self.send_tx(asset_unlock_tx_full) node.generate(1) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index b7493a4e239a..386f14d44e9a 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -93,20 +93,22 @@ def run_test(self): tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0))) txid_0 = tx.rehash() self.check_mempool_result( - result_expected=[{'txid': txid_0, 'allowed': True}], + result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee))}}], rawtxs=[raw_tx_0], ) self.log.info('A final transaction not in the mempool') coin = coins.pop() # Pick a random coin(base) to spend + output_amount = 0.025 raw_tx_final = node.signrawtransactionwithwallet(node.createrawtransaction( inputs=[{'txid': coin['txid'], 'vout': coin['vout'], "sequence": 0xffffffff}], # SEQUENCE_FINAL - outputs=[{node.getnewaddress(): 0.025}], + outputs=[{node.getnewaddress(): output_amount}], locktime=node.getblockcount() + 2000, # Can be anything ))['hex'] tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final))) + fee_expected = int(coin['amount']) - output_amount self.check_mempool_result( - result_expected=[{'txid': tx.rehash(), 'allowed': True}], + result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee_expected))}}], rawtxs=[tx.serialize().hex()], maxfeerate=0, ) @@ -190,7 +192,7 @@ def run_test(self): tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference))) # Reference tx should be valid on itself self.check_mempool_result( - result_expected=[{'txid': tx.rehash(), 'allowed': True}], + result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal(str(0.1 - 0.05))}}], rawtxs=[tx.serialize().hex()], maxfeerate=0, ) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index d5f45cae98f2..ddf3c292326a 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -508,6 +508,11 @@ def is_valid(self): return False return True + # Calculate the virtual transaction size using + # serialization size (does NOT use sigops). + def get_vsize(self): + return len(self.serialize()) + def __repr__(self): return "CTransaction(nVersion=%i vin=%s vout=%s nLockTime=%i)" \ % (self.nVersion, repr(self.vin), repr(self.vout), self.nLockTime) From cbbf6089e5ebe24aa2a12c71388be5d0e69a70db Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 23 Jan 2024 10:04:27 +0000 Subject: [PATCH 02/12] merge bitcoin#19498: Tidy up ProcessOrphanTx Co-authored-by: Konstantin Akimov --- src/net_processing.cpp | 62 ++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 06185e0bef39..253de2727722 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 { @@ -2533,13 +2544,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 +2565,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, + TxValidationState state; + + if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, porphanTx, false /* bypass_limits */, 0 /* nAbsurdFee */)) { 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 +2580,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, From 4acad29789162dbed94e026cc512e2f00a16fe57 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 25 Jan 2024 06:53:18 +0000 Subject: [PATCH 03/12] merge bitcoin#19339: re-delegate absurd fee checking from mempool to clients --- src/bench/block_assemble.cpp | 2 +- src/coinjoin/coinjoin.cpp | 21 ++++++++++++- src/coinjoin/coinjoin.h | 6 ++++ src/coinjoin/server.cpp | 6 ++-- src/llmq/ehf_signals.cpp | 2 +- src/net_processing.cpp | 4 +-- src/node/transaction.cpp | 38 ++++++++++++++++------- src/rpc/rawtransaction.cpp | 12 +++++-- src/test/txvalidation_tests.cpp | 3 +- src/test/txvalidationcache_tests.cpp | 2 +- src/test/validation_block_tests.cpp | 3 +- src/util/error.cpp | 2 +- src/validation.cpp | 18 ++++------- src/validation.h | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- test/functional/feature_asset_locks.py | 2 +- test/functional/rpc_fundrawtransaction.py | 2 +- test/functional/rpc_psbt.py | 2 +- test/functional/rpc_rawtransaction.py | 8 ++--- test/functional/wallet_create_tx.py | 8 ++--- 20 files changed, 92 insertions(+), 53 deletions(-) diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index d0307a094a47..3427981377cc 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -48,7 +48,7 @@ static void AssembleBlock(benchmark::Bench& bench) 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)}; + bool ret{::AcceptToMemoryPool(test_setup.m_node.chainman->ActiveChainstate(), *test_setup.m_node.mempool, state, txr, false /* bypass_limits */)}; assert(ret); } } diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index 81b5c46a9244..80af5f2376ab 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, TxValidationState &state, const CTransactionRef &tx, bool test_accept) { + AssertLockHeld(cs_main); + + CAmount fee{0}; + if (!AcceptToMemoryPool(active_chainstate, pool, state, tx, /* bypass_limits */ false, /* test_accept */ true, &fee)) { + /* Fetch fee and fast-fail if ATMP fails regardless */ + return false; + } else if (fee > 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, state, tx, /* bypass_limits */ false, test_accept); +} + // check to make sure the collateral provided by the client is valid bool CoinJoin::IsCollateralValid(CTxMemPool& mempool, const CTransaction& txCollateral) { @@ -352,7 +371,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, validationState, 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..6c0f01b32000 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, TxValidationState &state, + 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..6ca9f433d59d 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 {}; @@ -321,7 +319,7 @@ void CCoinJoinServer::CommitFinalTransaction() 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, validationState, 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 @@ -454,7 +452,7 @@ 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, validationState, 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..190ce7411da0 100644 --- a/src/llmq/ehf_signals.cpp +++ b/src/llmq/ehf_signals.cpp @@ -132,7 +132,7 @@ void CEHFSignalsHandler::HandleNewRecoveredSig(const CRecoveredSig& recoveredSig 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)) { + if (AcceptToMemoryPool(chainstate, mempool, state, tx_to_sent, /* bypass_limits=*/ false)) { connman.RelayTransaction(*tx_to_sent); } else { LogPrintf("CEHFSignalsHandler::HandleNewRecoveredSig -- AcceptToMemoryPool failed: %s\n", state.ToString()); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 253de2727722..487f88d414f0 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2568,7 +2568,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) TxValidationState state; if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, porphanTx, - false /* bypass_limits */, 0 /* nAbsurdFee */)) { + false /* bypass_limits */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(porphanTx->GetHash()); for (unsigned int i = 0; i < porphanTx->vout.size(); i++) { @@ -3629,7 +3629,7 @@ void PeerManagerImpl::ProcessMessage( TxValidationState state; if (!AlreadyHave(inv) && AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, ptx, - false /* bypass_limits */, 0 /* nAbsurdFee */)) { + false /* bypass_limits */)) { // Process custom txes, this changes AlreadyHave to "true" if (nInvType == MSG_DSTX) { LogPrint(BCLog::COINJOIN, "DSTX -- Masternode transaction accepted, txid=%s, peer=%d\n", diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 8685f48d4460..1421c3baac9a 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. + // Transaction is not already in the mempool. 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; + CAmount fee{0}; + if (max_tx_fee) { + // First, call ATMP with test_accept and check the fee. If ATMP + // fails here, return error immediately. + if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, state, tx, + bypass_limits, /* test_accept */ true, &fee)) { + return HandleATMPError(state, err_string); + } else if (fee > max_tx_fee) { + return TransactionError::MAX_FEE_EXCEEDED; } } + // Try to submit the transaction to the mempool. + if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, state, tx, + bypass_limits)) { + return HandleATMPError(state, err_string); + } // Transaction was accepted to the mempool. diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 8943b4fdd88c..df25b9f04d29 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1227,12 +1227,20 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) TxValidationState state; bool test_accept_res; - CAmount fee; + CAmount fee{0}; { 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, &fee); + false /* bypass_limits */, /* test_accept */ true, &fee); + } + + // Check that fee does not exceed maximum fee + if (test_accept_res && max_raw_tx_fee && fee > max_raw_tx_fee) { + result_0.pushKV("allowed", false); + result_0.pushKV("reject-reason", "max-fee-exceeded"); + result.push_back(std::move(result_0)); + return result; } result_0.pushKV("allowed", test_accept_res); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 7a7784efb080..5b5df96ec5e0 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -39,8 +39,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) BOOST_CHECK_EQUAL( false, AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, state, MakeTransactionRef(coinbaseTx), - true /* bypass_limits */, - 0 /* nAbsurdFee */)); + true /* bypass_limits */)); // Check that the transaction hasn't been added to mempool. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 3d33725ee5f5..5ebb74f48d69 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -29,7 +29,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) TxValidationState validationState; return AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, validationState, MakeTransactionRef(tx), - true /* bypass_limits */, 0 /* nAbsurdFee */); + true /* bypass_limits */); }; // Create a double-spend of mature coinbase txn: diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 9f6a56c15b87..f9d4fc6e65b3 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -301,8 +301,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) *m_node.mempool, state, tx, - /* bypass_limits */ false, - /* nAbsurdFee */ 0)); + /* bypass_limits */ false)); } } diff --git a/src/util/error.cpp b/src/util/error.cpp index 3e29083712ea..6c94b806837a 100644 --- a/src/util/error.cpp +++ b/src/util/error.cpp @@ -30,7 +30,7 @@ bilingual_str TransactionErrorString(const TransactionError err) case TransactionError::SIGHASH_MISMATCH: return Untranslated("Specified sighash value does not match value stored in PSBT"); case TransactionError::MAX_FEE_EXCEEDED: - return Untranslated("Fee exceeds maximum configured by -maxtxfee"); + return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)"); // no default case, so the compiler can warn about missing cases } assert(false); diff --git a/src/validation.cpp b/src/validation.cpp index 8e814bbe00d0..23ed280c9a26 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -460,7 +460,7 @@ void CChainState::MaybeUpdateMempoolForReorg( TxValidationState stateDummy; if (!fAddToMempool || (*it)->IsCoinBase() || !AcceptToMemoryPool(*this, *m_mempool, stateDummy, *it, - true /* bypass_limits */, 0 /* nAbsurdFee */)) { + true /* bypass_limits */)) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); @@ -548,7 +548,6 @@ class MemPoolAccept TxValidationState &m_state; const int64_t m_accept_time; const bool m_bypass_limits; - const CAmount& m_absurd_fee; /* * Return any outpoints which were not previously present in the coins * cache, but were added as a result of validating the tx for mempool @@ -640,7 +639,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) TxValidationState &state = args.m_state; const int64_t nAcceptTime = args.m_accept_time; const bool bypass_limits = args.m_bypass_limits; - const CAmount& nAbsurdFee = args.m_absurd_fee; std::vector& coins_to_uncache = args.m_coins_to_uncache; // Alias what we need out of ws @@ -809,10 +807,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (!bypass_limits && !CheckFeeRate(nSize, nModifiedFees, state)) return false; } - if (nAbsurdFee && nFees > nAbsurdFee) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, - "absurdly-high-fee", strprintf("%d > %d", nFees, nAbsurdFee)); - // Calculate in-mempool ancestors, up to a limit. std::string errString; if (!m_pool.CalculateMemPoolAncestors(*entry, setAncestors, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, m_limit_descendant_size, errString)) { @@ -992,10 +986,10 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs /** (try to) add transaction to memory pool with a specified acceptance time **/ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CChainState& active_chainstate, TxValidationState &state, const CTransactionRef &tx, int64_t nAcceptTime, bool bypass_limits, - const CAmount nAbsurdFee, bool test_accept, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) + bool test_accept, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::vector coins_to_uncache; - MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept, fee_out }; + MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, bypass_limits, coins_to_uncache, test_accept, fee_out }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); bool res = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); @@ -1017,11 +1011,11 @@ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPo } bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool bypass_limits, const CAmount nAbsurdFee, bool test_accept, CAmount* fee_out) + bool bypass_limits, bool test_accept, CAmount* fee_out) { const CChainParams& chainparams = Params(); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); - return AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, GetTime(), bypass_limits, nAbsurdFee, test_accept, fee_out); + return AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, GetTime(), bypass_limits, test_accept, fee_out); } bool GetTimestampIndex(const unsigned int &high, const unsigned int &low, std::vector &hashes) @@ -5511,7 +5505,7 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka LOCK(cs_main); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, nTime, - false /* bypass_limits */, 0 /* nAbsurdFee */, false /* test_accept */); + false /* bypass_limits */, false /* test_accept */); if (state.IsValid()) { ++count; } else { diff --git a/src/validation.h b/src/validation.h index cf19f71593a8..74923705f51e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -226,7 +226,7 @@ void PruneBlockFilesManual(CChainState& active_chainstate, int nManualPruneHeigh * @param[out] fee_out optional argument to return tx fee to the caller **/ bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, bool bypass_limits, - const CAmount nAbsurdFee, bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool GetUTXOCoin(const COutPoint& outpoint, Coin& coin); int GetUTXOHeight(const COutPoint& outpoint); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 8639db32cfbe..8b926e25d9f8 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -705,7 +705,7 @@ class CreateTransactionTestSetup : public TestChain100Setup const std::string strTransactionTooLarge = "Transaction too large"; const std::string strChangeIndexOutOfRange = "Change index out of range"; const std::string strExceededMaxTries = "Exceeded max tries."; - const std::string strMaxFeeExceeded = "Fee exceeds maximum configured by -maxtxfee"; + const std::string strMaxFeeExceeded = "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)"; CreateTransactionTestSetup() { diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index d93cf74494f0..c4d4f6491a76 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -333,7 +333,7 @@ def run_test(self): self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'vsize': asset_unlock_tx.get_vsize(), 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) self.check_mempool_result(tx=asset_unlock_tx_too_big_fee, - result_expected={'allowed': False, 'reject-reason' : 'absurdly-high-fee'}) + result_expected={'allowed': False, 'reject-reason' : 'max-fee-exceeded'}) self.check_mempool_result(tx=asset_unlock_tx_zero_fee, result_expected={'allowed': False, 'reject-reason' : 'bad-txns-assetunlock-fee-outofrange'}) # not-verified is a correct faiure from mempool. Mempool knows nothing about CreditPool indexes and he just report that signature is not validated diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index d6371b754baa..44d85e5e44fd 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -652,7 +652,7 @@ def test_option_feerate(self): result = self.nodes[3].fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) result3 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 10 * self.min_relay_tx_fee}) - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[3].fundrawtransaction, rawtx, {"feeRate": 1}) + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[3].fundrawtransaction, rawtx, {"feeRate": 1}) result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 7c9cba8936a4..42a93f034fef 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -95,7 +95,7 @@ def run_test(self): # feeRate of 10 DASH / KB produces a total fee well above -maxtxfee # previously this was silently capped at -maxtxfee - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10}) + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10}) # partially sign multisig things with node 1 psbtx = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2sh_pos}], {self.nodes[1].getnewaddress():9.99})['psbt'] diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index 2d4f243bc066..11aa9a7782ae 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -396,9 +396,9 @@ def run_test(self): # Thus, testmempoolaccept should reject testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']], 0.00001000)[0] assert_equal(testres['allowed'], False) - assert_equal(testres['reject-reason'], 'absurdly-high-fee') + assert_equal(testres['reject-reason'], 'max-fee-exceeded') # and sendrawtransaction should throw - assert_raises_rpc_error(-26, "absurdly-high-fee", self.nodes[2].sendrawtransaction, rawTxSigned['hex'], 0.00001000) + assert_raises_rpc_error(-25, 'Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)', self.nodes[2].sendrawtransaction, rawTxSigned['hex'], 0.00001000) # and the following calls should both succeed testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']])[0] assert_equal(testres['allowed'], True) @@ -420,9 +420,9 @@ def run_test(self): # Thus, testmempoolaccept should reject testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']])[0] assert_equal(testres['allowed'], False) - assert_equal(testres['reject-reason'], 'absurdly-high-fee') + assert_equal(testres['reject-reason'], 'max-fee-exceeded') # and sendrawtransaction should throw - assert_raises_rpc_error(-26, "absurdly-high-fee", self.nodes[2].sendrawtransaction, rawTxSigned['hex']) + assert_raises_rpc_error(-25, 'Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)', self.nodes[2].sendrawtransaction, rawTxSigned['hex']) # and the following calls should both succeed testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']], maxfeerate='0.20000000')[0] assert_equal(testres['allowed'], True) diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py index 5293c8aeeb38..3e8418f95224 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -45,12 +45,12 @@ def test_tx_size_too_large(self): self.restart_node(0, extra_args=[fee_setting]) assert_raises_rpc_error( -6, - "Fee exceeds maximum configured by -maxtxfee", + "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", lambda: self.nodes[0].sendmany(dummy="", amounts=outputs), ) assert_raises_rpc_error( -4, - "Fee exceeds maximum configured by -maxtxfee", + "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", lambda: self.nodes[0].fundrawtransaction(hexstring=raw_tx), ) @@ -59,12 +59,12 @@ def test_tx_size_too_large(self): self.nodes[0].settxfee(0.01) assert_raises_rpc_error( -6, - "Fee exceeds maximum configured by -maxtxfee", + "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", lambda: self.nodes[0].sendmany(dummy="", amounts=outputs), ) assert_raises_rpc_error( -4, - "Fee exceeds maximum configured by -maxtxfee", + "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", lambda: self.nodes[0].fundrawtransaction(hexstring=raw_tx), ) self.nodes[0].settxfee(0) From 2d6462030691d67d5e6dc0fbe24bb51b9ced99d1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 24 Jan 2024 22:29:59 +0300 Subject: [PATCH 04/12] merge bitcoin#19753: don't add AlreadyHave transactions to recentRejects Co-authored-by: UdjinM6 --- src/net_processing.cpp | 32 ++++++++++++++++-------------- test/functional/p2p_permissions.py | 11 +++++++++- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 487f88d414f0..5195a0d18af3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3626,10 +3626,25 @@ void PeerManagerImpl::ProcessMessage( LOCK2(cs_main, g_cs_orphans); + 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; + } + TxValidationState state; - if (!AlreadyHave(inv) && AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, ptx, - false /* bypass_limits */)) { + if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, ptx, false /* bypass_limits */)) { // Process custom txes, this changes AlreadyHave to "true" if (nInvType == MSG_DSTX) { LogPrint(BCLog::COINJOIN, "DSTX -- Masternode transaction accepted, txid=%s, peer=%d\n", @@ -3700,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 diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 3847c2d08ce7..d2801db92b52 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -143,11 +143,20 @@ def in_mempool(): self.log.debug("Check that node[1] will not send an invalid tx to node[0]") tx.vout[0].nValue += 1 txid = tx.rehash() + # Send the transaction twice. The first time, it'll be rejected by ATMP because it conflicts + # with a mempool transaction. The second time, it'll be in the recentRejects filter. p2p_rebroadcast_wallet.send_txs_and_test( [tx], self.nodes[1], success=False, - reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid), + reject_reason='{} from peer=0 was not accepted: txn-mempool-conflict'.format(txid) + ) + + p2p_rebroadcast_wallet.send_txs_and_test( + [tx], + self.nodes[1], + success=False, + reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid) ) def checkpermission(self, args, expectedPermissions, whitelisted): From 7f535b16fabf7b3346d4847c2decc046cb8872db Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 6 Dec 2020 00:14:17 +0000 Subject: [PATCH 05/12] merge bitcoin#20581: Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const continuation from 40c270030ed2288109571ebba2b917bc3ab643fc in dash#4919 --- src/validation.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 23ed280c9a26..c7fa6abd0853 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -585,13 +585,13 @@ class MemPoolAccept // Run the script checks using our policy flags. As this can be slow, we should // only invoke this on transactions that have otherwise passed policy checks. - bool PolicyScriptChecks(ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Re-run the script checks, using consensus flags, and try to cache the // result in the scriptcache. This should be done after // PolicyScriptChecks(). This requires that all inputs either be in our // utxo set or in the mempool. - bool ConsensusScriptChecks(ATMPArgs& args, Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Try to add the transaction to the mempool, removing any conflicts first. // Returns true if the transaction is in the mempool after any size @@ -843,7 +843,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) +bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) { const CTransaction& tx = *ws.m_ptx; @@ -860,7 +860,7 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute return true; } -bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) +bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) { const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; From 5efc93092518eeec19b570400a7a8e990d76c78f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 16 Jan 2024 15:30:11 +0000 Subject: [PATCH 06/12] merge bitcoin#20834: locks and docs in ATMP and CheckInputsFromMempoolAndCache --- src/validation.cpp | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c7fa6abd0853..c264acc0c12c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -487,30 +487,32 @@ void CChainState::MaybeUpdateMempoolForReorg( std::chrono::hours{gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); } -// Used to avoid mempool polluting consensus critical paths if CCoinsViewMempool -// were somehow broken and returning the wrong scriptPubKeys +/** +* Checks to avoid mempool polluting consensus critical paths since cached +* signature and script validity results will be reused if we validate this +* transaction again during block validation. +* */ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& view, const CTxMemPool& pool, - unsigned int flags, PrecomputedTransactionData& txdata, CCoinsViewCache& coins_tip) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) + const CCoinsViewCache& view, const CTxMemPool& pool, + unsigned int flags, PrecomputedTransactionData& txdata, CCoinsViewCache& coins_tip) + EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { AssertLockHeld(cs_main); - - // pool.cs should be locked already, but go ahead and re-take the lock here - // to enforce that mempool doesn't change between when we check the view - // and when we actually call through to CheckInputScripts - LOCK(pool.cs); + AssertLockHeld(pool.cs); assert(!tx.IsCoinBase()); for (const CTxIn& txin : tx.vin) { const Coin& coin = view.AccessCoin(txin.prevout); - // AcceptToMemoryPoolWorker has already checked that the coins are - // available, so this shouldn't fail. If the inputs are not available - // here then return false. + // This coin was checked in PreChecks and MemPoolAccept + // has been holding cs_main since then. + Assume(!coin.IsSpent()); if (coin.IsSpent()) return false; - // Check equivalence for available inputs. + // If the Coin is available, there are 2 possibilities: + // it is available in our current ChainstateActive UTXO set, + // or it's a UTXO provided by a transaction in our mempool. + // Ensure the scriptPubKeys in Coins from CoinsView are correct. const CTransactionRef& txFrom = pool.get(txin.prevout.hash); if (txFrom) { assert(txFrom->GetHash() == txin.prevout.hash); @@ -518,9 +520,9 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS assert(txFrom->vout[txin.prevout.n] == coin.out); } else { assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(coins_tip)); - const Coin& coinFromDisk = coins_tip.AccessCoin(txin.prevout); - assert(!coinFromDisk.IsSpent()); - assert(coinFromDisk.out == coin.out); + const Coin& coinFromUTXOSet = coins_tip.AccessCoin(txin.prevout); + assert(!coinFromUTXOSet.IsSpent()); + assert(coinFromUTXOSet.out == coin.out); } } @@ -585,13 +587,13 @@ class MemPoolAccept // Run the script checks using our policy flags. As this can be slow, we should // only invoke this on transactions that have otherwise passed policy checks. - bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Re-run the script checks, using consensus flags, and try to cache the // result in the scriptcache. This should be done after // PolicyScriptChecks(). This requires that all inputs either be in our // utxo set or in the mempool. - bool ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Try to add the transaction to the mempool, removing any conflicts first. // Returns true if the transaction is in the mempool after any size @@ -599,7 +601,7 @@ class MemPoolAccept bool Finalize(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Compare a package's feerate against minimum allowed. - bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state) + bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs) { CAmount mempoolRejectFee = m_pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size); if (mempoolRejectFee > 0 && package_fee < mempoolRejectFee) { From 6238ed2c6ec8c60d05e4f777e328e68dcd457bc2 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 1 Feb 2024 19:18:49 +0000 Subject: [PATCH 07/12] merge bitcoin#21062: return MempoolAcceptResult from ATMP --- src/bench/block_assemble.cpp | 5 +- src/coinjoin/coinjoin.cpp | 13 ++-- src/coinjoin/coinjoin.h | 2 +- src/coinjoin/server.cpp | 6 +- src/llmq/ehf_signals.cpp | 6 +- src/net_processing.cpp | 11 ++-- src/node/transaction.cpp | 20 +++--- src/rpc/rawtransaction.cpp | 53 +++++++--------- src/test/txvalidation_tests.cpp | 14 ++--- src/test/txvalidationcache_tests.cpp | 5 +- src/test/validation_block_tests.cpp | 9 +-- src/validation.cpp | 94 +++++++++++++--------------- src/validation.h | 44 +++++++++++-- 13 files changed, 142 insertions(+), 140 deletions(-) diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index 3427981377cc..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 */)}; - 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 80af5f2376ab..3a9c59f2ab4d 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/src/coinjoin/coinjoin.cpp @@ -308,21 +308,21 @@ bool CCoinJoinBaseSession::IsValidInOuts(const CTxMemPool& mempool, const std::v // 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, TxValidationState &state, const CTransactionRef &tx, bool test_accept) { +bool ATMPIfSaneFee(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef &tx, bool test_accept) { AssertLockHeld(cs_main); - CAmount fee{0}; - if (!AcceptToMemoryPool(active_chainstate, pool, state, tx, /* bypass_limits */ false, /* test_accept */ true, &fee)) { + 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 (fee > DEFAULT_MAX_RAW_TX_FEE) { + } 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, state, tx, /* bypass_limits */ false, test_accept); + 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 @@ -370,8 +370,7 @@ bool CoinJoin::IsCollateralValid(CTxMemPool& mempool, const CTransaction& txColl { LOCK(cs_main); - TxValidationState validationState; - if (!ATMPIfSaneFee(::ChainstateActive(), mempool, validationState, MakeTransactionRef(txCollateral), /* 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 6c0f01b32000..747997258cc6 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -381,7 +381,7 @@ class CDSTXManager }; -bool ATMPIfSaneFee(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, +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 6ca9f433d59d..0322eee4eff1 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -317,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 || !ATMPIfSaneFee(m_chainstate, mempool, validationState, finalTransaction)) { + 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 @@ -451,8 +450,7 @@ void CCoinJoinServer::ChargeRandomFees() const void CCoinJoinServer::ConsumeCollateral(const CTransactionRef& txref) const { LOCK(cs_main); - TxValidationState validationState; - if (!ATMPIfSaneFee(m_chainstate, mempool, validationState, txref, false /* bypass_limits */)) { + 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 190ce7411da0..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)) { + 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/net_processing.cpp b/src/net_processing.cpp index 5195a0d18af3..08ff51632233 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2565,10 +2565,10 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) if (orphan_it == mapOrphanTransactions.end()) continue; const CTransactionRef porphanTx = orphan_it->second.tx; - TxValidationState state; + const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, porphanTx, false /* bypass_limits */); + const TxValidationState& state = result.m_state; - if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, porphanTx, - false /* bypass_limits */)) { + if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(porphanTx->GetHash()); for (unsigned int i = 0; i < porphanTx->vout.size(); i++) { @@ -3642,9 +3642,10 @@ void PeerManagerImpl::ProcessMessage( return; } - TxValidationState state; + const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, ptx, false /* bypass_limits */); + const TxValidationState& state = result.m_state; - if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, state, ptx, false /* bypass_limits */)) { + 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", diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 1421c3baac9a..4461b405c266 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -54,22 +54,22 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } if (!node.mempool->exists(hashTx)) { // Transaction is not already in the mempool. - TxValidationState state; - CAmount fee{0}; - if (max_tx_fee) { + if (max_tx_fee > 0) { // First, call ATMP with test_accept and check the fee. If ATMP // fails here, return error immediately. - if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, state, tx, - bypass_limits, /* test_accept */ true, &fee)) { - return HandleATMPError(state, err_string); - } else if (fee > max_tx_fee) { + 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. - if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, state, tx, - bypass_limits)) { - return HandleATMPError(state, err_string); + 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/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index df25b9f04d29..fde9c1169c2c 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1225,45 +1225,36 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) UniValue result_0(UniValue::VOBJ); result_0.pushKV("txid", tx_hash.GetHex()); - TxValidationState state; - bool test_accept_res; - CAmount fee{0}; - { - ChainstateManager& chainman = EnsureChainman(node); - LOCK(cs_main); - test_accept_res = AcceptToMemoryPool(chainman.ActiveChainstate(), mempool, state, std::move(tx), - false /* bypass_limits */, /* test_accept */ true, &fee); - } - - // Check that fee does not exceed maximum fee - if (test_accept_res && max_raw_tx_fee && fee > max_raw_tx_fee) { - result_0.pushKV("allowed", false); - result_0.pushKV("reject-reason", "max-fee-exceeded"); - result.push_back(std::move(result_0)); - return result; - } - result_0.pushKV("allowed", test_accept_res); + ChainstateManager& chainman = EnsureChainman(node); + const MempoolAcceptResult accept_result = WITH_LOCK(cs_main, return AcceptToMemoryPool(chainman.ActiveChainstate(), mempool, std::move(tx), + false /* bypass_limits */, true /* test_accept */)); // Only return the fee and vsize if the transaction would pass ATMP. // These can be used to calculate the feerate. - if (test_accept_res) { - result_0.pushKV("vsize", virtual_size); - UniValue fees(UniValue::VOBJ); - fees.pushKV("base", ValueFromAmount(fee)); - result_0.pushKV("fees", fees); + if (accept_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + const CAmount fee = accept_result.m_base_fees.value(); + // Check that fee does not exceed maximum fee + if (max_raw_tx_fee && fee > max_raw_tx_fee) { + result_0.pushKV("allowed", false); + result_0.pushKV("reject-reason", "max-fee-exceeded"); + } else { + result_0.pushKV("allowed", true); + result_0.pushKV("vsize", virtual_size); + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(fee)); + result_0.pushKV("fees", fees); + } + result.push_back(std::move(result_0)); } else { - if (state.IsInvalid()) { - if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { - result_0.pushKV("reject-reason", "missing-inputs"); - } else { - result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason())); - } + result_0.pushKV("allowed", false); + const TxValidationState state = accept_result.m_state; + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { + result_0.pushKV("reject-reason", "missing-inputs"); } else { result_0.pushKV("reject-reason", state.GetRejectReason()); } + result.push_back(std::move(result_0)); } - - result.push_back(std::move(result_0)); return result; } diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 5b5df96ec5e0..326ec4466e1b 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -30,24 +30,20 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) BOOST_CHECK(CTransaction(coinbaseTx).IsCoinBase()); - TxValidationState state; - LOCK(cs_main); unsigned int initialPoolSize = m_node.mempool->size(); + const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, MakeTransactionRef(coinbaseTx), true /* bypass_limits */); - BOOST_CHECK_EQUAL( - false, - AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, state, MakeTransactionRef(coinbaseTx), - true /* bypass_limits */)); + BOOST_CHECK(result.m_result_type == MempoolAcceptResult::ResultType::INVALID); // Check that the transaction hasn't been added to mempool. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); // Check that the validation state reflects the unsuccessful attempt. - BOOST_CHECK(state.IsInvalid()); - BOOST_CHECK_EQUAL(state.GetRejectReason(), "coinbase"); - BOOST_CHECK(state.GetResult() == TxValidationResult::TX_CONSENSUS); + BOOST_CHECK(result.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "coinbase"); + BOOST_CHECK(result.m_state.GetResult() == TxValidationResult::TX_CONSENSUS); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 5ebb74f48d69..63fa0782a30e 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -27,9 +27,8 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) const auto ToMemPool = [this](const CMutableTransaction& tx) { LOCK(cs_main); - TxValidationState validationState; - return AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, validationState, MakeTransactionRef(tx), - true /* bypass_limits */); + const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, MakeTransactionRef(tx), true /* bypass_limits */); + return result.m_result_type == MempoolAcceptResult::ResultType::VALID; }; // Create a double-spend of mature coinbase txn: diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index f9d4fc6e65b3..e7938e922ecc 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -294,14 +294,9 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) // Add the txs to the tx pool { LOCK(cs_main); - TxValidationState state; for (const auto& tx : txs) { - BOOST_REQUIRE(AcceptToMemoryPool( - ::ChainstateActive(), - *m_node.mempool, - state, - tx, - /* bypass_limits */ false)); + const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, tx, false /* bypass_limits */); + BOOST_REQUIRE(result.m_result_type == MempoolAcceptResult::ResultType::VALID); } } diff --git a/src/validation.cpp b/src/validation.cpp index c264acc0c12c..64e86625915b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -457,10 +457,8 @@ void CChainState::MaybeUpdateMempoolForReorg( auto it = disconnectpool.queuedTx.get().rbegin(); while (it != disconnectpool.queuedTx.get().rend()) { // ignore validation errors in resurrected transactions - TxValidationState stateDummy; if (!fAddToMempool || (*it)->IsCoinBase() || - !AcceptToMemoryPool(*this, *m_mempool, stateDummy, *it, - true /* bypass_limits */)) { + AcceptToMemoryPool(*this, *m_mempool, *it, true /* bypass_limits */).m_result_type != MempoolAcceptResult::ResultType::VALID) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); @@ -547,7 +545,6 @@ class MemPoolAccept // around easier. struct ATMPArgs { const CChainParams& m_chainparams; - TxValidationState &m_state; const int64_t m_accept_time; const bool m_bypass_limits; /* @@ -559,11 +556,10 @@ class MemPoolAccept */ std::vector& m_coins_to_uncache; const bool m_test_accept; - CAmount* m_fee_out; }; // Single transaction acceptance - bool AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); private: // All the intermediate state that gets passed between the various levels @@ -573,10 +569,12 @@ class MemPoolAccept CTxMemPool::setEntries m_ancestors; std::unique_ptr m_entry; + CAmount m_base_fees; CAmount m_modified_fees; const CTransactionRef& m_ptx; const uint256& m_hash; + TxValidationState m_state; }; // Run the policy checks on a given transaction, excluding any script checks. @@ -587,18 +585,18 @@ class MemPoolAccept // Run the script checks using our policy flags. As this can be slow, we should // only invoke this on transactions that have otherwise passed policy checks. - bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Re-run the script checks, using consensus flags, and try to cache the // result in the scriptcache. This should be done after // PolicyScriptChecks(). This requires that all inputs either be in our // utxo set or in the mempool. - bool ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Try to add the transaction to the mempool, removing any conflicts first. // Returns true if the transaction is in the mempool after any size // limiting is performed, false otherwise. - bool Finalize(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool Finalize(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Compare a package's feerate against minimum allowed. bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs) @@ -638,12 +636,12 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Copy/alias what we need out of args const CChainParams& chainparams = args.m_chainparams; - TxValidationState &state = args.m_state; const int64_t nAcceptTime = args.m_accept_time; const bool bypass_limits = args.m_bypass_limits; std::vector& coins_to_uncache = args.m_coins_to_uncache; // Alias what we need out of ws + TxValidationState& state = ws.m_state; CTxMemPool::setEntries& setAncestors = ws.m_ancestors; std::unique_ptr& entry = ws.m_entry; CAmount& nModifiedFees = ws.m_modified_fees; @@ -760,16 +758,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final"); assert(std::addressof(g_chainman.m_blockman) == std::addressof(m_active_chainstate.m_blockman)); - CAmount nFees = 0; - if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_blockman.GetSpendHeight(m_view), nFees)) { + if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_blockman.GetSpendHeight(m_view), ws.m_base_fees)) { return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), state.ToString()); } - // If fee_out is passed, return the fee to the caller - if (args.m_fee_out) { - *args.m_fee_out = nFees; - } - // Check for non-standard pay-to-script-hash in inputs if (fRequireStandard && !AreInputsStandard(tx, m_view)) { return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs"); @@ -778,7 +770,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) unsigned int nSigOps = GetTransactionSigOpCount(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS); // nModifiedFees includes any fee deltas from PrioritiseTransaction - nModifiedFees = nFees; + nModifiedFees = ws.m_base_fees; m_pool.ApplyDelta(hash, nModifiedFees); // Keep track of transactions that spend a coinbase, which we re-scan @@ -793,7 +785,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } assert(std::addressof(::ChainActive()) == std::addressof(m_active_chainstate.m_chain)); - entry.reset(new CTxMemPoolEntry(ptx, nFees, nAcceptTime, m_active_chainstate.m_chain.Height(), + entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), fSpendsCoinbase, nSigOps, lp)); unsigned int nSize = entry->GetTxSize(); @@ -845,11 +837,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) +bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) { const CTransaction& tx = *ws.m_ptx; - - TxValidationState &state = args.m_state; + TxValidationState& state = ws.m_state; constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS; @@ -862,12 +853,11 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, Prec return true; } -bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) +bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) { const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; - - TxValidationState &state = args.m_state; + TxValidationState& state = ws.m_state; const CChainParams& chainparams = args.m_chainparams; // Check again against the current block tip's script verification @@ -895,11 +885,12 @@ bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, const Workspace& ws, P return true; } -bool MemPoolAccept::Finalize(ATMPArgs& args, Workspace& ws) + +bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) { const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; - TxValidationState &state = args.m_state; + TxValidationState& state = ws.m_state; const bool bypass_limits = args.m_bypass_limits; CTxMemPool::setEntries& setAncestors = ws.m_ancestors; @@ -944,15 +935,15 @@ bool MemPoolAccept::Finalize(ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) +MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) { auto start = Now(); AssertLockHeld(cs_main); LOCK(m_pool.cs); // mempool "read lock" (held through GetMainSignals().TransactionAddedToMempool()) - Workspace workspace(ptx); + Workspace ws(ptx); - if (!PreChecks(args, workspace)) return false; + if (!PreChecks(args, ws)) return MempoolAcceptResult(ws.m_state); // Only compute the precomputed transaction data if we need to verify // scripts (ie, other policy checks pass). We perform the inexpensive @@ -960,14 +951,16 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs // checks pass, to mitigate CPU exhaustion denial-of-service attacks. PrecomputedTransactionData txdata(*ptx); - if (!PolicyScriptChecks(args, workspace, txdata)) return false; + if (!PolicyScriptChecks(args, ws, txdata)) return MempoolAcceptResult(ws.m_state); - if (!ConsensusScriptChecks(args, workspace, txdata)) return false; + if (!ConsensusScriptChecks(args, ws, txdata)) return MempoolAcceptResult(ws.m_state); // Tx was accepted, but not added - if (args.m_test_accept) return true; + if (args.m_test_accept) { + return MempoolAcceptResult(ws.m_base_fees); + } - if (!Finalize(args, workspace)) return false; + if (!Finalize(args, ws)) return MempoolAcceptResult(ws.m_state); const int64_t nAcceptTime = args.m_accept_time; GetMainSignals().TransactionAddedToMempool(ptx, nAcceptTime); @@ -980,23 +973,26 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs statsClient.count("transactions.inputs", tx.vin.size(), 1.0f); statsClient.count("transactions.outputs", tx.vout.size(), 1.0f); - return true; + return MempoolAcceptResult(ws.m_base_fees); } } // anon namespace /** (try to) add transaction to memory pool with a specified acceptance time **/ -static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CChainState& active_chainstate, TxValidationState &state, const CTransactionRef &tx, - int64_t nAcceptTime, bool bypass_limits, - bool test_accept, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CChainState& active_chainstate, + const CTransactionRef &tx, int64_t nAcceptTime, + bool bypass_limits, bool test_accept) + EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::vector coins_to_uncache; - MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, bypass_limits, coins_to_uncache, test_accept, fee_out }; + MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); - bool res = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); - if (!res || test_accept) { - if (!res) LogPrint(BCLog::MEMPOOL, "%s: %s %s (%s)\n", __func__, tx->GetHash().ToString(), state.GetRejectReason(), state.GetDebugMessage()); + const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID || test_accept) { + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { + LogPrint(BCLog::MEMPOOL, "%s: %s %s (%s)\n", __func__, tx->GetHash().ToString(), result.m_state.GetRejectReason(), result.m_state.GetDebugMessage()); + } // Remove coins that were not present in the coins cache before calling; // AcceptSingleTransaction(); this is to prevent memory DoS in case we receive a large @@ -1009,15 +1005,13 @@ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPo // After we've (potentially) uncached entries, ensure our coins cache is still within its size limits BlockValidationState state_dummy; active_chainstate.FlushStateToDisk(state_dummy, FlushStateMode::PERIODIC); - return res; + return result; } -bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool bypass_limits, bool test_accept, CAmount* fee_out) +MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef &tx, bool bypass_limits, bool test_accept) { - const CChainParams& chainparams = Params(); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); - return AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, GetTime(), bypass_limits, test_accept, fee_out); + return AcceptToMemoryPoolWithTime(Params(), pool, active_chainstate, tx, GetTime(), bypass_limits, test_accept); } bool GetTimestampIndex(const unsigned int &high, const unsigned int &low, std::vector &hashes) @@ -5502,13 +5496,11 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka if (amountdelta) { pool.PrioritiseTransaction(tx->GetHash(), amountdelta); } - TxValidationState state; if (nTime > nNow - nExpiryTimeout) { LOCK(cs_main); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); - AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, state, tx, nTime, - false /* bypass_limits */, false /* test_accept */); - if (state.IsValid()) { + if (AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, tx, nTime, false /* bypass_limits */, + false /* test_accept */).m_result_type == MempoolAcceptResult::ResultType::VALID) { ++count; } else { // mempool may contain the transaction already, e.g. from diff --git a/src/validation.h b/src/validation.h index 74923705f51e..e544c1e89405 100644 --- a/src/validation.h +++ b/src/validation.h @@ -14,6 +14,7 @@ #include #include #include +#include #include // for ReadLE64 #include #include @@ -25,6 +26,7 @@ #include // For CTxMemPool::cs #include #include +#include #include #include @@ -221,12 +223,42 @@ void UnlinkPrunedFiles(const std::set& setFilesToPrune); /** Prune block files up to a given height */ void PruneBlockFilesManual(CChainState& active_chainstate, int nManualPruneHeight); -/** (try to) add transaction to memory pool - * plTxnReplaced will be appended to with all transactions replaced from mempool - * @param[out] fee_out optional argument to return tx fee to the caller **/ -bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool bypass_limits, - bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +/** +* Validation result for a single transaction mempool acceptance. +*/ +struct MempoolAcceptResult { + /** Used to indicate the results of mempool validation, + * including the possibility of unfinished validation. + */ + enum class ResultType { + VALID, //!> Fully validated, valid. + INVALID, //!> Invalid. + }; + ResultType m_result_type; + TxValidationState m_state; + + // The following fields are only present when m_result_type = ResultType::VALID + /** Raw base fees. */ + std::optional m_base_fees; + + /** Constructor for failure case */ + explicit MempoolAcceptResult(TxValidationState state) + : m_result_type(ResultType::INVALID), m_state(state), m_base_fees(std::nullopt) { + Assume(!state.IsValid()); // Can be invalid or error + } + + /** Constructor for success case */ + explicit MempoolAcceptResult(CAmount fees) + : m_result_type(ResultType::VALID), m_state(TxValidationState{}), m_base_fees(fees) {} +}; + +/** + * (Try to) add a transaction to the memory pool. + * @param[in] bypass_limits When true, don't enforce mempool fee limits. + * @param[in] test_accept When true, run validation checks but don't submit to mempool. + */ +MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx, + bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool GetUTXOCoin(const COutPoint& outpoint, Coin& coin); int GetUTXOHeight(const COutPoint& outpoint); From 0d76475dabb085768f6978229c06825988cb260f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 11 Feb 2021 06:23:04 -0800 Subject: [PATCH 08/12] merge bitcoin#21783: Make MempoolAcceptResult members const --- src/validation.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/validation.h b/src/validation.h index e544c1e89405..f96b665f2c35 100644 --- a/src/validation.h +++ b/src/validation.h @@ -234,12 +234,12 @@ struct MempoolAcceptResult { VALID, //!> Fully validated, valid. INVALID, //!> Invalid. }; - ResultType m_result_type; - TxValidationState m_state; + const ResultType m_result_type; + const TxValidationState m_state; // The following fields are only present when m_result_type = ResultType::VALID - /** Raw base fees. */ - std::optional m_base_fees; + /** Raw base fees in satoshis. */ + const std::optional m_base_fees; /** Constructor for failure case */ explicit MempoolAcceptResult(TxValidationState state) @@ -249,7 +249,7 @@ struct MempoolAcceptResult { /** Constructor for success case */ explicit MempoolAcceptResult(CAmount fees) - : m_result_type(ResultType::VALID), m_state(TxValidationState{}), m_base_fees(fees) {} + : m_result_type(ResultType::VALID), m_base_fees(fees) {} }; /** From 142790cab3cfb5faa29c3e5bb1751a36a524c327 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 16 Jan 2024 22:12:43 +0000 Subject: [PATCH 09/12] partial bitcoin#19668: Make runtime lock checks require compile-time lock checks contains: - af9ea55a72c94678b343f5dd98dc78f3a3ac58cb - 2ee7743fe723227f2ea1b031eddb14fc6863f4c8 (only changes to validation.h) --- src/net_processing.cpp | 9 ++++----- src/validation.h | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 08ff51632233..98cb54d55aba 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -848,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; }); @@ -1727,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; @@ -4607,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; @@ -4626,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/validation.h b/src/validation.h index f96b665f2c35..7512d5aaad1a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -299,7 +299,7 @@ bool CheckSequenceLocks(CChainState& active_chainstate, const CTransaction& tx, int flags, LockPoints* lp = nullptr, - bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs); /** * Closure representing one script verification From e2dda5915a75ff13590c653b28610864655ffc1b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 16 Jan 2024 20:11:49 +0000 Subject: [PATCH 10/12] merge bitcoin#21121: Small unit test improvements, including helper to make mempool transaction --- src/logging.cpp | 6 ++--- src/test/miner_tests.cpp | 3 +++ src/test/util/setup_common.cpp | 49 ++++++++++++++++++++++++++++++++++ src/test/util/setup_common.h | 17 ++++++++++++ src/util/time.cpp | 9 +++++-- src/util/time.h | 13 +++++++-- 6 files changed, 90 insertions(+), 7 deletions(-) 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/test/miner_tests.cpp b/src/test/miner_tests.cpp index d78aead71977..6d55d388073f 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -136,6 +136,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 +171,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 +206,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/util/setup_common.cpp b/src/test/util/setup_common.cpp index fdd726cbd6d9..7a913ae3ffb4 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -438,6 +438,55 @@ CBlock TestChainSetup::CreateBlock(const std::vector& txns, return CreateBlock(txns, scriptPubKey); } + +CMutableTransaction TestChainSetup::CreateValidMempoolTransaction(CTransactionRef input_transaction, + int input_vout, + int input_height, + CKey input_signing_key, + CScript output_destination, + CAmount output_amount) +{ + // Transaction we will submit to the mempool + CMutableTransaction mempool_txn; + + // Create an input + COutPoint outpoint_to_spend(input_transaction->GetHash(), input_vout); + CTxIn input(outpoint_to_spend); + mempool_txn.vin.push_back(input); + + // Create an output + CTxOut output(output_amount, output_destination); + mempool_txn.vout.push_back(output); + + // Sign the transaction + // - Add the signing key to a keystore + FillableSigningProvider keystore; + keystore.AddKey(input_signing_key); + // - Populate a CoinsViewCache with the unspent output + CCoinsView coins_view; + CCoinsViewCache coins_cache(&coins_view); + AddCoins(coins_cache, *input_transaction.get(), input_height); + // - Use GetCoin to properly populate utxo_to_spend, + Coin utxo_to_spend; + assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend)); + // - Then add it to a map to pass in to SignTransaction + std::map input_coins; + input_coins.insert({outpoint_to_spend, utxo_to_spend}); + // - Default signature hashing type + int nHashType = SIGHASH_ALL; + std::map input_errors; + assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors)); + + // Add transaction to the mempool + { + LOCK(cs_main); + const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, MakeTransactionRef(mempool_txn), /* bypass_limits */ false); + assert(result.m_result_type == MempoolAcceptResult::ResultType::VALID); + } + + return mempool_txn; +} + TestChainSetup::~TestChainSetup() { // Allow tx index to catch up with the block index cause otherwise diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index f5af0b58bb19..180b8924d41e 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -133,6 +133,23 @@ struct TestChainSetup : public RegTestingSetup //! Mine a series of new blocks on the active chain. void mineBlocks(int num_blocks); + /** + * Create a transaction and submit to the mempool. + * + * @param input_transaction The transaction to spend + * @param input_vout The vout to spend from the input_transaction + * @param input_height The height of the block that included the input_transaction + * @param input_signing_key The key to spend the input_transaction + * @param output_destination Where to send the output + * @param output_amount How much to send + */ + CMutableTransaction CreateValidMempoolTransaction(CTransactionRef input_transaction, + int input_vout, + int input_height, + CKey input_signing_key, + CScript output_destination, + CAmount output_amount = CAmount(1 * COIN)); + std::vector m_coinbase_txns; // For convenience, coinbase transactions CKey coinbaseKey; // private/public key needed to spend coinbase transactions }; diff --git a/src/util/time.cpp b/src/util/time.cpp index d65d4f25949a..d8f37f5843c1 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -95,9 +95,14 @@ void SetMockTime(int64_t nMockTimeIn) nMockTime.store(nMockTimeIn, std::memory_order_relaxed); } -int64_t GetMockTime() +void SetMockTime(std::chrono::seconds mock_time_in) { - return nMockTime.load(std::memory_order_relaxed); + nMockTime.store(mock_time_in.count(), std::memory_order_relaxed); +} + +std::chrono::seconds GetMockTime() +{ + return std::chrono::seconds(nMockTime.load(std::memory_order_relaxed)); } int64_t GetTimeMillis() diff --git a/src/util/time.h b/src/util/time.h index b7490c9b135e..3e0f7d9771cb 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -48,10 +48,19 @@ int64_t GetTimeMicros(); /** Returns the system time (not mockable) */ int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable -/** For testing. Set e.g. with the setmocktime rpc, or -mocktime argument */ +/** + * DEPRECATED + * Use SetMockTime with chrono type + * + * @param[in] nMockTimeIn Time in seconds. + */ void SetMockTime(int64_t nMockTimeIn); + +/** For testing. Set e.g. with the setmocktime rpc, or -mocktime argument */ +void SetMockTime(std::chrono::seconds mock_time_in); + /** For testing */ -int64_t GetMockTime(); +std::chrono::seconds GetMockTime(); /** Return system time (or mocked time, if set) */ template From 792b43054765e0f3f9208b2e57f2cdbdc2a5bd00 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 29 Jan 2024 15:01:14 +0000 Subject: [PATCH 11/12] partial bitcoin#20833: enable packages through testmempoolaccept excludes: - c9e1a26d1f17c8b98632b7796ffa8f8788b5a83c (will be added in future fuzzing PR) inapplicable: - 249f43f3cc52b0ffdf2c47aad95ba9d195f6a45e (we don't have RBF) --- doc/release-notes-5834.md | 8 + src/Makefile.am | 1 + src/policy/packages.h | 34 ++++ src/rpc/rawtransaction.cpp | 129 ++++++++----- src/test/miner_tests.cpp | 3 +- src/test/txvalidation_tests.cpp | 98 ++++++++++ src/test/util/setup_common.cpp | 7 +- src/test/util/setup_common.h | 4 +- src/txmempool.cpp | 18 +- src/txmempool.h | 10 +- src/validation.cpp | 158 +++++++++++++-- src/validation.h | 64 +++++- test/functional/mempool_accept.py | 3 +- test/functional/rpc_packages.py | 311 ++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 15 files changed, 764 insertions(+), 85 deletions(-) create mode 100644 src/policy/packages.h create mode 100755 test/functional/rpc_packages.py diff --git a/doc/release-notes-5834.md b/doc/release-notes-5834.md index fb5f652de683..b405071e8039 100644 --- a/doc/release-notes-5834.md +++ b/doc/release-notes-5834.md @@ -3,3 +3,11 @@ 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..8f0a279423e9 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 \ diff --git a/src/policy/packages.h b/src/policy/packages.h new file mode 100644 index 000000000000..4b1463dcb34e --- /dev/null +++ b/src/policy/packages.h @@ -0,0 +1,34 @@ +// 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 + +/** 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}; + +/** 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 {}; + +#endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index fde9c1169c2c..1d2aae46fcf2 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 25 (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,17 +1168,21 @@ 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 an earlier 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::NUM, "vsize", "Virtual transaction size as defined in BIP 141"}, + {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}, @@ -1200,62 +1208,85 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) 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"); - } - - 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."); + 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."); } - 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; + 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()); - - ChainstateManager& chainman = EnsureChainman(node); - const MempoolAcceptResult accept_result = WITH_LOCK(cs_main, return AcceptToMemoryPool(chainman.ActiveChainstate(), mempool, std::move(tx), - false /* bypass_limits */, true /* test_accept */)); - - // Only return the fee and vsize if the transaction would pass ATMP. - // These can be used to calculate the feerate. - if (accept_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { - const CAmount fee = accept_result.m_base_fees.value(); - // Check that fee does not exceed maximum fee - if (max_raw_tx_fee && fee > max_raw_tx_fee) { - result_0.pushKV("allowed", false); - result_0.pushKV("reject-reason", "max-fee-exceeded"); - } else { - result_0.pushKV("allowed", true); - result_0.pushKV("vsize", virtual_size); - UniValue fees(UniValue::VOBJ); - fees.pushKV("base", ValueFromAmount(fee)); - result_0.pushKV("fees", fees); + 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 we iterate through txns in order. If any transaction fee + // exceeds maxfeerate, we will keave 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()); } - result.push_back(std::move(result_0)); - } else { - result_0.pushKV("allowed", false); - const TxValidationState state = accept_result.m_state; - if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { - result_0.pushKV("reject-reason", "missing-inputs"); + 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 { + // 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()); + } } - result.push_back(std::move(result_0)); + rpc_result.push_back(result_inner); } - 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 6d55d388073f..2a48ddc83095 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 viewMempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool); + return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), viewMempool, tx, flags); } BlockAssembler AssemblerForTest(const CChainParams& params); }; diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 326ec4466e1b..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