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

+            "\nAlso see sendrawtransaction call.\n"
+            "\nArguments:\n"
+            "1. \"hexstring\"    (string, required) The hex string of the raw transaction)\n"
+            "2. allowhighfees    (boolean, optional, default=false) Allow high fees\n"


Fix alignment.

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

src/rpc/rawtransaction.cpp

+    UniValue result(UniValue::VOBJ);
+    result.push_back(Pair("hex", tx_hash.GetHex()));
+
+    {


IMO as it is this block is not needed.

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

src/rpc/rawtransaction.cpp

+            bool is_tx_in_chain = !view.AccessCoin(COutPoint(tx_hash, o)).IsSpent();
+            if (is_tx_in_chain) {
+                result.push_back(Pair("allowed", false));
+                result.push_back(Pair("error", "transaction already in block chain"));


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

src/validation.cpp

@@ -4386,7 +4395,8 @@ bool LoadMempool(void)
            if (nTime + nExpiryTimeout > nNow) {
                LOCK(cs_main);
                AcceptToMemoryPoolWithTime(chainparams, mempool, state, tx, nullptr /* pfMissingInputs */, nTime,
-                                           nullptr /* plTxnReplaced */, false /* bypass_limits */, 0 /* nAbsurdFee */);
+                                           nullptr /* plTxnReplaced */, false /* bypass_limits */, 0 /* nAbsurdFee */,
+                                           nullptr /* test_accept */);


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

@@ -998,6 +1078,7 @@ static const CRPCCommand commands[] =
    { "rawtransactions",    "sendrawtransaction",     &sendrawtransaction,     {"hexstring","allowhighfees"} },
    { "rawtransactions",    "combinerawtransaction",  &combinerawtransaction,  {"txs"} },
    { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */
+    { "rawtransactions",    "testmempoolaccept",      &testmempoolaccept,       {"hexstring","allowhighfees"} },


Extra space.

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

src/rpc/rawtransaction.cpp

+            + HelpExampleCli("createrawtransaction", "\"[{\\\"txid\\\" : \\\"mytxid\\\",\\\"vout\\\":0}]\" \"{\\\"myaddress\\\":0.01}\"") +
+            "Sign the transaction, and get back the hex\n"
+            + HelpExampleCli("signrawtransaction", "\"myhex\"") +
+            "\nSend the transaction (signed hex)\n"


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

src/rpc/rawtransaction.cpp

+            "{\n"
+            "  \"hex\"            (string) The transaction hash in hex\n"
+            "  \"allowed\"        (boolean) If the mempool allows this tx to be inserted\n"
+            "  \"reject-reason\"  (string) rejection string (if any)\n"


error? optional?

sipa

utACK fa12a9481ea7d930c75f109f3c10b200db70e501

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

src/txmempool.h

@@ -578,7 +578,7 @@ class CTxMemPool
    /** Populate setDescendants with all in-mempool descendants of hash.
     *  Assumes that setDescendants includes all in-mempool descendants of anything
     *  already in it.  */
-    void CalculateDescendants(txiter it, setEntries &setDescendants);
+    void CalculateDescendants(const txiter it, setEntries& setDescendants) const;


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

+            "2. allowhighfees    (boolean, optional, default=false) Allow high fees\n"
+            "\nResult:\n"
+            "{\n"
+            "  \"hex\"            (string) The transaction hash in hex\n"


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

+    result.push_back(Pair("hex", tx_hash.GetHex()));
+
+    LOCK(cs_main);
+    const CCoinsViewCache& view = *pcoinsTip;


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

+            "\nExamples:\n"
+            "\nCreate a transaction\n"
+            + HelpExampleCli("createrawtransaction", "\"[{\\\"txid\\\" : \\\"mytxid\\\",\\\"vout\\\":0}]\" \"{\\\"myaddress\\\":0.01}\"") +
+            "Sign the transaction, and get back the hex\n"


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

+            "{\n"
+            "  \"hex\"            (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'==false)\n"


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