Refactor CChain methods to use references, tests#34440
Refactor CChain methods to use references, tests#34440optout21 wants to merge 7 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Added new commit to change |
|
Added new commit to change |
1f8f7d4 Change BlockRequestAllowed() to take ref (optout) Pull request description: As [suggested here](#34416 (comment)), a minor refactor of `PeerManagerImpl::BlockRequestAllowed()` to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent. The change is local to the `PeerManagerImpl::BlockRequestAllowed()` class. Related to #34440. ACKs for top commit: maflcko: review ACK 1f8f7d4 🎐 l0rinc: tested ACK 1f8f7d4 stickies-v: ACK 1f8f7d4 Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19
Add basic unit tests to the `CChain` class, filling a gap.
Add (lengthier) unit tests for `CChain::FindFork()`.
The `CChain::Contains()` method dereferences its input without checking, potentially resulting in nullptr-dereference if invoked with `nullptr`. To avoid this possibility, its input is changed to a reference instead. Call sites are adapted accoringly. In a few cases an extra check is added to ensure the input is not null (if guard or only `Assume`, depending on the context.
To minimize chance of erroneous nullptr dereference, `CChain::Next()` is changed to take a reference instead of a pointer. Call sites have been adapted.
|
Rebased, following #34464 ; one commit got annihilated. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Refactor
CChainmethods (Contains(),Next(), etc.) to use references instead of pointers, to minimize the risk of accidentalnullptrdereference (memory access violation). Also add missing unit tests to theCChainclass.The
CChain::Contains()method (insrc/chain.h) dereferences its input without checking. TheNext()method also calls into this with anullptrif invoked withnullptr. While most call sites have indirect guarantee that the input is notnullptr, 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:
CChainclass methodsCChain::Contains()to take referenceCChain::Next()to take referenceCChain::FindFork()PeerManagerImpl::BlockRequestAllowed()to take referenceTODO:
CChaininternals to store references instead of pointers/FindFork,FindEarliestAtLeast,FindForkInGlobalIndex,blockman.AddToBlockIndex, etc.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 .