New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: add wtxid to mempool entry output #11203

Merged
merged 2 commits into from Sep 6, 2017

Conversation

Projects
None yet
9 participants
@sdaftuar
Member

sdaftuar commented Aug 31, 2017

We already cache this information in the mempool, so including it in the output of rpc calls is basically free.

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Aug 31, 2017

Member

Ideally this change should affect some functional tests. IMO there should be a test that asserts the entry keys.

Member

promag commented Aug 31, 2017

Ideally this change should affect some functional tests. IMO there should be a test that asserts the entry keys.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs
Member

instagibbs commented Aug 31, 2017

utACK 7e5d596

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Aug 31, 2017

Member

@promag Added a test.

Member

sdaftuar commented Aug 31, 2017

@promag Added a test.

@luke-jr

Should be "hash" (not "wxtxid") to be clear and consistent with the rest of the RPC interface.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Sep 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...

Member

sdaftuar commented Sep 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 2, 2017

Member

It's a hash of the transaction. It's not a transaction id. wtxid makes no sense.

Member

luke-jr commented Sep 2, 2017

It's a hash of the transaction. It's not a transaction id. wtxid makes no sense.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 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.

Member

sipa commented Sep 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Sep 3, 2017

utACK 617c459.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 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?

Member

luke-jr commented Sep 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@MeshCollider

MeshCollider Sep 3, 2017

Member

utACK 617c459, agree with sipa on the hash v. wtxid because I think consistency with BIP 141 is clearer

Member

MeshCollider commented Sep 3, 2017

utACK 617c459, agree with sipa on the hash v. wtxid because I think consistency with BIP 141 is clearer

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 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).

Member

sipa commented Sep 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@promag

promag Sep 4, 2017

Member

Still missing tests for getrawmempool, getmempooldescendants and getmempoolancestors (with verbose=true) as these also use entryToJSON().

Member

promag commented Sep 4, 2017

Still missing tests for getrawmempool, getmempooldescendants and getmempoolancestors (with verbose=true) as these also use entryToJSON().

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Sep 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.

Member

sdaftuar commented Sep 5, 2017

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

@@ -363,6 +364,7 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
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()));

This comment has been minimized.

@laanwj

laanwj Sep 5, 2017

Member

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

@laanwj

laanwj Sep 5, 2017

Member

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

This comment has been minimized.

@sdaftuar

sdaftuar Sep 6, 2017

Member

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.

@sdaftuar

sdaftuar Sep 6, 2017

Member

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.

This comment has been minimized.

@laanwj

laanwj Sep 6, 2017

Member

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.

@laanwj

laanwj Sep 6, 2017

Member

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.

This comment has been minimized.

@sipa

sipa Sep 6, 2017

Member

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

@sipa

sipa Sep 6, 2017

Member

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

This comment has been minimized.

@laanwj

laanwj Sep 6, 2017

Member

Oh, oops, never mind.

@laanwj

laanwj Sep 6, 2017

Member

Oh, oops, never mind.

@laanwj laanwj changed the title from RPC: add wtxid to mempool entry output to rpc: add wtxid to mempool entry output Sep 6, 2017

@laanwj laanwj merged commit 617c459 into bitcoin:master Sep 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 6, 2017

Merge #11203: rpc: add wtxid to mempool entry output
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

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment