refactor: Use typesafe Wtxid in compact block encodings #29752
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
AngusP
force-pushed
the
typesafe-txid-in-cmpctblock
branch
from
58ba422
to
ea8570b
Compare
…ad of ambiguous uint256. Wtxid/Txid types introduced in bitcoin#28107
AngusP
force-pushed
the
typesafe-txid-in-cmpctblock
branch
from
ea8570b
to
e178a52
Compare
src/blockencodings.cpp
Outdated
| if (extra_txn[i] == nullptr) | ||
| continue; |
There was a problem hiding this comment.
I added this mullptr check because some functional tests failed -- as I understand it, this was sort-of a bug in the previous implementation but didn't cause a crash, just 'potentially bad' behaviour?
It may be worth adding a new test to explicitly check this case (and see what the previous impl did with the pair<uint256, CTransactionRef>), but I'd need someone to point to where that test should live.
Also self-nit, a break; instead of continue; may be better as AFAIK this is a ring buffer that'll be sequentially added-to? But again as I'm not that familiar with the code, unsure.
There was a problem hiding this comment.
I haven't looked at this, but it may have been intentional to compare against a default constructed uint256 (an array of zeros), so that the runtime is constant and does not depend on the current fill-level of the data structure.
There was a problem hiding this comment.
as I understand it, this was sort-of a bug in the previous implementation but didn't cause a crash, just 'potentially bad' behaviour?
It would actually be a crash. Just to recap the bug that exists on master:
- If the ring buffer is not fully filled, then it contains default initialized
uint256s (as the first pair element) - We loop over the entire ring buffer in
PartiallyDownloadedBlock::InitData, compute the short id from the wtxid stored in the first pair element and check if that short id exists for the current compact block. - If a short id computed from the default initialized
uint256collides with an existing short id, then we might end up de-referencing anullptrhere.
This would only happen if the ring buffer has not been fully filled (i.e. there are nullptrs present) and there is a short id collision (short ids are 6 bytes in size), so this is extremely rare and it can also not be remotely triggered by a peer.
To verify this bug I've amended our partially_downloaded_block fuzz harness and changed the short id size to 1 byte here: dergoegge@c6580c0.
It may be worth adding a new test to explicitly check this case (and see what the previous impl did with the pair<uint256, CTransactionRef>), but I'd need someone to point to where that test should live.
I like that thought. As noted above, I've verified the bug on master by amending one of our fuzz tests. On master, that test does not include default initialized pairs in the ring buffer, which is probably why that bug wasn't found so far (It would still be very rare with 6 byte short ids).
I think allowing the short id creation to be mocked (or maybe just the short id size) would be helpful to create a test. This would also generally help with getting various other short id collision edge cases under test (through unit or fuzz testing). My amended fuzz test might be a good starting point but the changes I have made to blockencodings.cpp should be configurable by a test instead of being hard-coded.
(I don't think adding a test is a blocker for this PR but perhaps a nice follow up)
There was a problem hiding this comment.
A relatively simple test could be to a extra_txn.resize(1) within src/test/blockencodings_tests.cpp (currently it is empty)?
This bug description looks correct to me.
src/blockencodings.cpp
Outdated
| if (extra_txn[i] == nullptr) | ||
| continue; |
There was a problem hiding this comment.
A relatively simple test could be to a extra_txn.resize(1) within src/test/blockencodings_tests.cpp (currently it is empty)?
This bug description looks correct to me.
… of a vec of pair<Wtxid, CTransactionRef> All `CTransactionRef` have `.GetWitnessHash()` that returns a cached `const Wtxid` (since fac1223), so we don't need to pass transaction refs around with their IDs as they're easy to get from a ref.
AngusP
force-pushed
the
typesafe-txid-in-cmpctblock
branch
from
e178a52
to
a8203e9
Compare
The first commit replaces
uint256with typesafeWtxid(orTxid) types introduced in #28107.The second commit then simplifies the extra tx
vectorto just be ofCTransactionRefs instead of astd::pair<Wtxid, CTransactionRef>, as it's easy to get aWtxidfrom a transaction ref.