refactor: simplify GetAncestor #31778

1. 
   kcalvinalvin commented at 6:19 am on February 2, 2025: contributor

   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.

2. 
   DrahtBot commented at 6:19 am on February 2, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31778.

   Reviews

   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.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31835 (validation: set BLOCK_FAILED_CHILD correctly by stratospher)

   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.

3. DrahtBot added the label Refactoring on Feb 2, 2025
4. refactor: simplify GetAncestor

   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.

   5983f166c9
5. kcalvinalvin force-pushed on Feb 2, 2025
6. 
   Chand-ra commented at 4:58 pm on March 9, 2025: none

   Concept ACK 5983f1 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?

Contributors
kcalvinalvin DrahtBot Chand-ra

Labels
Refactoring