rpc: Add testmempoolaccept #11742

maflcko · 2017-11-21T13:49:41Z

To check if a single raw transaction makes it into the current transaction pool, one had to call sendrawtransaction. However, on success, this adds the transaction to the mempool with no easy way to undo.

The call testmempoolaccept is introduced to provide a way to solely check the result without changing the mempool state.

promag

Should note that two equal calls to testmempoolaccept in a short period can return different values?

promag · 2017-11-21T15:43:14Z

src/rpc/rawtransaction.cpp

Fix alignment.

promag · 2017-11-21T15:44:30Z

src/rpc/rawtransaction.cpp

IMO as it is this block is not needed.

promag · 2017-11-21T15:45:30Z

src/rpc/rawtransaction.cpp

promag · 2017-11-21T15:49:16Z

src/validation.cpp

Already default value. Or remove default value from declaration?

Note this is ATMPWT, not ATMP ;)

promag · 2017-11-22T18:35:45Z

src/rpc/rawtransaction.cpp

Extra space.

promag · 2017-11-22T18:37:34Z

src/rpc/rawtransaction.cpp

promag · 2017-11-22T18:41:37Z

src/rpc/rawtransaction.cpp

error? optional?

sipa

utACK fa12a9481ea7d930c75f109f3c10b200db70e501

sipa · 2017-12-06T04:27:59Z

src/txmempool.h

This first const has no effect.

NicolasDorier · 2017-12-06T04:57:26Z

I am so happy about this, big concept ACK. Some tests would be nice.

jonasschnelli · 2017-12-06T07:07:00Z

Nice. Finally.
utACK fa12a9481ea7d930c75f109f3c10b200db70e501

A comment for the new parameter of AcceptToMemoryPool in validation.h would be nice.

TheBlueMatt

Cool.

TheBlueMatt · 2017-12-06T22:21:15Z

src/rpc/rawtransaction.cpp

Seems somewhat redundant to return the parameter as-is, no?

Thanks, will rename this to txid and the parameter to rawtx to make clear that they are different.

TheBlueMatt · 2017-12-06T22:37:29Z

src/rpc/rawtransaction.cpp

Ugh, I meannnnn, checking against pcoinsTip really sucks. Maybe just call ATMP and then check for the "txn-already-known" return? Or just return false (since its not "accepted to mempool", so I'd kinda expect that) and the rejection reason will be txn-already-known.

I guess this was borrowed from sendrawtransaction.

There, there is an extra test bool fHaveMempool = mempool.exists(hashTx),
but it could be replaced by checking the ATMP error txn-already-in-mempool.

However in other PR I recall @sipa said it was kind of bad thing to rely on these rejection reasons.

Yeah, this is mostly a convenience check to provide a helpful message in case the tx recently confirmed for whatever reason. Otherwise you'd be left with missing_inputs, which is correct but might not be helpful at first.

On a second thought, I don't think it makes sense to have the blockchain check here. Will return false as suggested by @TheBlueMatt

promag

Are you planning to add functional test in this PR?

promag · 2017-12-06T23:05:32Z

src/rpc/rawtransaction.cpp

Missing \n.

There is no new line to indicate a paragraph, which groups the rpcs into two logical sections.

promag · 2017-12-06T23:12:32Z

src/rpc/rawtransaction.cpp

when 'allowed' is false

Thx, fixed.

maflcko · 2017-12-14T04:31:59Z

Removed the unused imports to make travis green.

instagibbs · 2017-12-19T14:45:16Z

can someone remind me why this one is acceptable while the other ~dozen attempts have failed?

NicolasDorier · 2017-12-19T16:32:31Z

@instagibbs Revelant history #7552 from @laanwj

I think this one can work because it has a better name. verifyrawtransaction is complex because there is lot's of bike-shed about the args and what verifying means. The meaning of testmempoolaccept is obvious and can't be misinterpreted, nor would it make sense to have more parameters.

instagibbs

utACK

instagibbs · 2017-12-19T16:50:56Z

src/rpc/rawtransaction.cpp

+        } else if (missing_inputs) {
+            result.push_back(Pair("reject-reason", "Missing inputs"));
+        } else {
+            result.push_back(Pair("reject-reason", state.GetRejectReason()));


I understand this is reflected in sendraw as well, but humor me: what times is state not IsInvalid but it would reject it?

maflcko · 2017-12-19T18:18:26Z

@instagibbs Previous pulls were:

[WIP] [RPC] Add verifyrawtransaction RPC #11201: Solves the general case (a list of transactions), but ended up in a state of {WIP, needs review, will revisit later}. Haven't checked what is actually missing there.
rpc: Add verifyrawtransactions call #7552: Also solves the general case (a list of transactions), but tried to implement some tx pool checking logic in a separate function. Then people raised concerns that this might result in unwanted behavior. (I.e. verifytransaction tells you everything is fine, but your mempool rejects). Also ended in a state {WIP, needs review, will revisit later}...

I think this pull (testmempoolaccept) nicely exploits the existing checks for consensus/standardness that are called in ATMP, without touching too much of validation code. A future pull can extend this rpc to take lists of more than one raw transaction and feed them into an ephemeral mempool (e.g. based on the current mempool).

greenaddress · 2017-12-19T21:17:44Z

conceptACK, seems useful and more accessible/convenient than current alternatives

promag · 2017-12-19T23:26:27Z

src/rpc/rawtransaction.cpp

+            // clang-format off
+            "testmempoolaccept [\"rawtxs\"] ( allowhighfees )\n"
+            "\nReturns if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
+            "\nThis checks if the transaction violates our consesus or policy rules.\n"


Type: consesus.

Nit, s/our/the.

promag · 2017-12-19T23:26:56Z

src/rpc/rawtransaction.cpp

+            "testmempoolaccept [\"rawtxs\"] ( allowhighfees )\n"
+            "\nReturns if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
+            "\nThis checks if the transaction violates our consesus or policy rules.\n"
+            "\nAlso see sendrawtransaction call.\n"


Nit, just See sendrawtransaction.

promag · 2017-12-19T23:28:56Z

src/rpc/rawtransaction.cpp

+
+    RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VBOOL});
+    if (request.params[0].get_array().size() != 1) {
+        throw JSONRPCError(RPC_TYPE_ERROR, "Array must contain exactly one raw transaction for now");


RPC_INVALID_PARAMETER instead? Type is already correct (it's an array at least).

promag · 2017-12-19T23:31:24Z

src/rpc/rawtransaction.cpp

+    }
+
+    UniValue result(UniValue::VARR);
+


Nit, remove empty line.

promag · 2017-12-19T23:33:15Z

src/rpc/rawtransaction.cpp

+            " {\n"
+            "  \"txid\"           (string) The transaction hash in hex\n"
+            "  \"allowed\"        (boolean) If the mempool allows this tx to be inserted\n"
+            "  \"reject-reason\"  (string) rejection string (only present when 'allowed' is false)\n"


Nit, upper case Rejection.

promag · 2017-12-19T23:34:41Z

src/rpc/rawtransaction.cpp

+
+    ObserveSafeMode();
+
+    RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VBOOL});


Remove? Below there is request.params[0].get_array() and request.params[1].get_bool().

That is the way we do it everywhere. I assume the rational is to fail early and provide useful feedback instead of accepting/processing each parameter separately and leaving any later ones unchecked.

The error would be different thought. I don't think failing a bit later is that bad. This also doesn't work well with polymorphic arguments. At the end we have a mix of these, so I tend to use univalue getters to validate. We could add support to throw if any extra parameter is not get/validated.

Yes, the error is different in that the RPCTypeCheck will tell you what the wrong type was, which is useful.
I fail to see how the edge case of polymorphic arguments is relevant to this pull request.

nit, missing test for second argument type and also tests for missing/extra arguments.

promag · 2017-12-19T23:37:41Z

src/rpc/rawtransaction.cpp

+    CValidationState state;
+    bool missing_inputs;
+    bool test_accept_res;
+    bool res = AcceptToMemoryPool(mempool, state, std::move(tx), &missing_inputs,


Nit, we could avoid loose locks, I mean, it's more clear where the lock/unlock happens:

{ LOCK(cs_main); bool res = AcceptToMemoryPool(...); assert(!res); }

promag · 2017-12-19T23:40:14Z

src/rpc/rawtransaction.cpp

+        if (state.IsInvalid()) {
+            result_0.pushKV("reject-reason", strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason()));
+        } else if (missing_inputs) {
+            result_0.pushKV("reject-reason", "Missing inputs");


Nit, not sure if relevant at all, but this format doesn't match with others. How about missing-inputs?

maflcko · 2017-12-20T22:17:38Z

Added a commit to address @promag's feedback.

laanwj · 2018-02-16T17:03:22Z

Concept ACK

maflcko · 2018-03-29T15:12:47Z

I can do that refactoring in a follow up pr, if people think that is helpful.

maflcko · 2018-03-30T20:16:36Z

Fun-Fact: I just realized an identical implementation was committed in gfanti@7148d4b a year ago. (Not including rpc changes and tests)

instagibbs · 2018-04-02T13:53:07Z

utACK b55555d

b55555d rpc: Add testmempoolaccept (MarcoFalke) Pull request description: To check if a single raw transaction makes it into the current transaction pool, one had to call `sendrawtransaction`. However, on success, this adds the transaction to the mempool with no easy way to undo. The call `testmempoolaccept` is introduced to provide a way to solely check the result without changing the mempool state. Tree-SHA512: 5afd9311190135cee8fc1f229c7d39bf893f1028f29e28d34f70df820198ff97b4bf86b41cbbd6e6c36a5c30073cefa92d541c74a4939c7a2a6fa283dfd41b63

jamesob · 2018-04-02T15:06:40Z

Post-merge utACK b55555d

Change looks useful and well implemented. The tests are great! In some sense it's sad that the functional tests you've introduced for this particular RPC call are the only place where we're testing basic mempool rejection logic (e.g. scriptsig-not-pushonly, multi-op-return), but it's great that we've got explicit tests for those cases now.

instagibbs · 2018-04-02T18:28:53Z

yes, forgot to mention the tests were very extensive and impressive

jtimon · 2018-04-03T02:31:19Z

post merge fast review ack. +1 on this being interesting for the tests alone, even if nobody was going to use it (but I know some people will, and I remember concept acking something very similar before, for traceability, #7552 ).

fafcad3 doc: Add testmempoolaccept to release-notes (MarcoFalke) Pull request description: Some fixups for #11742: * Add release notes for the new rpc * Fix a typo in the original pull * Make the mempool reference passed to `CheckInputsFromMempoolAndCache` const, since that function is called before we return from ATMP and we must not modify the mempool. Tree-SHA512: 72c459ba69f7698a69c91d2592f10f7fb1864846c7d8c525050d48286f92ba5ec5fe554c54235b52fbd9a8f00226c526ad84584641ec39084e1a1310a261510d

glozow · 2020-08-12T17:01:47Z

src/rpc/rawtransaction.cpp

+
+    RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VBOOL});
+    if (request.params[0].get_array().size() != 1) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Array must contain exactly one raw transaction for now");


Hey @MarcoFalke, sorry for the throwback, but was there a plan have testmempoolaccept accept multiple transactions at some point?

Yes, see #11742 (comment)

Fwiw i opened an issue to track this some time ago #18480 . Feel free to pick it up if you want (just signal it so we don't end up both working on it :) ), as i'm not going to work on this soon.

b55555d rpc: Add testmempoolaccept (MarcoFalke) Pull request description: To check if a single raw transaction makes it into the current transaction pool, one had to call `sendrawtransaction`. However, on success, this adds the transaction to the mempool with no easy way to undo. The call `testmempoolaccept` is introduced to provide a way to solely check the result without changing the mempool state. Tree-SHA512: 5afd9311190135cee8fc1f229c7d39bf893f1028f29e28d34f70df820198ff97b4bf86b41cbbd6e6c36a5c30073cefa92d541c74a4939c7a2a6fa283dfd41b63

…ing const fafcad3 doc: Add testmempoolaccept to release-notes (MarcoFalke) Pull request description: Some fixups for bitcoin#11742: * Add release notes for the new rpc * Fix a typo in the original pull * Make the mempool reference passed to `CheckInputsFromMempoolAndCache` const, since that function is called before we return from ATMP and we must not modify the mempool. Tree-SHA512: 72c459ba69f7698a69c91d2592f10f7fb1864846c7d8c525050d48286f92ba5ec5fe554c54235b52fbd9a8f00226c526ad84584641ec39084e1a1310a261510d

b55555d rpc: Add testmempoolaccept (MarcoFalke) Pull request description: To check if a single raw transaction makes it into the current transaction pool, one had to call `sendrawtransaction`. However, on success, this adds the transaction to the mempool with no easy way to undo. The call `testmempoolaccept` is introduced to provide a way to solely check the result without changing the mempool state. Tree-SHA512: 5afd9311190135cee8fc1f229c7d39bf893f1028f29e28d34f70df820198ff97b4bf86b41cbbd6e6c36a5c30073cefa92d541c74a4939c7a2a6fa283dfd41b63

…ing const fafcad3 doc: Add testmempoolaccept to release-notes (MarcoFalke) Pull request description: Some fixups for bitcoin#11742: * Add release notes for the new rpc * Fix a typo in the original pull * Make the mempool reference passed to `CheckInputsFromMempoolAndCache` const, since that function is called before we return from ATMP and we must not modify the mempool. Tree-SHA512: 72c459ba69f7698a69c91d2592f10f7fb1864846c7d8c525050d48286f92ba5ec5fe554c54235b52fbd9a8f00226c526ad84584641ec39084e1a1310a261510d

maflcko added RPC/REST/ZMQ Validation labels Nov 21, 2017

promag reviewed Nov 21, 2017

View reviewed changes

maflcko force-pushed the Mf1711-rpcMempoolAcceptOne branch 2 times, most recently from fa6f969 to e0c1f05 Compare November 22, 2017 18:20

promag reviewed Nov 22, 2017

View reviewed changes

maflcko force-pushed the Mf1711-rpcMempoolAcceptOne branch 2 times, most recently from 50ec02e to fa12a94 Compare December 6, 2017 01:49

sipa reviewed Dec 6, 2017

View reviewed changes

src/txmempool.h Outdated

Copy link

Member

sipa Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first const has no effect.

TheBlueMatt reviewed Dec 6, 2017

View reviewed changes

promag reviewed Dec 6, 2017

View reviewed changes

maflcko force-pushed the Mf1711-rpcMempoolAcceptOne branch 7 times, most recently from a9bc799 to 293b972 Compare December 13, 2017 22:03

maflcko force-pushed the Mf1711-rpcMempoolAcceptOne branch from 293b972 to fa82ebf Compare December 18, 2017 14:44

instagibbs approved these changes Dec 19, 2017

View reviewed changes

promag reviewed Dec 19, 2017

View reviewed changes

maflcko added the Needs release note label Apr 1, 2018

laanwj merged commit b55555d into bitcoin:master Apr 2, 2018

maflcko deleted the Mf1711-rpcMempoolAcceptOne branch April 2, 2018 15:10

maflcko removed the Needs release note label Apr 2, 2018

maflcko mentioned this pull request Apr 2, 2018

Add testmempoolaccept to release-notes, Add missing const #12860

Merged

maflcko mentioned this pull request Jun 24, 2018

Verify transaction without propagating #4162

Closed

darosior mentioned this pull request Apr 2, 2020

Transactions chain and testmempoolaccept #18480

Closed

glozow reviewed Aug 12, 2020

View reviewed changes

ariard mentioned this pull request May 10, 2021

rpc/validation: enable packages through testmempoolaccept #20833

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022


		ObserveSafeMode();

		RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VBOOL});

rpc: Add testmempoolaccept #11742

rpc: Add testmempoolaccept #11742

Uh oh!

Conversation

maflcko commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Dec 6, 2017

Uh oh!

jonasschnelli commented Dec 6, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 14, 2017

Uh oh!

instagibbs commented Dec 19, 2017

Uh oh!

NicolasDorier commented Dec 19, 2017

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greenaddress commented Dec 19, 2017

maflcko commented Nov 21, 2017 •

edited

Loading

maflcko commented Dec 19, 2017 •

edited

Loading

promag Mar 24, 2018 •

edited

Loading