The if statement in GetAncestor was quite hard to make sense of. Simplifying it improves readability and the added test ensures that the performance remains the same.
I’m not quite sure if the test makes sense to have but including it as it’s the code used to make sure no loss in performance was made. Tested locally up to height 1«29.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31778.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | Chand-ra |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
The if statement in GetAncestor was quite hard to make sense of.
Simplifying it improves readability and the added test ensures that the
performance remains the same.
CBlockIndex::GetAncestor is easier to comprehend with this change, but I’m not sure if duplicating the two versions in test/blockchain_tests.cpp is the best way to test the change. A single test to verify GetAncestor agnostic of its implementation might be better?
🐙 This pull request conflicts with the target branch and needs rebase.