rpc: Add `verifyrawtransactions` call

laanwj commented at 5:55 pm on February 17, 2016: member

Add a RPC call to verify a set of raw transactions without propagating them. It has an extensible API to customize verification options.

Implements #4162.

 0verifyrawtransactions ["hexstring",...] ( options )
 1
 2Verifies one or more raw transactions (serialized, hex-encoded). If transactions depend on each other, they must be provided in order.
 3
 4Arguments:
 51. ["hexstring",...] (array of strings, required) The hex string of the raw transactions)
 62. options   (json object, optional)
 7     {
 8       "include_mempool"          (boolean, optional, default=true) Whether to include the mem pool
 9       "check_final"              (boolean, optional, default=true) Check that the transactions will be final by next block
10       "check_standard"           (boolean, optional, default=true) Perform transaction standard checks
11     }
12
13Result:
14null if the verification was successful, otherwise an error object:
15{
16  "index":n,                (numeric) Index in transactions array of failed transaction
17  "hash":"hex",             (string) Transaction hash of failed transaction
18  "code": n,                (numeric) Reject code
19  "reason": "text"          (string) Reject reason
20  "debug_message": "text"   (string) Reject debug message
21}

TODO:

~~RPC test~~
~~Call AreInputsStandard() to check inputs for standardness~~

laanwj added the label RPC on Feb 17, 2016

laanwj force-pushed on Feb 17, 2016

laanwj added the label Feature on Feb 17, 2016

jonasschnelli commented at 2:36 pm on February 18, 2016: contributor

Concept ACK.

dcousens commented at 1:47 am on February 19, 2016: contributor

Concept ACK

gmaxwell commented at 1:51 am on February 19, 2016: contributor

Concept-ACK.

Should the sense of final and non-standard be inverted in the options? It would be surprising to pass a transaction to this RPC, have it pass…. then find that the network won’t accept it. Practically speaking, except for special cases and developers standardness is pretty much as forceful as validity.

An obvious future extension wrt finality would be adding a field in the result that says when the transaction will become final.

dcousens commented at 4:20 am on February 19, 2016: contributor

~~@gmaxwell would it not be easier to make an optional parameter the estimated block height?~~ ~~This would probably need to be specified for each transaction if we’re passing a list (so you can do CSV chains etc).~~ Or would it be better to just give that feedback to the user as the “earliest possible block height” it could be included in? [per transaction]

edit: As suggested by @gmaxwell (I think), IMHO the best approach here is to give feedback to the user as to the earliest time they could be included.

laanwj commented at 8:25 am on February 19, 2016: member

it would be surprising to pass a transaction to this RPC, have it pass…. then find that the network won’t accept it.

That’s why those checks are enabled (true) by default. For specific cases they could be disabled. Only for the coinbase check I couldn’t think of any reason why one’d want to disable it, so I left that out.

An obvious future extension wrt finality would be adding a field in the result that says when the transaction will become final.

Yes, would be nice - by using an object both for the options and the error, I made the API extensible enough to add such bells and whistles. (either a “pass a simulated height”, or “report when final” approach, or both, could fit in)

laanwj force-pushed on Feb 19, 2016

instagibbs commented at 10:15 pm on February 19, 2016: member

~~concept~~ tested ACK, this is great for testing smart contract stuff with timelocks.

jtimon commented at 3:03 am on February 20, 2016: contributor

concept ack, but this is very incomplete, I suggest we keep it open without merging too fast.

We’re arbitrarily calling IsStandardTx() but not AreInputsStandard(). Those are just two easily functions containing relay/ming policy, but there’s much more related code spread out there.

Nit: Remove call to IsStandardTx() in verifytx, we can make another rpc call (or add an option in this one) called “verify tx according to consensus rules and also accept tx according to some confirgurable policy parameter”. But please let’s not add any policy code until we have a complete Consensus::VerifyTx(). Wait a minute, Consensus::VerifyTx() doesn’t even exist on master yet, not in a complete form and neither in an incomplete form.

Nit: Let’s please introduce Consensus::VerifyTx() before exposing anything else, otherwise you’ll have to duplicate some code from AcceptToMemoryPool and/or ConnectBlock. Could you please consider rebasing this PR on top of #7565 ?

dcousens commented at 4:05 pm on February 21, 2016: contributor

IMHO agreed this would be best implemented by Consensus::VerifyTx or the like.

I also like the idea of a parameter for rejection based purely on consensus, and irrespective of local node policy.

in src/rpc/rawtransaction.cpp: in 48a481a464 outdated

845+    if (fHelp || params.size() < 1 || params.size() > 2)
846+        throw runtime_error(
847+            "verifyrawtransactions [\"hexstring\",...] ( options )\n"
848+            "\nVerifies one or more raw transactions (serialized, hex-encoded). If transactions depend on each other, they must be provided in order.\n"
849+            "\nArguments:\n"
850+            "1. [\"hexstring\",...] (array of strings, required) The hex string of the raw transactions)\n"

MarcoFalke commented at 2:15 pm on February 25, 2016:

Nit: Appears to be missing the left parenthesis.

instagibbs commented at 2:18 pm on February 25, 2016:

or adding a superfluous right one

in src/rpc/rawtransaction.cpp: in 48a481a464 outdated

850+            "1. [\"hexstring\",...] (array of strings, required) The hex string of the raw transactions)\n"
851+            "2. options   (json object, optional)\n"
852+            "     {\n"
853+            "       \"include_mempool\"          (boolean, optional, default=true) Whether to include the mem pool\n"
854+            "       \"check_final\"              (boolean, optional, default=true) Check that the transactions will be final by next block\n"
855+            "       \"check_standard\"           (boolean, optional, default=true) Perform transaction standard checks\n"

jtimon commented at 7:22 pm on February 25, 2016:

As said I would leave this out for now, until we have more relay/mining policy encapsulated to expose here in a more generic way. EDIT: I didn’t noticed that you are also using this for choosing the script flags…then I guess I would one of these:

A) Replace this parameter with a flags one and leave calling to IsStandardTx() for later. B) Maintain IsStandardTx() but also add AreInputsStandard(). Replace \"check_standard\" (boolean, optional, default=true) with something more generic and extensible like \"policy\" (string, optional, default=standard).

I would personally prefer option A to avoid starting to duplicate policy code from AcceptToMemoryPool here.

laanwj commented at 1:35 pm on March 3, 2016: member

@jtimon I don’t feel like doing all kinds of refactors here (too much risk to break other things). Please just review this for correctness.

laanwj commented at 1:37 pm on March 3, 2016: member

I also like the idea of a parameter for rejection based purely on consensus, and irrespective of local node policy.

Right - that’s possible already. Disable check_standard, and possible check_final, or am I missing something?

jtimon commented at 1:46 pm on March 3, 2016: contributor

Fair enough, concept ack. I would still prefer a more generic and extensible option than bool check_standard (string policy=“default”/“none” ?). I still don’t know why isstandard is called but areinputsstandard isn’t. There’s more to-be-consensus code out of here (that’s what I mean by this is incomplete ).

laanwj commented at 1:47 pm on March 3, 2016: member

I still don’t know why isstandard is called but areinputsstandard isn’t.

Yes, that’s a legit bug (I added it to TODOs in the opening post)

I would still prefer a more generic and extensible option than bool check_standard (string policy=“default”/“none” ?).

What other options would you imagine there? Adding more options to the options structure is free, so there could always be a “standard_policy”: “string” added later.

in qa/rpc-tests/rawtransactions.py: in 48a481a464 outdated

224+        assert(err is None)
225+        err   = self.nodes[0].verifyrawtransactions([rawTx2],{'include_mempool':False})
226+        assert(err is not None)
227+        assert(err['code'] == 16)
228+        assert(err['reason'].startswith('bad-txns-inputs-missingorspent'))
229+

mrbandrews commented at 4:12 pm on March 10, 2016:

I wrote some code to test the ordering of dependent tx’s, then realized this code here does the same thing my code did. If you want to also test calling verifyrawtransaxtions with dependent tx’s, you could add this code at this point. It was suggested to me to copy-paste the git diff result:

 0diff --git a/qa/rpc-tests/rawtransactions.py b/qa/rpc-tests/rawtransactions.py
 1index 9039bc6..9dd8c84 100755
 2--- a/qa/rpc-tests/rawtransactions.py
 3+++ b/qa/rpc-tests/rawtransactions.py
 4@@ -226,6 +226,19 @@ class RawTransactionsTest(BitcoinTestFramework):
 5         assert(err['code'] == 16)
 6         assert(err['reason'].startswith('bad-txns-inputs-missingorspent'))
 7
 8+        #check ordering of dependent tx's in verifyrawtransactions
 9+        parent  = rawTxSigned['hex']
10+        child   = rawTx2
11+        err1     = self.nodes[2].verifyrawtransactions([parent, child], {'include_mempool':True})
12+        err2     = self.nodes[2].verifyrawtransactions([parent, child], {'include_mempool':False})
13+        assert(err1 is None and err2 is None)
14+
15+        # out-of-order succeeds with mempool, fails without
16+        err1     = self.nodes[2].verifyrawtransactions([child, parent], {'include_mempool':True})
17+        err2     = self.nodes[2].verifyrawtransactions([child, parent], {'include_mempool':False})
18+        assert(err1 is None and err2 is not None)
19+        assert(err2['reason'].startswith('bad-txns-inputs-missingorspent'))
20+
21         # mine the transaction into a block and check end result
22         rawTx = self.nodes[0].decoderawtransaction(rawTxSigned['hex'])
23         self.sync_all()

laanwj commented at 8:08 am on March 11, 2016:

Thanks, will add that test!

laanwj force-pushed on Mar 14, 2016

rpc: Add `verifyrawtransactions` call

Add a RPC call to verify a set of raw transactions without propagating them.

Implements #4162.

    verifyrawtransactions ["hexstring",...] ( options )

    Verifies one or more raw transactions (serialized, hex-encoded). If transactions depend on each other, they must be provided in order.

    Arguments:
    1. ["hexstring",...] (array of strings, required) The hex string of the raw transactions)
    2. options   (json object, optional)
         {
           "include_mempool"          (boolean, optional, default=true) Whether to include the mem pool
           "check_final"              (boolean, optional, default=true) Check that the transactions will be final by next block
           "check_standard"           (boolean, optional, default=true) Perform transaction standard checks
         }

    Result:
    null if the verification was successful, otherwise an error object:
    {
      "index":n,                (numeric) Index in transactions array of failed transaction
      "hash":"hex",             (string) Transaction hash of failed transaction
      "code": n,                (numeric) Reject code
      "reason": "text"          (string) Reject reason
      "debug_message": "text"   (string) Reject debug message
    }

add22596b1

squashme: improve logic, add tests eb76988fbc

squashme: check for invalid options

For future extensibility it's important that unrecognized options are rejected.

509da5dea4

squashme: add test for include_mempool option a8c26fcbed

squashme: obvious end result test d1cda05535

squashme: call CheckSequenceLocks and AreInputsStandard 0352287d03

test: Add dependent transaction test for `verifyrawtransactions` 1a714070e4

squashme: move CheckSequenceLocks after HaveInputs

more consistent with AcceptToMemoryPool

fffdc98d99

squashme: Factor out to VerifyTransaction function

Local for now, but this could, in time, be shared by
various places that want to verify transactions.

da0595da66

in qa/rpc-tests/rawtransactions.py: in 0ae331b5bd outdated

56@@ -56,6 +57,14 @@ def run_test(self):
57         rawtx   = self.nodes[2].createrawtransaction(inputs, outputs)
58         rawtx   = self.nodes[2].signrawtransaction(rawtx)
59 
60+        # check missing input also using verifyrawtransactions
61+        err   = self.nodes[2].verifyrawtransactions([rawtx['hex']])
62+        assert(err is not None)
63+        assert(err['index'] == 0)
64+        assert(err['code'] == 16)

MarcoFalke commented at 3:26 pm on March 14, 2016:

0Assertion failed: 
1
2  File "/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
3
4    self.run_test()
5
6  File "/home/travis/build/bitcoin/bitcoin/bitcoin-x86_64-unknown-linux-gnu/qa/rpc-tests/rawtransactions.py", line 64, in run_test
7
8    assert(err['code'] == 16)

laanwj commented at 5:29 pm on March 14, 2016:

Hmm, interesting, so CheckSequenceLocks actually looks up the inputs, moving it after HaveInputs (more consistent with AcceptToMemoryPool anyhow)

laanwj force-pushed on Mar 18, 2016

laanwj commented at 2:00 pm on March 18, 2016: member

I’ve factored out the transaction verification to a (local) VerifyTransaction function.

squashme: restructure VerifyTransaction

Use a more familiar structure now that VerifyTransaction is a function.

70ad9e320d

squashme: update release notes for new `verifyrawtransactions` RPC 85f77799a2

laanwj closed this on Mar 28, 2016

laanwj commented at 9:27 am on March 28, 2016: member

Too much disagreement on how this should be implemented, not going to bother anymore.

jtimon commented at 11:04 am on March 28, 2016: contributor

I don’t understand why this is closed. I asked to wait for some refactors first. You said no, fair enough, they can be done later. Then you create the verifyTx function, but not in the consensus package or dir, and including policy code. I can’t say I like that (I’m not going to link to the VerifyTx I like anymore because as said it requieres refactors and I was waiting for 0.12.1 first. I guess I wouldn’t mind if the function was called VerifyAndAcceptTx or something like that.

But why not continue with your original version? Wasn’t I the only person complaining and didn’t I said fair enough and concept ACK? I really can’t see “too much disagreement here, unless you’re talking about some conversation on IRC or somewhere else. If that’s the case, a link to the log of the conversation would be welcomed here for traceability.

laanwj commented at 11:17 am on March 28, 2016: member

But why not continue with your original version? Wasn’t I the only person complaining and didn’t I said fair enough and concept ACK? I really can’t see “too much disagreement here, unless you’re talking about some conversation on IRC

Well someone else on IRC was worried that the output of this doesn’t exactly match the output of sendrawtransaction.

0<runeks> wumpus: Awesome! Looks like just what I need.
1<runeks> But, as others in the pull request have mentioned, we better be damn sure that its return result is *always* the same as that of sendrawtransaction.
2<runeks> Looking at the code, to me it doesn't seem trivial achieve this.
3<runeks> Implementation-wise, it would seem a lot more robust to use the same code path as sendrawtransaction, but abort before the transaction is committed, and just return "valid".
4<wumpus> that would be a non-trivial impact to the current transaction handling code
5<wumpus> not a good idea with all the softforks etc going on
6<wumpus> anyhow, closing it then, feel free to implement it in a way you think is better
7<wumpus> I thought this was a good way to get the API merged, it could always be improved later
8<wumpus> also the 'verify multiple transactions' part would be more difficult to implement if you want to bolt this onto main.cpp

I don’t however want to just bolt this on to main.cpp in the current state. Without refactors that’d make the code even harder to understand, and makes it virtually impossible to handle multiple dependent transactions without actually writing to the mempool. (and the mempool logic in AcceptToMempool really gets in the way, e.g. the replacment logic)

So if this solution is not acceptable for the mean time, I suppose we should wait for those refactors to materialize first.

jtimon commented at 11:27 am on March 28, 2016: contributor

Thank you.

Why the additional requirement of this having to return the same as sendrawtx?

Wouldn’t everything be much easier if VerifyTx ONLY did consensus checks? As said we can have a VerifyAndAcceptTx later thatalso does relay/mempool policy stuff as well (but, yeah, I agree the policy code needs many refactors if you want to use any of the fee-related code in the CtxMempool and AcceptToMemoryPool from here).

laanwj commented at 12:05 pm on March 28, 2016: member

Why the additional requirement of this having to return the same as sendrawtx?

That is the default assumption. I suppose if we can document the differences (e.g. no mempool replacement, no fee checks) that’d help.

If you want to do only consensus checks you can already do that with this code, just disable the standardness check (and possibly the use mempool flag). I do think checking standardness etc is useful in some cases though.

jtimon commented at 1:21 pm on March 29, 2016: contributor

Well, yeah, documenting the differences seems much simpler to make the relay/mempool policy checks complete. So called “standardness” is encapsulated in policy/policy.o and its dependencies, but completing the rest of the policy checks won’t be so simple I think. I would rather complete consensus or to-be-consensus checks first (ie bips 68, 112 and 113). With or without “standardness”, I may backport this code to https://github.com/jtimon/bitcoin/tree/jt (after I rebase it on top of bip9, etc) to see if it can help me make some point about encapsulation or initiate some API discussion at the rpc level (something that I’ve failed to achieve at the C API level, with, say, https://github.com/jtimon/bitcoin/commit/11fa11a64b73a3c40a733033a7344c4b93045233 ).

If anyone else wants to continue this for master directly, I’m happy to re-review, though.

NicolasDorier commented at 11:20 am on July 15, 2016: contributor

Wouldn’t the easiest way to make this feature is to create a dummy block, with a dummy coinbase, and add the transaction inside it and call ConnectBlock with fJustCheck to true ?

jtimon commented at 11:39 am on July 15, 2016: contributor

I don’t think the goal of this PR was offering the functionality the “easiest way”, but since it was checking policy things as well, I’m not sure I ever understood the PR. In any case, what you propose (which doesn’t sound very elegant) would also not be complete: currently you would need to also call ContextualCheckBlock() with that dummy block for the tx to be fully verified.

NicolasDorier commented at 2:02 pm on July 15, 2016: contributor

At the API level, I think users care only about one parameter: Whether we check against Node Policy or Consensus Policy. I think the current PR was attempting too many options.

I’ll probably try to make it later since this PR already did most of the heavy work.

laanwj commented at 6:55 am on July 18, 2016: member

Wouldn’t the easiest way to make this feature is to create a dummy block, with a dummy coinbase, and add the transaction inside it and call ConnectBlock with fJustCheck to true ?

Originally I meant this as a pre-check whether sendrawtransaction will accept a chain of transactions. Creating a dummy block would work to check consensus validity only.

I think the current PR was attempting too many options.

Only three options! And I think all of them make sense in some context.

DrahtBot locked this on Sep 8, 2021

rpc: Add verifyrawtransactions call #7552

rpc: Add `verifyrawtransactions` call #7552