The two assumptions there were described as test-only, which has led to confusion whether they should exist. However, they are necessary in general, as the changed comment explains - without them, the check would fail everywhere where it is enabled. The second commit moves this assert down to the other checks.
Closes #29261
These exceptions are not related to situations specific to tests,
but are required in general:
Without the first check CheckBlockindex could fail for blocks where we
only know the header.
Without the second, it could fail when blocks are received out of order.
There is a designated section meant for the actual consistency
checks, marked by a comment.
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.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, maflcko, naumenkogs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
4948+ unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
4949+ assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
4950+ // Transaction may be completely unset - happens if only the header was accepted but the block hasn't been processed.
4951+ || (pindex->nChainTx == 0 && pindex->nTx == 0)
4952+ // nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet)
4953+ || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
0 // nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet)
1 || (pindex->nChainTx == 0 && pindex->nTx >=1 && prev_chain_tx == 0 && pindex->pprev)
nit: Missing nTx check, according to comment?
nit: Missing nTx check, according to comment?
IMO, the suggestion to add pindex->nTx >=1 does not make the code clearer, and logically adding it should not have any effect because if nTx is 0 (nTx is unsigned), that’s just the previous “Transaction may be completely unset case” and this comparison will never be executed.
Code review ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5. Thanks for figuring this issue out and fixing it. Would suggest changing pr name from “improve comments” to “fix misleading comments” since previous comments were wrong about the reasons the conditions are needed.
I also think it would be good to split up the assert and make it stricter, so feel free to use the code from #29261 (comment) to use in the second commit, if that helps. (Would be good to keep the first commit unchanged because it’s a direct and straightforward fix.)
ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5 🌦
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5 🌦
3/nSt0HlzU30n9tRv3NbydVteYOku+aPTyCh6PYQEjYJEORLApik/3CZKJsb6mUFPkY5ZQj5R4YjViUjIDo8GBg==
Changed the title as suggested.
I also think it would be good to split up the assert and make it stricter, so feel free to use the code from #29261 (comment)
The suggestion looks good to me just by reading the code, but I want to test it more before including it. Will either update the PR in a few days, or alternatively (I don’t mind either way) that could be a follow-up to this trivial doc-only PR.
