-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blockchain: Add ancestor optimization to finding Ancestor #1688
blockchain: Add ancestor optimization to finding Ancestor #1688
Conversation
Pull Request Test Coverage Report for Build 530293241Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a cool optimization, but I think the core algorithm could be implemented a in simpler manner for clarity, and also tested better. Completed an initial pass, but will do another to really grok the algorithm.
blockchain/blockindex.go
Outdated
heightSkip := getSkipHeight(heightWalk) | ||
heightSkipPrev := getSkipHeight(heightWalk - 1) | ||
|
||
if indexWalk.ancestor != nil && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a mouth-full of a conditional...
It also isn't immediately clear the significance (nor origin) of the -2
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of everything here and changed the logic to be simpler.
idk why that -2 is there as well... Some sipa magic
Commenting here because I just saw @Roasbeef's tweet linking to it. FYI I implemented something similar in dcrd back in 2019 in what is imo a simpler way and should be a simple backport: |
52c9778
to
126d920
Compare
On startup, Ancestor call was taking a lot of time when the node was loading the blockindex onto memory. This change speeds up the Ancestor function significantly and speeds up the node during startup. On testnet3 at blockheight ~2,500,000, the startup was around 30seconds on current main and was 5 seconds with this change. Below is a benchstat result showing the significant speedup. goos: darwin goarch: arm64 pkg: github.com/utreexo/utreexod/blockchain │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ Ancestor-8 120819.301µ ± 5% 7.013µ ± 19% -99.99% (p=0.000 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ Ancestor-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ Ancestor-8 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal
126d920
to
f396b3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎢
This significantly cut down on node start up time on testnet + mainnet.
This mimics the behavior of Ancestor() in bitcoind