bitcoin

mirror of https://github.com/bitcoin/bitcoin.git synced 2026-05-13 07:23:21 +02:00

Files

Ava Chow 0cbd220294 Merge bitcoin/bitcoin#34440 : refactor: Change CChain methods to use references, add tests

7c75244ade Change pindexMostWork parameter of ActivateBestChainStep() to reference (optout)
c5eb283bca Change CChain::FindFork() to take ref (optout)
20b58e281a Change CChain::Next() to take reference (optout)
fe2d6e25e0 Change CChain::Contains() to take reference (optout)
db56bcd692 test: Add CChain::FindFork() tests (optout)
8333abdd91 test: Add CChain basic tests (optout)

Pull request description:

  Refactor `CChain` methods (`Contains()`, `Next()`, `FindFork()`) to use references instead of pointers, to minimize the risk of accidental `nullptr` dereference (memory access violation). Also add missing unit tests to the `CChain` class.

  The `CChain::Contains()` method (in `src/chain.h`) dereferences its input without checking. The `Next()` method also calls into this with a `nullptr` if invoked with `nullptr`. While most call sites have indirect guarantee that the input is not `nullptr`, it's not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer.

  Changes:

  - Add basic unit tests for `CChain` class methods
  - Add unit tests for `CChain::FindFork()`
  - Change `CChain::Contains()` to take reference
  - Change `CChain::Next()` to take reference
  - Change `CChain::FindFork()` to take reference
  - Change `pindexMostWork` parameter of `ActivateBestChainStep()` to reference
  - Rename changed parameters (`* pindex` --> `& index`)

  Alternative. A simpler change is to stick with pointers, with extra checks where needed, see #34416 .

  This change is remotely related to and indirectly triggered by #32875 .

  Further ideas, not considered in this PR:

  - Change `InvalidateBlock()` and `PreciousBlock()` to take references.
  - Change `CChain` internals to store references instead of pointers
  - Change CChain to always have at least one element (genesis), that way there is always genesis and tip.
  - Check related methods to return reference (guaranteed non-null) -- `FindFork`, `FindEarliestAtLeast`, `FindForkInGlobalIndex`, `blockman.AddToBlockIndex`, etc.

ACKs for top commit:
  l0rinc:
    reACK 7c75244ade
  maflcko:
    re-review ACK 7c75244ade 🌅
  achow101:
    ACK 7c75244ade
  hodlinator:
    re-ACK 7c75244ade

Tree-SHA512: 122f40120058f7e1f0273b3afed9c54966c05f06b6f2fee45bc48430617f24a5e4320a9bb7bb0ac986f2accfa22fabae5cc941b949758ddca2e9fcd472b46c33