diff --git a/doc/release-notes-28121.md b/doc/release-notes-28121.md new file mode 100644 index 0000000000..911b7c5620 --- /dev/null +++ b/doc/release-notes-28121.md @@ -0,0 +1,2 @@ +The RPC `testmempoolaccept` response now includes a "reject-details" field in some cases, +similar to the complete error messages returned by `sendrawtransaction` (#28121) \ No newline at end of file diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index b634dea561..bc76361fba 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -71,7 +71,7 @@ std::optional GetEntriesForConflicts(const CTransaction& tx, // descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple // times), but we just want to be conservative to avoid doing too much work. if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) { - return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", + return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)", txid.ToString(), nConflictingCount, MAX_REPLACEMENT_CANDIDATES); diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 6d462a7e85..52260d5794 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -146,7 +146,8 @@ static RPCHelpMan testmempoolaccept() {RPCResult{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, }}, }}, - {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"}, + {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection reason (only present when 'allowed' is false)"}, + {RPCResult::Type::STR, "reject-details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"}, }}, } }, @@ -245,6 +246,7 @@ static RPCHelpMan testmempoolaccept() result_inner.pushKV("reject-reason", "missing-inputs"); } else { result_inner.pushKV("reject-reason", state.GetRejectReason()); + result_inner.pushKV("reject-details", state.ToString()); } } rpc_result.push_back(std::move(result_inner)); diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index 557fcc7cea..60b3fb4e20 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -148,6 +148,10 @@ class BIP65Test(BitcoinTestFramework): # create and test one invalid tx per CLTV failure reason (5 in total) for i in range(5): spendtx = wallet.create_self_transfer()['tx'] + assert_equal(len(spendtx.vin), 1) + coin = spendtx.vin[0] + coin_txid = format(coin.prevout.hash, '064x') + coin_vout = coin.prevout.n cltv_invalidate(spendtx, i) expected_cltv_reject_reason = [ @@ -159,12 +163,15 @@ class BIP65Test(BitcoinTestFramework): ][i] # First we show that this tx is valid except for CLTV by getting it # rejected from the mempool for exactly that reason. + spendtx_txid = spendtx.hash + spendtx_wtxid = spendtx.getwtxid() assert_equal( [{ - 'txid': spendtx.hash, - 'wtxid': spendtx.getwtxid(), + 'txid': spendtx_txid, + 'wtxid': spendtx_wtxid, 'allowed': False, 'reject-reason': expected_cltv_reject_reason, + 'reject-details': expected_cltv_reject_reason + f", input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:{coin_vout}" }], self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0), ) diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index f6b832106b..0c3b0f1224 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -109,18 +109,23 @@ class BIP66Test(BitcoinTestFramework): self.log.info("Test that transactions with non-DER signatures cannot appear in a block") block.nVersion = 4 - spendtx = self.create_tx(self.coinbase_txids[1]) + coin_txid = self.coinbase_txids[1] + spendtx = self.create_tx(coin_txid) unDERify(spendtx) spendtx.rehash() # First we show that this tx is valid except for DERSIG by getting it # rejected from the mempool for exactly that reason. + spendtx_txid = spendtx.hash + spendtx_wtxid = spendtx.getwtxid() assert_equal( [{ - 'txid': spendtx.hash, - 'wtxid': spendtx.getwtxid(), + 'txid': spendtx_txid, + 'wtxid': spendtx_wtxid, 'allowed': False, 'reject-reason': 'mandatory-script-verify-flag-failed (Non-canonical DER signature)', + 'reject-details': 'mandatory-script-verify-flag-failed (Non-canonical DER signature), ' + + f"input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:0" }], self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0), ) diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index d9700e2ee2..ef2ecfab9e 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -103,14 +103,22 @@ class ReplaceByFeeTest(BitcoinTestFramework): """Simple doublespend""" # we use MiniWallet to create a transaction template with inputs correctly set, # and modify the output (amount, scriptPubKey) according to our needs - tx = self.wallet.create_self_transfer()["tx"] + tx = self.wallet.create_self_transfer(fee_rate=Decimal("0.003"))["tx"] tx1a_txid = self.nodes[0].sendrawtransaction(tx.serialize().hex()) # Should fail because we haven't changed the fee tx.vout[0].scriptPubKey[-1] ^= 1 + tx.rehash() + tx_hex = tx.serialize().hex() # This will raise an exception due to insufficient fee - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex(), 0) + reject_reason = "insufficient fee" + reject_details = f"{reject_reason}, rejecting replacement {tx.hash}; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB" + res = self.nodes[0].testmempoolaccept(rawtxs=[tx_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx_hex, 0) + # Extra 0.1 BTC fee tx.vout[0].nValue -= int(0.1 * COIN) @@ -154,7 +162,14 @@ class ReplaceByFeeTest(BitcoinTestFramework): dbl_tx_hex = dbl_tx.serialize().hex() # This will raise an exception due to insufficient fee - assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, dbl_tx_hex, 0) + reject_reason = "insufficient fee" + reject_details = f"{reject_reason}, rejecting replacement {dbl_tx.hash}, less fees than conflicting txs; 3.00 < 4.00" + res = self.nodes[0].testmempoolaccept(rawtxs=[dbl_tx_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, dbl_tx_hex, 0) + + # Accepted with sufficient fee dbl_tx.vout[0].nValue = int(0.1 * COIN) @@ -273,22 +288,30 @@ class ReplaceByFeeTest(BitcoinTestFramework): utxo1 = self.make_utxo(self.nodes[0], int(1.2 * COIN)) utxo2 = self.make_utxo(self.nodes[0], 3 * COIN) - tx1a_utxo = self.wallet.send_self_transfer( + tx1a = self.wallet.send_self_transfer( from_node=self.nodes[0], utxo_to_spend=utxo1, sequence=0, fee=Decimal("0.1"), - )["new_utxo"] + ) + tx1a_utxo = tx1a["new_utxo"] # Direct spend an output of the transaction we're replacing. - tx2_hex = self.wallet.create_self_transfer_multi( + tx2 = self.wallet.create_self_transfer_multi( utxos_to_spend=[utxo1, utxo2, tx1a_utxo], sequence=0, amount_per_output=int(COIN * tx1a_utxo["value"]), - )["hex"] + )["tx"] + tx2_hex = tx2.serialize().hex() # This will raise an exception - assert_raises_rpc_error(-26, "bad-txns-spends-conflicting-tx", self.nodes[0].sendrawtransaction, tx2_hex, 0) + reject_reason = "bad-txns-spends-conflicting-tx" + reject_details = f"{reject_reason}, {tx2.hash} spends conflicting transaction {tx1a['tx'].hash}" + res = self.nodes[0].testmempoolaccept(rawtxs=[tx2_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx2_hex, 0) + # Spend tx1a's output to test the indirect case. tx1b_utxo = self.wallet.send_self_transfer( @@ -319,14 +342,21 @@ class ReplaceByFeeTest(BitcoinTestFramework): fee=Decimal("0.1"), ) - tx2_hex = self.wallet.create_self_transfer_multi( + tx2 = self.wallet.create_self_transfer_multi( utxos_to_spend=[confirmed_utxo, unconfirmed_utxo], sequence=0, amount_per_output=1 * COIN, - )["hex"] + )["tx"] + tx2_hex = tx2.serialize().hex() # This will raise an exception - assert_raises_rpc_error(-26, "replacement-adds-unconfirmed", self.nodes[0].sendrawtransaction, tx2_hex, 0) + reject_reason = "replacement-adds-unconfirmed" + reject_details = f"{reject_reason}, replacement {tx2.hash} adds unconfirmed input, idx 1" + res = self.nodes[0].testmempoolaccept(rawtxs=[tx2_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, tx2_hex, 0) + def test_too_many_replacements(self): """Replacements that evict too many transactions are rejected""" @@ -368,7 +398,13 @@ class ReplaceByFeeTest(BitcoinTestFramework): double_tx_hex = double_tx.serialize().hex() # This will raise an exception - assert_raises_rpc_error(-26, "too many potential replacements", self.nodes[0].sendrawtransaction, double_tx_hex, 0) + reject_reason = "too many potential replacements" + reject_details = f"{reject_reason}, rejecting replacement {double_tx.hash}; too many potential replacements ({MAX_REPLACEMENT_LIMIT + 1} > {MAX_REPLACEMENT_LIMIT})" + res = self.nodes[0].testmempoolaccept(rawtxs=[double_tx_hex])[0] + assert_equal(res["reject-reason"], reject_reason) + assert_equal(res["reject-details"], reject_details) + assert_raises_rpc_error(-26, f"{reject_details}", self.nodes[0].sendrawtransaction, double_tx_hex, 0) + # If we remove an input, it should pass double_tx.vin.pop() diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index ea609bbb34..27ecc3b4a8 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -67,6 +67,8 @@ class MempoolAcceptanceTest(BitcoinTestFramework): if "fees" in r: r["fees"].pop("effective-feerate") r["fees"].pop("effective-includes") + if "reject-details" in r: + r.pop("reject-details") assert_equal(result_expected, result_test) assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py index e00e7b1ae4..f74d00e37c 100755 --- a/test/functional/mempool_accept_wtxid.py +++ b/test/functional/mempool_accept_wtxid.py @@ -100,13 +100,15 @@ class MempoolWtxidTest(BitcoinTestFramework): "txid": child_one_txid, "wtxid": child_one_wtxid, "allowed": False, - "reject-reason": "txn-already-in-mempool" + "reject-reason": "txn-already-in-mempool", + "reject-details": "txn-already-in-mempool" }]) assert_equal(node.testmempoolaccept([child_two.serialize().hex()])[0], { "txid": child_two_txid, "wtxid": child_two_wtxid, "allowed": False, - "reject-reason": "txn-same-nonwitness-data-in-mempool" + "reject-reason": "txn-same-nonwitness-data-in-mempool", + "reject-details": "txn-same-nonwitness-data-in-mempool" }) # sendrawtransaction will not throw but quits early when the exact same transaction is already in mempool diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py index 7c0947a33d..4dc6f8fe36 100755 --- a/test/functional/mempool_package_rbf.py +++ b/test/functional/mempool_package_rbf.py @@ -219,7 +219,7 @@ class PackageRBFTest(BitcoinTestFramework): package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) - assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (102 > 100)\n", pkg_results["package_msg"]) + assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (102 > 100)", pkg_results["package_msg"]) self.assert_mempool_contents(expected=expected_txns) # Make singleton tx to conflict with in next batch @@ -234,7 +234,7 @@ class PackageRBFTest(BitcoinTestFramework): package_parent = self.wallet.create_self_transfer_multi(utxos_to_spend=double_spending_coins, fee_per_output=parent_fee_per_conflict) package_child = self.wallet.create_self_transfer(fee_rate=child_feerate, utxo_to_spend=package_parent["new_utxos"][0]) pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0) - assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (101 > 100)\n", pkg_results["package_msg"]) + assert_equal(f"package RBF failed: too many potential replacements, rejecting replacement {package_child['tx'].rehash()}; too many potential replacements (101 > 100)", pkg_results["package_msg"]) self.assert_mempool_contents(expected=expected_txns) # Finally, evict MAX_REPLACEMENT_CANDIDATES diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index f5d42d0c34..a2f9210f94 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -110,17 +110,21 @@ class RPCPackagesTest(BitcoinTestFramework): self.assert_testres_equal(package_bad, testres_bad) self.log.info("Check testmempoolaccept tells us when some transactions completed validation successfully") - tx_bad_sig_hex = node.createrawtransaction([{"txid": coin["txid"], "vout": 0}], + tx_bad_sig_hex = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["amount"] - Decimal("0.0001")}) tx_bad_sig = tx_from_hex(tx_bad_sig_hex) testres_bad_sig = node.testmempoolaccept(self.independent_txns_hex + [tx_bad_sig_hex]) # By the time the signature for the last transaction is checked, all the other transactions # have been fully validated, which is why the node returns full validation results for all # transactions here but empty results in other cases. + tx_bad_sig_txid = tx_bad_sig.rehash() + tx_bad_sig_wtxid = tx_bad_sig.getwtxid() assert_equal(testres_bad_sig, self.independent_txns_testres + [{ - "txid": tx_bad_sig.rehash(), - "wtxid": tx_bad_sig.getwtxid(), "allowed": False, - "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)" + "txid": tx_bad_sig_txid, + "wtxid": tx_bad_sig_wtxid, "allowed": False, + "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", + "reject-details": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size), " + + f"input 0 of {tx_bad_sig_txid} (wtxid {tx_bad_sig_wtxid}), spending {coin['txid']}:{coin['vout']}" }]) self.log.info("Check testmempoolaccept reports txns in packages that exceed max feerate") @@ -304,7 +308,8 @@ class RPCPackagesTest(BitcoinTestFramework): 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" + "reject-reason": "bip125-replacement-disallowed", + "reject-details": "bip125-replacement-disallowed" }] self.assert_testres_equal(self.independent_txns_hex + [replacement_tx["hex"]], testres_rbf_package)