rpc: add wtxid to mempool entry output #11203

pull sdaftuar wants to merge 2 commits into bitcoin:master from sdaftuar:2017-08-add-wtxid-to-mempool-entry changing 2 files +5 −0

sdaftuar commented at 1:43 pm on August 31, 2017: member

We already cache this information in the mempool, so including it in the output of rpc calls is basically free.
RPC: add wtxid to mempool entry output 7e5d5965d1
promag commented at 1:50 pm on August 31, 2017: member

Ideally this change should affect some functional tests. IMO there should be a test that asserts the entry keys.
instagibbs commented at 1:51 pm on August 31, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/11203/commits/7e5d5965d12f4e110c026124ed326b03291fe469
qa: rpc test for wtxid in mempool entry 617c459c6c
sdaftuar commented at 5:18 pm on August 31, 2017: member

@promag Added a test.
fanquake added the label RPC/REST/ZMQ on Sep 1, 2017
luke-jr changes_requested
luke-jr commented at 7:16 am on September 2, 2017: member

Should be "hash" (not "wxtxid") to be clear and consistent with the rest of the RPC interface.
sdaftuar commented at 1:08 pm on September 2, 2017: member

wtxid is defined in BIP 141. Perhaps we should fix the rest of the RPC interface instead (how many places do we return this currently?); hash seems less informative to me…
luke-jr commented at 4:46 pm on September 2, 2017: member

It’s a hash of the transaction. It’s not a transaction id. wtxid makes no sense.
sipa commented at 7:02 pm on September 2, 2017: member

@luke-jr Imagine that at some point a new piece of extra witness data gets added in a softfork way. At that point there will be 3 hashes; the txid, the hash-with-witness-but-not-witness2, and the hash-of-everything. If we have a field for “overall hash, with everything included”, it will break software that knows about witnesses but not witness2.

So, while I don’t care much about hash vs txid nomenclature, it makes sense to have a name that is segwit-specific. Just “hash” sets the wrong expectation going further, and given that BIP141 calls it wtxid, I think that’s perfectly good choice.
jonasschnelli commented at 5:34 pm on September 3, 2017: contributor

utACK 617c459c6c56911e70855dbe139ab095f435e73e.
luke-jr commented at 10:02 pm on September 3, 2017: member

@sipa It would seem there are two things right now: txid and hash of the entire tx. If we add a third state, both of these things would remain the same. Hashes are opaque, so it’s not like software can see it and fail to try to decode it…? What use case(s) do you have in mind?
meshcollider commented at 10:13 pm on September 3, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/commit/617c459c6c56911e70855dbe139ab095f435e73e, agree with sipa on the hash v. wtxid because I think consistency with BIP 141 is clearer
sipa commented at 10:14 pm on September 3, 2017: member

@luke-jr My point is that a “hash of all the data, regardless of what you think it contains” is not useful to anyone. It arbitrarily changes meaning as witnesses are added (if ever).
promag commented at 2:44 pm on September 4, 2017: member

Still missing tests for getrawmempool, getmempooldescendants and getmempoolancestors (with verbose=true) as these also use entryToJSON().
sdaftuar commented at 4:22 pm on September 5, 2017: member

@promag If you think that’s important then I think it’d make more sense to have a separate test that checks the output of those rpc’s to ensure they are consistent; I’m going to leave this PR alone.
in src/rpc/blockchain.cpp:367 in 617c459c6c
363@@ -363,6 +364,7 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e) 364 info.push_back(Pair("ancestorcount", e.GetCountWithAncestors())); 365 info.push_back(Pair("ancestorsize", e.GetSizeWithAncestors())); 366 info.push_back(Pair("ancestorfees", e.GetModFeesWithAncestors())); 367+ info.push_back(Pair("wtxid", mempool.vTxHashes[e.vTxHashesIdx].first.ToString()));
laanwj commented at 11:28 pm on September 5, 2017:

Is it guaranteed that the entry exists in mempool.vTxHashes? if not, using [] will potentially cause a leak by adding an empty record.

sdaftuar commented at 5:02 pm on September 6, 2017:

Yes, it’s guaranteed – this is used in compact block reconstruction.

See CTxMemPool::removeUnchecked() and ::addUnchecked() for the code that keeps this in sync with the mempool entries.

laanwj commented at 5:49 pm on September 6, 2017:

Thanks for the explanation, that makes sense. Maybe we could add an assertion here assert(mempool.vTxHashes.count(e.vTxHashesIdx)), just in case. But I don’t insist.

sipa commented at 6:13 pm on September 6, 2017:

@laanwj vTxHashes is a vector - calling [] out of bounds doesn’t add an empty record, it’s just undefined behaviour.

laanwj commented at 6:24 pm on September 6, 2017:

Oh, oops, never mind.
laanwj renamed this:
~~RPC: add wtxid to mempool entry output~~
rpc: add wtxid to mempool entry output
on Sep 6, 2017
laanwj merged this on Sep 6, 2017
laanwj closed this on Sep 6, 2017
laanwj referenced this in commit d745b4cf7b on Sep 6, 2017
luke-jr referenced this in commit 290aea2f73 on Nov 11, 2017
luke-jr referenced this in commit 2ba6ba0b12 on Nov 11, 2017
PastaPastaPasta referenced this in commit f4c8320dea on Sep 23, 2019
DrahtBot locked this on Sep 8, 2021

Contributors
sdaftuar promag instagibbs luke-jr sipa jonasschnelli meshcollider laanwj

Labels
RPC/REST/ZMQ