-
Notifications
You must be signed in to change notification settings - Fork 203
[Storehouse] Storehouse checkpoint validator #8257
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
base: master
Are you sure you want to change the base?
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
fdab802 to
8212b18
Compare
fxamacker
left a comment
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 left some comments about a possible deadlock and also about removing some limitations from the current approach.
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.
Thanks for updating and resolving the deadlock! 👍
I only left a few more minor comments for consideration.
| } | ||
|
|
||
| // IsErrMismatch returns true if the given error is an ErrMismatch or wraps an ErrMismatch. | ||
| func IsErrMismatch(err error) bool { |
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.
IsErrMismatch doesn't appear to be used.
| type ErrMismatch struct { | ||
| RegisterID flow.RegisterID | ||
| Height uint64 | ||
| StoredLength int | ||
| ExpectedLength int | ||
| Message string | ||
| } |
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.
Maybe also include StoredData and ExpectedData in the ErrMismatch struct.
| log.Error(). | ||
| Str("owner", mismatchErr.RegisterID.Owner). | ||
| Str("key", mismatchErr.RegisterID.Key). | ||
| Uint64("height", mismatchErr.Height). | ||
| Int("stored_length", mismatchErr.StoredLength). | ||
| Int("expected_length", mismatchErr.ExpectedLength). | ||
| Msg("register value mismatch") |
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.
Maybe we can simplify this when ErrMismatch.Error() returns a complete message with owner, key, height, and other information.
| log.Error(). | |
| Str("owner", mismatchErr.RegisterID.Owner). | |
| Str("key", mismatchErr.RegisterID.Key). | |
| Uint64("height", mismatchErr.Height). | |
| Int("stored_length", mismatchErr.StoredLength). | |
| Int("expected_length", mismatchErr.ExpectedLength). | |
| Msg("register value mismatch") | |
| log.Error().Err(err).Msg("register value mismatch") |
| func (e *ErrMismatch) Error() string { | ||
| if e.Message != "" { | ||
| return e.Message | ||
| } | ||
| return fmt.Sprintf("register value mismatch: owner=%s, key=%s, height=%d, stored_length=%d, expected_length=%d", | ||
| e.RegisterID.Owner, e.RegisterID.Key, e.Height, e.StoredLength, e.ExpectedLength) | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return &ErrMismatch{ | ||
| RegisterID: registerID, | ||
| Height: height, | ||
| StoredLength: 0, | ||
| ExpectedLength: len(payload.Value()), | ||
| Message: fmt.Sprintf("register not found in store: owner=%s, key=%s, height=%d", registerID.Owner, registerID.Key, height), | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Close #8244
This PR implemented a basic validator to check the consistency between payloads stored in checkpoint file and in storehouse.
Tested the validation with Testnet EN3, it took 42 mins to complete the validation, and no mismatch was found 🎉