Merge bitcoin#28848: bugfix, Change up submitpackage results to return results for all transactions

f23ba24 test_submitpackage: only make a chain of 3 txns (Greg Sanders)
e67a345 doc: submitpackage vsize results are sigops-adjusted (Greg Sanders)
b67db52 RPC submitpackage: change return format to allow partial errors (Greg Sanders)

Pull request description:

  This was prompted by errors being returned that didn't "make any sense" to me, because it would for example return a "fee too low" error, when the "real" error was the child had something invalid, which disallowed CPFP evaluation. Rather than make judgment calls on what error is important(which is currently just return the "first"!), we simply return all errors and let the callers determine what's best.

  Added a top level `package_msg` for quick eye-balling of general success of the package.

  This PR also fixes a couple bugs:

  1) Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes `PKG_TX` failure.
  2) "other-wtxid" is uncovered by tests, but IIUC was previously required to return "fees" and "vsize" results, but did not. I just make those results optional.

ACKs for top commit:
  Sjors:
    Light re-utACK f23ba24
  achow101:
    ACK f23ba24
  glozow:
    utACK f23ba24, thanks for taking the suggestions

Tree-SHA512: ebfd716a4fed9e8c2dea3d2181ba6a6171b06718d29ac2324c67b7a30b374d199f7e1739f91ab5d036be172d0479de9bc89c32263ee62143c0338b9b622d0cca

Author: achow101

src/rpc/mempool.cpp

@@ -818,7 +818,7 @@ 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"
+        "The package will be validated according to consensus and mempool policy rules. If any transaction passes, it 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"
         ,
@@ -832,19 +832,21 @@ static RPCHelpMan submitpackage()
         RPCResult{
             RPCResult::Type::OBJ, "", "",
             {
+                {RPCResult::Type::STR, "package_msg", "The transaction package result message. \"success\" indicates all transactions were accepted into or are already in the mempool."},
                 {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::NUM, "vsize", /*optional=*/true, "Sigops-adjusted virtual transaction size."},
+                        {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",
@@ -884,57 +886,77 @@ 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::string package_msg = "success";
+
+            // First catch package-wide errors, continue if we can
             switch(package_result.m_state.GetResult()) {
-                case PackageValidationResult::PCKG_RESULT_UNSET: break;
-                case PackageValidationResult::PCKG_POLICY:
+                case PackageValidationResult::PCKG_RESULT_UNSET:
                 {
-                    throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE,
-                        package_result.m_state.GetRejectReason());
+                    // Belt-and-suspenders check; everything should be successful here
+                    CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size());
+                    for (const auto& tx : txns) {
+                        CHECK_NONFATAL(mempool.exists(GenTxid::Txid(tx->GetHash())));
+                    }
+                    break;
                 }
                 case PackageValidationResult::PCKG_MEMPOOL_ERROR:
                 {
+                    // This only happens with internal bug; user should stop and report
                     throw JSONRPCTransactionError(TransactionError::MEMPOOL_ERROR,
                         package_result.m_state.GetRejectReason());
                 }
+                case PackageValidationResult::PCKG_POLICY:
                 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();
+                    // Package-wide error we want to return, but we also want to return individual responses
+                    package_msg = package_result.m_state.GetRejectReason();
+                    CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size() ||
+                            package_result.m_tx_results.empty());
+                    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;
+                }
+
+                // We do not expect an error here; we are only broadcasting things already/still in mempool
                 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)",
+                        strprintf("transaction broadcast failed: %s (%d transactions were broadcast successfully)",
                             err_string, num_broadcast));
                 }
                 num_broadcast++;
             }
+
             UniValue rpc_result{UniValue::VOBJ};
+            rpc_result.pushKV("package_msg", package_msg);
             UniValue tx_result_map{UniValue::VOBJ};
             std::set<uint256> 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());
+                auto it = package_result.m_tx_results.find(tx->GetWitnessHash());
+                if (it == package_result.m_tx_results.end()) {
+                    // No results, report error and continue
+                    result_inner.pushKV("error", "unevaluated");
+                    continue;
+                }
                 const auto& tx_result = it->second;
-                if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) {
+                switch(it->second.m_result_type) {
+                case 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) {
+                    break;
+                case MempoolAcceptResult::ResultType::INVALID:
+                    result_inner.pushKV("error", it->second.m_state.ToString());
+                    break;
+                case MempoolAcceptResult::ResultType::VALID:
+                case 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()));
@@ -955,6 +977,7 @@ static RPCHelpMan submitpackage()
                             replaced_txids.insert(ptx->GetHash());
                         }
                     }
+                    break;
                 }
                 tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner);
             }

test/functional/mempool_limit.py

@@ -125,8 +125,9 @@ 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_equal(res["package_msg"], "transaction failed")
+        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 +206,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)
+            assert_equal(node.submitpackage(package_hex)["package_msg"], "transaction failed")
 
         # Maximum size must never be exceeded.
         assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"])
@@ -273,7 +274,9 @@ 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_equal(res["package_msg"], "transaction failed")
+        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"])
@@ -321,6 +324,7 @@ def run_test(self):
         package_txns.append(tx_child)
 
         submitpackage_result = node.submitpackage([tx["hex"] for tx in package_txns])
+        assert_equal(submitpackage_result["package_msg"], "success")
 
         rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]]
         poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]]
@@ -366,7 +370,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"]]:
+            assert_equal(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)

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

test/functional/rpc_packages.py

@@ -304,6 +304,7 @@ def test_submit_child_with_parents(self, num_parents, partial_submit):
         submitpackage_result = node.submitpackage(package=[tx["hex"] for tx in package_txns])
 
         # Check that each result is present, with the correct size and fees
+        assert_equal(submitpackage_result["package_msg"], "success")
         for package_txn in package_txns:
             tx = package_txn["tx"]
             assert tx.getwtxid() in submitpackage_result["tx-results"]
@@ -334,9 +335,26 @@ def test_submitpackage(self):
 
         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)]
+        legacy_pool = node.getrawmempool()
+        chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=3)]
         assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex)
+        assert_equal(legacy_pool, node.getrawmempool())
 
+        # Create a transaction chain such as only the parent gets accepted (by making the child's
+        # version non-standard). Make sure the parent does get broadcast.
+        self.log.info("If a package is partially submitted, transactions included in mempool get broadcast")
+        peer = node.add_p2p_connection(P2PTxInvStore())
+        txs = self.wallet.create_self_transfer_chain(chain_length=2)
+        bad_child = tx_from_hex(txs[1]["hex"])
+        bad_child.nVersion = -1
+        hex_partial_acceptance = [txs[0]["hex"], bad_child.serialize().hex()]
+        res = node.submitpackage(hex_partial_acceptance)
+        assert_equal(res["package_msg"], "transaction failed")
+        first_wtxid = txs[0]["tx"].getwtxid()
+        assert "error" not in res["tx-results"][first_wtxid]
+        sec_wtxid = bad_child.getwtxid()
+        assert_equal(res["tx-results"][sec_wtxid]["error"], "version")
+        peer.wait_for_broadcast([first_wtxid])
 
 if __name__ == "__main__":
     RPCPackagesTest().main()