rpc: add wtxid to mempool entry output #11203

1. 
   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.
2. RPC: add wtxid to mempool entry output 7e5d5965d1
3. 
   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.
4. 
   instagibbs commented at 1:51 pm on August 31, 2017: member

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

   @promag Added a test.
7. fanquake added the label RPC/REST/ZMQ on Sep 1, 2017
8. luke-jr changes_requested
9. 
   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.
10. 
    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…
11. 
    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.
12. 
    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.

13. 
    jonasschnelli commented at 5:34 pm on September 3, 2017: contributor

    utACK 617c459c6c56911e70855dbe139ab095f435e73e.
14. 
    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?
15. 
    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
16. 
    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).
17. 
    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().
18. 
    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.
19. 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.
20. laanwj renamed this:
    RPC: add wtxid to mempool entry output
    rpc: add wtxid to mempool entry output
    on Sep 6, 2017
21. laanwj merged this on Sep 6, 2017
22. laanwj closed this on Sep 6, 2017

    ---

23. laanwj referenced this in commit d745b4cf7b on Sep 6, 2017
24. luke-jr referenced this in commit 290aea2f73 on Nov 11, 2017
25. luke-jr referenced this in commit 2ba6ba0b12 on Nov 11, 2017
26. PastaPastaPasta referenced this in commit f4c8320dea on Sep 23, 2019
27. DrahtBot locked this on Sep 8, 2021

Contributors
sdaftuar promag instagibbs luke-jr sipa jonasschnelli meshcollider laanwj

Labels
RPC/REST/ZMQ