-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix peak height race #16776
Fix peak height race #16776
Conversation
1aa3af3
to
7ddf4b8
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…between updating the height-to-hash map and the peak height. This leaves a window for a call to get_peak() to fail, because the peak height doesn't exist in height-to-hash map. This patch moves the suspension point to make these updates atomic. This allows for removing a hack in a test, that previously had to catch this exception
7ddf4b8
to
89b4732
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Pull Request Test Coverage Report for Build 6787938941Warning: 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.
aok
Purpose:
When we add a block, there's a subtle co-routine suspension point in between updating the height-to-hash map and the peak height. There's a comment in the code to highlight this now.
This creates a window where a call to
get_peak()
fails because the peak height doesn't exist in height-to-hash map.This is demonstrated in #16774 .
This patch moves the suspension point to make these updates to the height-to-hash map and
peak_height
atomic.Then it's possible to remove the try-catch handler in the test.
Current Behavior:
get_peak()
may throw an exception if called at an unfortunate time.New Behavior:
get_peak()
always work