Page MenuHomePhabricator
Files F13710999

D16458.diff
No OneTemporary
Actions

D16458.diff
View Options

diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -784,7 +784,7 @@
"Submit a package of raw transactions (serialized, hex-encoded) to "
"local node (-regtest only).\n"
"The package will be validated according to consensus and mempool "
- "policy rules. If all transactions pass, they will be accepted to "
+ "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"
@@ -811,6 +811,10 @@
"",
"",
{
+ {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 txid",
@@ -818,10 +822,11 @@
"txid",
"transaction txid",
{
- {RPCResult::Type::NUM, "vsize",
+ {RPCResult::Type::NUM, "vsize", /*optional=*/true,
"Virtual transaction size."},
{RPCResult::Type::OBJ,
"fees",
+ /*optional=*/true,
"Transaction fees",
{
{RPCResult::Type::STR_AMOUNT, "base",
@@ -841,6 +846,9 @@
"transaction txid in hex"},
}},
}},
+ {RPCResult::Type::STR, "error", /*optional=*/true,
+ "The transaction error string, if it was rejected by "
+ "the mempool"},
}}}},
},
},
@@ -881,39 +889,48 @@
::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:
+ case PackageValidationResult::PCKG_RESULT_UNSET: {
+ // 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(tx->GetId()));
+ }
break;
- case PackageValidationResult::PCKG_POLICY: {
- throw JSONRPCTransactionError(
- TransactionError::INVALID_PACKAGE,
- package_result.m_state.GetRejectReason());
}
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->GetId());
- 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(tx->GetId())) {
+ 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=*/Amount::zero(),
@@ -921,47 +938,57 @@
if (err != TransactionError::OK) {
throw JSONRPCTransactionError(
err,
- strprintf("transaction broadcast failed: %s (all "
- "transactions were submitted, %d "
+ 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};
for (const auto &tx : txns) {
- auto it = package_result.m_tx_results.find(tx->GetId());
- CHECK_NONFATAL(it != package_result.m_tx_results.end());
UniValue result_inner{UniValue::VOBJ};
+ auto it = package_result.m_tx_results.find(tx->GetId());
+ 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::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", it->second.m_base_fees.value());
- if (tx_result.m_result_type ==
- MempoolAcceptResult::ResultType::VALID) {
- // Effective feerate is not provided for MEMPOOL_ENTRY
- // (already in mempool) 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",
- tx_result.m_effective_feerate.value().GetFeePerK());
- UniValue effective_includes_res(UniValue::VARR);
- for (const auto &txid :
- tx_result.m_txids_fee_calculations.value()) {
- effective_includes_res.push_back(txid.ToString());
+ switch (it->second.m_result_type) {
+ 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", it->second.m_base_fees.value());
+ if (tx_result.m_result_type ==
+ MempoolAcceptResult::ResultType::VALID) {
+ // Effective feerate is not provided for
+ // MEMPOOL_ENTRY (already in mempool) 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",
+ tx_result.m_effective_feerate.value()
+ .GetFeePerK());
+ UniValue effective_includes_res(UniValue::VARR);
+ for (const auto &txid :
+ tx_result.m_txids_fee_calculations.value()) {
+ effective_includes_res.push_back(
+ txid.ToString());
+ }
+ fees.pushKV("effective-includes",
+ effective_includes_res);
}
- fees.pushKV("effective-includes",
- effective_includes_res);
- }
- result_inner.pushKV("fees", fees);
+ result_inner.pushKV("fees", fees);
+ break;
}
tx_result_map.pushKV(tx->GetId().GetHex(), result_inner);
}
diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py
--- a/test/functional/mempool_limit.py
+++ b/test/functional/mempool_limit.py
@@ -181,8 +181,8 @@
# 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.
@@ -245,6 +245,7 @@
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["txid"]]
poor_parent_result = submitpackage_result["tx-results"][tx_poor["txid"]]
@@ -353,12 +354,11 @@
),
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 txid in [tx_parent_just_below["txid"], tx_child_just_above["txid"]]:
+ assert_equal(res["tx-results"][txid]["error"], "mempool full")
self.test_mid_package_eviction()
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -11,7 +11,7 @@
from test_framework.p2p import P2PTxInvStore
from test_framework.test_framework import BitcoinTestFramework
from test_framework.txtools import pad_tx
-from test_framework.util import assert_equal, assert_fee_amount, assert_raises_rpc_error
+from test_framework.util import assert_equal, assert_fee_amount
from test_framework.wallet import DEFAULT_FEE, MiniWallet
@@ -380,6 +380,7 @@
)
# 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 (txid := tx.get_id()) in submitpackage_result["tx-results"]
@@ -415,12 +416,32 @@
self.log.info("Submitpackage only allows packages of 1 child with its parents")
# Chain of 3 transactions has too many generations
+ legacy_pool = node.getrawmempool()
chain_hex = [
t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)
]
- assert_raises_rpc_error(
- -25, "not-child-with-parents", node.submitpackage, chain_hex
+ res = node.submitpackage(chain_hex)
+ assert_equal(res["package_msg"], "package-not-child-with-parents")
+ 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 = FromHex(CTransaction(), 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_txid = txs[0]["txid"]
+ assert "error" not in res["tx-results"][first_txid]
+ sec_txid = bad_child.get_id()
+ assert_equal(res["tx-results"][sec_txid]["error"], "version")
+ peer.wait_for_broadcast([first_txid])
if __name__ == "__main__":

File Metadata

Mime Type
text/plain
Expires
Sat, Apr 26, 10:03 (1 h, 11 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
5573185
Default Alt Text
D16458.diff (14 KB)

Event Timeline