Skip to content
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

Fixes accounts lt hash verification at startup #3465

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

brooksprumo
Copy link

@brooksprumo brooksprumo commented Nov 4, 2024

Problem

When testing out accounts lt hash startup verification, if the accounts lt hash verification fails when running in the background, we don't panic/bring down the whole node.

This is because of the early returns.

After the background verification ends, we need to call .background_finished(). Otherwise we'll continue waiting on the result of the verification. I introduced the early returns in a previous PR, and so if the verification fails we'll never call background_finished() and thus are never notified.

Summary of Changes

Remove the early returns so that background_finished() is always called.

@brooksprumo brooksprumo self-assigned this Nov 4, 2024
@brooksprumo brooksprumo marked this pull request as ready for review November 4, 2024 19:55
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

looks like it is a good candidate for "Drop" guard - we could introduce a VerificationFinishGuard and when it drops, call background_finished().

@brooksprumo brooksprumo merged commit eb3b18e into anza-xyz:master Nov 4, 2024
40 checks passed
@brooksprumo brooksprumo deleted the lthash/verify branch November 4, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants