-
Notifications
You must be signed in to change notification settings - Fork 179
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
Expose minimum required version to cadence interface #6560
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
a79c9b8
Expose version boundary to cadence interface
janezpodhostnik f82f503
cleanup and test
janezpodhostnik f36aafe
fix invalidation in indexer
janezpodhostnik 59269e0
Merge branch 'master' into janez/version-boundary-to-cadence
janezpodhostnik fe18d5a
switch to a different aproach
janezpodhostnik 4166e0a
fix tests
janezpodhostnik b2fa1c3
fix lint
janezpodhostnik 2109f64
cleanup
janezpodhostnik a72cd52
address review comments
janezpodhostnik 2ccf370
add more comments
janezpodhostnik d9d5ef4
Merge branch 'master' into janez/version-boundary-to-cadence
janezpodhostnik 9c47667
swith to single mapped version
janezpodhostnik 456e8a6
fix test
janezpodhostnik 58e84cd
allpy review comments
janezpodhostnik b2ee0e9
fix mocks
janezpodhostnik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -62,6 +62,12 @@ type Environment interface { | |||||||||||||||||||||
error, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// GetCurrentVersionBoundary executes the getCurrentVersionBoundary function on the NodeVersionBeacon contract. | ||||||||||||||||||||||
// the function will return the version boundary (version, block height) that is currently in effect. | ||||||||||||||||||||||
// the version boundary currently in effect is the highest one not above the current block height. | ||||||||||||||||||||||
// if there is no existing version boundary lower than the current block height, the function will return version 0 and block height 0. | ||||||||||||||||||||||
GetCurrentVersionBoundary() (cadence.Value, error) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment, but this is a different function. This one just exposes the one on |
||||||||||||||||||||||
|
||||||||||||||||||||||
// AccountInfo | ||||||||||||||||||||||
GetAccount(address flow.Address) (*flow.Account, error) | ||||||||||||||||||||||
GetAccountKeys(address flow.Address) ([]flow.AccountPublicKey, error) | ||||||||||||||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
package environment | ||
|
||
import ( | ||
"github.com/coreos/go-semver/semver" | ||
|
||
"github.com/onflow/flow-go/fvm/storage/state" | ||
) | ||
|
||
// MinimumCadenceRequiredVersion returns the minimum required cadence version for the current environment | ||
// in semver format. | ||
type MinimumCadenceRequiredVersion interface { | ||
MinimumRequiredVersion() (string, error) | ||
} | ||
|
||
type minimumCadenceRequiredVersion struct { | ||
txnPreparer state.NestedTransactionPreparer | ||
} | ||
|
||
func NewMinimumCadenceRequiredVersion( | ||
txnPreparer state.NestedTransactionPreparer, | ||
) MinimumCadenceRequiredVersion { | ||
return minimumCadenceRequiredVersion{ | ||
txnPreparer: txnPreparer, | ||
} | ||
} | ||
|
||
// MinimumRequiredVersion The returned cadence version can be used by cadence runtime for supporting feature flag. | ||
// The feature flag in cadence allows ENs to produce consistent results even if running with | ||
// different cadence versions at the same height, which is useful for rolling out cadence | ||
// upgrade without all ENs restarting all together. | ||
// For instance, we would like to grade cadence from v1 to v3, where v3 has a new cadence feature. | ||
// We first make a cadence v2 that has feature flag only turned on when the MinimumRequiredVersion() | ||
// method returns v2 or above. | ||
// So cadence v2 with the feature flag turned off will produce the same result as v1 which doesn't have the feature. | ||
// And cadence v2 with the feature flag turned on will also produce the same result as v3 which has the feature. | ||
// The feature flag allows us to roll out cadence v2 to all ENs which was running v1. | ||
// And we use the MinimumRequiredVersion to control when the feature flag should be switched from off to on. | ||
// And the switching should happen at the same height for all ENs. | ||
// | ||
// The height-based switch over can be done by using VersionBeacon, however, the VersionBeacon only | ||
// defines the flow-go version, not cadence version. | ||
// So we first read the current minimum required flow-go version from the VersionBeacon control, | ||
// and map it to the cadence version to be used by cadence to decide feature flag status. | ||
// | ||
// For instance, let’s say all ENs are running flow-go v0.37.0 with cadence v1. | ||
// We first create a version mapping entry for flow-go v0.37.1 to cadence v2, and roll out v0.37.1 to all ENs. | ||
// v0.37.1 ENs will produce the same result as v0.37.0 ENs, because the current version beacon still returns v0.37.0, | ||
// which maps zero cadence version, and cadence will keep the feature flag off. | ||
// | ||
// After all ENs have upgraded to v0.37.1, we send out a version beacon to switch to v0.37.1 at a future height, | ||
// let’s say height 1000. | ||
// Then what happens is that: | ||
// 1. ENs running v0.37.0 will crash after height 999, until upgrade to higher version | ||
// 2. ENs running v0.37.1 will execute with cadence v2 with feature flag off up until height 999, and from height 1000, | ||
// the feature flag will be on, which means all v0.37.1 ENs will again produce consistent results for blocks above 1000. | ||
// | ||
// After height 1000 have been sealed, we can roll out v0.37.2 to all ENs with cadence v3, and it will produce the consistent | ||
// result as v0.37.1. | ||
func (c minimumCadenceRequiredVersion) MinimumRequiredVersion() (string, error) { | ||
executionParameters := c.txnPreparer.ExecutionParameters() | ||
|
||
// map the minimum required flow-go version to a minimum required cadence version | ||
cadenceVersion := mapToCadenceVersion(executionParameters.ExecutionVersion, minimumFvmToMinimumCadenceVersionMapping) | ||
|
||
return cadenceVersion.String(), nil | ||
} | ||
|
||
func mapToCadenceVersion(flowGoVersion semver.Version, versionMapping FlowGoToCadenceVersionMapping) semver.Version { | ||
if versionGreaterThanOrEqualTo(flowGoVersion, versionMapping.FlowGoVersion) { | ||
return versionMapping.CadenceVersion | ||
} else { | ||
return semver.Version{} | ||
} | ||
} | ||
|
||
func versionGreaterThanOrEqualTo(version semver.Version, other semver.Version) bool { | ||
return version.Compare(other) >= 0 | ||
} | ||
|
||
type FlowGoToCadenceVersionMapping struct { | ||
FlowGoVersion semver.Version | ||
CadenceVersion semver.Version | ||
} | ||
|
||
// This could also be a map, but ist not needed because we only expect one entry at a give time | ||
// we won't be fixing 2 separate issues at 2 separate version with one deploy. | ||
var minimumFvmToMinimumCadenceVersionMapping = FlowGoToCadenceVersionMapping{ | ||
// Leaving this example in, so it's easier to understand | ||
// | ||
// FlowGoVersion: *semver.New("0.37.0"), | ||
// CadenceVersion: *semver.New("1.0.0"), | ||
// | ||
} | ||
|
||
func SetFVMToCadenceVersionMappingForTestingOnly(mapping FlowGoToCadenceVersionMapping) { | ||
minimumFvmToMinimumCadenceVersionMapping = mapping | ||
} | ||
|
||
var _ MinimumCadenceRequiredVersion = (*minimumCadenceRequiredVersion)(nil) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package environment | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/coreos/go-semver/semver" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func Test_MapToCadenceVersion(t *testing.T) { | ||
flowV0 := semver.Version{} | ||
cadenceV0 := semver.Version{} | ||
flowV1 := semver.Version{ | ||
Major: 0, | ||
Minor: 37, | ||
Patch: 0, | ||
} | ||
cadenceV1 := semver.Version{ | ||
Major: 1, | ||
Minor: 0, | ||
Patch: 0, | ||
} | ||
|
||
mapping := FlowGoToCadenceVersionMapping{ | ||
FlowGoVersion: flowV1, | ||
CadenceVersion: cadenceV1, | ||
} | ||
|
||
t.Run("no mapping, v0", func(t *testing.T) { | ||
version := mapToCadenceVersion(flowV0, FlowGoToCadenceVersionMapping{}) | ||
|
||
require.Equal(t, cadenceV0, version) | ||
}) | ||
|
||
t.Run("v0", func(t *testing.T) { | ||
version := mapToCadenceVersion(flowV0, mapping) | ||
|
||
require.Equal(t, semver.Version{}, version) | ||
}) | ||
t.Run("v1 - delta", func(t *testing.T) { | ||
|
||
v := flowV1 | ||
v.Patch -= 1 | ||
|
||
version := mapToCadenceVersion(v, mapping) | ||
|
||
require.Equal(t, cadenceV0, version) | ||
}) | ||
t.Run("v1", func(t *testing.T) { | ||
version := mapToCadenceVersion(flowV1, mapping) | ||
|
||
require.Equal(t, cadenceV1, version) | ||
}) | ||
t.Run("v1 + delta", func(t *testing.T) { | ||
|
||
v := flowV1 | ||
v.BumpPatch() | ||
|
||
version := mapToCadenceVersion(v, mapping) | ||
|
||
require.Equal(t, cadenceV1, version) | ||
}) | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Optional:
This introduce unnecessary complexity to the GetMinimumExecutionVersion function.
And create a shortcut for tests. Can we mock the the smart contract call in tests instead?
I saw there are also other WithXXX functions having the same pattern which is to use some flag variables only for testing. IMO, this is a bad practice. We shouldn't have production code skipping some logic (shortcuts) only for tests.
I'm ok for now, but better address this altogether in a separate PR.
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.
For most test this was not used. I only used it for tests that are testing the programs cache loading and resetting. The reason I did that is that an extra contract is loading now, and those tests were off by 1. I think its cleaner for those tests to focus on what they were testing without this interference.
I agree its not the bast pattern, and I will try mocking in a separate PR.
However I am thinking of also using this setting for bootstrapping...