From 6a7d4b364a9aa72dd03faffb9e82bb7ac2413f8d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 5 Oct 2023 19:02:02 -0400 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#27609: rpc: allow submitpackage to be called outside of regtest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5b878be742dbfcd232d949d2df1fff4743aec3d8 [doc] add release note for submitpackage (glozow) 7a9bb2a2a59ba49f80519c8435229abec2432486 [rpc] allow submitpackage to be called outside of regtest (glozow) 5b9087a9a7da2602485e85e0b163dc3cbd2daf31 [rpc] require package to be a tree in submitpackage (glozow) e32ba1599c599e75b1da3393f71f633de860505f [txpackages] IsChildWithParentsTree() (glozow) b4f28cc345ef9c5261c4a8d743654a44784c7802 [doc] parent pay for child in aggregate CheckFeeRate (glozow) Pull request description: Permit (restricted topology) submitpackage RPC outside of regtest. Suggested in https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1510851570 This RPC should be safe but still experimental - interface may change, not all features (e.g. package RBF) are implemented, etc. If a miner wants to expose this to people, they can effectively use "package relay" before the p2p changes are implemented. However, please note **this is not package relay**; transactions submitted this way will not relay to other nodes if the feerates are below their mempool min fee. Users should put this behind some kind of rate limit or permissions. ACKs for top commit: instagibbs: ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 achow101: ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 dergoegge: Code review ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 ajtowns: ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8 ariard: Code Review ACK 5b878be742. Though didn’t manually test the PR. Tree-SHA512: 610365c0b2ffcccd55dedd1151879c82de1027e3319712bcb11d54f2467afaae4d05dca5f4b25f03354c80845fef538d3938b958174dda8b14c10670537a6524 --- doc/release-notes-27609.md | 14 +++ src/policy/packages.cpp | 15 ++++ src/policy/packages.h | 4 + src/rpc/mempool.cpp | 155 ++++++++++++++++++++++++++++++++ src/test/txpackage_tests.cpp | 3 + src/validation.cpp | 21 +++++ test/functional/rpc_packages.py | 117 ++++++++++++++++++++++++ 7 files changed, 329 insertions(+) create mode 100644 doc/release-notes-27609.md diff --git a/doc/release-notes-27609.md b/doc/release-notes-27609.md new file mode 100644 index 000000000000..b8cecbd88252 --- /dev/null +++ b/doc/release-notes-27609.md @@ -0,0 +1,14 @@ +- A new RPC, `submitpackage`, has been added. It can be used to submit a list of raw hex + transactions to the mempool to be evaluated as a package using consensus and mempool policy rules. +These policies include package CPFP, allowing a child with high fees to bump a parent below the +mempool minimum feerate (but not minimum relay feerate). + + - Warning: successful submission does not mean the transactions will propagate throughout the + network, as package relay is not supported. + + - Not all features are available. The package is limited to a child with all of its + unconfirmed parents, and no parent may spend the output of another parent. Also, package + RBF is not supported. Refer to doc/policy/packages.md for more details on package policies + and limitations. + + - This RPC is experimental. Its interface may change. diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index 67918c9decf0..67c8e3da9e49 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -81,3 +81,18 @@ bool IsChildWithParents(const Package& package) return std::all_of(package.cbegin(), package.cend() - 1, [&input_txids](const auto& ptx) { return input_txids.count(ptx->GetHash()) > 0; }); } + +bool IsChildWithParentsTree(const Package& package) +{ + if (!IsChildWithParents(package)) return false; + std::unordered_set parent_txids; + std::transform(package.cbegin(), package.cend() - 1, std::inserter(parent_txids, parent_txids.end()), + [](const auto& ptx) { return ptx->GetHash(); }); + // Each parent must not have an input who is one of the other parents. + return std::all_of(package.cbegin(), package.cend() - 1, [&](const auto& ptx) { + for (const auto& input : ptx->vin) { + if (parent_txids.count(input.prevout.hash) > 0) return false; + } + return true; + }); +} diff --git a/src/policy/packages.h b/src/policy/packages.h index 47c6717b0330..6d734d6cb33f 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -59,4 +59,8 @@ bool CheckPackage(const Package& txns, PackageValidationState& state); */ bool IsChildWithParents(const Package& package); +/** Context-free check that a package IsChildWithParents() and none of the parents depend on each + * other (the package is a "tree"). + */ +bool IsChildWithParentsTree(const Package& package); #endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index ab527f39f824..f9da8e7cdf97 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -468,6 +468,160 @@ RPCHelpMan savemempool() }; } +static RPCHelpMan submitpackage() +{ + return RPCHelpMan{"submitpackage", + "Submit a package of raw transactions (serialized, hex-encoded) to local node.\n" + "The package must consist of a child with its parents, and none of the parents may depend on one another.\n" + "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.\n" + "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies.\n" + "Warning: successful submission does not mean the transactions will propagate throughout the network.\n" + , + { + {"package", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of raw transactions.", + { + {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""}, + }, + }, + }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by wtxid", + { + {RPCResult::Type::OBJ, "wtxid", "transaction wtxid", { + {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, + {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."}, + {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."}, + {RPCResult::Type::OBJ, "fees", "Transaction fees", { + {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, + {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/true, "if the transaction was not already in the mempool, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."}, + {RPCResult::Type::ARR, "effective-includes", /*optional=*/true, "if effective-feerate is provided, the wtxids of the transactions whose fees and vsizes are included in effective-feerate.", + {{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, + }}, + }}, + }} + }}, + {RPCResult::Type::ARR, "replaced-transactions", /*optional=*/true, "List of txids of replaced transactions", + { + {RPCResult::Type::STR_HEX, "", "The transaction id"}, + }}, + }, + }, + RPCExamples{ + HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") + + HelpExampleCli("submitpackage", "[rawtx1, rawtx2]") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue + { + 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."); + } + + std::vector txns; + txns.reserve(raw_transactions.size()); + for (const auto& rawtx : raw_transactions.getValues()) { + CMutableTransaction mtx; + if (!DecodeHexTx(mtx, rawtx.get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, + "TX decode failed: " + rawtx.get_str() + " Make sure the tx has at least one input."); + } + txns.emplace_back(MakeTransactionRef(std::move(mtx))); + } + if (!IsChildWithParentsTree(txns)) { + throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, "package topology disallowed. not child-with-parents or parents depend on each other."); + } + + NodeContext& node = EnsureAnyNodeContext(request.context); + CTxMemPool& mempool = EnsureMemPool(node); + Chainstate& chainstate = EnsureChainman(node).ActiveChainstate(); + const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/ false)); + + // First catch any errors. + switch(package_result.m_state.GetResult()) { + case PackageValidationResult::PCKG_RESULT_UNSET: break; + case PackageValidationResult::PCKG_POLICY: + { + throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, + package_result.m_state.GetRejectReason()); + } + case PackageValidationResult::PCKG_MEMPOOL_ERROR: + { + throw JSONRPCTransactionError(TransactionError::MEMPOOL_ERROR, + package_result.m_state.GetRejectReason()); + } + case PackageValidationResult::PCKG_TX: + { + for (const auto& tx : txns) { + auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); + if (it != package_result.m_tx_results.end() && it->second.m_state.IsInvalid()) { + throw JSONRPCTransactionError(TransactionError::MEMPOOL_REJECTED, + strprintf("%s failed: %s", tx->GetHash().ToString(), it->second.m_state.GetRejectReason())); + } + } + // If a PCKG_TX error was returned, there must have been an invalid transaction. + NONFATAL_UNREACHABLE(); + } + } + size_t num_broadcast{0}; + for (const auto& tx : txns) { + std::string err_string; + const auto err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, /*relay=*/true, /*wait_callback=*/true); + if (err != TransactionError::OK) { + throw JSONRPCTransactionError(err, + strprintf("transaction broadcast failed: %s (all transactions were submitted, %d transactions were broadcast successfully)", + err_string, num_broadcast)); + } + num_broadcast++; + } + UniValue rpc_result{UniValue::VOBJ}; + UniValue tx_result_map{UniValue::VOBJ}; + std::set replaced_txids; + for (const auto& tx : txns) { + auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); + CHECK_NONFATAL(it != package_result.m_tx_results.end()); + UniValue result_inner{UniValue::VOBJ}; + result_inner.pushKV("txid", tx->GetHash().GetHex()); + const auto& tx_result = it->second; + if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { + result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex()); + } + if (it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || + it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY) { + result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()}); + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value())); + if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + // Effective feerate is not provided for MEMPOOL_ENTRY transactions even + // though modified fees is known, because it is unknown whether package + // feerate was used when it was originally submitted. + fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); + UniValue effective_includes_res(UniValue::VARR); + for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) { + effective_includes_res.push_back(wtxid.ToString()); + } + fees.pushKV("effective-includes", effective_includes_res); + } + result_inner.pushKV("fees", fees); + if (it->second.m_replaced_transactions.has_value()) { + for (const auto& ptx : it->second.m_replaced_transactions.value()) { + replaced_txids.insert(ptx->GetHash()); + } + } + } + tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); + } + rpc_result.pushKV("tx-results", tx_result_map); + UniValue replaced_list(UniValue::VARR); + for (const uint256& hash : replaced_txids) replaced_list.push_back(hash.ToString()); + rpc_result.pushKV("replaced-transactions", replaced_list); + return rpc_result; + }, + }; +} + void RegisterMempoolRPCCommands(CRPCTable& t) { static const CRPCCommand commands[]{ @@ -479,6 +633,7 @@ void RegisterMempoolRPCCommands(CRPCTable& t) {"blockchain", &getmempoolinfo}, {"blockchain", &getrawmempool}, {"blockchain", &savemempool}, + {"rawtransactions", &submitpackage}, }; for (const auto& c : commands) { t.appendCommand(c.name, &c); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 2db16400d694..f3257b6947bd 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -145,6 +145,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100NoDIP0001Setup) BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted"); BOOST_CHECK(IsChildWithParents({tx_parent, tx_child})); + BOOST_CHECK(IsChildWithParentsTree({tx_parent, tx_child})); } // 24 Parents and 1 Child @@ -170,6 +171,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100NoDIP0001Setup) PackageValidationState state; BOOST_CHECK(CheckPackage(package, state)); BOOST_CHECK(IsChildWithParents(package)); + BOOST_CHECK(IsChildWithParentsTree(package)); package.erase(package.begin()); BOOST_CHECK(IsChildWithParents(package)); @@ -202,6 +204,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100NoDIP0001Setup) BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child})); BOOST_CHECK(IsChildWithParents({tx_parent, tx_child})); BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child, tx_child})); + BOOST_CHECK(!IsChildWithParentsTree({tx_parent, tx_parent_also_child, tx_child})); // IsChildWithParents does not detect unsorted parents. BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child})); BOOST_CHECK(CheckPackage({tx_parent, tx_parent_also_child, tx_child}, state)); diff --git a/src/validation.cpp b/src/validation.cpp index 0df6f5aaa852..3d875d03761f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1197,6 +1197,27 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: m_viewmempool.PackageAddTransaction(ws.m_ptx); } + // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee. + // For transactions consisting of exactly one child and its parents, it suffices to use the + // package feerate (total modified fees / total virtual size) to check this requirement. + // Note that this is an aggregate feerate; this function has not checked that there are transactions + // too low feerate to pay for themselves, or that the child transactions are higher feerate than + // their parents. Using aggregate feerate may allow "parents pay for child" behavior and permit + // a child that is below mempool minimum feerate. To avoid these behaviors, callers of + // AcceptMultipleTransactions need to restrict txns topology (e.g. to ancestor sets) and check + // the feerates of individuals and subsets. + const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, + [](int64_t sum, auto& ws) { return sum + ws.m_vsize; }); + const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, + [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; }); + const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize); + TxValidationState placeholder_state; + if (args.m_package_feerates && + !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low"); + return PackageMempoolAcceptResult(package_state, {}); + } + // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. Also, CPFP carve out can increase the limit for individual // transactions, but this exemption is not extended to packages in CheckPackageLimits(). diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 740a8c02750f..3788b54c9862 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -10,19 +10,25 @@ from test_framework.address import ADDRESS_BCRT1_P2SH_OP_TRUE from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import ( + MAX_BIP125_RBF_SEQUENCE, tx_from_hex, ) +from test_framework.p2p import P2PTxInvStore from test_framework.script import ( CScript, OP_TRUE, ) from test_framework.util import ( assert_equal, + assert_fee_amount, + assert_raises_rpc_error, ) from test_framework.wallet import ( create_child_with_parents, create_raw_chain, + DEFAULT_FEE, make_chain, + MiniWallet, ) class RPCPackagesTest(BitcoinTestFramework): @@ -78,6 +84,13 @@ def run_test(self): self.test_multiple_children() self.test_multiple_parents() self.test_conflicting() + + # Initialize MiniWallet for tests that need it + self.wallet = MiniWallet(node) + self.wallet.rescan_utxos() + + self.test_rbf() + self.test_submitpackage() def test_independent(self): @@ -262,6 +275,110 @@ def test_conflicting(self): {"txid": tx1.rehash(), "package-error": "conflict-in-package"}, {"txid": tx2.rehash(), "package-error": "conflict-in-package"} ]) + def test_rbf(self): + node = self.nodes[0] + + coin = self.wallet.get_utxo() + fee = Decimal("0.00125000") + replaceable_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = fee) + testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]])[0] + assert_equal(testres_replaceable["txid"], replaceable_tx["txid"]) + assert_equal(testres_replaceable["wtxid"], replaceable_tx["wtxid"]) + assert testres_replaceable["allowed"] + assert_equal(testres_replaceable["vsize"], replaceable_tx["tx"].get_vsize()) + assert_equal(testres_replaceable["fees"]["base"], fee) + assert_fee_amount(fee, replaceable_tx["tx"].get_vsize(), testres_replaceable["fees"]["effective-feerate"]) + assert_equal(testres_replaceable["fees"]["effective-includes"], [replaceable_tx["wtxid"]]) + + # Replacement transaction is identical except has double the fee + replacement_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = 2 * fee) + testres_rbf_conflicting = node.testmempoolaccept([replaceable_tx["hex"], replacement_tx["hex"]]) + assert_equal(testres_rbf_conflicting, [ + {"txid": replaceable_tx["txid"], "wtxid": replaceable_tx["wtxid"], "package-error": "conflict-in-package"}, + {"txid": replacement_tx["txid"], "wtxid": replacement_tx["wtxid"], "package-error": "conflict-in-package"} + ]) + + self.log.info("Test that packages cannot conflict with mempool transactions, even if a valid BIP125 RBF") + # This transaction is a valid BIP125 replace-by-fee + self.wallet.sendrawtransaction(from_node=node, tx_hex=replaceable_tx["hex"]) + testres_rbf_single = node.testmempoolaccept([replacement_tx["hex"]]) + assert testres_rbf_single[0]["allowed"] + testres_rbf_package = self.independent_txns_testres_blank + [{ + "txid": replacement_tx["txid"], "wtxid": replacement_tx["wtxid"], "allowed": False, + "reject-reason": "bip125-replacement-disallowed" + }] + self.assert_testres_equal(self.independent_txns_hex + [replacement_tx["hex"]], testres_rbf_package) + + def assert_equal_package_results(self, node, testmempoolaccept_result, submitpackage_result): + """Assert that a successful submitpackage result is consistent with testmempoolaccept + results and getmempoolentry info. Note that the result structs are different and, due to + policy differences between testmempoolaccept and submitpackage (i.e. package feerate), + some information may be different. + """ + for testres_tx in testmempoolaccept_result: + # Grab this result from the submitpackage_result + submitres_tx = submitpackage_result["tx-results"][testres_tx["wtxid"]] + assert_equal(submitres_tx["txid"], testres_tx["txid"]) + # No "allowed" if the tx was already in the mempool + if "allowed" in testres_tx and testres_tx["allowed"]: + assert_equal(submitres_tx["vsize"], testres_tx["vsize"]) + assert_equal(submitres_tx["fees"]["base"], testres_tx["fees"]["base"]) + entry_info = node.getmempoolentry(submitres_tx["txid"]) + assert_equal(submitres_tx["vsize"], entry_info["vsize"]) + assert_equal(submitres_tx["fees"]["base"], entry_info["fees"]["base"]) + + def test_submit_child_with_parents(self, num_parents, partial_submit): + node = self.nodes[0] + peer = node.add_p2p_connection(P2PTxInvStore()) + + package_txns = [] + presubmitted_wtxids = set() + for _ in range(num_parents): + parent_tx = self.wallet.create_self_transfer(fee=DEFAULT_FEE) + package_txns.append(parent_tx) + if partial_submit and random.choice([True, False]): + node.sendrawtransaction(parent_tx["hex"]) + presubmitted_wtxids.add(parent_tx["wtxid"]) + child_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx["new_utxo"] for tx in package_txns], fee_per_output=10000) #DEFAULT_FEE + package_txns.append(child_tx) + + testmempoolaccept_result = node.testmempoolaccept(rawtxs=[tx["hex"] for tx in package_txns]) + submitpackage_result = node.submitpackage(package=[tx["hex"] for tx in package_txns]) + + # Check that each result is present, with the correct size and fees + for package_txn in package_txns: + tx = package_txn["tx"] + assert tx.getwtxid() in submitpackage_result["tx-results"] + wtxid = tx.getwtxid() + assert wtxid in submitpackage_result["tx-results"] + tx_result = submitpackage_result["tx-results"][wtxid] + assert_equal(tx_result["txid"], tx.rehash()) + assert_equal(tx_result["vsize"], tx.get_vsize()) + assert_equal(tx_result["fees"]["base"], DEFAULT_FEE) + if wtxid not in presubmitted_wtxids: + assert_fee_amount(DEFAULT_FEE, tx.get_vsize(), tx_result["fees"]["effective-feerate"]) + assert_equal(tx_result["fees"]["effective-includes"], [wtxid]) + + # submitpackage result should be consistent with testmempoolaccept and getmempoolentry + self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result) + + # The node should announce each transaction. No guarantees for propagation. + peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) + self.generate(node, 1) + + def test_submitpackage(self): + node = self.nodes[0] + + self.log.info("Submitpackage valid packages with 1 child and some number of parents") + for num_parents in [1, 2, 24]: + self.test_submit_child_with_parents(num_parents, False) + self.test_submit_child_with_parents(num_parents, True) + + self.log.info("Submitpackage only allows packages of 1 child with its parents") + # Chain of 3 transactions has too many generations + chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)] + assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex) + if __name__ == "__main__": RPCPackagesTest().main() From d2402143fbf18900667b8ff8b8b7cb41f958154c Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 4 Aug 2025 05:26:32 -0500 Subject: [PATCH 2/2] fix: adapt submitpackage RPC for Dash - Replace wtxid with txid throughout the codebase - Replace GetWitnessHash() with GetHash() - Remove BIP 141 references from vsize description - Remove witness-specific result handling (DIFFERENT_WITNESS) - Remove RBF test functionality that is not supported in Dash - Remove effective-feerate and effective-includes fields not available in Dash - Remove replaced-transactions functionality not in Dash's MempoolAcceptResult Resolves witness/RBF issues identified in reviewer comments. --- src/rpc/mempool.cpp | 43 ++++++------------------- test/functional/rpc_packages.py | 56 ++++++--------------------------- 2 files changed, 18 insertions(+), 81 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index f9da8e7cdf97..6f583627fa5c 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -487,25 +487,20 @@ static RPCHelpMan submitpackage() RPCResult{ RPCResult::Type::OBJ, "", "", { - {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by wtxid", + {RPCResult::Type::OBJ_DYN, "tx-results", "transaction results keyed by txid", { - {RPCResult::Type::OBJ, "wtxid", "transaction wtxid", { + {RPCResult::Type::OBJ, "txid", "transaction txid", { {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, - {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."}, - {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."}, + {RPCResult::Type::NUM, "vsize", "Virtual transaction size."}, {RPCResult::Type::OBJ, "fees", "Transaction fees", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/true, "if the transaction was not already in the mempool, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."}, - {RPCResult::Type::ARR, "effective-includes", /*optional=*/true, "if effective-feerate is provided, the wtxids of the transactions whose fees and vsizes are included in effective-feerate.", - {{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, + {RPCResult::Type::ARR, "effective-includes", /*optional=*/true, "if effective-feerate is provided, the txids of the transactions whose fees and vsizes are included in effective-feerate.", + {{RPCResult::Type::STR_HEX, "", "transaction txid in hex"}, }}, }}, }} }}, - {RPCResult::Type::ARR, "replaced-transactions", /*optional=*/true, "List of txids of replaced transactions", - { - {RPCResult::Type::STR_HEX, "", "The transaction id"}, - }}, }, }, RPCExamples{ @@ -555,7 +550,7 @@ static RPCHelpMan submitpackage() case PackageValidationResult::PCKG_TX: { for (const auto& tx : txns) { - auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); + auto it = package_result.m_tx_results.find(tx->GetHash()); if (it != package_result.m_tx_results.end() && it->second.m_state.IsInvalid()) { throw JSONRPCTransactionError(TransactionError::MEMPOOL_REJECTED, strprintf("%s failed: %s", tx->GetHash().ToString(), it->second.m_state.GetRejectReason())); @@ -578,45 +573,25 @@ static RPCHelpMan submitpackage() } UniValue rpc_result{UniValue::VOBJ}; UniValue tx_result_map{UniValue::VOBJ}; - std::set replaced_txids; for (const auto& tx : txns) { - auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); + auto it = package_result.m_tx_results.find(tx->GetHash()); CHECK_NONFATAL(it != package_result.m_tx_results.end()); UniValue result_inner{UniValue::VOBJ}; result_inner.pushKV("txid", tx->GetHash().GetHex()); const auto& tx_result = it->second; - if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { - result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex()); - } if (it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY) { result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()}); UniValue fees(UniValue::VOBJ); fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value())); if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { - // Effective feerate is not provided for MEMPOOL_ENTRY transactions even - // though modified fees is known, because it is unknown whether package - // feerate was used when it was originally submitted. - fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); - UniValue effective_includes_res(UniValue::VARR); - for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) { - effective_includes_res.push_back(wtxid.ToString()); - } - fees.pushKV("effective-includes", effective_includes_res); + // Note: effective-feerate and effective-includes not available in Dash implementation } result_inner.pushKV("fees", fees); - if (it->second.m_replaced_transactions.has_value()) { - for (const auto& ptx : it->second.m_replaced_transactions.value()) { - replaced_txids.insert(ptx->GetHash()); - } - } } - tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); + tx_result_map.pushKV(tx->GetHash().GetHex(), result_inner); } rpc_result.pushKV("tx-results", tx_result_map); - UniValue replaced_list(UniValue::VARR); - for (const uint256& hash : replaced_txids) replaced_list.push_back(hash.ToString()); - rpc_result.pushKV("replaced-transactions", replaced_list); return rpc_result; }, }; diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 3788b54c9862..a2684e8787cc 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -10,7 +10,6 @@ from test_framework.address import ADDRESS_BCRT1_P2SH_OP_TRUE from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import ( - MAX_BIP125_RBF_SEQUENCE, tx_from_hex, ) from test_framework.p2p import P2PTxInvStore @@ -89,7 +88,6 @@ def run_test(self): self.wallet = MiniWallet(node) self.wallet.rescan_utxos() - self.test_rbf() self.test_submitpackage() @@ -275,39 +273,6 @@ def test_conflicting(self): {"txid": tx1.rehash(), "package-error": "conflict-in-package"}, {"txid": tx2.rehash(), "package-error": "conflict-in-package"} ]) - def test_rbf(self): - node = self.nodes[0] - - coin = self.wallet.get_utxo() - fee = Decimal("0.00125000") - replaceable_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = fee) - testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]])[0] - assert_equal(testres_replaceable["txid"], replaceable_tx["txid"]) - assert_equal(testres_replaceable["wtxid"], replaceable_tx["wtxid"]) - assert testres_replaceable["allowed"] - assert_equal(testres_replaceable["vsize"], replaceable_tx["tx"].get_vsize()) - assert_equal(testres_replaceable["fees"]["base"], fee) - assert_fee_amount(fee, replaceable_tx["tx"].get_vsize(), testres_replaceable["fees"]["effective-feerate"]) - assert_equal(testres_replaceable["fees"]["effective-includes"], [replaceable_tx["wtxid"]]) - - # Replacement transaction is identical except has double the fee - replacement_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = 2 * fee) - testres_rbf_conflicting = node.testmempoolaccept([replaceable_tx["hex"], replacement_tx["hex"]]) - assert_equal(testres_rbf_conflicting, [ - {"txid": replaceable_tx["txid"], "wtxid": replaceable_tx["wtxid"], "package-error": "conflict-in-package"}, - {"txid": replacement_tx["txid"], "wtxid": replacement_tx["wtxid"], "package-error": "conflict-in-package"} - ]) - - self.log.info("Test that packages cannot conflict with mempool transactions, even if a valid BIP125 RBF") - # This transaction is a valid BIP125 replace-by-fee - self.wallet.sendrawtransaction(from_node=node, tx_hex=replaceable_tx["hex"]) - testres_rbf_single = node.testmempoolaccept([replacement_tx["hex"]]) - assert testres_rbf_single[0]["allowed"] - testres_rbf_package = self.independent_txns_testres_blank + [{ - "txid": replacement_tx["txid"], "wtxid": replacement_tx["wtxid"], "allowed": False, - "reject-reason": "bip125-replacement-disallowed" - }] - self.assert_testres_equal(self.independent_txns_hex + [replacement_tx["hex"]], testres_rbf_package) def assert_equal_package_results(self, node, testmempoolaccept_result, submitpackage_result): """Assert that a successful submitpackage result is consistent with testmempoolaccept @@ -317,7 +282,7 @@ def assert_equal_package_results(self, node, testmempoolaccept_result, submitpac """ for testres_tx in testmempoolaccept_result: # Grab this result from the submitpackage_result - submitres_tx = submitpackage_result["tx-results"][testres_tx["wtxid"]] + submitres_tx = submitpackage_result["tx-results"][testres_tx["txid"]] assert_equal(submitres_tx["txid"], testres_tx["txid"]) # No "allowed" if the tx was already in the mempool if "allowed" in testres_tx and testres_tx["allowed"]: @@ -332,13 +297,13 @@ def test_submit_child_with_parents(self, num_parents, partial_submit): peer = node.add_p2p_connection(P2PTxInvStore()) package_txns = [] - presubmitted_wtxids = set() + presubmitted_txids = set() for _ in range(num_parents): parent_tx = self.wallet.create_self_transfer(fee=DEFAULT_FEE) package_txns.append(parent_tx) if partial_submit and random.choice([True, False]): node.sendrawtransaction(parent_tx["hex"]) - presubmitted_wtxids.add(parent_tx["wtxid"]) + presubmitted_txids.add(parent_tx["txid"]) child_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx["new_utxo"] for tx in package_txns], fee_per_output=10000) #DEFAULT_FEE package_txns.append(child_tx) @@ -348,22 +313,19 @@ def test_submit_child_with_parents(self, num_parents, partial_submit): # Check that each result is present, with the correct size and fees for package_txn in package_txns: tx = package_txn["tx"] - assert tx.getwtxid() in submitpackage_result["tx-results"] - wtxid = tx.getwtxid() - assert wtxid in submitpackage_result["tx-results"] - tx_result = submitpackage_result["tx-results"][wtxid] - assert_equal(tx_result["txid"], tx.rehash()) + txid = tx.rehash() + assert txid in submitpackage_result["tx-results"] + tx_result = submitpackage_result["tx-results"][txid] + assert_equal(tx_result["txid"], txid) assert_equal(tx_result["vsize"], tx.get_vsize()) assert_equal(tx_result["fees"]["base"], DEFAULT_FEE) - if wtxid not in presubmitted_wtxids: - assert_fee_amount(DEFAULT_FEE, tx.get_vsize(), tx_result["fees"]["effective-feerate"]) - assert_equal(tx_result["fees"]["effective-includes"], [wtxid]) + # Note: effective-feerate and effective-includes not available in Dash # submitpackage result should be consistent with testmempoolaccept and getmempoolentry self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result) # The node should announce each transaction. No guarantees for propagation. - peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) + peer.wait_for_broadcast([tx["tx"].rehash() for tx in package_txns]) self.generate(node, 1) def test_submitpackage(self):