The first commit replaces uint256 with typesafe Wtxid (or Txid) types introduced in #28107.
The second commit then simplifies the extra tx vector to just be of CTransactionRefs instead of a std::pair<Wtxid, CTransactionRef>, as it’s easy to get a Wtxid from a transaction ref.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See 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.
133@@ -134,11 +134,11 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
134 }
135
136 for (size_t i = 0; i < extra_txn.size(); i++) {
137- uint64_t shortid = cmpctblock.GetShortID(extra_txn[i].first);
138+ uint64_t shortid = cmpctblock.GetShortID(extra_txn[i]->GetWitnessHash());
139 std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
if here checking the pointer first. vExtraTxnForCompact is a fixed-size ring buffer so it’s possible to be dereferencing a nullptr before it gets fully populated.
Wtxid/Txid types introduced in #28107
133@@ -134,11 +134,13 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
134 }
135
136 for (size_t i = 0; i < extra_txn.size(); i++) {
137- uint64_t shortid = cmpctblock.GetShortID(extra_txn[i].first);
138+ if (extra_txn[i] == nullptr)
139+ continue;
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.
uint256 (an array of zeros), so that the runtime is constant and does not depend on the current fill-level of the data structure.
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:
uint256s (as the first pair element)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.uint256 collides with an existing short id, then we might end up de-referencing a nullptr here.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: https://github.com/dergoegge/bitcoin/commit/c6580c058a541f3a0c94172d2b0db99d77e79285.
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)
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.
ACK e178a52647fb6a8844191961fbf74266e22d5df8
The second commit seems sensible as wtxid is cached instead of being calculated on the fly since fac1223a568fa1ad6dd602350598eed278d115e8 (nit maybe mention in the commit message), though I don’t think having the pair is that bad either.
All `CTransactionRef` have `.GetWitnessHash()` that returns a cached `const Wtxid` (since fac1223a568fa1ad6dd602350598eed278d115e8),
so we don't need to pass transaction refs around with their IDs as they're easy to get from a ref.