-
Notifications
You must be signed in to change notification settings - Fork 175
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
[Execution] Refactor Checker Engine #5184
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5184 +/- ##
==========================================
- Coverage 56.63% 56.62% -0.02%
==========================================
Files 985 987 +2
Lines 94333 94448 +115
==========================================
+ Hits 53427 53480 +53
- Misses 36935 36986 +51
- Partials 3971 3982 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
||
// TODO: better to query seals from protocol state, | ||
// switch to state.Final().LastSealed() when available | ||
sealedExecuted, seal, err := c.findLatestSealedAtHeight(lastExecutedHeight) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
||
// findLatestSealedAtHeight finds the latest sealed block at the given height | ||
func (c *Core) findLatestSealedAtHeight(finalizedHeight uint64) (*flow.Header, *flow.Seal, error) { | ||
_, seal, err := c.state.AtHeight(finalizedHeight).SealedResult() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
engine/execution/checker/core.go
Outdated
return nil, nil, nil, fmt.Errorf("could not get the last sealed block: %w", err) | ||
} | ||
|
||
return lastSealed, lastFinal, lastSeal, err |
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.
nit
return lastSealed, lastFinal, lastSeal, err | |
return lastSealed, lastFinal, lastSeal, 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.
Looks good. Just left comments about some cosmetic issues
engine/execution/checker/core.go
Outdated
ticker := time.NewTicker(tickInterval) | ||
defer ticker.Stop() // critical for ticker to be garbage collected | ||
for { | ||
select { | ||
case <-ticker.C: | ||
err := c.runCheck() | ||
if err != nil { | ||
return err | ||
} | ||
case <-ctx.Done(): | ||
return 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.
It might be cleaner to do this part in the engine.go
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.
Good point
engine/execution/checker/core.go
Outdated
// it skips when the last sealed has not been executed, and last executed has not been finalized. | ||
func (c *Core) runCheck() error { |
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.
// it skips when the last sealed has not been executed, and last executed has not been finalized. | |
func (c *Core) runCheck() error { | |
// RunCheck skips when the last sealed has not been executed, and last executed has not been finalized. | |
func (c *Core) RunCheck() error { |
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.
Any particular reason to make this method public?
nvm, saw your comments
return err | ||
} | ||
|
||
mycommitAtLastSealed, err := c.execState.StateCommitmentByBlockID(lastSealedBlock.ID()) |
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.
mycommitAtLastSealed, err := c.execState.StateCommitmentByBlockID(lastSealedBlock.ID()) | |
myCommitAtLastSealed, err := c.execState.StateCommitmentByBlockID(lastSealedBlock.ID()) |
59777e8
to
e44e377
Compare
Co-authored-by: Janez Podhostnik <67895329+janezpodhostnik@users.noreply.github.com>
Close #5173
This PR refactors the checker engine to close #5173