include verbose "reject-details" field in testmempoolaccept response #28121

pinheadmz · 2023-07-21T14:47:06Z

Adds a new field reject-details in testmempoolaccept responses to include m_debug_message from ValidationState. This string is the complete error message thrown by the mempool in response to sendrawtransaction.

The extra verbosity is helpful to consumers of testmempoolaccept, which is sort of a debug tool anyway.

example:

{
"txid": "07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8",
"wtxid": "5dc243b1b92ee2f5a43134eb3e23449be03d1abb3d7f3c03c836ed0f13c50185",
"allowed": false,
"reject-reason": "insufficient fee",
"reject-details": "insufficient fee, rejecting replacement 07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB"
}

DrahtBot · 2023-07-21T14:47:09Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28121.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, glozow
Concept ACK	instagibbs, kristapsk, jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

luke-jr · 2023-07-25T18:35:44Z

Why not pass the details in m_debug_message and add that as a new field?

pinheadmz · 2023-07-26T13:48:13Z

Why not pass the details in m_debug_message and add that as a new field?

My thinking was to match exactly the single combined string returned by sendrawtransaction. But if adding the new field gets your ACK @luke-jr I'll make the change ;-)

glozow

Concept ACK to providing the debug string, I'm sure it'd be helpful for users. Would be nice to do this for submitpackage as well.

I'm not sure if we try to keep strings for reject-reason stable at all. If we do, I'd be in favor of adding a new field error_msg with the full string instead of a separate field with just the debug message.

pinheadmz · 2023-09-01T15:54:29Z

I'm not sure if we try to keep strings for reject-reason stable at all.

You mean like how block reject reasons are defined in BIP22?

jonatack

lgtm ACK bcdc3646c3e24930433e30b6363ca2d6f6fb9198 modulo the question of a new field vs the appending to the reject_reason output.

jonatack · 2023-09-07T21:45:48Z

test/functional/feature_rbf.py

In commit bafd175ca898c3ff74ae7927a3e9e3607ea3c8cd, can make a helper for this.

diff

--- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -107,6 +107,11 @@ class ReplaceByFeeTest(BitcoinTestFramework): return self.wallet.get_utxo(txid=tx["txid"], vout=tx["sent_vout"]) + def assert_rejected_by_testmempoolaccept(self, tx_hex, msg): + result = self.nodes[0].testmempoolaccept(rawtxs=[tx_hex])[0] + assert not result["allowed"] + assert msg in result["reject-reason"] + def test_simple_doublespend(self): """Simple doublespend""" # we use MiniWallet to create a transaction template with inputs correctly set, @@ -121,9 +126,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception due to insufficient fee debug_message = f"insufficient fee, rejecting replacement {tx.hash}; new feerate" assert_raises_rpc_error(-26, debug_message, self.nodes[0].sendrawtransaction, tx.serialize().hex(), 0) - res = self.nodes[0].testmempoolaccept(rawtxs=[tx.serialize().hex()])[0] - assert not res['allowed'] - assert debug_message in res['reject-reason'] + self.assert_rejected_by_testmempoolaccept(tx.serialize().hex(), debug_message) # Extra 0.1 BTC fee tx.vout[0].nValue -= int(0.1 * COIN) @@ -168,9 +171,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception due to insufficient fee debug_message = f"insufficient fee, rejecting replacement {dbl_tx['txid']}; less fees than conflicting txs" assert_raises_rpc_error(-26, debug_message, self.nodes[0].sendrawtransaction, dbl_tx["hex"], 0) - res = self.nodes[0].testmempoolaccept(rawtxs=[dbl_tx["hex"]])[0] - assert not res['allowed'] - assert debug_message in res['reject-reason'] + self.assert_rejected_by_testmempoolaccept(dbl_tx["hex"], debug_message) # Accepted with sufficient fee dbl_tx["tx"].vout[0].nValue = int(0.1 * COIN) @@ -307,9 +308,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception debug_message = f"bad-txns-spends-conflicting-tx, {tx2['txid']} spends conflicting transaction {tx1a['txid']}" assert_raises_rpc_error(-26, debug_message, self.nodes[0].sendrawtransaction, tx2["hex"], 0) - res = self.nodes[0].testmempoolaccept(rawtxs=[tx2["hex"]])[0] - assert not res['allowed'] - assert debug_message in res['reject-reason'] + self.assert_rejected_by_testmempoolaccept(tx2["hex"], debug_message) # Spend tx1a's output to test the indirect case. tx1b_utxo = self.wallet.send_self_transfer( @@ -349,9 +348,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception debug_message = f"replacement-adds-unconfirmed, replacement {tx2['txid']} adds unconfirmed input, idx" assert_raises_rpc_error(-26, debug_message, self.nodes[0].sendrawtransaction, tx2["hex"], 0) - res = self.nodes[0].testmempoolaccept(rawtxs=[tx2["hex"]])[0] - assert not res['allowed'] - assert debug_message in res['reject-reason'] + self.assert_rejected_by_testmempoolaccept(tx2["hex"], debug_message) def test_too_many_replacements(self): """Replacements that evict too many transactions are rejected""" @@ -394,9 +391,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): # This will raise an exception debug_message = f"too many potential replacements, rejecting replacement {double_tx['txid']}; too many potential replacements" assert_raises_rpc_error(-26, debug_message, self.nodes[0].sendrawtransaction, double_tx["hex"], 0) - res = self.nodes[0].testmempoolaccept(rawtxs=[double_tx["hex"]])[0] - assert not res['allowed'] - assert debug_message in res['reject-reason'] + self.assert_rejected_by_testmempoolaccept(double_tx["hex"], debug_message) # If we remove an input, it should pass double_tx["tx"].vin.pop() @@ -720,9 +715,7 @@ class ReplaceByFeeTest(BitcoinTestFramework): tx.rehash() debug_message = f"insufficient fee, rejecting replacement {tx.hash}; not enough additional fees to relay" assert_raises_rpc_error(-26, debug_message, self.nodes[0].sendrawtransaction, tx.serialize().hex()) - res = self.nodes[0].testmempoolaccept(rawtxs=[tx.serialize().hex()])[0] - assert not res['allowed'] - assert debug_message in res['reject-reason'] + self.assert_rejected_by_testmempoolaccept(tx.serialize().hex(), debug_message) def test_fullrbf(self):

glozow · 2023-09-26T12:16:10Z

I'm not sure if we try to keep strings for reject-reason stable at all.

You mean like how block reject reasons are defined in BIP22?

Yeah exactly. Like if somebody has a fee bumper that's triggered by reject-reason == "mempool min fee not met" or something.

pinheadmz · 2023-09-26T20:13:10Z

Yeah exactly. Like if somebody has a fee bumper that's triggered by reject-reason == "mempool min fee not met" or something.

Yeah ok so, the detailed message is appended to the original so "somebody" would just need to change their == to a "starts with" or "includes" or regex checker. In my opinion, this API-breaking change is why we have major version numbers and release notes. So I'm not sure how we decide whats precious or not. Either way is fine with me.

DrahtBot · 2023-09-27T06:38:23Z

Needs rebase if still relevant

pinheadmz · 2023-10-17T19:48:52Z

Needs rebase if still relevant

Done 🙏

instagibbs · 2023-10-19T14:50:19Z

concept ACK

unfortunately I know people do use reject-reason strings for various informational purposes. Would be a good project to gather those uses and maybe think of better error codes to capture these?

pinheadmz · 2023-10-19T14:57:11Z

@instagibbs thanks for taking a look! Not sure what you mean about gathering uses though. Are you agreeing with @luke-jr that the debug messages should have their own field in TMA response object and leave reject-reason alone?

instagibbs · 2023-10-19T14:58:26Z

Not sure what you mean about gathering uses though.

Looking at why error codes aren't enough for production uses

pinheadmz · 2023-10-19T15:08:02Z

Looking at why error codes aren't enough for production uses

I'm working on an RBF-batcher with CardCoins and we were getting insufficient fee errors in test from testmempoolaccept(), which could be reported from two different places. Inspiration for this PR came from that ambiguity.

bitcoin/src/validation.cpp

Line 983 in 6e721c9

    
           return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);

bitcoin/src/validation.cpp

Line 1004 in 6e721c9

    
           return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);

kristapsk · 2023-12-20T07:41:35Z

Concept ACK

0xB10C · 2024-04-09T15:08:38Z

I have a slight preference to keep the reject-reason string as is, but like the idea of having more information on rejections. In https://github.com/0xB10C/find-non-standard-tx and for https://bitcoin-data.github.io/non-standard-transactions/ I use the short reject-reason. Maybe add a new field for details?

pinheadmz · 2024-12-04T20:35:16Z

Rebased on master (+cmake), and then rebased on master again after #31112 merged. This PR now adds some extra test coverage to the logging in 31112 as well.

rkrux

Successful make and functional tests. Partial review, will complete the review soon.

I tested it out on local with 2 cases: passing in an unsigned transaction, and then passing in a tx with incorrect input.

Simulated `testmempoolaccept` failures

➜  ~ bitcoinclireg testmempoolaccept "[\"02000000011d7064f723f688af381d4f2d7f1d8c7ea9d1e0199531b636f3e7499d68dd21900000000000ffffffff0178de052a010000001600148d6b22f6e4c0c5dc96529ca5ebc6ad0cde42966800000000\"]"
[
  {
    "txid": "7d7d6f5e1e5fdc0955ac16309636597c5dc9e117950a3aee2203fb60177131fc",
    "wtxid": "7d7d6f5e1e5fdc0955ac16309636597c5dc9e117950a3aee2203fb60177131fc",
    "allowed": false,
    "reject-reason": "mandatory-script-verify-flag-failed (Witness program hash mismatch)",
    "reject-details": "mandatory-script-verify-flag-failed (Witness program hash mismatch), input 0 of 7d7d6f5e1e5fdc0955ac16309636597c5dc9e117950a3aee2203fb60177131fc (wtxid 7d7d6f5e1e5fdc0955ac16309636597c5dc9e117950a3aee2203fb60177131fc), spending 9021dd689d49e7f336b6319519e0d1a97e8c1d7f2d4f1d38af88f623f764701d:0"
  }
]
➜  ~ bitcoinclireg testmempoolaccept "[\"02000000012d7064f723f688af381d4f2d7f1d8c7ea9d1e0199531b636f3e7499d68dd21900000000000ffffffff0178de052a010000001600148d6b22f6e4c0c5dc96529ca5ebc6ad0cde42966800000000\"]"
[
  {
    "txid": "3b9d266452396611cc6843b2a39adc555538a6f7c59e4cbb653f5826756be0a1",
    "wtxid": "3b9d266452396611cc6843b2a39adc555538a6f7c59e4cbb653f5826756be0a1",
    "allowed": false,
    "reject-reason": "missing-inputs"
  }
]

rkrux · 2024-12-06T12:31:48Z

src/rpc/mempool.cpp

                    result_inner.pushKV("allowed", false);
                    const TxValidationState state = tx_result.m_state;
                    if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
                        result_inner.pushKV("reject-reason", "missing-inputs");


Not returning the details from here because it'd be same as "missing-inputs"?

Actually in this case m_reject_reason would be "bad-txns-inputs-missingorspent" and m_reject_details would be an empty string, but the RPC response has this hard coded instead (covered by mempool_accept.py)

rkrux · 2024-12-06T13:32:13Z

test/functional/feature_rbf.py

Nit: Extra line

got it thanks

rkrux · 2024-12-06T13:40:04Z

Rebased on master (+cmake), and then rebased on master again after #31112 merged. This PR now adds some extra test coverage to the logging in 31112 as well.

Thank you for rebasing on master, I can test it out on local now.

pinheadmz marked this pull request as ready for review July 21, 2023 14:52

DrahtBot mentioned this pull request Jul 22, 2023

Enforce incentive compatibility for all RBF replacements #26451

Closed

glozow added the RPC/REST/ZMQ label Jul 24, 2023

glozow reviewed Aug 30, 2023

View reviewed changes

jonatack reviewed Sep 7, 2023

View reviewed changes

DrahtBot added the CI failed label Sep 15, 2023

pinheadmz force-pushed the tma-debug branch from bcdc364 to 63430c3 Compare October 17, 2023 19:48

pinheadmz force-pushed the tma-debug branch from 63430c3 to d53bc34 Compare October 18, 2023 00:24

DrahtBot removed the CI failed label Oct 18, 2023

DrahtBot mentioned this pull request Oct 18, 2023

Cluster mempool implementation #28676

Closed

8 tasks

DrahtBot added the CI failed label Jan 7, 2024

DrahtBot mentioned this pull request Jan 18, 2024

Wallet: Add maxfeerate wallet startup option #29278

Draft

achow101 requested review from 0xB10C and theStack April 9, 2024 15:04

pinheadmz changed the title ~~include verbose debug messages in testmempoolaccept reject-reason~~ include verbose "debug-message" field in testmempoolaccept response Apr 16, 2024

pinheadmz force-pushed the tma-debug branch from d53bc34 to e2f22e5 Compare April 16, 2024 19:58

pinheadmz force-pushed the tma-debug branch 2 times, most recently from 6f4c80d to 08946d3 Compare December 4, 2024 19:00

Matthew Zipkin added 2 commits December 4, 2024 14:37

rpc: include verbose reject-details field in testmempoolaccept response

221c789

rbf: remove unecessary newline at end of error string

f9650e1

pinheadmz force-pushed the tma-debug branch from 08946d3 to ed33337 Compare December 4, 2024 19:38

DrahtBot mentioned this pull request Dec 5, 2024

test: implements helper functions for unit conversion #31420

Closed

DrahtBot removed the CI failed label Dec 5, 2024

rkrux reviewed Dec 6, 2024

View reviewed changes

rkrux approved these changes Dec 9, 2024

View reviewed changes

include verbose "reject-details" field in testmempoolaccept response #28121

include verbose "reject-details" field in testmempoolaccept response #28121

Uh oh!

Conversation

pinheadmz commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

luke-jr commented Jul 25, 2023

Uh oh!

pinheadmz commented Jul 26, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Sep 1, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

glozow commented Sep 26, 2023

Uh oh!

pinheadmz commented Sep 26, 2023

Uh oh!

DrahtBot commented Sep 27, 2023

Uh oh!

pinheadmz commented Oct 17, 2023

Uh oh!

instagibbs commented Oct 19, 2023

Uh oh!

pinheadmz commented Oct 19, 2023

Uh oh!

instagibbs commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented Oct 19, 2023

Uh oh!

kristapsk commented Dec 20, 2023

Uh oh!

0xB10C commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

pinheadmz Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

rkrux Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

pinheadmz Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

rkrux commented Dec 6, 2024

Uh oh!

pinheadmz commented Jul 21, 2023 •

edited

Loading

DrahtBot commented Jul 21, 2023 •

edited

Loading

instagibbs commented Oct 19, 2023 •

edited

Loading

0xB10C commented Apr 9, 2024 •

edited

Loading

pinheadmz commented Dec 4, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading