From 63a6d4664b8f97a1ee74e6591d356ceec0e9278f Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Fri, 10 Nov 2023 15:12:34 -0500 Subject: [PATCH] RPC submitpackage: change return format to allow partial errors Behavior prior to this commit allows some transactions to enter into the local mempool but not be reported to the user when encountering a PackageValidationResult::PCKG_TX result. This is further compounded with the fact that any transactions submitted to the mempool during this call would also not be relayed to peers, resulting in unexpected behavior. Fix this by, if encountering a PCKG_TX error, reporting all wtxids, along with a new error field, and broadcasting every transaction that was deemed VALID. Note that this also changes fees and vsize to optional, which should also remove an issue with other-wtxid cases. --- src/rpc/mempool.cpp | 46 ++++++++++++++++++++------- test/functional/mempool_limit.py | 13 +++++--- test/functional/mempool_sigoplimit.py | 4 +-- 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index b387febc1d2677..db10a8f8e07071 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -841,14 +841,15 @@ static RPCHelpMan submitpackage() {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::NUM, "vsize", /*optional=*/true, "Virtual transaction size as defined in BIP 141."}, + {RPCResult::Type::OBJ, "fees", /*optional=*/true, "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::STR, "error", /*optional=*/true, "The transaction error string, if it was rejected by the mempool"}, }} }}, {RPCResult::Type::ARR, "replaced-transactions", /*optional=*/true, "List of txids of replaced transactions", @@ -888,9 +889,18 @@ static RPCHelpMan submitpackage() 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. + std::map> wtixd_to_error_strings; + + // First catch any errors and capture useful error strings. switch(package_result.m_state.GetResult()) { - case PackageValidationResult::PCKG_RESULT_UNSET: break; + case PackageValidationResult::PCKG_RESULT_UNSET: + { + CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size()); + for (const auto& tx : txns) { + wtixd_to_error_strings.insert({tx->GetWitnessHash(), std::nullopt}); + } + break; + } case PackageValidationResult::PCKG_POLICY: { throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, @@ -905,17 +915,28 @@ static RPCHelpMan submitpackage() { 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 (it != package_result.m_tx_results.end()) { + if (it->second.m_state.IsInvalid()) { + wtixd_to_error_strings.insert({tx->GetWitnessHash(), it->second.m_state.ToString()}); + } else { + wtixd_to_error_strings.insert({tx->GetWitnessHash(), std::nullopt}); + } + } else { + // No result returned; report this helpfully + wtixd_to_error_strings.insert({tx->GetWitnessHash(), "unevaluated"}); } } - // If a PCKG_TX error was returned, there must have been an invalid transaction. - NONFATAL_UNREACHABLE(); + break; } } + size_t num_broadcast{0}; for (const auto& tx : txns) { + // We don't want to re-submit the txn for validation in BroadcastTransaction + if (!mempool.exists(GenTxid::Txid(tx->GetHash()))) { + continue; + } + 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) { @@ -936,8 +957,9 @@ static RPCHelpMan submitpackage() 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 || + } else if (it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID) { + result_inner.pushKV("error", wtixd_to_error_strings.at(tx->GetWitnessHash()).value()); + } else 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); @@ -959,6 +981,8 @@ static RPCHelpMan submitpackage() replaced_txids.insert(ptx->GetHash()); } } + } else { + NONFATAL_UNREACHABLE(); } tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); } diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index a1147f70f33e10..71843f790c0c50 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -125,8 +125,8 @@ def test_rbf_carveout_disallowed(self): utxo_to_spend=tx_B["new_utxo"], confirmed_only=True ) - - assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]]) + res = node.submitpackage([tx_B["hex"], tx_C["hex"]]) + assert "too-long-mempool-chain" in res["tx-results"][tx_C["wtxid"]]["error"] def test_mid_package_eviction(self): node = self.nodes[0] @@ -205,7 +205,7 @@ def test_mid_package_eviction(self): # Package should be submitted, temporarily exceeding maxmempool, and then evicted. with node.assert_debug_log(expected_msgs=["rolling minimum fee bumped"]): - assert_raises_rpc_error(-26, "mempool full", node.submitpackage, package_hex) + node.submitpackage(package_hex) # Maximum size must never be exceeded. assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"]) @@ -273,7 +273,8 @@ def test_mid_package_replacement(self): package_hex = [cpfp_parent["hex"], replacement_tx["hex"], child["hex"]] # Package should be submitted, temporarily exceeding maxmempool, and then evicted. - assert_raises_rpc_error(-26, "bad-txns-inputs-missingorspent", node.submitpackage, package_hex) + res = node.submitpackage(package_hex) + assert len([tx_res for _, tx_res in res["tx-results"].items() if "error" in tx_res and tx_res["error"] == "bad-txns-inputs-missingorspent"]) # Maximum size must never be exceeded. assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"]) @@ -366,7 +367,9 @@ def run_test(self): assert_greater_than(worst_feerate_btcvb, (parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize())) assert_greater_than(mempoolmin_feerate, (parent_fee) / (tx_parent_just_below["tx"].get_vsize())) assert_greater_than((parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize()), mempoolmin_feerate / 1000) - assert_raises_rpc_error(-26, "mempool full", node.submitpackage, [tx_parent_just_below["hex"], tx_child_just_above["hex"]]) + res = node.submitpackage([tx_parent_just_below["hex"], tx_child_just_above["hex"]]) + for wtxid in [tx_parent_just_below["wtxid"], tx_child_just_above["wtxid"]]: + res["tx-results"][wtxid]["error"] == "mempool full" self.log.info('Test passing a value below the minimum (5 MB) to -maxmempool throws an error') self.stop_node(0) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index fbec6d0dc8b7b1..14dd9cc80e725b 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/test/functional/mempool_sigoplimit.py @@ -34,7 +34,6 @@ assert_equal, assert_greater_than, assert_greater_than_or_equal, - assert_raises_rpc_error, ) from test_framework.wallet import MiniWallet from test_framework.wallet_util import generate_keypair @@ -169,7 +168,8 @@ def create_bare_multisig_tx(utxo_to_spend=None): assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"]) # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()]) + res = self.nodes[0].submitpackage([tx_parent.serialize().hex(), tx_child.serialize().hex()]) + assert "too-long-mempool-chain" in res["tx-results"][tx_child.getwtxid()]["error"] assert tx_parent.rehash() in self.nodes[0].getrawmempool() # Transactions are tiny in weight