rpc: add wtxid to mempool entry output#11203
Conversation
|
Ideally this change should affect some functional tests. IMO there should be a test that asserts the entry keys. |
|
utACK 7e5d596 |
|
@promag Added a test. |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
Should be "hash" (not "wxtxid") to be clear and consistent with the rest of the RPC interface.
|
|
|
It's a hash of the transaction. It's not a transaction id. |
|
@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. |
|
utACK 617c459. |
|
@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? |
|
utACK 617c459, agree with sipa on the |
|
@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). |
|
Still missing tests for |
|
@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. |
| info.push_back(Pair("ancestorcount", e.GetCountWithAncestors())); | ||
| info.push_back(Pair("ancestorsize", e.GetSizeWithAncestors())); | ||
| info.push_back(Pair("ancestorfees", e.GetModFeesWithAncestors())); | ||
| info.push_back(Pair("wtxid", mempool.vTxHashes[e.vTxHashesIdx].first.ToString())); |
There was a problem hiding this comment.
Is it guaranteed that the entry exists in mempool.vTxHashes? if not, using [] will potentially cause a leak by adding an empty record.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@laanwj vTxHashes is a vector - calling [] out of bounds doesn't add an empty record, it's just undefined behaviour.
laanwj
changed the title
617c459 qa: rpc test for wtxid in mempool entry (Suhas Daftuar) 7e5d596 RPC: add wtxid to mempool entry output (Suhas Daftuar) Pull request description: We already cache this information in the mempool, so including it in the output of rpc calls is basically free. Tree-SHA512: 2757e1bfca028103937e4b76ce1a5d805846bad5d3d9dd631dcc5f87721bcc0e9d19e437e02053ef1dd3b38b503f0fca8c0b8492cac37dfbd70256a3665f704c
Github-Pull: bitcoin#11203 Rebased-From: 7e5d596
Github-Pull: bitcoin#11203 Rebased-From: 617c459
9 participants
We already cache this information in the mempool, so including it in the output of rpc calls is basically free.