Skip to content

Commit

Permalink
RPC submitpackage: change return format to allow partial errors
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@instagibbs
instagibbs committed Nov 10, 2023
1 parent 1fdd832 commit 63a6d46
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 18 deletions.
46 changes: 35 additions & 11 deletions src/rpc/mempool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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<Wtxid, std::optional<std::string>> 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,
Expand All @@ -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) {
Expand All @@ -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);
Expand All @@ -959,6 +981,8 @@ static RPCHelpMan submitpackage()
replaced_txids.insert(ptx->GetHash());
}
}
} else {
NONFATAL_UNREACHABLE();
}
tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner);
}
Expand Down
13 changes: 8 additions & 5 deletions test/functional/mempool_limit.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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"])
Expand Down Expand Up @@ -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"])
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions test/functional/mempool_sigoplimit.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 63a6d46

Please sign in to comment.