-
Notifications
You must be signed in to change notification settings - Fork 471
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
ledger refactoring: fix catchpoint tracker #3214
ledger refactoring: fix catchpoint tracker #3214
Conversation
Huh, so should I have been seeing mismatched block digests show up more than I did? |
Codecov Report
@@ Coverage Diff @@
## master #3214 +/- ##
==========================================
- Coverage 47.43% 47.41% -0.03%
==========================================
Files 369 369
Lines 59494 59494
==========================================
- Hits 28222 28209 -13
- Misses 27987 27999 +12
- Partials 3285 3286 +1
Continue to review full report at Codecov.
|
I depends on the precise scenario. A node that was using the fast catchup would produce an incorrect catchpoint label from that point and on. This would "fix itself" if the node happen to reset ( although, the historical generated catchpoint labels would remain incorrect ). |
ct.catchpointWriting = 0 | ||
// keep these channel closed if we're not generating catchpoint | ||
ct.catchpointSlowWriting = make(chan struct{}, 1) | ||
close(ct.catchpointSlowWriting) |
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.
Hrmm ... maybe renaming the loadFromDisk tracker interface method to "reloadFromDisk" or "resetFromDisk" or something to make it more clear for future authors that a tracker could have this method called multiple times during its lifetime?
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.
I'm not opposing to that. I think that the existing method description in the interface definition already define that.
Summary
When implementing the catchpoint tracker, I missed the re-initilization location for some of the local variables.
This would generate incorrect catchpoint labels after a node completes a fast-catchup.
#3085
Test Plan