Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Expose minimum required version to cadence interface #6560

Merged
merged 15 commits into from
Oct 28, 2024
Merged

Expose minimum required version to cadence interface #6560

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a79c9b8
Expose version boundary to cadence interface
janezpodhostnik Oct 11, 2024
f82f503
cleanup and test
janezpodhostnik Oct 22, 2024
f36aafe
fix invalidation in indexer
janezpodhostnik Oct 22, 2024
59269e0
Merge branch 'master' into janez/version-boundary-to-cadence
janezpodhostnik Oct 22, 2024
fe18d5a
switch to a different aproach
janezpodhostnik Oct 23, 2024
4166e0a
fix tests
janezpodhostnik Oct 23, 2024
b2fa1c3
fix lint
janezpodhostnik Oct 23, 2024
2109f64
cleanup
janezpodhostnik Oct 23, 2024
a72cd52
address review comments
janezpodhostnik Oct 24, 2024
2ccf370
add more comments
janezpodhostnik Oct 25, 2024
d9d5ef4
Merge branch 'master' into janez/version-boundary-to-cadence
janezpodhostnik Oct 25, 2024
9c47667
swith to single mapped version
janezpodhostnik Oct 25, 2024
456e8a6
fix test
janezpodhostnik Oct 25, 2024
58e84cd
allpy review comments
janezpodhostnik Oct 25, 2024
b2ee0e9
fix mocks
janezpodhostnik Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions engine/execution/computation/computer/computer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,7 @@ func TestBlockExecutor_ExecuteBlock(t *testing.T) {
},
),
),
fvm.WithReadVersionFromNodeVersionBeacon(false),
)

vm := fvm.NewVirtualMachine()
Expand Down Expand Up @@ -816,7 +817,9 @@ func TestBlockExecutor_ExecuteBlock(t *testing.T) {
runtime.Config{},
func(_ runtime.Config) runtime.Runtime {
return rt
})))
})),
fvm.WithReadVersionFromNodeVersionBeacon(false),
)

vm := fvm.NewVirtualMachine()

Expand Down Expand Up @@ -929,7 +932,9 @@ func TestBlockExecutor_ExecuteBlock(t *testing.T) {
runtime.Config{},
func(_ runtime.Config) runtime.Runtime {
return rt
})))
})),
fvm.WithReadVersionFromNodeVersionBeacon(false),
)

vm := fvm.NewVirtualMachine()

Expand Down
9 changes: 9 additions & 0 deletions fvm/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.

Copy link
Contributor Author

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...

func WithReadVersionFromNodeVersionBeacon(enabled bool) Option {
return func(ctx Context) Context {
ctx.ReadVersionFromNodeVersionBeacon = enabled
return ctx
}
}
28 changes: 14 additions & 14 deletions fvm/environment/derived_data_invalidator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func (u ContractUpdates) Any() bool {
type DerivedDataInvalidator struct {
ContractUpdates

MeterParamOverridesUpdated bool
ExecutionParametersUpdated bool
}

var _ derived.TransactionInvalidator = DerivedDataInvalidator{}
Expand All @@ -37,16 +37,16 @@ func NewDerivedDataInvalidator(
) DerivedDataInvalidator {
return DerivedDataInvalidator{
ContractUpdates: contractUpdates,
MeterParamOverridesUpdated: meterParamOverridesUpdated(
ExecutionParametersUpdated: executionParametersUpdated(
executionSnapshot,
meterStateRead),
}
}

// meterParamOverridesUpdated returns true if the meter param overrides have been updated
// executionParametersUpdated returns true if the meter param overrides have been updated
// this is done by checking if the registers needed to compute the meter param overrides
// have been touched in the execution snapshot
func meterParamOverridesUpdated(
func executionParametersUpdated(
executionSnapshot *snapshot.ExecutionSnapshot,
meterStateRead *snapshot.ExecutionSnapshot,
) bool {
Expand All @@ -73,16 +73,16 @@ func (invalidator DerivedDataInvalidator) ProgramInvalidator() derived.ProgramIn
return ProgramInvalidator{invalidator}
}

func (invalidator DerivedDataInvalidator) MeterParamOverridesInvalidator() derived.MeterParamOverridesInvalidator {
return MeterParamOverridesInvalidator{invalidator}
func (invalidator DerivedDataInvalidator) ExecutionParametersInvalidator() derived.ExecutionParametersInvalidator {
return ExecutionParametersInvalidator{invalidator}
}

type ProgramInvalidator struct {
DerivedDataInvalidator
}

func (invalidator ProgramInvalidator) ShouldInvalidateEntries() bool {
return invalidator.MeterParamOverridesUpdated ||
return invalidator.ExecutionParametersUpdated ||
invalidator.ContractUpdates.Any()
}

Expand All @@ -91,7 +91,7 @@ func (invalidator ProgramInvalidator) ShouldInvalidateEntry(
program *derived.Program,
_ *snapshot.ExecutionSnapshot,
) bool {
if invalidator.MeterParamOverridesUpdated {
if invalidator.ExecutionParametersUpdated {
// if meter parameters changed we need to invalidate all programs
return true
}
Expand Down Expand Up @@ -124,18 +124,18 @@ func (invalidator ProgramInvalidator) ShouldInvalidateEntry(
return false
}

type MeterParamOverridesInvalidator struct {
type ExecutionParametersInvalidator struct {
DerivedDataInvalidator
}

func (invalidator MeterParamOverridesInvalidator) ShouldInvalidateEntries() bool {
return invalidator.MeterParamOverridesUpdated
func (invalidator ExecutionParametersInvalidator) ShouldInvalidateEntries() bool {
return invalidator.ExecutionParametersUpdated
}

func (invalidator MeterParamOverridesInvalidator) ShouldInvalidateEntry(
func (invalidator ExecutionParametersInvalidator) ShouldInvalidateEntry(
_ struct{},
_ derived.MeterParamOverrides,
_ derived.StateExecutionParameters,
_ *snapshot.ExecutionSnapshot,
) bool {
return invalidator.MeterParamOverridesUpdated
return invalidator.ExecutionParametersUpdated
}
20 changes: 12 additions & 8 deletions fvm/environment/derived_data_invalidator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestDerivedDataProgramInvalidator(t *testing.T) {
})
t.Run("meter parameters invalidator invalidates all entries", func(t *testing.T) {
invalidator := environment.DerivedDataInvalidator{
MeterParamOverridesUpdated: true,
ExecutionParametersUpdated: true,
}.ProgramInvalidator()

require.True(t, invalidator.ShouldInvalidateEntries())
Expand Down Expand Up @@ -207,23 +207,23 @@ func TestDerivedDataProgramInvalidator(t *testing.T) {

func TestMeterParamOverridesInvalidator(t *testing.T) {
invalidator := environment.DerivedDataInvalidator{}.
MeterParamOverridesInvalidator()
ExecutionParametersInvalidator()

require.False(t, invalidator.ShouldInvalidateEntries())
require.False(t, invalidator.ShouldInvalidateEntry(
struct{}{},
derived.MeterParamOverrides{},
derived.StateExecutionParameters{},
nil))

invalidator = environment.DerivedDataInvalidator{
ContractUpdates: environment.ContractUpdates{},
MeterParamOverridesUpdated: true,
}.MeterParamOverridesInvalidator()
ExecutionParametersUpdated: true,
}.ExecutionParametersInvalidator()

require.True(t, invalidator.ShouldInvalidateEntries())
require.True(t, invalidator.ShouldInvalidateEntry(
struct{}{},
derived.MeterParamOverrides{},
derived.StateExecutionParameters{},
nil))
}

Expand Down Expand Up @@ -265,7 +265,11 @@ func TestMeterParamOverridesUpdated(t *testing.T) {
txnState, err := blockDatabase.NewTransaction(0, state.DefaultParameters())
require.NoError(t, err)

computer := fvm.NewMeterParamOverridesComputer(ctx, txnState)
computer := fvm.NewExecutionParametersComputer(
ctx.Logger,
ctx,
txnState,
)

overrides, err := computer.Compute(txnState, struct{}{})
require.NoError(t, err)
Expand Down Expand Up @@ -300,7 +304,7 @@ func TestMeterParamOverridesUpdated(t *testing.T) {
environment.ContractUpdates{},
snapshot,
meterStateRead)
require.Equal(t, expected, invalidator.MeterParamOverridesUpdated)
require.Equal(t, expected, invalidator.ExecutionParametersUpdated)
}

executionSnapshot, err = txnState.FinalizeMainTransaction()
Expand Down
2 changes: 2 additions & 0 deletions fvm/environment/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ type Environment interface {
error,
)

GetCurrentVersionBoundary() (cadence.Value, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 SystemContracts to the Environment interface


// AccountInfo
GetAccount(address flow.Address) (*flow.Account, error)
GetAccountKeys(address flow.Address) ([]flow.AccountPublicKey, error)
Expand Down
4 changes: 4 additions & 0 deletions fvm/environment/facade_env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type facadeEnvironment struct {
ValueStore

*SystemContracts
MinimumRequiredVersion

UUIDGenerator
AccountLocalIDGenerator
Expand Down Expand Up @@ -103,6 +104,9 @@ func newFacadeEnvironment(
),

SystemContracts: systemContracts,
MinimumRequiredVersion: NewMinimumRequiredVersion(
txnState,
),

UUIDGenerator: NewUUIDGenerator(
tracer,
Expand Down
76 changes: 76 additions & 0 deletions fvm/environment/minimum_required_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package environment

import (
"github.com/coreos/go-semver/semver"

"github.com/onflow/flow-go/fvm/storage/state"
)

// MinimumRequiredVersion returns the minimum required cadence version for the current environment
// in semver format.
type MinimumRequiredVersion interface {
MinimumRequiredVersion() (string, error)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be more specific?

Suggested change
type MinimumRequiredVersion interface {
MinimumRequiredVersion() (string, error)
}
type MinimumRequiredCadenceVersion interface {
MinimumRequiredCadenceVersion() (string, error)
}

Copy link
Contributor Author

@janezpodhostnik janezpodhostnik Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe we keep the method name as MinimumRequiredVersion, but rename the interface as MinimumCadenceRequiredVersion ?


type minimumRequiredVersion struct {
txnPreparer state.NestedTransactionPreparer
}

func NewMinimumRequiredVersion(
txnPreparer state.NestedTransactionPreparer,
) MinimumRequiredVersion {
return minimumRequiredVersion{
txnPreparer: txnPreparer,
}
}

func (c minimumRequiredVersion) MinimumRequiredVersion() (string, error) {
Copy link
Member

@zhangchiqing zhangchiqing Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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) {

executionParameters := c.txnPreparer.ExecutionParameters()

cadenceVersion := mapToCadenceVersion(executionParameters.ExecutionVersion)

return cadenceVersion.String(), nil
}

func mapToCadenceVersion(version semver.Version) semver.Version {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too vague, it's better to be more explicit.

Suggested change
func mapToCadenceVersion(version semver.Version) semver.Version {
func mapToCadenceVersion(flowGoVersion semver.Version) semver.Version {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
func mapToCadenceVersion(version semver.Version) semver.Version {
func mapToCadenceVersion(version semver.Version, fvmToCadenceVersionMapping []VersionMapEntry{) semver.Version {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a few cases:

  1. when version is 0.0.0, and fvmToCadenceVersionMapping is empty
  2. when version is 0.0.0, and fvmToCadenceVersionMapping is [0.37.0 : 1.0.0]
  3. when version is 0.37.20, and fvmToCadenceVersionMapping is [0.37.0: 1.0.0]
  4. when version is 0.37.20, and fvmToCadenceVersionMapping is [v0.37.24 : 1.0.0]
  5. when version is 0.37.20, and fvmToCadenceVersionMapping is [v0.37.24 : 1.0.0, v0.37.25 : 1.0.1]
  6. when version is 0.37.20, and fvmToCadenceVersionMapping is [v0.37.19 : 1.0.0, v0.37.20 : 1.0.1, v0.37.21 : 1.0.2, v0.37.20 : 1.0.3]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can move this pure function outside

return version.Compare(versionToCompare) >= 0
}

for _, entry := range fvmToCadenceVersionMapping {
if greaterThanOrEqualTo(version, entry.FlowGoVersion) {
cadenceVersion = entry.CadenceVersion
} else {
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

}
}

return cadenceVersion
}

type VersionMapEntry struct {
FlowGoVersion semver.Version
CadenceVersion semver.Version
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the version map doesn't need to know that this is a mapping between minimums.


// FVMToCadenceVersionMapping is a hardcoded mapping between FVM versions and Cadence versions.
// Entries are only needed for cadence versions where cadence intends to switch behaviour
// based on the version.
// This should be ordered.
janezpodhostnik marked this conversation as resolved.
Show resolved Hide resolved

var fvmToCadenceVersionMapping = []VersionMapEntry{
// 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 []VersionMapEntry) {
fvmToCadenceVersionMapping = mapping
}

var _ MinimumRequiredVersion = (*minimumRequiredVersion)(nil)
30 changes: 30 additions & 0 deletions fvm/environment/mock/environment.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

52 changes: 52 additions & 0 deletions fvm/environment/mock/minimum_required_version.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading