-
Notifications
You must be signed in to change notification settings - Fork 170
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
[General] Root snapshot QC validation #2038
Conversation
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. Some minor suggestions.
state/protocol/badger/validity.go
Outdated
clusterRootBlock := &model.Block{ | ||
BlockID: clusterBlock.ID(), | ||
View: clusterBlock.Header.View, | ||
ProposerID: clusterBlock.Header.ProposerID, | ||
QC: nil, | ||
PayloadHash: clusterBlock.Header.PayloadHash, | ||
Timestamp: clusterBlock.Header.Timestamp, | ||
} |
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.
could we extract it into a function, and reuse here?
@@ -212,6 +212,12 @@ func finalize(cmd *cobra.Command, args []string) { | |||
log.Fatal().Err(err).Msg("the generated root snapshot is invalid") | |||
} | |||
|
|||
// validate the generated root snapshot QCs | |||
err = badger.IsValidRootSnapshotQCs(snapshot) |
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.
could we move IsValidRootSnapshotQCs
into IsValidRootSnapshot
?
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.
We cannot because IsValidRootSnapshot
is used in ~130 tests, moving it will break every test or require us to generate a completely valid QC instead of fixtures.
Codecov Report
@@ Coverage Diff @@
## master #2038 +/- ##
==========================================
- Coverage 57.63% 57.49% -0.15%
==========================================
Files 631 631
Lines 36233 36285 +52
==========================================
- Hits 20884 20861 -23
- Misses 12732 12807 +75
Partials 2617 2617
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Just one comment, other LGTM. Thanks for this
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
bors merge |
3444: Add AccountKeysCount to AccountKeyReader to match changes in Cadence's runtime Interface r=turbolent a=dreamsmasher Closes [Cadence issue 1388](onflow/cadence#1388). Two new fields/methods have been added to `PublicAccount.keys` and `AuthAccount.keys` in [Cadence PR 2038](onflow/cadence#2038): `pub let count: Int` `pub fun forEach(f: ((AccountKey): Bool))` Both are implemented top of `AccountKeyReader.GetAccountKey` and `Accounts.GetPublicKeyCount`. As part of [#2038](onflow/cadence#2038), the core runtime `Interface` was updated to require `AccountKeysCount`, which was previously implemented as part of the FVM's `Accounts` struct but not exposed to Cadence. This PR introduces `AccountKeysCount` to the `AccountKeyReader` interface to fulfill these new upstream requirements. Blocked by onflow/cadence#2093 Co-authored-by: Naomi Liu <naomi.liu@dapperlabs.com>
3444: Add AccountKeysCount to AccountKeyReader to match changes in Cadence's runtime Interface r=turbolent a=dreamsmasher Closes [Cadence issue 1388](onflow/cadence#1388). Two new fields/methods have been added to `PublicAccount.keys` and `AuthAccount.keys` in [Cadence PR 2038](onflow/cadence#2038): `pub let count: Int` `pub fun forEach(f: ((AccountKey): Bool))` Both are implemented top of `AccountKeyReader.GetAccountKey` and `Accounts.GetPublicKeyCount`. As part of [#2038](onflow/cadence#2038), the core runtime `Interface` was updated to require `AccountKeysCount`, which was previously implemented as part of the FVM's `Accounts` struct but not exposed to Cadence. This PR introduces `AccountKeysCount` to the `AccountKeyReader` interface to fulfill these new upstream requirements. Blocked by onflow/cadence#2093 Co-authored-by: Naomi Liu <naomi.liu@dapperlabs.com> Co-authored-by: Bastian Müller <bastian@axiomzen.co>
3444: Add AccountKeysCount to AccountKeyReader to match changes in Cadence's runtime Interface r=turbolent a=dreamsmasher Closes [Cadence issue 1388](onflow/cadence#1388). Two new fields/methods have been added to `PublicAccount.keys` and `AuthAccount.keys` in [Cadence PR 2038](onflow/cadence#2038): `pub let count: Int` `pub fun forEach(f: ((AccountKey): Bool))` Both are implemented top of `AccountKeyReader.GetAccountKey` and `Accounts.GetPublicKeyCount`. As part of [#2038](onflow/cadence#2038), the core runtime `Interface` was updated to require `AccountKeysCount`, which was previously implemented as part of the FVM's `Accounts` struct but not exposed to Cadence. This PR introduces `AccountKeysCount` to the `AccountKeyReader` interface to fulfill these new upstream requirements. Blocked by onflow/cadence#2093 3450: Clean up fvm mocks r=pattyshack a=pattyshack 1. Removed dead mock directory fvm/mock 2. Updated makefile to wipe environment/mock directory before regenerating mocks 3. Updated test to stop using dead mocks Co-authored-by: Naomi Liu <naomi.liu@dapperlabs.com> Co-authored-by: Bastian Müller <bastian@axiomzen.co> Co-authored-by: Patrick Lee <patrick.lee@dapperlabs.com>
3440: Test ErrorsCollector against SplitError r=pattyshack a=pattyshack Forgot to add test for this during the refactoring. 3444: Add AccountKeysCount to AccountKeyReader to match changes in Cadence's runtime Interface r=turbolent a=dreamsmasher Closes [Cadence issue 1388](onflow/cadence#1388). Two new fields/methods have been added to `PublicAccount.keys` and `AuthAccount.keys` in [Cadence PR 2038](onflow/cadence#2038): `pub let count: Int` `pub fun forEach(f: ((AccountKey): Bool))` Both are implemented top of `AccountKeyReader.GetAccountKey` and `Accounts.GetPublicKeyCount`. As part of [#2038](onflow/cadence#2038), the core runtime `Interface` was updated to require `AccountKeysCount`, which was previously implemented as part of the FVM's `Accounts` struct but not exposed to Cadence. This PR introduces `AccountKeysCount` to the `AccountKeyReader` interface to fulfill these new upstream requirements. Blocked by onflow/cadence#2093 Co-authored-by: Patrick Lee <patrick.lee@dapperlabs.com> Co-authored-by: Naomi Liu <naomi.liu@dapperlabs.com> Co-authored-by: Bastian Müller <bastian@axiomzen.co>
3444: Add AccountKeysCount to AccountKeyReader to match changes in Cadence's runtime Interface r=turbolent a=dreamsmasher Closes [Cadence issue 1388](onflow/cadence#1388). Two new fields/methods have been added to `PublicAccount.keys` and `AuthAccount.keys` in [Cadence PR 2038](onflow/cadence#2038): `pub let count: Int` `pub fun forEach(f: ((AccountKey): Bool))` Both are implemented top of `AccountKeyReader.GetAccountKey` and `Accounts.GetPublicKeyCount`. As part of [#2038](onflow/cadence#2038), the core runtime `Interface` was updated to require `AccountKeysCount`, which was previously implemented as part of the FVM's `Accounts` struct but not exposed to Cadence. This PR introduces `AccountKeysCount` to the `AccountKeyReader` interface to fulfill these new upstream requirements. Blocked by onflow/cadence#2093 Co-authored-by: Naomi Liu <naomi.liu@dapperlabs.com> Co-authored-by: Bastian Müller <bastian@axiomzen.co>
https://github.com/dapperlabs/flow-go/issues/6145
Context
This PR implements validation of QCs that are part of the root snapshot.
A few important points that were implemented in different way comparing to what is described in ticket:
IsValidRootSnapshot
. (IsValidRootSnapshot
is used in ~130 tests which breaks all of them since we use fixtures for root snapshot).FlowNodeBuilder
andfinalize
tool.