[WIP] [RPC] Add verifyrawtransaction RPC #11201

justicz · 2017-08-31T04:18:57Z

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 · 2017-08-31T04:25:46Z

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 · 2017-08-31T04:34:36Z

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 · 2017-08-31T04:38:16Z

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

sdaftuar · 2017-08-31T14:25:12Z

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 · 2017-08-31T14:51:26Z

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 · 2017-08-31T21:17:15Z

Seems related to #7552

justicz · 2017-09-01T00:44:12Z

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:

{
    "transactions": [ "hex_tx0", "hex_tx1", ... ],
    "use_local_policy": true // default true
}

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?

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

dcousens · 2017-09-06T05:36:12Z

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.

verifyrawtransactions [txhex, ...] [txos] {
  allow_missing_inputs: false,
  allow_spent_inputs: false, // allow_mempool_conflicts?
  local_policy: true,
  ... etc
}

sdaftuar · 2017-09-06T12:14:47Z

src/rpc/rawtransaction.cpp

+                } else if (missing_inputs) {
+                    // We want to ignore the "missing inputs" case here, since the input
+                    // may actually be in the mempool. If the input is truly missing, we
+                    // will find out later after the ancestor and consensus check.


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.

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?

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 · 2017-09-06T12:45:26Z

@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.

sdaftuar · 2017-09-06T12:46:16Z

src/txmempool.cpp

    // Used by AcceptToMemoryPool(), which DOES do
    // all the appropriate checks.
    LOCK(cs);
+


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

sdaftuar · 2017-09-06T13:36:03Z

src/validation.cpp

-            if (plTxnReplaced)
-                plTxnReplaced->push_back(it->GetSharedTx());
+        if (!fDryRun) {
+            // Remove conflicting transactions from the mempool


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.

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

justicz · 2017-09-07T00:46:16Z

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 · 2017-09-13T02:05:12Z

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 · 2017-10-12T07:01:24Z

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

wtogami · 2017-10-23T15:49:03Z

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

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

justicz changed the title ~~[RPC] Add verifyrawtransaction RPC~~ [WIP] [RPC] Add verifyrawtransaction RPC Sep 6, 2017

add dryRunMapTx to mempool

95eb8b6

sdaftuar reviewed Sep 6, 2017

View reviewed changes

justicz closed this Oct 12, 2017

maflcko mentioned this pull request Dec 19, 2017

rpc: Add testmempoolaccept #11742

Merged

fjahr mentioned this pull request Apr 22, 2020

Transactions chain and testmempoolaccept #18480

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[WIP] [RPC] Add verifyrawtransaction RPC #11201

[WIP] [RPC] Add verifyrawtransaction RPC #11201

Uh oh!

Conversation

justicz commented Aug 31, 2017

Uh oh!

gmaxwell commented Aug 31, 2017

Uh oh!

luke-jr commented Aug 31, 2017

Uh oh!

justicz commented Aug 31, 2017

Uh oh!

sdaftuar commented Aug 31, 2017

Uh oh!

achow101 commented Aug 31, 2017

Uh oh!

jtimon commented Aug 31, 2017

Uh oh!

justicz commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

justicz Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Sep 6, 2017

Uh oh!

sdaftuar Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

justicz Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

justicz Sep 7, 2017

Choose a reason for hiding this comment

Uh oh!

justicz commented Sep 7, 2017

Uh oh!

justicz commented Sep 13, 2017

Uh oh!

justicz commented Oct 12, 2017

Uh oh!

wtogami commented Oct 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

justicz commented Sep 1, 2017 •

edited

Loading

dcousens commented Sep 6, 2017 •

edited

Loading