validation: prevent FindMostWorkChain from causing UB#317
Conversation
In a pruned node undergoing a deep reorg, FindMostWorkChain can insert duplicate entries into m_blocks_unlinked. This can happen when: - Traversing from one candidate tip to the fork point adds blocks whose parents have been pruned. - Traversing from another candidate tip over the same fork inserts the same pairs again, since the blocks are shared across both branches. When we finally download the missing parent from our peer and call ReceivedBlockTransactions to process m_blocks_unlinked, the same entry may be processed multiple times. This can lead to re-insertion into setBlockIndexCandidates with a modified nSequenceId, violating its ordering invariants and causing undefined behavior. So avoid duplicate insertions into m_blocks_unlinked here. Co-authored-by: Martin Zumsande <mzumsande@gmail.com> (cherry picked from commit c787b3b)
when pruned node2 finally receives the blocks from node0, node2 will process duplicate entires in ReceivedBlockTransactions twice but it will only insert into setBlockIndexCandidates if the block has more work that the current chain tip. the duplicate entries in m_blocks_unlinked in this test are from height 1171 to 1294. before this commit - we invalidated height 1320 and chain tip became 1319. so we won't add duplicate entries (all have <1319) into setBlockIndexCandidates and won't have coverage for this UB scenario. with this commit - we invalidate height 1295 and chain tip became 1294. so we will process the duplicate entry 1294 in m_blocks_unlinked and have coverage for this UB. Co-authored-by: Martin Zumsande <mzumsande@gmail.com> (cherry picked from commit ca4a380)
| assert(!foundInUnlinked); // No duplicates in m_blocks_unlinked | ||
| foundInUnlinked = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This assert now enforces a global no-duplicates invariant, but the dedup only lives in FindMostWorkChain. The two other insert sites (ReceivedBlockTransactions and LoadBlockIndex) stay raw and are thus load-bearing for this assert without defending it themselves. They are safe today (disjoint block sets: 4316 only runs when pprev lacks a chain tx count, which FindMostWorkChain candidates always have), but a future change to those preconditions would surface only as an assert-crash under -checkblockindex. Upstream avoided the coupling with a shared AddUnlinkedBlock() helper. A one-line comment at the raw insert noting the invariant would suffice if not adopting the helper.
| first_reorg_height = self.nodes[2].getblockcount() | ||
| curchainhash = self.nodes[2].getblockhash(self.mainchainheight) | ||
| self.nodes[2].invalidateblock(curchainhash) | ||
| block_hash_1295 = self.nodes[2].getblockhash(1295) |
There was a problem hiding this comment.
Hardcoded 1295 silently depends on the deterministic block counts in create_big_chain/create_chain_with_staleblocks staying fixed. If that construction ever shifts, getblockhash(1295) raises an opaque RPC error rather than a clear assertion. Matches upstream so fine as-is; noting the fragility.
|
Any plans to pick this back up? The two points from my review are still open: the new assert enforces a global no-duplicates invariant while the dedup only lives in |
4 participants
See bitcoin#35070 for details