Improve LastCommonAncestor performance + add tests

[sipa]

In theory, the LastCommonAncestor function in chain.cpp can take $\mathcal{O}(n)$ time, walking over the entire chain, if the forking point is very early, which could take ~milliseconds. I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.

This PR modifies the algorithm to make use of the CBlockIndex::pskip skip pointers to find the forking point in sublinear time (a simulation shows that for heights up to $34 \cdot 4^k - 2$ and $k \geq 8$, no more than $k^2 + 10k + 13$ steps are ever needed), in a way that should be nearly free - at worst the same number of memory accesses should be made, with a tiny increase in computation.

As it appears we didn't really have tests for this function, unit tests are added for that function as well as CBlockIndex::GetAncestor().

This is inspired by #32180 (comment)

[DrahtBot]

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/33515.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, mzumsande, furszy, optout21, stratospher, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[furszy]

Code review ACK 4fe3de2

[vasild]

Approach ACK 4fe3de2

[vasild]

This assumes that all blocks at the same height have equal pskip height. This is true currently, but if changed, then this code will trigger the Assume in debug and will do null pointer dereference in release builds. Looking at GetSkipHeight(), it derives its result deterministically from height only (good).

Is it worth here, if pa->pskip->nHeight != pb->pskip->nHeight, then to fall back to one-by-one iterating?

[sipa]

I don't think so - that eventuality (pskip depending on more than just height) is dealt with through the Assume(). I guess I can add a comment to clarify that.

[vasild]

ACK 3635d62

[mzumsande]

Code Review ACK 3635d62
The added test is slightly faster with the changes.

I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.

I think it would require really long forked chains for this to become relevant, so basically in consensus split scenarios?!

[mzumsande]

If I understand it correctly, we don't have to worry about dereferencing a nullptr here, because pskip is only nullptr for genesis, and if one of the blocks was genesis, we couldn't get to this spot.

[furszy]

ACK 3635d62

[optout21]

Why remove this? Is it just too obvious after the while (pa != pb)?

[optout21]

I think what is meant here is that equal-height blocks have equal-height skip values, the pointer can be different (if on different forks). I suggest changing to "height skip values"

[optout21]

ACK 3635d62

With the new chain_test test, this optimization reduces the number of steps in LastCommonAncestor by at least 5-fold (e.g. from 5898713 to 1119703), an obvious optimization.
However, timing the tests (pre and post optimization) I could not measure a difference.
I've left two non-blocking nit comments.

[optout21]

However, timing the tests (pre and post optimization) I could not measure a difference.

Most of the CPU time in the test is taken by the NaiveGetAncestor and NaiveLastCommonAncestor naive implementations. Taken out calls to those, I have measured a 47% speedup in the execution of the test (user time).

[stratospher]

ACK 3635d62.

Seems to work better when there are fewer but deeper forks.

Got this when running each LCA algorithm 10,000 times on the same block pairs in the same block constellation:

• No forks - kind of same
  LastCommonAncestor : 962us
  NaiveLastCommonAncestor : 986us

• 0.1% forks - 18x slower
  LastCommonAncestor : 1103us
  NaiveLastCommonAncestor : 20481us

• 0.5% forks - 11x slower
  LastCommonAncestor : 1030us
  NaiveLastCommonAncestor : 11591us

• 1% forks - 8x slower
  LastCommonAncestor : 1023us
  NaiveLastCommonAncestor : 8580us

• 5% forks - 3.44x slower
  LastCommonAncestor : 781us
  NaiveLastCommonAncestor : 2688us

Though it doesn't matter much since deep forks are a rare scenario.

[achow101]

ACK 3635d62