include verbose "debug-message" field in testmempoolaccept response #28121

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

Adds a new field debug-message in testmempoolaccept responses to include m_debug_message from ValidationState. This string is part of 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",
  "debug-message": "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

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	glozow, 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

Reviewers, this pull request conflicts with the following ones:

#28676 ([WIP] Cluster mempool implementation by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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 bcdc364 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

+        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']


In commit bafd175, 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-04-16T19:59:39Z

Thanks for taking a look, @0xB10C. I've refactored the PR to add a new field instead of changing anything already in place. Edited PR title and description. I made the tests super clear that the complete string is returned by submitrawtransaction whereas its split into two fields in testmempoolaccept

0xB10C · 2024-05-05T10:21:32Z

I've played around a bit with this in 0xB10C/find-non-standard-tx#2.

I've seen output for the following reasons:

too-long-mempool-chain: e.g.
- exceeds descendant size limit for tx 9f95e53da39ece93ceba3cc78a443a1aef0fc656bf921bbd2aee7f6533f0604f [limit: 101000]
- exceeds ancestor size limit [limit: 101000]
- 2256302dedb181f83bc51248215e51f680f77d07e11ad7e9e34c2e483ad10e7b [limit: 26]
min relay fee not met: e.g. 0 < 200

Other reasons like e.g. scriptpubkey, tx-size, dust, missing-inputs, multi-op-return have an empty debug-message.

I haven't yet taken a closer look at the code.

pinheadmz · 2024-05-06T15:42:41Z

Other reasons like e.g. scriptpubkey, tx-size, dust, missing-inputs, multi-op-return have an empty debug-message.

debug-message was empty like ""? I'd expect the key to not be present at all:

if (!state.GetDebugMessage().empty()) result_inner.pushKV("debug-message", state.GetDebugMessage());

0xB10C · 2024-05-06T16:31:53Z

debug-message was empty like ""? I'd expect the key to not be present at all:

You're right. I should have been clearer in my comment: the field wasn't present. My tooling handles the missing field by setting the value to "".

pinheadmz · 2024-05-14T19:08:19Z

force-push to fix merge conflict which also looked like a git mistake on my part in the first place 😬

jonatack

Concept ACK d2917e7

jonatack · 2024-05-16T22:48:06Z

src/rpc/mempool.cpp

@@ -141,6 +141,7 @@ static RPCHelpMan testmempoolaccept()
                        }},
                    }},
                    {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"},
+                    {RPCResult::Type::STR, "debug-message", /*optional=*/true, "Rejection details (only present when 'allowed' is false and a message is provided)"},


c1be9d4

it might be more user-friendly to call the field reject-details?

edit: (also, per include verbose "debug-message" field in testmempoolaccept response #28121 (comment), the rationale for adding this field isn't only for debugging)

maybe just me, but a message is provided is potentially confusing, as it might sound like user input

Suggested change

{RPCResult::Type::STR, "debug-message", /*optional=*/true, "Rejection details (only present when 'allowed' is false and a message is provided)"},

{RPCResult::Type::STR, "reject-details", /*optional=*/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"},

jonatack · 2024-05-16T22:48:19Z

src/rpc/mempool.cpp

@@ -141,6 +141,7 @@ static RPCHelpMan testmempoolaccept()
                        }},
                    }},
                    {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"},


c1be9d4 while touching the next line, string here might be a relic from when the field type wasn't auto-generated in the help, ISTM reason would be clearer

Suggested change

{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)"},

jonatack · 2024-05-16T22:54:46Z

doc/release-notes-28121.md

@@ -0,0 +1,5 @@
+RPC


d2917e7 unsure if Updated RPCs would be better here, feel free to ignore

jonatack · 2024-05-16T22:57:28Z

doc/release-notes-28121.md

+RPC
+---
+
+`testmempoolaccept` response will now include verbose field "debug-message"


d2917e7 the new field won't always be included (and why "verbose"?)

Suggested change

`testmempoolaccept` response will now include verbose field "debug-message"

The RPC `testmempoolaccept` response now includes a "reject-details" field in some cases

pinheadmz · 2024-06-06T20:04:34Z

Thanks for reviewing @jonatack I like the idea of "reject-details" and addressed your other comments as well

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

[WIP] Cluster mempool implementation #28676

Draft

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

Open

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 from e2f22e5 to a3a4ade Compare April 16, 2024 20:17

DrahtBot removed the CI failed label Apr 16, 2024

0xB10C added a commit to 0xB10C/rust-bitcoincore-rpc that referenced this pull request Apr 18, 2024

add: support for debug-message from bitcoin/bitcoin#28121

087866a

0xB10C mentioned this pull request Apr 18, 2024

add: debug_message to csv output 0xB10C/find-non-standard-tx#2

Open

DrahtBot mentioned this pull request May 1, 2024

test: use sleepy wait-for-log in reindex readonly #30006

Merged

DrahtBot added the Needs rebase label May 13, 2024

pinheadmz force-pushed the tma-debug branch from a3a4ade to d2917e7 Compare May 14, 2024 19:07

DrahtBot removed the Needs rebase label May 14, 2024

jonatack reviewed May 16, 2024

View reviewed changes

pinheadmz added 4 commits June 6, 2024 16:02

rpc: include verbose debug-message field in testmempoolaccept response

9b4184b

rbf: remove unecessary newline at end of error string

422e949

test: cover testmempoolaccept debug-message in RBF test

f8ee85a

doc: add release note about testmempoolaccept debug-message

17286b4

pinheadmz force-pushed the tma-debug branch from d2917e7 to 17286b4 Compare June 6, 2024 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include verbose "debug-message" field in testmempoolaccept response #28121

include verbose "debug-message" field in testmempoolaccept response #28121

pinheadmz commented Jul 21, 2023 •

edited

DrahtBot commented Jul 21, 2023 •

edited

luke-jr commented Jul 25, 2023

pinheadmz commented Jul 26, 2023

glozow left a comment

pinheadmz commented Sep 1, 2023

jonatack left a comment

jonatack Sep 7, 2023

glozow commented Sep 26, 2023

pinheadmz commented Sep 26, 2023

DrahtBot commented Sep 27, 2023

pinheadmz commented Oct 17, 2023

instagibbs commented Oct 19, 2023

pinheadmz commented Oct 19, 2023

instagibbs commented Oct 19, 2023 •

edited

pinheadmz commented Oct 19, 2023

kristapsk commented Dec 20, 2023

0xB10C commented Apr 9, 2024 •

edited

pinheadmz commented Apr 16, 2024

0xB10C commented May 5, 2024

pinheadmz commented May 6, 2024

0xB10C commented May 6, 2024

pinheadmz commented May 14, 2024

jonatack left a comment

jonatack May 16, 2024 •

edited

jonatack May 16, 2024

jonatack May 16, 2024 •

edited

jonatack May 16, 2024 •

edited

pinheadmz commented Jun 6, 2024

	{RPCResult::Type::STR, "debug-message", /optional=/true, "Rejection details (only present when 'allowed' is false and a message is provided)"},
	{RPCResult::Type::STR, "reject-details", /optional=/true, "Rejection details (only present when 'allowed' is false and rejection details exist)"},

	{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)"},

	`testmempoolaccept` response will now include verbose field "debug-message"
	The RPC `testmempoolaccept` response now includes a "reject-details" field in some cases

include verbose "debug-message" field in testmempoolaccept response #28121

Are you sure you want to change the base?

include verbose "debug-message" field in testmempoolaccept response #28121

Conversation

pinheadmz commented Jul 21, 2023 • edited

DrahtBot commented Jul 21, 2023 • edited

Code Coverage

Reviews

Conflicts

luke-jr commented Jul 25, 2023

pinheadmz commented Jul 26, 2023

glozow left a comment

Choose a reason for hiding this comment

pinheadmz commented Sep 1, 2023

jonatack left a comment

Choose a reason for hiding this comment

jonatack Sep 7, 2023

Choose a reason for hiding this comment

glozow commented Sep 26, 2023

pinheadmz commented Sep 26, 2023

DrahtBot commented Sep 27, 2023

pinheadmz commented Oct 17, 2023

instagibbs commented Oct 19, 2023

pinheadmz commented Oct 19, 2023

instagibbs commented Oct 19, 2023 • edited

pinheadmz commented Oct 19, 2023

kristapsk commented Dec 20, 2023

0xB10C commented Apr 9, 2024 • edited

pinheadmz commented Apr 16, 2024

0xB10C commented May 5, 2024

pinheadmz commented May 6, 2024

0xB10C commented May 6, 2024

pinheadmz commented May 14, 2024

jonatack left a comment

Choose a reason for hiding this comment

jonatack May 16, 2024 • edited

Choose a reason for hiding this comment

jonatack May 16, 2024

Choose a reason for hiding this comment

jonatack May 16, 2024 • edited

Choose a reason for hiding this comment

jonatack May 16, 2024 • edited

Choose a reason for hiding this comment

pinheadmz commented Jun 6, 2024

pinheadmz commented Jul 21, 2023 •

edited

DrahtBot commented Jul 21, 2023 •

edited

instagibbs commented Oct 19, 2023 •

edited

0xB10C commented Apr 9, 2024 •

edited

jonatack May 16, 2024 •

edited

jonatack May 16, 2024 •

edited

jonatack May 16, 2024 •

edited