explainrawtransaction: new RPC to show UTXO info of raw transaction inputs #4226

When decoding raw transactions, it is useful to have information about the inputs beyond their txid and vout. If this information is available, this patch adds it to the json output of decoderawtransaction.

On IRC recently I was explaining to somebody how to interpret the output of decoderawtransaction. It was a pain to tell him to go look up each of the input txouts separately, when I knew they were all unspent and sitting in his wallet, so his bitcoind had ready access to them.

That turns it into not a pure utility function anymore. I'm not sure I like that.

Agreed, but as a practical matter most of the time I use decoderawtransaction I care about the inputs.

I also worry about the performance hit if someone is doing tons of decoderawtransactions for some reason: the new impure behaviour involves a utxo search for every input.

What if I added a flag (default off) to get this new behaviour?

I think it may be useful to have an RPC to look this information up, but agree with @sipa that it doesn't belong in decoderawtransaction. Decoderawtransaction should to just that, decode a transaction.

::nods:: just split out the rpc, and perhaps the other one should only work if txindex=1 so that it can always give complete results rather than 'mysteriously' failing to expand some transactions?

I don't think we want to fail on txindex=0. If someone without -txindex is using the raw transaction API chances are they're spending their own money, so their wallet will know the inputs.

I'll split out the RPC to leave decoderawtransaction alone and add a new "explainrawtransaction" which does an input decode, or fails with a nice message if any input is unknown.

Can you have it display the fees and priority too?

I'm not quite sure how to handle the case of the fee when not all the inputs could be resolved though. Omitting it might give the impression that there is no fee.

Oh, excellent idea. I think in the case that any inputs can't be resolved, we should just fail and recommend decoderawtransaction instead.

Oh, duh good point.

So one issue is that you ought to be able to give it multiple transactions, e.g. a chain of unconfirmed transactions so that it can resolve those even if they aren't mempooled yet. This might be more complex than you want to handle right now, but when setting up the api keep that in mind that— if you agree— it might make sense to make the output a list.

OK, I have refreshed the PR with the new explainrawtransaction RPC. I put the output in a list as suggested and added a TODO note to support chaining transactions together, since I would indeed like that feature eventually.

are they're spending their own money, so their wallet will know the inputs @apoelstra but this doesn't look in the wallet at all, does it?

@laanwj it does not, but neither does gettxout and I recall that working for wallet transactions (somehow) back when I had a non-txindex node. Perhaps this was just coincidence (or I am misremembering). Can you clarify this?

@apoelstra That may have to do with the transactions coincidentally being in the mempool or UTXO set as well. It doesn't check the wallet in any way (and probably shouldn't, in the interest of keeping it general and independent of wallet-enabled or not).

BTW: isn't all this information already available through the current API?

Sure, it'd require a second pass where you call gettxout for the inputs, but I would prefer it that way. It would avoid adding the coinsview logic into TxToJSON, which looks like confusing concerns (and messy) to me.

@laanwj Yes, the information is all available through the current API. (This is why I tried to tackle it despite my unclear understanding of where what data is stored — I could just look at the other RPCs!)

The usecase here is verification of user-created rawtransactions. Using the current API you need to call gettxout for every input, sum their values and calculate the fee, all of which offer opportunity for mistakes. I'd expect frequent users of the current API write their own shell tools around bitcoin-cli to accomplish all that. Since bitcoind has all the available information and this is good practice before every signrawtransaction, I think it should be its own RPC.

As for adding the coinsview stuff to TxToJSON, I definitely agree with you from a code cleanliness perspective. It's an artifact of my initial approach to extend decoderawtransaction. I'll push a new commit which pulls the coinsview logic into the explainrawtransaction function call where it belongs.

I haven't yet tested the various failure modes (only the syntax-error unit tests included in the commit), but I'm throwing this out there for comments on the design.

Oops, in the last commit I had moved the coinsview logic into explainrawtransaction, but forgot to remove it from TxToJSON. This push fixes that, so TxToJSON is untouched. Now the commit is entirely added code for the new RPC.

Note that wallet transactions are now considered first when looking for inputs, and moving to the mempool and txindex if this fails. So there should be no requirement on txindex for wallet transactions now.

Concept ACK. Needs rebase + re-review by others, due to amount of change since first posting. Feedback:

• Minor: RPCTypeCheck() replaces manually verifying a JSON object's member types
• Avoid directly locking and accessing the mempool. Update CTxMemPool class with a helper you need, and then use that helper in your code.
• Updating the PR title and description would be helpful

Thanks @jgarzik. I've rebased the PR and replaced the typechecks with RPCTypeCheck.

I'm not sure what you mean by "update CTxMemPool with a helper". I'm locking the pool, looking up a utxo, then unlocking it. (The gettxout RPC does this too.) How should that be refactored?

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4226_1c20f52cc412ee9c2714fc09b8cd6deea983547d/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

I agree with @jgarzik that ideally all mempool-touching code should be refactored into methods inside CTxMempool, but the code right now violates this pretty much everywhere. I don't think it's reasonable to ask that here.

No, not asking for a refactor. The general principle is "don't make the situation worse"

To be specific, the lock should ideally be only taken inside the mempool class, and the code is slowly being migrated in that direction. We don't want to move in the opposite direction, do the "ugly" thing (take lock manually, touch mempool internals manually) without a good reason.

That means

• Avoid adding new places outside CTxMemPool that take the mempool lock, and reach a hand into the mempool's internals.
• If you find yourself needing to do that, then create a new method inside CTxMemPool class that satisfies the mempool manipulation or query you wish to accomplish. That was the genesis of CTxMemPool::addUnchecked() and CTxMemPool::queryHashes().

Is this PR a necessary exception to that general migration?

I like this functionality in principal. Will be nice not to have to rely on 3rd party apps like insight-api.

Hi @btcdrak, thanks for the heads up. I think the only blocker right now is @jgarzik's suggested refactoring (which is no big deal, this PR just got bumped to the back burner and I forgot about it).

Will try to have it together this week.

Ping

Pong. I have not forgotten about this, it's just on the back burner because I need to learn more of the codebase to implement the changes you requested.

I'll rebase it today.

This pull has been idle for almost six month, so I'm closing it. @apoelstra let me know if you start work on this again then I'll reopen.