[WIP] [RPC] Add verifyrawtransaction RPC

justicz commented at 4:18 am on August 31, 2017: none

Currently, there is not an RPC that allows for the validation of a raw transaction without actually submitting that transaction to the network. This PR implements verifyrawtransaction, which accepts a hex encoded raw transaction and determines if it could be included in the next block. It does this by creating a “fake” block with the proposed transaction, and then checking that the block is valid with TestBlockValidity.

This is my first attempt at contributing to bitcoin, so if I’ve already managed to break any unspoken (or spoken) rules, please let me know 😅

gmaxwell commented at 4:25 am on August 31, 2017: contributor

Well there is already a recommend way to do this: use a block proposal.

The general limitation with a ‘verifyrawtransaction’ approach is that it doesn’t let you test a graph of unconfirmed transactions, but a block proposal does.

Looks like internally your rpc is doing more or less what a block proposal does, but handling more of the things around it. So perhaps it would be useful to do.

It could obviously be extended to accept an array of transactions to do the right thing with respect to multiple… but another open question is what about parents in the mempool. Right now your code will fail a transaction that spends the output of a mempool transaction. Sometimes that would be desirable but I think often not.

luke-jr commented at 4:34 am on August 31, 2017: member

Seems awfully inefficient to call CNB just to throw away the chosen transaction-set. Also, the block fees will be wrong. If you don’t want to build a block from scratch, maybe abstract that out of CNB first (separate commit, same PR)?

(To revise your PR, just use git push --force over the same branch.)

justicz commented at 4:38 am on August 31, 2017: none

Got it, thanks for the feedback. I’ll address these issues now

sdaftuar commented at 2:25 pm on August 31, 2017: member

I agree with the general usefulness of a verifyrawtransaction RPC (especially one that takes a list of unconfirmed transactions, and ideally one that allows for checking based on inputs that may be in the mempool), but one issue with this block proposal approach is that you won’t be testing against local policy, just the consensus rules. So be aware that a True result here won’t necessarily mean that the node would accept the transactions and relay them.

achow101 commented at 2:51 pm on August 31, 2017: member

Concept ACK, but as others have said, there are other things to consider when verifying a transaction that are not done here. Those need to be documented.

jtimon commented at 9:17 pm on August 31, 2017: contributor

Seems related to https://github.com/bitcoin/bitcoin/pull/7552

justicz commented at 0:44 am on September 1, 2017: none

Thanks much for that link @jtimon, somehow I missed that.

It seems like everyone agrees that there is the potential for some useful functionality here, there’s just some uncertainty in what exactly the API for this should look like. After reading #7552, it seems like this uncertainty is largely what has kept verifyrawtransaction functionality from being included in the past.

What if verifyrawtransaction became verifyrawtransactions as in #7552, and took an argument as follows:

0{
1    "transactions": [ "hex_tx0", "hex_tx1", ... ],
2    "use_local_policy": true // default true
3}

I think this achieves all of the behavior we want; users can set “use_local_policy” to false if they only want to check consensus rules. I will also make it handle ancestors in the mempool correctly as brought up by @gmaxwell, unless that should be another flag?

I think I can do a lot of this with a minor refactor of AcceptToMemoryPoolWorker.

Does this sound good?

fanquake added the label RPC/REST/ZMQ on Sep 1, 2017

add verifyrawtransaction rpc

add tests

fix typo

add dryrun flag to AcceptToMemoryPool]

check each transaction against local policy rules

stop doing needless work in CreateNewBlock

update docs

add ancestor topological sort

add better tests for multi ancestors and policy flag

add moar tests

make everything snake_case

99b1800ce9

dcousens commented at 5:36 am on September 6, 2017: contributor

Use signrawtransaction to verify a transaction? If you have unknown/chained utxos… you can provide them. If you wanted to check against local policy, maybe this PR could instead add that as an optional flag It doesn’t require wallet support, but if you have wallet support enabled, you should otherwise compare the hex returned to enforce it wasn’t modified.

As discussed on IRC, signrawtransaction only verifies transaction scripts, not the validity of the transaction otherwise. E.g input value >= output value, output value > MAX_MONEY, etc.

concept ACK. IMHO the API could mimic signrawtransaction (minus the private keys) with some configuration options as to policy/mempool and other checks.

0verifyrawtransactions [txhex, ...] [txos] {
1  allow_missing_inputs: false,
2  allow_spent_inputs: false, // allow_mempool_conflicts?
3  local_policy: true,
4  ... etc
5}

justicz renamed this:
~~[RPC] Add verifyrawtransaction RPC~~
[WIP] [RPC] Add verifyrawtransaction RPC
on Sep 6, 2017

add dryRunMapTx to mempool 95eb8b6f4c

in src/rpc/rawtransaction.cpp:1074 in 95eb8b6f4c

1069+                    result.push_back(Pair("reason", strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason())));
1070+                    return result;
1071+                } else if (missing_inputs) {
1072+                    // We want to ignore the "missing inputs" case here, since the input
1073+                    // may actually be in the mempool. If the input is truly missing, we
1074+                    // will find out later after the ancestor and consensus check.

sdaftuar commented at 12:14 pm on September 6, 2017:

If ATMP fails for a transaction due to missing inputs, then almost none of the policy checks will be evaluated for the transaction, because that’s one of the first things we (must) do – for instance you can’t check the feerate of the transaction without having the inputs available.

sdaftuar commented at 12:37 pm on September 6, 2017:

Actually on further review, it looks like you’ve added these “dry run” transactions to the mempool, so now I think if ATMP returns “missing inputs”, that should reflect an actual failure? Is there a case where that doesn’t hold?

justicz commented at 0:48 am on September 7, 2017:

Yeah – this code is leftover from before I figured out the issue you mentioned in your first comment. I was working on getting rid of this when you left your review 😝

sdaftuar commented at 12:45 pm on September 6, 2017: member

@justicz Thanks for working on this. I think there’s still work to be done here, but this seems like a promising direction.

Rather than add the dryRunMap to the mempool, though, I think it’d be cleaner if we instead just added a caching layer in front of the mempool, which holds these dry run transactions. ATMPWorker already takes a mempool as an argument, so I think it’d be straightforward to abstract the mempool interface away (and I guess clean up the mempool interface so that callers aren’t grabbing at internals), and then pass in a cache that holds dry run transactions in it, as well as transactions that get evicted from the mempool (eg due to RBF). Then I think we could ensure that the mempool itself is a const object in this context that is clearly unaffected, with all changes happening only in the cache.

If we can achieve that, that would make the review and maintenance burden of this feature pretty minimal, which I think would be awesome.

in src/txmempool.cpp:367 in 95eb8b6f4c

363     NotifyEntryAdded(entry.GetSharedTx());
364     // Add to memory pool without checking anything.
365     // Used by AcceptToMemoryPool(), which DOES do
366     // all the appropriate checks.
367     LOCK(cs);
368+

sdaftuar commented at 12:46 pm on September 6, 2017:

The NotifyEntryAdded callback a few lines up shouldn’t trigger for dry run transactions.

justicz commented at 0:50 am on September 7, 2017:

👍

in src/validation.cpp:843 in 95eb8b6f4c

848-                    FormatMoney(nModifiedFees - nConflictingFees),
849-                    (int)nSize - (int)nConflictingSize);
850-            if (plTxnReplaced)
851-                plTxnReplaced->push_back(it->GetSharedTx());
852+        if (!fDryRun) {
853+            // Remove conflicting transactions from the mempool

sdaftuar commented at 1:36 pm on September 6, 2017:

Since you only are removing conflicts in non-dryrun situations, doesn’t that mean that someone calling verifyrawtransactions with a pair of tx’s, one of which would replace existing transactions, and one of which depends on an existing transaction, would potentially succeed? Yet when actually trying to send, the second would necessarily fail.

justicz commented at 0:49 am on September 7, 2017:

I think you’re right. I’ll fix this

justicz commented at 0:46 am on September 7, 2017: none

Thanks for the comments @sdaftuar! I think I agree with all of the points you made and will work on separating things out of the mempool more cleanly.

justicz commented at 2:05 am on September 13, 2017: none

Hey @sdaftuar, it’s not done yet (still need to implement eviction/properly count descendants) but I’ve been working on implementing the feature in the way you described over here: https://github.com/justicz/bitcoin/pull/3 . I broke the relevant parts of the CTxMemPool interface out into a CTxMemPoolBase class and then implemented a CTxMockMemPool that I pass into ATMP.

Is this more what you had in mind? I’m modifying some of the const CTxMemPool methods to accept a cache so that I can avoid duplicating a lot of the logic.

justicz commented at 7:01 am on October 12, 2017: none

Closing this PR, will make a new one in the future if I can come up with an implementation I am happy with

justicz closed this on Oct 12, 2017

wtogami commented at 3:49 pm on October 23, 2017: contributor

Please ping when you do come up with a better implementation. I have strong interest in this!

MarcoFalke locked this on Sep 8, 2021

[WIP] [RPC] Add verifyrawtransaction RPC #11201