Expose minimum required version to cadence interface #6560
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6560 +/- ##
========================================
Coverage 41.12% 41.12%
========================================
Files 2036 2038 +2
Lines 180114 180257 +143
========================================
+ Hits 74067 74131 +64
- Misses 99833 99905 +72
- Partials 6214 6221 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -287,11 +287,17 @@ func (c *IndexerCore) updateProgramCache(header *flow.Header, events []flow.Even | |||
|
|
|||
| tx.AddInvalidator(&accessInvalidator{ | |||
| programs: &programInvalidator{ | |||
| invalidated: updatedContracts, | |||
| invalidated: updatedContracts, | |||
| invalidateAll: hasAuthorizedTransaction(collections, c.serviceAddress), | |||
There was a problem hiding this comment.
This was missing
janezpodhostnik
changed the title
There was a problem hiding this comment.
Mainly just looked at how this will integrate with Cadence, e.g. https://github.com/dapperlabs/cadence-internal/pull/270
fvm/executionParameters.go
Outdated
| @@ -336,3 +357,39 @@ func GetExecutionMemoryLimit( | |||
|
|
|||
| return uint64(memoryLimitRaw), nil | |||
| } | |||
|
|
|||
| func GetMinimumExecutionVersion( | |||
There was a problem hiding this comment.
This method seems have nothing to do with Minimum, no?
| func GetMinimumExecutionVersion( | |
| func GetCurrentExecutionVersion( |
Please also add comments.
There was a problem hiding this comment.
The version as defined on the Node version beacon is not the current version, but the minimum required version. That is why I called the method this way. I will add a comment.
| @@ -392,3 +392,12 @@ func WithEVMTracer(tracer debug.EVMTracer) Option { | |||
| return ctx | |||
| } | |||
| } | |||
|
|
|||
| // WithReadVersionFromNodeVersionBeacon sets whether the version from the node version beacon should be read | |||
| // this should only be disabled for testing | |||
There was a problem hiding this comment.
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.
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...
| } | ||
| boundaries[i] = boundary | ||
| } | ||
|
|
||
| return boundaries, nil | ||
| } | ||
|
|
||
| func VersionBoundary(value cadence.Value) ( |
There was a problem hiding this comment.
Added. Though this is not new code. Its just extracted from above.
| @@ -62,6 +62,8 @@ type Environment interface { | |||
| error, | |||
| ) | |||
|
|
|||
| GetCurrentVersionBoundary() (cadence.Value, error) | |||
There was a problem hiding this comment.
| GetCurrentVersionBoundary() (cadence.Value, error) | |
| // GetCurrentVersionBoundary returns the current version boundary stored in the VersionBeacon | |
| // system contract. | |
| // There are three cases: | |
| // 1. If it's never set in the smart contract, the current value would be a zero version boundary value. | |
| // 2. The current boundary value is set with a higher version than the node, and | |
| // a higher height than the current block to be executed. | |
| // 3. The current boundary value is set with a lower or same version than the node, and | |
| //. a lower height than the current block to be executed. | |
| GetCurrentVersionBoundary() (cadence.Value, error) |
There was a problem hiding this comment.
Added a comment, but this is a different function. This one just exposes the one on SystemContracts to the Environment interface
| return cadenceVersion.String(), nil | ||
| } | ||
|
|
||
| func mapToCadenceVersion(version semver.Version) semver.Version { |
There was a problem hiding this comment.
Too vague, it's better to be more explicit.
| func mapToCadenceVersion(version semver.Version) semver.Version { | |
| func mapToCadenceVersion(flowGoVersion semver.Version) semver.Version { |
There was a problem hiding this comment.
Can we add lots of test cases to this function to describe the behavior?
I would suggest to let it take the fvmToCadenceVersionMapping as input, so that it's easy to write unittests.
| func mapToCadenceVersion(version semver.Version) semver.Version { | |
| func mapToCadenceVersion(version semver.Version, fvmToCadenceVersionMapping []VersionMapEntry{) semver.Version { |
There was a problem hiding this comment.
There are quite a few cases:
- when version is
0.0.0, andfvmToCadenceVersionMappingis empty - when version is
0.0.0, andfvmToCadenceVersionMappingis[0.37.0 : 1.0.0] - when version is
0.37.20, andfvmToCadenceVersionMappingis[0.37.0: 1.0.0] - when version is
0.37.20, andfvmToCadenceVersionMappingis[v0.37.24 : 1.0.0] - when version is
0.37.20, andfvmToCadenceVersionMappingis[v0.37.24 : 1.0.0, v0.37.25 : 1.0.1] - when version is
0.37.20, andfvmToCadenceVersionMappingis[v0.37.19 : 1.0.0, v0.37.20 : 1.0.1, v0.37.21 : 1.0.2, v0.37.20 : 1.0.3]
There was a problem hiding this comment.
added tests
| type VersionMapEntry struct { | ||
| FlowGoVersion semver.Version | ||
| CadenceVersion semver.Version | ||
| } |
There was a problem hiding this comment.
| type VersionMapEntry struct { | |
| FlowGoVersion semver.Version | |
| CadenceVersion semver.Version | |
| } | |
| // MinVersionMapEntry defines a version map between flow-go and cadence version, | |
| // such that cadence at that version must be run with a flow-go version that is greater | |
| // or equal to the given min version. | |
| type MinVersionMapEntry struct { | |
| MinFlowGoVersion semver.Version | |
| CadenceVersion semver.Version | |
| } |
There was a problem hiding this comment.
Technically the version map doesn't need to know that this is a mapping between minimums.
| return semver.Version{}, fmt.Errorf("could not parse current version boundary: %w", err) | ||
| } | ||
|
|
||
| semVer, err := semver.NewVersion(boundary.Version) |
There was a problem hiding this comment.
I feel the name "current" in GetCurrentVersionBoundary is confusing, because there are two cases at least:
- boundary.Version > the version the node is currently running at.
This means there is a up coming version change at the future height, which means in this case boundary.Height is greater than the height of the block to be executed - boundary.Version <= the version the node is currently running at.
In this case, there could be no version beacon was set in the past, or if set, the boundary.Height must be lower than the Height of the block to be executed.
However, we just return the Version value here without the Height, it means we won't distinguish these two cases. Is that right?
I think we might need two variables:
- boundary.Version
- IsVersionForUnreachedFutureHeight bool
IsVersionForUnreachedFutureHeight is true if the version is a higher version for the future height.
IsVersionForUnreachedFutureHeight is false if the version is a lower version for the past height or same height as the block to be executed.
I think we only need to map version when IsVersionForUnreachedFutureHeight is true, right?
And since there are two cases here, regardless I think it's better explain each case in comments for the map version function .
There was a problem hiding this comment.
that is why current is in the name, because it is the current version boundary that is in effect (boundary.Height <= current height). so there is just one case, and that is boundary.Version <= the version the node is currently running at. The reason is that is that if we were in the situation where boundary.Version > the version the execution of this transaction would not be permitted (that logic is in stop control)
I already added extra comments to GetCurrentVersionBoundary but I'll add some more here.
| // return 0.0.0 if there is no mapping for the version | ||
| var cadenceVersion = semver.Version{} | ||
|
|
||
| greaterThanOrEqualTo := func(version semver.Version, versionToCompare semver.Version) bool { |
There was a problem hiding this comment.
we can move this pure function outside
| ) | ||
|
|
||
| func Test_MapToCadenceVersion(t *testing.T) { | ||
| v0 := semver.Version{} |
There was a problem hiding this comment.
| v0 := semver.Version{} | |
| cadenceV0 := semver.Version{} |
| flowV1 := semver.Version{ | ||
| Major: 1, | ||
| Minor: 2, | ||
| Patch: 3, | ||
| PreRelease: "rc.1", | ||
| } | ||
| flowV2 := semver.Version{ | ||
| Major: 2, | ||
| Minor: 2, | ||
| Patch: 3, | ||
| PreRelease: "rc.1", | ||
| } | ||
|
|
||
| cadenceV1 := semver.Version{ | ||
| Major: 2, | ||
| Minor: 1, | ||
| Patch: 3, | ||
| PreRelease: "rc.2", | ||
| } | ||
| cadenceV2 := semver.Version{ | ||
| Major: 12, | ||
| Minor: 0, | ||
| Patch: 0, | ||
| PreRelease: "", | ||
| } |
There was a problem hiding this comment.
| flowV1 := semver.Version{ | |
| Major: 1, | |
| Minor: 2, | |
| Patch: 3, | |
| PreRelease: "rc.1", | |
| } | |
| flowV2 := semver.Version{ | |
| Major: 2, | |
| Minor: 2, | |
| Patch: 3, | |
| PreRelease: "rc.1", | |
| } | |
| cadenceV1 := semver.Version{ | |
| Major: 2, | |
| Minor: 1, | |
| Patch: 3, | |
| PreRelease: "rc.2", | |
| } | |
| cadenceV2 := semver.Version{ | |
| Major: 12, | |
| Minor: 0, | |
| Patch: 0, | |
| PreRelease: "", | |
| } | |
| flowV1 := semver.Version{ | |
| Major: 0, | |
| Minor: 37, | |
| Patch: 0, | |
| } | |
| flowV2 := semver.Version{ | |
| Major: 0, | |
| Minor: 37, | |
| Patch: 1, | |
| } | |
| cadenceV1 := semver.Version{ | |
| Major: 1, | |
| Minor: 0, | |
| Patch: 0, | |
| } | |
| cadenceV2 := semver.Version{ | |
| Major: 1, | |
| Minor: 0, | |
| Patch: 1, | |
| } |
There was a problem hiding this comment.
👍 I changed the versions a little bit, because in the tests I bump the patch version and I dont want overlap
| return cadenceVersion.String(), nil | ||
| } | ||
|
|
||
| func mapToCadenceVersion(flowGoVersion semver.Version, versionMapping []VersionMapEntry) semver.Version { |
There was a problem hiding this comment.
| func mapToCadenceVersion(flowGoVersion semver.Version, versionMapping []VersionMapEntry) semver.Version { | |
| // find the highest VersionEntry from the versionMapping that is below the given flowGoVersion, and | |
| // return its CadenceVersion | |
| func mapToCadenceVersion(flowGoVersion semver.Version, versionMapping []VersionMapEntry) semver.Version { |
| func (c minimumRequiredVersion) MinimumRequiredVersion() (string, error) { | ||
| executionParameters := c.txnPreparer.ExecutionParameters() | ||
|
|
||
| cadenceVersion := mapToCadenceVersion(executionParameters.ExecutionVersion, fvmToCadenceVersionMapping) |
There was a problem hiding this comment.
| cadenceVersion := mapToCadenceVersion(executionParameters.ExecutionVersion, fvmToCadenceVersionMapping) | |
| // given the minimum required version defined by executionParameters.ExecutionVersion, | |
| // find the highest verion entry in the version mapping that satisfies the minimum version | |
| // requirement, and returns its cadence version | |
| cadenceVersion := mapToCadenceVersion(executionParameters.ExecutionVersion, fvmToCadenceVersionMapping) |
| type MinimumRequiredVersion interface { | ||
| MinimumRequiredVersion() (string, error) | ||
| } |
There was a problem hiding this comment.
Should we be more specific?
| type MinimumRequiredVersion interface { | |
| MinimumRequiredVersion() (string, error) | |
| } | |
| type MinimumRequiredCadenceVersion interface { | |
| MinimumRequiredCadenceVersion() (string, error) | |
| } |
There was a problem hiding this comment.
this one is a bit tricky. This method satisfies the cadence runtime interface (defined by cadence) and from a cadence perspective it does not make sense to name it a MinimumRequiredCadenceVersion so its just named MinimumRequiredVersion and in the FVM ve just have to implement that interface method.
I'll add a comment
There was a problem hiding this comment.
I see. Maybe we keep the method name as MinimumRequiredVersion, but rename the interface as MinimumCadenceRequiredVersion ?
| if greaterThanOrEqualTo(flowGoVersion, entry.FlowGoVersion) { | ||
| cadenceVersion = entry.CadenceVersion | ||
| } else { | ||
| break |
There was a problem hiding this comment.
Why break here?
It seems we always just look at the very first version entry only, in that case, why not just having one version entry instead of a mapping?
If we are going to hardcode a version map entry, I would say we just hard code the default one:
var DefaultVersionMapEntry = VersionMapEntry{
FlowGoVersion: semver.Version{"0.0.0"}, CadenceVersion: semver.Version{"0.0.0"} }
// change this with versions FlowGoVersion: F, and CadenceVersion: C, where
// CadenceVersion C had a feature toggle which won't be enabled if FlowGoVersion is below F.
var RequiredVersionMapEntry = VersionMapEntry{
FlowGoVersion: semver.Version{"0.0.0"}, CadenceVersion: semver.Version{"0.0.0"} }
then we can simplify this function into:
func mapToCadenceVersion(flowGoVersion semver.Version, versionMapping VersionMapEntry) semver.Version {
if greateThanOrEqualTo(flowGoVersion, versionMapping.FlowGoVersion) {
return versionMapping.CadenceVersion
}
return DefaultVersionEntry.CadenceVersion
}
| @@ -185,11 +185,9 @@ const ( | |||
| FVMEnvRandomSourceHistoryProvider SpanName = "fvm.env.randomSourceHistoryProvider" | |||
| FVMEnvCreateAccount SpanName = "fvm.env.createAccount" | |||
| FVMEnvAddAccountKey SpanName = "fvm.env.addAccountKey" | |||
| FVMEnvAddEncodedAccountKey SpanName = "fvm.env.addEncodedAccountKey" | |||
| type MinimumRequiredVersion interface { | ||
| MinimumRequiredVersion() (string, error) | ||
| } |
There was a problem hiding this comment.
I see. Maybe we keep the method name as MinimumRequiredVersion, but rename the interface as MinimumCadenceRequiredVersion ?
| } | ||
| } | ||
|
|
||
| func (c minimumRequiredVersion) MinimumRequiredVersion() (string, error) { |
There was a problem hiding this comment.
| func (c minimumRequiredVersion) MinimumRequiredVersion() (string, error) { | |
| // The returned cadence version can be used by cadnece 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 minimumRequiredVersion) MinimumRequiredVersion() (string, error) { |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This adds a minimum required version to the cadence interface.
This is a cadence version mapped from the flow go version as defined in the Node version beacon contract.
How it works:
Defining the version
FVM gettting the version
FVM serving the version to cadence
Rolling node upgrades using cadence minimum required version
We are running flow-go v3.0.0 (cadence v15.0.0) and we want to deploy a (small) change