From 61df9b3223bae7617e6db531184fd9b1f13c89f6 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 22 Aug 2024 16:10:28 +0100 Subject: [PATCH 01/24] feat: pseudo-generic extra payloads in `params.ChainConfig` and `params.Rules` --- libevm/examples/extraparams/extraparams.go | 36 +++ libevm/examples/go.mod | 13 ++ libevm/examples/go.sum | 14 ++ libevm/pseudo/type.go | 121 ++++++++++ libevm/pseudo/type_test.go | 68 ++++++ params/config.go | 9 +- params/config.libevm.go | 164 ++++++++++++++ params/config.libevm_test.go | 250 +++++++++++++++++++++ 8 files changed, 674 insertions(+), 1 deletion(-) create mode 100644 libevm/examples/extraparams/extraparams.go create mode 100644 libevm/examples/go.mod create mode 100644 libevm/examples/go.sum create mode 100644 libevm/pseudo/type.go create mode 100644 libevm/pseudo/type_test.go create mode 100644 params/config.libevm.go create mode 100644 params/config.libevm_test.go diff --git a/libevm/examples/extraparams/extraparams.go b/libevm/examples/extraparams/extraparams.go new file mode 100644 index 00000000000..fd240a0d409 --- /dev/null +++ b/libevm/examples/extraparams/extraparams.go @@ -0,0 +1,36 @@ +package extraparams + +import ( + "math/big" + + "github.com/ethereum/go-ethereum/libevm/pseudo" + "github.com/ethereum/go-ethereum/params" +) + +func init() { + params.RegisterExtras(params.Extras[ChainConfigExtra, RulesExtra]{ + NewForRules: constructRulesExtra, + }) +} + +type ChainConfigExtra struct { + MyFeatureTime *uint64 +} + +type RulesExtra struct { + IsMyFeature bool +} + +func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx *ChainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *RulesExtra { + return &RulesExtra{ + IsMyFeature: isMerge && cEx.MyFeatureTime != nil && *cEx.MyFeatureTime < timestamp, + } +} + +func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { + return pseudo.NewValueUnsafe[*ChainConfigExtra](c.ExtraPayload()).Get() +} + +func FromRules(r *params.Rules) *RulesExtra { + return pseudo.NewValueUnsafe[*RulesExtra](r.ExtraPayload()).Get() +} diff --git a/libevm/examples/go.mod b/libevm/examples/go.mod new file mode 100644 index 00000000000..22735c18a92 --- /dev/null +++ b/libevm/examples/go.mod @@ -0,0 +1,13 @@ +module libevm/examples + +go 1.22.4 + +replace github.com/ethereum/go-ethereum => ../../ + +require github.com/ethereum/go-ethereum v0.0.0-00010101000000-000000000000 + +require ( + github.com/holiman/uint256 v1.3.1 // indirect + golang.org/x/crypto v0.22.0 // indirect + golang.org/x/sys v0.20.0 // indirect +) diff --git a/libevm/examples/go.sum b/libevm/examples/go.sum new file mode 100644 index 00000000000..29f549c59fb --- /dev/null +++ b/libevm/examples/go.sum @@ -0,0 +1,14 @@ +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/holiman/uint256 v1.3.1 h1:JfTzmih28bittyHM8z360dCjIA9dbPIBlcTI6lmctQs= +github.com/holiman/uint256 v1.3.1/go.mod h1:EOMSn4q6Nyt9P6efbI3bueV4e1b3dGlUCXeiRV4ng7E= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= +github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= +golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= +golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= +golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/libevm/pseudo/type.go b/libevm/pseudo/type.go new file mode 100644 index 00000000000..4cd1beec7a9 --- /dev/null +++ b/libevm/pseudo/type.go @@ -0,0 +1,121 @@ +// Package pseudo ... +package pseudo + +import ( + "encoding/json" + "fmt" +) + +// Zero ... +func Zero[T any]() (*Type, *Value[T]) { + var x T + return From[T](x) +} + +// From ... +func From[T any](x T) (*Type, *Value[T]) { + t := &Type{ + val: &concrete[T]{ + val: x, + }, + } + return t, NewValueUnsafe[T](t) +} + +// OnlyType ... +func OnlyType[T any](t *Type, _ *Value[T]) *Type { + return t +} + +// A Type ... +type Type struct { + val value +} + +func (t *Type) Interface() any { return t.val.get() } + +// func (t *Type) Set(v any) error { return t.val.Set(v) } +// func (t *Type) MustSet(v any) { t.val.MustSet(v) } +func (t *Type) MarshalJSON() ([]byte, error) { return t.val.MarshalJSON() } +func (t *Type) UnmarshalJSON(b []byte) error { return t.val.UnmarshalJSON(b) } + +var ( + _ json.Marshaler = (*Type)(nil) + _ json.Unmarshaler = (*Type)(nil) +) + +func NewValueUnsafe[T any](t *Type) *Value[T] { + return &Value[T]{t: t} +} + +func NewValue[T any](t *Type) (*Value[T], error) { + var x T + if !t.val.canSetTo(x) { + return nil, fmt.Errorf("cannot create *Accessor[%T] with *Type carrying %T", x, t.val.get()) + } + return NewValueUnsafe[T](t), nil +} + +type Value[T any] struct { + t *Type +} + +func (a *Value[T]) Get() T { return a.t.val.get().(T) } +func (a *Value[T]) Set(v T) { a.t.val.mustSet(v) } + +type value interface { + get() any + canSetTo(any) bool + set(any) error + mustSet(any) + + json.Marshaler + json.Unmarshaler +} + +type concrete[T any] struct { + val T +} + +func (c *concrete[T]) get() any { return c.val } + +func (c *concrete[T]) canSetTo(v any) bool { + _, ok := v.(T) + return ok +} + +type InvalidTypeError[T any] struct { + SetTo any +} + +func (e *InvalidTypeError[T]) Error() string { + var t T + return fmt.Sprintf("cannot set %T to %T", t, e.SetTo) +} + +func (c *concrete[T]) set(v any) error { + vv, ok := v.(T) + if !ok { + return &InvalidTypeError[T]{SetTo: v} + } + c.val = vv + return nil +} + +func (c *concrete[T]) mustSet(v any) { + if err := c.set(v); err != nil { + panic(err) + } + _ = 0 // for happy-path coverage inspection +} + +func (c *concrete[T]) MarshalJSON() ([]byte, error) { return json.Marshal(c.val) } + +func (c *concrete[T]) UnmarshalJSON(b []byte) error { + var v T + if err := json.Unmarshal(b, &v); err != nil { + return err + } + c.val = v + return nil +} diff --git a/libevm/pseudo/type_test.go b/libevm/pseudo/type_test.go new file mode 100644 index 00000000000..964969500fd --- /dev/null +++ b/libevm/pseudo/type_test.go @@ -0,0 +1,68 @@ +package pseudo + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestType(t *testing.T) { + testType(t, "Zero[int]", Zero[int], 0, 42, "I'm not an int") + testType(t, "Zero[string]", Zero[string], "", "hello, world", 99) + + testType( + t, "From[uint](314159)", + func() (*Type, *Value[uint]) { + return From[uint](314159) + }, + 314159, 0, struct{}{}, + ) + + testType(t, "nil pointer", Zero[*float64], (*float64)(nil), new(float64), 0) +} + +func testType[T any](t *testing.T, name string, ctor func() (*Type, *Value[T]), init T, setTo T, invalid any) { + t.Run(name, func(t *testing.T) { + typ, val := ctor() + assert.Equal(t, init, val.Get()) + val.Set(setTo) + assert.Equal(t, setTo, val.Get()) + + t.Run("set to invalid type", func(t *testing.T) { + wantErr := &InvalidTypeError[T]{SetTo: invalid} + + assertError := func(t *testing.T, err any) { + t.Helper() + switch err := err.(type) { + case *InvalidTypeError[T]: + assert.Equal(t, wantErr, err) + default: + t.Errorf("got error %v; want %v", err, wantErr) + } + } + + t.Run(fmt.Sprintf("Set(%T{%v})", invalid, invalid), func(t *testing.T) { + assertError(t, typ.val.set(invalid)) + }) + + t.Run(fmt.Sprintf("MustSet(%T{%v})", invalid, invalid), func(t *testing.T) { + defer func() { + assertError(t, recover()) + }() + typ.val.mustSet(invalid) + }) + }) + + t.Run("JSON round trip", func(t *testing.T) { + buf, err := json.Marshal(typ) + require.NoError(t, err) + + got, gotVal := Zero[T]() + require.NoError(t, json.Unmarshal(buf, &got)) + assert.Equal(t, val.Get(), gotVal.Get()) + }) + }) +} diff --git a/params/config.go b/params/config.go index 21ede457fd6..2e5850c440d 100644 --- a/params/config.go +++ b/params/config.go @@ -21,6 +21,7 @@ import ( "math/big" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/libevm/pseudo" "github.com/ethereum/go-ethereum/params/forks" ) @@ -365,6 +366,8 @@ type ChainConfig struct { // Various consensus engines Ethash *EthashConfig `json:"ethash,omitempty"` Clique *CliqueConfig `json:"clique,omitempty"` + + extra *pseudo.Type // See RegisterExtras() } // EthashConfig is the consensus engine configs for proof-of-work based sealing. @@ -902,6 +905,8 @@ type Rules struct { IsBerlin, IsLondon bool IsMerge, IsShanghai, IsCancun, IsPrague bool IsVerkle bool + + extra *pseudo.Type // See RegisterExtras() } // Rules ensures c's ChainID is not nil. @@ -912,7 +917,7 @@ func (c *ChainConfig) Rules(num *big.Int, isMerge bool, timestamp uint64) Rules } // disallow setting Merge out of order isMerge = isMerge && c.IsLondon(num) - return Rules{ + r := Rules{ ChainID: new(big.Int).Set(chainID), IsHomestead: c.IsHomestead(num), IsEIP150: c.IsEIP150(num), @@ -930,4 +935,6 @@ func (c *ChainConfig) Rules(num *big.Int, isMerge bool, timestamp uint64) Rules IsPrague: isMerge && c.IsPrague(num, timestamp), IsVerkle: isMerge && c.IsVerkle(num, timestamp), } + c.addRulesExtra(&r, num, isMerge, timestamp) + return r } diff --git a/params/config.libevm.go b/params/config.libevm.go new file mode 100644 index 00000000000..884a5f2574b --- /dev/null +++ b/params/config.libevm.go @@ -0,0 +1,164 @@ +package params + +import ( + "encoding/json" + "fmt" + "math/big" + "reflect" + + "github.com/ethereum/go-ethereum/libevm/pseudo" +) + +// Extras are arbitrary payloads to be added as extra fields in [ChainConfig] +// and [Rules] structs. See [RegisterExtras]. +type Extras[C any, R any] struct { + // NewForRules, if non-nil is called at the end of [ChainConfig.Rules] with + // the newly created [Rules] and the [ChainConfig] extra payload. Its + // returned value will be the extra payload of the [Rules]. If NewForRules + // is nil then so too will the [Rules] extra payload be a nil `*R`. + // + // NewForRules MAY modify the [Rules] but MUST NOT modify the [ChainConfig]. + NewForRules func(_ *ChainConfig, _ *Rules, _ *C, blockNum *big.Int, isMerge bool, timestamp uint64) *R +} + +// RegisterExtras registers the types `C` and `R` such that they are carried as +// extra payloads in [ChainConfig] and [Rules] structs, respectively. It is +// expected to be called in an `init()` function and MUST NOT be called more +// than once. Both `C` and `R` MUST be structs. +// +// After registration, JSON unmarshalling of a [ChainConfig] will create a new +// `*C` and unmarshal the JSON key "extra" into it. Conversely, JSON marshalling +// will populate the "extra" key with the contents of the `*C`. Calls to +// [ChainConfig.Rules] will call the `NewForRules` function of the registered +// [Extras] to create a new `*R`. +// +// The payloads can be accessed via the [ChainConfig.ExtraPayload] and +// [Rules.ExtraPayload] methods, which will always return a `*C` or `*R` +// respectively however these pointers may themselves be nil. +// +// As the `ExtraPayload()` methods are not generic and return `any`, their +// values MUST be type-asserted to the returned type; failure to do so may +// result in a typed-nil bug. This pattern most-closely resembles a fully +// generic implementation and users SHOULD wrap the type assertions in a shared +// package. +func RegisterExtras[C any, R any](e Extras[C, R]) { + if registeredExtras != nil { + panic("re-registration of Extras") + } + mustBeStruct[C]() + mustBeStruct[R]() + registeredExtras = &e +} + +func mustBeStruct[T any]() { + var x T + if k := reflect.TypeOf(x).Kind(); k != reflect.Struct { + panic(notStructMessage[T]()) + } +} + +func notStructMessage[T any]() string { + var x T + return fmt.Sprintf("%T is not a struct", x) +} + +var registeredExtras interface { + nilForChainConfig() *pseudo.Type + nilForRules() *pseudo.Type + newForChainConfig() *pseudo.Type + newForRules(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type +} + +var ( + _ json.Unmarshaler = (*ChainConfig)(nil) + _ json.Marshaler = (*ChainConfig)(nil) +) + +// UnmarshalJSON ... TODO +func (c *ChainConfig) UnmarshalJSON(data []byte) error { + // We need to bypass this UnmarshalJSON() method when we again call + // json.Unmarshal(). The `raw` type won't inherit the method. + type raw ChainConfig + cc := &struct { + *raw + Extra json.RawMessage `json:"extra"` + }{raw: (*raw)(c)} + + if err := json.Unmarshal(data, cc); err != nil { + return err + } + if registeredExtras == nil || len(cc.Extra) == 0 { + return nil + } + + extra := registeredExtras.newForChainConfig() + if err := json.Unmarshal(cc.Extra, extra); err != nil { + return err + } + c.extra = extra + return nil +} + +// MarshalJSON ... TODO +func (c *ChainConfig) MarshalJSON() ([]byte, error) { + type raw ChainConfig + cc := &struct { + *raw + Extra any `json:"extra"` + }{raw: (*raw)(c), Extra: c.extra} + return json.Marshal(cc) +} + +func (c *ChainConfig) addRulesExtra(r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) { + r.extra = nil + if registeredExtras != nil { + r.extra = registeredExtras.newForRules(c, r, blockNum, isMerge, timestamp) + } +} + +// ExtraPayload returns the extra payload carried by the ChainConfig and can +// only be called if [RegisterExtras] was called. The returned value is always +// of type `*C` as registered, but may be nil. Callers MUST immediately +// type-assert the returned value to `*C` to avoid typed-nil bugs. See the +// example for the intended usage pattern. +func (c *ChainConfig) ExtraPayload() *pseudo.Type { + if registeredExtras == nil { + panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", c)) + } + if c.extra == nil { + c.extra = registeredExtras.nilForChainConfig() + } + return c.extra +} + +// ExtraPayload returns the extra payload carried by the Rules and can only be +// called if [RegisterExtras] was called. The returned value is always of type +// `*R` as registered, but may be nil. Callers MUST immediately type-assert the +// returned value to `*R` to avoid typed-nil bugs. See the example on +// [ChainConfig.ExtraPayload] for the intended usage pattern. +func (r *Rules) ExtraPayload() *pseudo.Type { + if registeredExtras == nil { + panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", r)) + } + if r.extra == nil { + r.extra = registeredExtras.nilForRules() + } + return r.extra +} + +func (Extras[C, R]) nilForChainConfig() *pseudo.Type { return pseudo.OnlyType(pseudo.Zero[*C]()) } +func (Extras[C, R]) nilForRules() *pseudo.Type { return pseudo.OnlyType(pseudo.Zero[*R]()) } + +func (*Extras[C, R]) newForChainConfig() *pseudo.Type { + var x C + return pseudo.OnlyType(pseudo.From(&x)) +} + +func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type { + if e.NewForRules == nil { + return e.nilForRules() + } + return pseudo.OnlyType( + pseudo.From(e.NewForRules(c, r, c.extra.Interface().(*C), blockNum, isMerge, timestamp)), + ) +} diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go new file mode 100644 index 00000000000..18824f1457a --- /dev/null +++ b/params/config.libevm_test.go @@ -0,0 +1,250 @@ +package params + +import ( + "encoding/json" + "fmt" + "log" + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/libevm/pseudo" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func testOnlyClearRegisteredExtras() { + registeredExtras = nil +} + +func ExampleRegisterExtras() { + type ( + chainConfigExtra struct { + Foo string `json:"foo"` + } + rulesExtra struct { + FooCopy string + } + ) + + // In practice, this would be called inside an init() func. + RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ + NewForRules: func(cc *ChainConfig, r *Rules, cEx *chainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *rulesExtra { + // This function is called at the end of ChainConfig.Rules(), + // receiving a pointer to the Rules that will be returned. It MAY + // modify the Rules but MUST NOT modify the ChainConfig. The value + // that it returns will be available via Rules.ExtraPayload(). + return &rulesExtra{ + FooCopy: fmt.Sprintf("copy of: %q", cEx.Foo), + } + }, + }) + defer testOnlyClearRegisteredExtras() + + // ChainConfig now unmarshals any JSON field named "extra" into a pointer to + // the registered type, which is available via the ExtraPayload() method. + buf := []byte(`{ + "chainId": 1234, + "extra": { + "foo": "hello, world" + } + }`) + + var config ChainConfig + if err := json.Unmarshal(buf, &config); err != nil { + log.Fatal(err) + } + + fmt.Println(config.ChainID) + // The values returned by ExtraPayload() are guaranteed to be pointers to + // the registered types. They MAY, however, be nil pointers. In practice, + // callers SHOULD abstract the type assertion in a reusable function to + // provide a seamless devex. + ccExtra := config.ExtraPayload().Interface().(*chainConfigExtra) + rules := config.Rules(nil, false, 0) + rExtra := rules.ExtraPayload().Interface().(*rulesExtra) + + if ccExtra != nil { + fmt.Println(ccExtra.Foo) + } + if rExtra != nil { + fmt.Println(rExtra.FooCopy) + } + + // Output: + // 1234 + // hello, world + // copy of: "hello, world" +} + +func ExampleChainConfig_ExtraPayload() { + type ( + chainConfigExtra struct{} + rulesExtra struct{} + ) + // Typically called in an `init()` function. + RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ /*...*/ }) + defer testOnlyClearRegisteredExtras() + + var c ChainConfig // Sourced from elsewhere, typically unmarshalled from JSON. + + // Both ChainConfig.ExtraPayload() and Rules.ExtraPayload() return `any` + // that are guaranteed to be pointers to the registered types. + extra := c.ExtraPayload().Interface().(*chainConfigExtra) + + // Act on the extra payload... + if extra != nil { + // ... + } +} + +type rawJSON struct { + json.RawMessage +} + +var ( + _ json.Unmarshaler = (*rawJSON)(nil) + _ json.Marshaler = (*rawJSON)(nil) +) + +func TestRegisterExtras(t *testing.T) { + type ( + ccExtraA struct { + A string `json:"a"` + } + rulesExtraA struct { + A string + } + ccExtraB struct { + B string `json:"b"` + } + rulesExtraB struct { + B string + } + ) + + tests := []struct { + name string + register func() + ccExtra *pseudo.Type + wantRulesExtra any + }{ + { + name: "Rules payload copied from ChainConfig payload", + register: func() { + RegisterExtras(Extras[ccExtraA, rulesExtraA]{ + NewForRules: func(cc *ChainConfig, r *Rules, ex *ccExtraA, _ *big.Int, _ bool, _ uint64) *rulesExtraA { + return &rulesExtraA{ + A: ex.A, + } + }, + }) + }, + ccExtra: pseudo.OnlyType(pseudo.From(&ccExtraA{ + A: "hello", + })), + wantRulesExtra: &rulesExtraA{ + A: "hello", + }, + }, + { + name: "no NewForRules() function results in typed but nil pointer", + register: func() { + RegisterExtras(Extras[ccExtraB, rulesExtraB]{}) + }, + ccExtra: pseudo.OnlyType(pseudo.From(&ccExtraB{ + B: "world", + })), + wantRulesExtra: (*rulesExtraB)(nil), + }, + { + name: "custom JSON handling honoured", + register: func() { + RegisterExtras(Extras[rawJSON, struct{}]{}) + }, + ccExtra: pseudo.OnlyType(pseudo.From(&rawJSON{ + RawMessage: []byte(`"hello, world"`), + })), + wantRulesExtra: (*struct{})(nil), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.register() + defer testOnlyClearRegisteredExtras() + + in := &ChainConfig{ + ChainID: big.NewInt(142857), + extra: tt.ccExtra, + } + + buf, err := json.Marshal(in) + require.NoError(t, err) + + got := new(ChainConfig) + require.NoError(t, json.Unmarshal(buf, got)) + assert.Equal(t, tt.ccExtra.Interface(), got.ExtraPayload().Interface()) + assert.Equal(t, in, got) + // TODO: do we need an explicit test of the JSON output, or is a + // Marshal-Unmarshal round trip sufficient? + + gotRules := got.Rules(nil, false, 0) + assert.Equal(t, tt.wantRulesExtra, gotRules.ExtraPayload().Interface()) + }) + } +} + +func TestExtrasPanic(t *testing.T) { + assertPanics( + t, func() { + RegisterExtras(Extras[int, struct{}]{}) + }, + notStructMessage[int](), + ) + + assertPanics( + t, func() { + RegisterExtras(Extras[struct{}, bool]{}) + }, + notStructMessage[bool](), + ) + + assertPanics( + t, func() { + new(ChainConfig).ExtraPayload() + }, + "before RegisterExtras", + ) + + assertPanics( + t, func() { + new(Rules).ExtraPayload() + }, + "before RegisterExtras", + ) + + RegisterExtras(Extras[struct{}, struct{}]{}) + defer testOnlyClearRegisteredExtras() + + assertPanics( + t, func() { + RegisterExtras(Extras[struct{}, struct{}]{}) + }, + "re-registration", + ) +} + +func assertPanics(t *testing.T, fn func(), wantContains string) { + t.Helper() + defer func() { + switch r := recover().(type) { + case nil: + t.Error("function did not panic as expected") + case string: + assert.Contains(t, r, wantContains) + default: + t.Fatalf("BAD TEST SETUP: recover() got unsupported type %T", r) + } + }() + fn() +} From 5cbdefb20ae6db2cca451decd8d98ae107dac30c Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 22 Aug 2024 17:40:15 +0100 Subject: [PATCH 02/24] feat: `params.ExtraPayloadGetter` for end-user type safety --- libevm/examples/extraparams/extraparams.go | 9 +++++---- libevm/examples/go.mod | 2 +- libevm/examples/go.sum | 4 ++-- params/config.libevm.go | 16 +++++++++++++++- params/config.libevm_test.go | 17 +++++++++-------- 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/libevm/examples/extraparams/extraparams.go b/libevm/examples/extraparams/extraparams.go index fd240a0d409..0ca03cd3b69 100644 --- a/libevm/examples/extraparams/extraparams.go +++ b/libevm/examples/extraparams/extraparams.go @@ -3,12 +3,13 @@ package extraparams import ( "math/big" - "github.com/ethereum/go-ethereum/libevm/pseudo" "github.com/ethereum/go-ethereum/params" ) +var getter params.ExtraPayloadGetter[ChainConfigExtra, RulesExtra] + func init() { - params.RegisterExtras(params.Extras[ChainConfigExtra, RulesExtra]{ + getter = params.RegisterExtras(params.Extras[ChainConfigExtra, RulesExtra]{ NewForRules: constructRulesExtra, }) } @@ -28,9 +29,9 @@ func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx *ChainConfi } func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { - return pseudo.NewValueUnsafe[*ChainConfigExtra](c.ExtraPayload()).Get() + return getter.FromChainConfig(c) } func FromRules(r *params.Rules) *RulesExtra { - return pseudo.NewValueUnsafe[*RulesExtra](r.ExtraPayload()).Get() + return getter.FromRules(r) } diff --git a/libevm/examples/go.mod b/libevm/examples/go.mod index 22735c18a92..84e13d9b63f 100644 --- a/libevm/examples/go.mod +++ b/libevm/examples/go.mod @@ -9,5 +9,5 @@ require github.com/ethereum/go-ethereum v0.0.0-00010101000000-000000000000 require ( github.com/holiman/uint256 v1.3.1 // indirect golang.org/x/crypto v0.22.0 // indirect - golang.org/x/sys v0.20.0 // indirect + golang.org/x/sys v0.22.0 // indirect ) diff --git a/libevm/examples/go.sum b/libevm/examples/go.sum index 29f549c59fb..145acea3bb9 100644 --- a/libevm/examples/go.sum +++ b/libevm/examples/go.sum @@ -8,7 +8,7 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= -golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= -golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI= +golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/params/config.libevm.go b/params/config.libevm.go index 884a5f2574b..8fe40e709d5 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -41,13 +41,27 @@ type Extras[C any, R any] struct { // result in a typed-nil bug. This pattern most-closely resembles a fully // generic implementation and users SHOULD wrap the type assertions in a shared // package. -func RegisterExtras[C any, R any](e Extras[C, R]) { +func RegisterExtras[C any, R any](e Extras[C, R]) ExtraPayloadGetter[C, R] { if registeredExtras != nil { panic("re-registration of Extras") } mustBeStruct[C]() mustBeStruct[R]() registeredExtras = &e + return ExtraPayloadGetter[C, R]{} +} + +// An ExtraPayloadGettter ... +type ExtraPayloadGetter[C any, R any] struct{} + +// FromChainConfig ... +func (ExtraPayloadGetter[C, R]) FromChainConfig(c *ChainConfig) *C { + return pseudo.NewValueUnsafe[*C](c.ExtraPayload()).Get() +} + +// FromRules ... +func (ExtraPayloadGetter[C, R]) FromRules(r *Rules) *R { + return pseudo.NewValueUnsafe[*R](r.ExtraPayload()).Get() } func mustBeStruct[T any]() { diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 18824f1457a..26f14d2cef2 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -26,8 +26,9 @@ func ExampleRegisterExtras() { } ) - // In practice, this would be called inside an init() func. - RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ + // In practice, this would be called inside an init() func and the `getter` + // used to access the ExtraPayload() values in a type-safe way. + getter := RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ NewForRules: func(cc *ChainConfig, r *Rules, cEx *chainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *rulesExtra { // This function is called at the end of ChainConfig.Rules(), // receiving a pointer to the Rules that will be returned. It MAY @@ -49,8 +50,8 @@ func ExampleRegisterExtras() { } }`) - var config ChainConfig - if err := json.Unmarshal(buf, &config); err != nil { + config := new(ChainConfig) + if err := json.Unmarshal(buf, config); err != nil { log.Fatal(err) } @@ -59,9 +60,9 @@ func ExampleRegisterExtras() { // the registered types. They MAY, however, be nil pointers. In practice, // callers SHOULD abstract the type assertion in a reusable function to // provide a seamless devex. - ccExtra := config.ExtraPayload().Interface().(*chainConfigExtra) + ccExtra := getter.FromChainConfig(config) rules := config.Rules(nil, false, 0) - rExtra := rules.ExtraPayload().Interface().(*rulesExtra) + rExtra := getter.FromRules(&rules) if ccExtra != nil { fmt.Println(ccExtra.Foo) @@ -82,14 +83,14 @@ func ExampleChainConfig_ExtraPayload() { rulesExtra struct{} ) // Typically called in an `init()` function. - RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ /*...*/ }) + getter := RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ /*...*/ }) defer testOnlyClearRegisteredExtras() var c ChainConfig // Sourced from elsewhere, typically unmarshalled from JSON. // Both ChainConfig.ExtraPayload() and Rules.ExtraPayload() return `any` // that are guaranteed to be pointers to the registered types. - extra := c.ExtraPayload().Interface().(*chainConfigExtra) + extra := getter.FromChainConfig(&c) // Act on the extra payload... if extra != nil { From 4548d7333e3ca25d4eb1fb24f0c7a66e21f70d8e Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 22 Aug 2024 19:46:28 +0100 Subject: [PATCH 03/24] refactor: payloads only available through `params.ExtraPayloadGetter` --- libevm/examples/go.mod | 13 ------------- libevm/examples/go.sum | 14 -------------- params/config.libevm.go | 18 +++++++++--------- params/config.libevm_test.go | 8 ++++---- .../example.libevm_test.go | 0 5 files changed, 13 insertions(+), 40 deletions(-) delete mode 100644 libevm/examples/go.mod delete mode 100644 libevm/examples/go.sum rename libevm/examples/extraparams/extraparams.go => params/example.libevm_test.go (100%) diff --git a/libevm/examples/go.mod b/libevm/examples/go.mod deleted file mode 100644 index 84e13d9b63f..00000000000 --- a/libevm/examples/go.mod +++ /dev/null @@ -1,13 +0,0 @@ -module libevm/examples - -go 1.22.4 - -replace github.com/ethereum/go-ethereum => ../../ - -require github.com/ethereum/go-ethereum v0.0.0-00010101000000-000000000000 - -require ( - github.com/holiman/uint256 v1.3.1 // indirect - golang.org/x/crypto v0.22.0 // indirect - golang.org/x/sys v0.22.0 // indirect -) diff --git a/libevm/examples/go.sum b/libevm/examples/go.sum deleted file mode 100644 index 145acea3bb9..00000000000 --- a/libevm/examples/go.sum +++ /dev/null @@ -1,14 +0,0 @@ -github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= -github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/holiman/uint256 v1.3.1 h1:JfTzmih28bittyHM8z360dCjIA9dbPIBlcTI6lmctQs= -github.com/holiman/uint256 v1.3.1/go.mod h1:EOMSn4q6Nyt9P6efbI3bueV4e1b3dGlUCXeiRV4ng7E= -github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= -github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= -golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= -golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI= -golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= -gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/params/config.libevm.go b/params/config.libevm.go index 8fe40e709d5..23384b6de95 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -32,8 +32,8 @@ type Extras[C any, R any] struct { // [ChainConfig.Rules] will call the `NewForRules` function of the registered // [Extras] to create a new `*R`. // -// The payloads can be accessed via the [ChainConfig.ExtraPayload] and -// [Rules.ExtraPayload] methods, which will always return a `*C` or `*R` +// The payloads can be accessed via the [ChainConfig.extraPayload] and +// [Rules.extraPayload] methods, which will always return a `*C` or `*R` // respectively however these pointers may themselves be nil. // // As the `ExtraPayload()` methods are not generic and return `any`, their @@ -56,12 +56,12 @@ type ExtraPayloadGetter[C any, R any] struct{} // FromChainConfig ... func (ExtraPayloadGetter[C, R]) FromChainConfig(c *ChainConfig) *C { - return pseudo.NewValueUnsafe[*C](c.ExtraPayload()).Get() + return pseudo.NewValueUnsafe[*C](c.extraPayload()).Get() } // FromRules ... func (ExtraPayloadGetter[C, R]) FromRules(r *Rules) *R { - return pseudo.NewValueUnsafe[*R](r.ExtraPayload()).Get() + return pseudo.NewValueUnsafe[*R](r.extraPayload()).Get() } func mustBeStruct[T any]() { @@ -130,12 +130,12 @@ func (c *ChainConfig) addRulesExtra(r *Rules, blockNum *big.Int, isMerge bool, t } } -// ExtraPayload returns the extra payload carried by the ChainConfig and can +// extraPayload returns the extra payload carried by the ChainConfig and can // only be called if [RegisterExtras] was called. The returned value is always // of type `*C` as registered, but may be nil. Callers MUST immediately // type-assert the returned value to `*C` to avoid typed-nil bugs. See the // example for the intended usage pattern. -func (c *ChainConfig) ExtraPayload() *pseudo.Type { +func (c *ChainConfig) extraPayload() *pseudo.Type { if registeredExtras == nil { panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", c)) } @@ -145,12 +145,12 @@ func (c *ChainConfig) ExtraPayload() *pseudo.Type { return c.extra } -// ExtraPayload returns the extra payload carried by the Rules and can only be +// extraPayload returns the extra payload carried by the Rules and can only be // called if [RegisterExtras] was called. The returned value is always of type // `*R` as registered, but may be nil. Callers MUST immediately type-assert the // returned value to `*R` to avoid typed-nil bugs. See the example on -// [ChainConfig.ExtraPayload] for the intended usage pattern. -func (r *Rules) ExtraPayload() *pseudo.Type { +// [ChainConfig.extraPayload] for the intended usage pattern. +func (r *Rules) extraPayload() *pseudo.Type { if registeredExtras == nil { panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", r)) } diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 26f14d2cef2..4f78888db3b 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -184,13 +184,13 @@ func TestRegisterExtras(t *testing.T) { got := new(ChainConfig) require.NoError(t, json.Unmarshal(buf, got)) - assert.Equal(t, tt.ccExtra.Interface(), got.ExtraPayload().Interface()) + assert.Equal(t, tt.ccExtra.Interface(), got.extraPayload().Interface()) assert.Equal(t, in, got) // TODO: do we need an explicit test of the JSON output, or is a // Marshal-Unmarshal round trip sufficient? gotRules := got.Rules(nil, false, 0) - assert.Equal(t, tt.wantRulesExtra, gotRules.ExtraPayload().Interface()) + assert.Equal(t, tt.wantRulesExtra, gotRules.extraPayload().Interface()) }) } } @@ -212,14 +212,14 @@ func TestExtrasPanic(t *testing.T) { assertPanics( t, func() { - new(ChainConfig).ExtraPayload() + new(ChainConfig).extraPayload() }, "before RegisterExtras", ) assertPanics( t, func() { - new(Rules).ExtraPayload() + new(Rules).extraPayload() }, "before RegisterExtras", ) diff --git a/libevm/examples/extraparams/extraparams.go b/params/example.libevm_test.go similarity index 100% rename from libevm/examples/extraparams/extraparams.go rename to params/example.libevm_test.go From d209bb6bc468898d5fc757c83a02079e27654dfa Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 22 Aug 2024 19:47:41 +0100 Subject: [PATCH 04/24] chore: make `libevm/examples/extraparams` a `params` testable example --- params/config.libevm_test.go | 89 ++--------------------------------- params/example.libevm_test.go | 69 ++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 97 deletions(-) diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 4f78888db3b..cdbdf3cce6e 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -2,8 +2,6 @@ package params import ( "encoding/json" - "fmt" - "log" "math/big" "testing" @@ -16,88 +14,6 @@ func testOnlyClearRegisteredExtras() { registeredExtras = nil } -func ExampleRegisterExtras() { - type ( - chainConfigExtra struct { - Foo string `json:"foo"` - } - rulesExtra struct { - FooCopy string - } - ) - - // In practice, this would be called inside an init() func and the `getter` - // used to access the ExtraPayload() values in a type-safe way. - getter := RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ - NewForRules: func(cc *ChainConfig, r *Rules, cEx *chainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *rulesExtra { - // This function is called at the end of ChainConfig.Rules(), - // receiving a pointer to the Rules that will be returned. It MAY - // modify the Rules but MUST NOT modify the ChainConfig. The value - // that it returns will be available via Rules.ExtraPayload(). - return &rulesExtra{ - FooCopy: fmt.Sprintf("copy of: %q", cEx.Foo), - } - }, - }) - defer testOnlyClearRegisteredExtras() - - // ChainConfig now unmarshals any JSON field named "extra" into a pointer to - // the registered type, which is available via the ExtraPayload() method. - buf := []byte(`{ - "chainId": 1234, - "extra": { - "foo": "hello, world" - } - }`) - - config := new(ChainConfig) - if err := json.Unmarshal(buf, config); err != nil { - log.Fatal(err) - } - - fmt.Println(config.ChainID) - // The values returned by ExtraPayload() are guaranteed to be pointers to - // the registered types. They MAY, however, be nil pointers. In practice, - // callers SHOULD abstract the type assertion in a reusable function to - // provide a seamless devex. - ccExtra := getter.FromChainConfig(config) - rules := config.Rules(nil, false, 0) - rExtra := getter.FromRules(&rules) - - if ccExtra != nil { - fmt.Println(ccExtra.Foo) - } - if rExtra != nil { - fmt.Println(rExtra.FooCopy) - } - - // Output: - // 1234 - // hello, world - // copy of: "hello, world" -} - -func ExampleChainConfig_ExtraPayload() { - type ( - chainConfigExtra struct{} - rulesExtra struct{} - ) - // Typically called in an `init()` function. - getter := RegisterExtras(Extras[chainConfigExtra, rulesExtra]{ /*...*/ }) - defer testOnlyClearRegisteredExtras() - - var c ChainConfig // Sourced from elsewhere, typically unmarshalled from JSON. - - // Both ChainConfig.ExtraPayload() and Rules.ExtraPayload() return `any` - // that are guaranteed to be pointers to the registered types. - extra := getter.FromChainConfig(&c) - - // Act on the extra payload... - if extra != nil { - // ... - } -} - type rawJSON struct { json.RawMessage } @@ -171,6 +87,7 @@ func TestRegisterExtras(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + testOnlyClearRegisteredExtras() tt.register() defer testOnlyClearRegisteredExtras() @@ -196,6 +113,9 @@ func TestRegisterExtras(t *testing.T) { } func TestExtrasPanic(t *testing.T) { + testOnlyClearRegisteredExtras() + defer testOnlyClearRegisteredExtras() + assertPanics( t, func() { RegisterExtras(Extras[int, struct{}]{}) @@ -225,7 +145,6 @@ func TestExtrasPanic(t *testing.T) { ) RegisterExtras(Extras[struct{}, struct{}]{}) - defer testOnlyClearRegisteredExtras() assertPanics( t, func() { diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index 0ca03cd3b69..16a88b37084 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -1,37 +1,82 @@ -package extraparams +package params_test import ( + "encoding/json" + "fmt" + "log" "math/big" "github.com/ethereum/go-ethereum/params" ) -var getter params.ExtraPayloadGetter[ChainConfigExtra, RulesExtra] - -func init() { +// TODO: explain why this isn't in an init() +func initFn() { getter = params.RegisterExtras(params.Extras[ChainConfigExtra, RulesExtra]{ NewForRules: constructRulesExtra, }) } +var getter params.ExtraPayloadGetter[ChainConfigExtra, RulesExtra] + +func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { + return getter.FromChainConfig(c) +} + +func FromRules(r *params.Rules) *RulesExtra { + return getter.FromRules(r) +} + type ChainConfigExtra struct { - MyFeatureTime *uint64 + MyForkTime *uint64 `json:"myForkTime"` } type RulesExtra struct { - IsMyFeature bool + IsMyFork bool } func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx *ChainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *RulesExtra { return &RulesExtra{ - IsMyFeature: isMerge && cEx.MyFeatureTime != nil && *cEx.MyFeatureTime < timestamp, + IsMyFork: cEx.MyForkTime != nil && *cEx.MyForkTime <= timestamp, } } -func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { - return getter.FromChainConfig(c) -} +func ExampleRegisterExtras() { + initFn() // TODO: explain -func FromRules(r *params.Rules) *RulesExtra { - return getter.FromRules(r) + const forkTime = 530003640 + jsonData := fmt.Sprintf(`{ + "chainId": 1234, + "extra": { + "myForkTime": %d + } + }`, forkTime) + + // ChainConfig now unmarshals any JSON field named "extra" into a pointer to + // the registered type, which is available via the ExtraPayload() method. + config := new(params.ChainConfig) + if err := json.Unmarshal([]byte(jsonData), config); err != nil { + log.Fatal(err) + } + + fmt.Println(config.ChainID) // original geth fields work as expected + + ccExtra := FromChainConfig(config) + if ccExtra != nil && ccExtra.MyForkTime != nil { + fmt.Println(*ccExtra.MyForkTime) + } + + for _, time := range []uint64{forkTime - 1, forkTime, forkTime + 1} { + rules := config.Rules(nil, false, time) + rExtra := FromRules(&rules) + if rExtra != nil { + fmt.Printf("%+v\n", rExtra) + } + } + + // Output: + // 1234 + // 530003640 + // &{IsMyFork:false} + // &{IsMyFork:true} + // &{IsMyFork:true} } From 3944da579929d1ad696ca5d3f17d77544c7d2a4d Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Fri, 23 Aug 2024 14:40:43 +0100 Subject: [PATCH 05/24] doc: `libevm/pseudo` package comments and improved readability --- libevm/pseudo/type.go | 132 ++++++++++++++++++++++++----------- libevm/pseudo/type_test.go | 23 ++++-- params/config.libevm.go | 14 ++-- params/config.libevm_test.go | 12 ++-- 4 files changed, 122 insertions(+), 59 deletions(-) diff --git a/libevm/pseudo/type.go b/libevm/pseudo/type.go index 4cd1beec7a9..8c453f4cb0e 100644 --- a/libevm/pseudo/type.go +++ b/libevm/pseudo/type.go @@ -1,4 +1,15 @@ -// Package pseudo ... +// Package pseudo provides a bridge between generic and non-generic code via +// pseudo-types and pseudo-values. With careful usage, there is minimal +// reduction in type safety. +// +// Adding generic type parameters to anything (e.g. struct, function, etc) +// "pollutes" all code that uses the generic type. Refactoring all uses isn't +// always feasible, and a [Type] acts as an intermediate fix. Although their +// constructors are generic, they are not, and they are instead coupled with a +// generic [Value] that SHOULD be used for access. +// +// Packages typically SHOULD NOT expose a [Type] and SHOULD instead provide +// users with a type-safe [Value]. package pseudo import ( @@ -6,63 +17,101 @@ import ( "fmt" ) -// Zero ... -func Zero[T any]() (*Type, *Value[T]) { - var x T - return From[T](x) +// A Type wraps a strongly-typed value without exposing information about its +// type. It can be used in lieu of a generic field / parameter. +type Type struct { + val value +} + +// A Value provides strongly-typed access to the payload carried by a [Type]. +type Value[T any] struct { + t *Type +} + +// A Pseudo type couples a [Type] and a [Value]. If returned by a constructor +// from this package, both wrap the same payload. +type Pseudo[T any] struct { + Type *Type + Value *Value[T] } -// From ... -func From[T any](x T) (*Type, *Value[T]) { +// TypeAndValue is a convenience function for splitting the contents of `p`, +// typically at construction. +func (p *Pseudo[T]) TypeAndValue() (*Type, *Value[T]) { + return p.Type, p.Value +} + +// From returns a Pseudo[T] constructed from `v`. +func From[T any](v T) *Pseudo[T] { t := &Type{ val: &concrete[T]{ - val: x, + val: v, }, } - return t, NewValueUnsafe[T](t) + return &Pseudo[T]{t, MustNewValue[T](t)} } -// OnlyType ... -func OnlyType[T any](t *Type, _ *Value[T]) *Type { - return t -} - -// A Type ... -type Type struct { - val value +// Zero is equivalent to [From] called with the [zero value] of type `T`. Note +// that pointers, slices, maps, etc. will therefore be nil. +// +// [zero value]: https://go.dev/tour/basics/12 +func Zero[T any]() *Pseudo[T] { + var x T + return From[T](x) } +// Interface returns the wrapped value as an `any`, equivalent to +// [reflect.Value.Interface]. Prefer [Value.Get]. func (t *Type) Interface() any { return t.val.get() } -// func (t *Type) Set(v any) error { return t.val.Set(v) } -// func (t *Type) MustSet(v any) { t.val.MustSet(v) } -func (t *Type) MarshalJSON() ([]byte, error) { return t.val.MarshalJSON() } -func (t *Type) UnmarshalJSON(b []byte) error { return t.val.UnmarshalJSON(b) } - -var ( - _ json.Marshaler = (*Type)(nil) - _ json.Unmarshaler = (*Type)(nil) -) - -func NewValueUnsafe[T any](t *Type) *Value[T] { - return &Value[T]{t: t} -} - +// NewValue constructs a [Value] from a [Type], first confirming that `t` wraps +// a payload of type `T`. func NewValue[T any](t *Type) (*Value[T], error) { var x T if !t.val.canSetTo(x) { - return nil, fmt.Errorf("cannot create *Accessor[%T] with *Type carrying %T", x, t.val.get()) + return nil, fmt.Errorf("cannot create *Value[%T] with *Type carrying %T", x, t.val.get()) } - return NewValueUnsafe[T](t), nil + return &Value[T]{t}, nil } -type Value[T any] struct { - t *Type +// MustNewValue is equivalent to [NewValue] except that it panics instead of +// returning an error. +func MustNewValue[T any](t *Type) *Value[T] { + v, err := NewValue[T](t) + if err != nil { + panic(err) + } + return v } -func (a *Value[T]) Get() T { return a.t.val.get().(T) } +// Get returns the value. +func (a *Value[T]) Get() T { return a.t.val.get().(T) } + +// Set sets the value. func (a *Value[T]) Set(v T) { a.t.val.mustSet(v) } +// MarshalJSON implements the [json.Marshaler] interface. +func (t *Type) MarshalJSON() ([]byte, error) { return t.val.MarshalJSON() } + +// UnmarshalJSON implements the [json.Unmarshaler] interface. +func (t *Type) UnmarshalJSON(b []byte) error { return t.val.UnmarshalJSON(b) } + +// MarshalJSON implements the [json.Marshaler] interface. +func (v *Value[T]) MarshalJSON() ([]byte, error) { return v.t.MarshalJSON() } + +// UnmarshalJSON implements the [json.Unmarshaler] interface. +func (v *Value[T]) UnmarshalJSON(b []byte) error { return v.t.UnmarshalJSON(b) } + +var _ = []interface { + json.Marshaler + json.Unmarshaler +}{ + (*Type)(nil), + (*Value[struct{}])(nil), + (*concrete[struct{}])(nil), +} + +// A value is a non-generic wrapper around a [concrete] struct. type value interface { get() any canSetTo(any) bool @@ -84,11 +133,14 @@ func (c *concrete[T]) canSetTo(v any) bool { return ok } -type InvalidTypeError[T any] struct { +// An invalidTypeError is returned by [conrete.set] if the value is incompatible +// with its type. This should never leave this package and exists only to +// provide precise testing of unhappy paths. +type invalidTypeError[T any] struct { SetTo any } -func (e *InvalidTypeError[T]) Error() string { +func (e *invalidTypeError[T]) Error() string { var t T return fmt.Sprintf("cannot set %T to %T", t, e.SetTo) } @@ -96,7 +148,9 @@ func (e *InvalidTypeError[T]) Error() string { func (c *concrete[T]) set(v any) error { vv, ok := v.(T) if !ok { - return &InvalidTypeError[T]{SetTo: v} + // Other invariants in this implementation (aim to) guarantee that this + // will never happen. + return &invalidTypeError[T]{SetTo: v} } c.val = vv return nil diff --git a/libevm/pseudo/type_test.go b/libevm/pseudo/type_test.go index 964969500fd..27ecf7e497e 100644 --- a/libevm/pseudo/type_test.go +++ b/libevm/pseudo/type_test.go @@ -15,7 +15,7 @@ func TestType(t *testing.T) { testType( t, "From[uint](314159)", - func() (*Type, *Value[uint]) { + func() *Pseudo[uint] { return From[uint](314159) }, 314159, 0, struct{}{}, @@ -24,20 +24,20 @@ func TestType(t *testing.T) { testType(t, "nil pointer", Zero[*float64], (*float64)(nil), new(float64), 0) } -func testType[T any](t *testing.T, name string, ctor func() (*Type, *Value[T]), init T, setTo T, invalid any) { +func testType[T any](t *testing.T, name string, ctor func() *Pseudo[T], init T, setTo T, invalid any) { t.Run(name, func(t *testing.T) { - typ, val := ctor() + typ, val := ctor().TypeAndValue() assert.Equal(t, init, val.Get()) val.Set(setTo) assert.Equal(t, setTo, val.Get()) t.Run("set to invalid type", func(t *testing.T) { - wantErr := &InvalidTypeError[T]{SetTo: invalid} + wantErr := &invalidTypeError[T]{SetTo: invalid} assertError := func(t *testing.T, err any) { t.Helper() switch err := err.(type) { - case *InvalidTypeError[T]: + case *invalidTypeError[T]: assert.Equal(t, wantErr, err) default: t.Errorf("got error %v; want %v", err, wantErr) @@ -60,9 +60,20 @@ func testType[T any](t *testing.T, name string, ctor func() (*Type, *Value[T]), buf, err := json.Marshal(typ) require.NoError(t, err) - got, gotVal := Zero[T]() + got, gotVal := Zero[T]().TypeAndValue() require.NoError(t, json.Unmarshal(buf, &got)) assert.Equal(t, val.Get(), gotVal.Get()) }) }) } + +func ExamplePseudo_TypeAndValue() { + typ, val := From("hello").TypeAndValue() + + // But, if only one is needed: + typ = From("world").Type + val = From("this isn't coupled to the Type").Value + + _ = typ + _ = val +} diff --git a/params/config.libevm.go b/params/config.libevm.go index 23384b6de95..c212995d5c5 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -56,12 +56,12 @@ type ExtraPayloadGetter[C any, R any] struct{} // FromChainConfig ... func (ExtraPayloadGetter[C, R]) FromChainConfig(c *ChainConfig) *C { - return pseudo.NewValueUnsafe[*C](c.extraPayload()).Get() + return pseudo.MustNewValue[*C](c.extraPayload()).Get() } // FromRules ... func (ExtraPayloadGetter[C, R]) FromRules(r *Rules) *R { - return pseudo.NewValueUnsafe[*R](r.extraPayload()).Get() + return pseudo.MustNewValue[*R](r.extraPayload()).Get() } func mustBeStruct[T any]() { @@ -160,19 +160,17 @@ func (r *Rules) extraPayload() *pseudo.Type { return r.extra } -func (Extras[C, R]) nilForChainConfig() *pseudo.Type { return pseudo.OnlyType(pseudo.Zero[*C]()) } -func (Extras[C, R]) nilForRules() *pseudo.Type { return pseudo.OnlyType(pseudo.Zero[*R]()) } +func (Extras[C, R]) nilForChainConfig() *pseudo.Type { return pseudo.Zero[*C]().Type } +func (Extras[C, R]) nilForRules() *pseudo.Type { return pseudo.Zero[*R]().Type } func (*Extras[C, R]) newForChainConfig() *pseudo.Type { var x C - return pseudo.OnlyType(pseudo.From(&x)) + return pseudo.From(&x).Type } func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type { if e.NewForRules == nil { return e.nilForRules() } - return pseudo.OnlyType( - pseudo.From(e.NewForRules(c, r, c.extra.Interface().(*C), blockNum, isMerge, timestamp)), - ) + return pseudo.From(e.NewForRules(c, r, c.extra.Interface().(*C), blockNum, isMerge, timestamp)).Type } diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index cdbdf3cce6e..bc940750389 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -56,9 +56,9 @@ func TestRegisterExtras(t *testing.T) { }, }) }, - ccExtra: pseudo.OnlyType(pseudo.From(&ccExtraA{ + ccExtra: pseudo.From(&ccExtraA{ A: "hello", - })), + }).Type, wantRulesExtra: &rulesExtraA{ A: "hello", }, @@ -68,9 +68,9 @@ func TestRegisterExtras(t *testing.T) { register: func() { RegisterExtras(Extras[ccExtraB, rulesExtraB]{}) }, - ccExtra: pseudo.OnlyType(pseudo.From(&ccExtraB{ + ccExtra: pseudo.From(&ccExtraB{ B: "world", - })), + }).Type, wantRulesExtra: (*rulesExtraB)(nil), }, { @@ -78,9 +78,9 @@ func TestRegisterExtras(t *testing.T) { register: func() { RegisterExtras(Extras[rawJSON, struct{}]{}) }, - ccExtra: pseudo.OnlyType(pseudo.From(&rawJSON{ + ccExtra: pseudo.From(&rawJSON{ RawMessage: []byte(`"hello, world"`), - })), + }).Type, wantRulesExtra: (*struct{})(nil), }, } From 6d3dec185068336ed3b768dde6e6cb9d75639dc2 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Fri, 23 Aug 2024 15:12:33 +0100 Subject: [PATCH 06/24] doc: `params.*Extra*` comments and improved readability --- params/config.libevm.go | 108 +++++++++++++++++++--------------- params/config.libevm_test.go | 2 +- params/example.libevm_test.go | 2 +- 3 files changed, 63 insertions(+), 49 deletions(-) diff --git a/params/config.libevm.go b/params/config.libevm.go index c212995d5c5..8c56f5e0f4a 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -12,13 +12,13 @@ import ( // Extras are arbitrary payloads to be added as extra fields in [ChainConfig] // and [Rules] structs. See [RegisterExtras]. type Extras[C any, R any] struct { - // NewForRules, if non-nil is called at the end of [ChainConfig.Rules] with - // the newly created [Rules] and the [ChainConfig] extra payload. Its - // returned value will be the extra payload of the [Rules]. If NewForRules - // is nil then so too will the [Rules] extra payload be a nil `*R`. + // NewRules, if non-nil is called at the end of [ChainConfig.Rules] with the + // newly created [Rules] and other context from the method call. Its + // returned value will be the extra payload of the [Rules]. If NewRules is + // nil then so too will the [Rules] extra payload be a nil `*R`. // - // NewForRules MAY modify the [Rules] but MUST NOT modify the [ChainConfig]. - NewForRules func(_ *ChainConfig, _ *Rules, _ *C, blockNum *big.Int, isMerge bool, timestamp uint64) *R + // NewRules MAY modify the [Rules] but MUST NOT modify the [ChainConfig]. + NewRules func(_ *ChainConfig, _ *Rules, _ *C, blockNum *big.Int, isMerge bool, timestamp uint64) *R } // RegisterExtras registers the types `C` and `R` such that they are carried as @@ -28,19 +28,16 @@ type Extras[C any, R any] struct { // // After registration, JSON unmarshalling of a [ChainConfig] will create a new // `*C` and unmarshal the JSON key "extra" into it. Conversely, JSON marshalling -// will populate the "extra" key with the contents of the `*C`. Calls to -// [ChainConfig.Rules] will call the `NewForRules` function of the registered -// [Extras] to create a new `*R`. +// will populate the "extra" key with the contents of the `*C`. Both the +// [json.Marshaler] and [json.Unmarshaler] interfaces are honoured if +// implemented by `C` and/or `R.` // -// The payloads can be accessed via the [ChainConfig.extraPayload] and -// [Rules.extraPayload] methods, which will always return a `*C` or `*R` -// respectively however these pointers may themselves be nil. +// Calls to [ChainConfig.Rules] will call the `NewRules` function of the +// registered [Extras] to create a new `*R`. // -// As the `ExtraPayload()` methods are not generic and return `any`, their -// values MUST be type-asserted to the returned type; failure to do so may -// result in a typed-nil bug. This pattern most-closely resembles a fully -// generic implementation and users SHOULD wrap the type assertions in a shared -// package. +// The payloads can be accessed via the [ExtraPayloadGetter.FromChainConfig] and +// [ExtraPayloadGetter.FromRules] methods of the getter returned by +// RegisterExtras. func RegisterExtras[C any, R any](e Extras[C, R]) ExtraPayloadGetter[C, R] { if registeredExtras != nil { panic("re-registration of Extras") @@ -51,15 +48,28 @@ func RegisterExtras[C any, R any](e Extras[C, R]) ExtraPayloadGetter[C, R] { return ExtraPayloadGetter[C, R]{} } -// An ExtraPayloadGettter ... -type ExtraPayloadGetter[C any, R any] struct{} +// registeredExtras holds the [Extras] registered via [RegisterExtras]. As we +// don't know `C` and `R` at compile time, it must be an interface. +var registeredExtras interface { + nilForChainConfig() *pseudo.Type + nilForRules() *pseudo.Type + newForChainConfig() *pseudo.Type + newForRules(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type +} + +// An ExtraPayloadGettter provides strongly typed access to the extra payloads +// carried by [ChainConfig] and [Rules] structs. The only valid way to construct +// a getter is by a call to [RegisterExtras]. +type ExtraPayloadGetter[C any, R any] struct { + _ struct{} // make godoc show unexported fields so nobody tries to make their own getter ;) +} -// FromChainConfig ... +// FromChainConfig returns the ChainConfig's extra payload. func (ExtraPayloadGetter[C, R]) FromChainConfig(c *ChainConfig) *C { return pseudo.MustNewValue[*C](c.extraPayload()).Get() } -// FromRules ... +// FromRules returns the Rules' extra payload. func (ExtraPayloadGetter[C, R]) FromRules(r *Rules) *R { return pseudo.MustNewValue[*R](r.extraPayload()).Get() } @@ -71,24 +81,14 @@ func mustBeStruct[T any]() { } } +// notStructMessage returns the message with which [mustBeStruct] might panic. +// It exists to avoid change-detector tests should the message contents change. func notStructMessage[T any]() string { var x T return fmt.Sprintf("%T is not a struct", x) } -var registeredExtras interface { - nilForChainConfig() *pseudo.Type - nilForRules() *pseudo.Type - newForChainConfig() *pseudo.Type - newForRules(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type -} - -var ( - _ json.Unmarshaler = (*ChainConfig)(nil) - _ json.Marshaler = (*ChainConfig)(nil) -) - -// UnmarshalJSON ... TODO +// UnmarshalJSON implements the [json.Unmarshaler] interface. func (c *ChainConfig) UnmarshalJSON(data []byte) error { // We need to bypass this UnmarshalJSON() method when we again call // json.Unmarshal(). The `raw` type won't inherit the method. @@ -113,7 +113,7 @@ func (c *ChainConfig) UnmarshalJSON(data []byte) error { return nil } -// MarshalJSON ... TODO +// MarshalJSON implements the [json.Marshaler] interface. func (c *ChainConfig) MarshalJSON() ([]byte, error) { type raw ChainConfig cc := &struct { @@ -123,6 +123,13 @@ func (c *ChainConfig) MarshalJSON() ([]byte, error) { return json.Marshal(cc) } +var _ interface { + json.Marshaler + json.Unmarshaler +} = (*ChainConfig)(nil) + +// addRulesExtra is called at the end of [ChainConfig.Rules]; it exists to +// abstract the libevm-specific behaviour outside of original geth code. func (c *ChainConfig) addRulesExtra(r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) { r.extra = nil if registeredExtras != nil { @@ -130,13 +137,15 @@ func (c *ChainConfig) addRulesExtra(r *Rules, blockNum *big.Int, isMerge bool, t } } -// extraPayload returns the extra payload carried by the ChainConfig and can -// only be called if [RegisterExtras] was called. The returned value is always -// of type `*C` as registered, but may be nil. Callers MUST immediately -// type-assert the returned value to `*C` to avoid typed-nil bugs. See the -// example for the intended usage pattern. +// extraPayload returns the ChainConfig's extra payload iff [RegisterExtras] has +// already been called. If the payload hasn't been populated (typically via +// unmarshalling of JSON), a nil value is constructed and returned. func (c *ChainConfig) extraPayload() *pseudo.Type { if registeredExtras == nil { + // This will only happen if someone constructs an [ExtraPayloadGetter] + // directly, without a call to [RegisterExtras]. + // + // See https://google.github.io/styleguide/go/best-practices#when-to-panic panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", c)) } if c.extra == nil { @@ -145,13 +154,10 @@ func (c *ChainConfig) extraPayload() *pseudo.Type { return c.extra } -// extraPayload returns the extra payload carried by the Rules and can only be -// called if [RegisterExtras] was called. The returned value is always of type -// `*R` as registered, but may be nil. Callers MUST immediately type-assert the -// returned value to `*R` to avoid typed-nil bugs. See the example on -// [ChainConfig.extraPayload] for the intended usage pattern. +// extraPayload is equivalent to [ChainConfig.extraPayload]. func (r *Rules) extraPayload() *pseudo.Type { if registeredExtras == nil { + // See ChainConfig.extraPayload() equivalent. panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", r)) } if r.extra == nil { @@ -160,6 +166,10 @@ func (r *Rules) extraPayload() *pseudo.Type { return r.extra } +/** + * Start of Extras implementing the registeredExtras interface. + */ + func (Extras[C, R]) nilForChainConfig() *pseudo.Type { return pseudo.Zero[*C]().Type } func (Extras[C, R]) nilForRules() *pseudo.Type { return pseudo.Zero[*R]().Type } @@ -169,8 +179,12 @@ func (*Extras[C, R]) newForChainConfig() *pseudo.Type { } func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type { - if e.NewForRules == nil { + if e.NewRules == nil { return e.nilForRules() } - return pseudo.From(e.NewForRules(c, r, c.extra.Interface().(*C), blockNum, isMerge, timestamp)).Type + return pseudo.From(e.NewRules(c, r, c.extra.Interface().(*C), blockNum, isMerge, timestamp)).Type } + +/** + * End of Extras implementing the registeredExtras interface. + */ diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index bc940750389..49e6278f75c 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -49,7 +49,7 @@ func TestRegisterExtras(t *testing.T) { name: "Rules payload copied from ChainConfig payload", register: func() { RegisterExtras(Extras[ccExtraA, rulesExtraA]{ - NewForRules: func(cc *ChainConfig, r *Rules, ex *ccExtraA, _ *big.Int, _ bool, _ uint64) *rulesExtraA { + NewRules: func(cc *ChainConfig, r *Rules, ex *ccExtraA, _ *big.Int, _ bool, _ uint64) *rulesExtraA { return &rulesExtraA{ A: ex.A, } diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index 16a88b37084..97e32baddf5 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -12,7 +12,7 @@ import ( // TODO: explain why this isn't in an init() func initFn() { getter = params.RegisterExtras(params.Extras[ChainConfigExtra, RulesExtra]{ - NewForRules: constructRulesExtra, + NewRules: constructRulesExtra, }) } From e196474ea993ff5ad9b858d1c671834bd2a9d06c Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Fri, 23 Aug 2024 15:30:12 +0100 Subject: [PATCH 07/24] doc: `params.ExtraPayloadGetter` comments and improved readability --- params/example.libevm_test.go | 68 ++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index 97e32baddf5..c6c99219fa7 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -1,3 +1,12 @@ +// In practice, everything in this file except for the Example() function SHOULD +// be a standalone package, typically called `extraparams`. As long as this new +// package is imported anywhere, its init() function will register the "extra" +// types, which can be accessed via [extraparams.FromChainConfig] and/or +// [extraparams.FromRules]. In all other respects, the [params.ChainConfig] and +// [params.Rules] types will act as expected. +// +// The Example() function demonstrates how the `extraparams` package might be +// used from elsewhere. package params_test import ( @@ -9,8 +18,12 @@ import ( "github.com/ethereum/go-ethereum/params" ) -// TODO: explain why this isn't in an init() +// In practice this would be a regular init() function but nuances around the +// testing of this package require it to be called in the Example(). func initFn() { + // This registration makes *all* [params.ChainConfig] and [params.Rules] + // instances respect the payload types. They do not need to be modified to + // know about `extraparams`. getter = params.RegisterExtras(params.Extras[ChainConfigExtra, RulesExtra]{ NewRules: constructRulesExtra, }) @@ -18,30 +31,41 @@ func initFn() { var getter params.ExtraPayloadGetter[ChainConfigExtra, RulesExtra] -func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { - return getter.FromChainConfig(c) -} - -func FromRules(r *params.Rules) *RulesExtra { - return getter.FromRules(r) +// constructRulesExtra acts as an adjunct to the [params.ChainConfig.Rules] +// method. Its primary purpose is to construct the extra payload for the +// [params.Rules] but it MAY also modify the [params.Rules]. +func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx *ChainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *RulesExtra { + return &RulesExtra{ + IsMyFork: cEx.MyForkTime != nil && *cEx.MyForkTime <= timestamp, + } } +// ChainConfigExtra can be any struct. Here it just mirrors a common pattern in +// the standard [params.ChainConfig] struct. type ChainConfigExtra struct { MyForkTime *uint64 `json:"myForkTime"` } +// RulesExtra can be any struct. It too mirrors a common pattern in +// [params.Rules]. type RulesExtra struct { IsMyFork bool } -func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx *ChainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *RulesExtra { - return &RulesExtra{ - IsMyFork: cEx.MyForkTime != nil && *cEx.MyForkTime <= timestamp, - } +// FromChainConfig returns the extra payload carried by the ChainConfig. +func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { + return getter.FromChainConfig(c) +} + +// FromRules returns the extra payload carried by the Rules. +func FromRules(r *params.Rules) *RulesExtra { + return getter.FromRules(r) } -func ExampleRegisterExtras() { - initFn() // TODO: explain +// This example demonstrates how the rest of this file would be used from a +// *different* package. +func ExampleExtraPayloadGetter() { + initFn() // Outside of an example this is unnecessary as the function will be a regular init(). const forkTime = 530003640 jsonData := fmt.Sprintf(`{ @@ -51,31 +75,33 @@ func ExampleRegisterExtras() { } }`, forkTime) - // ChainConfig now unmarshals any JSON field named "extra" into a pointer to - // the registered type, which is available via the ExtraPayload() method. + // Because [params.RegisterExtras] has been called, unmarshalling a JSON + // field of "extra" into a [params.ChainConfig] will populate a new value of + // the registered type. This can be accessed with the [FromChainConfig] + // function. config := new(params.ChainConfig) if err := json.Unmarshal([]byte(jsonData), config); err != nil { log.Fatal(err) } - fmt.Println(config.ChainID) // original geth fields work as expected + fmt.Println("Chain ID", config.ChainID) // original geth fields work as expected - ccExtra := FromChainConfig(config) + ccExtra := FromChainConfig(config) // extraparams.FromChainConfig() in practice if ccExtra != nil && ccExtra.MyForkTime != nil { - fmt.Println(*ccExtra.MyForkTime) + fmt.Println("Fork time", *ccExtra.MyForkTime) } for _, time := range []uint64{forkTime - 1, forkTime, forkTime + 1} { rules := config.Rules(nil, false, time) - rExtra := FromRules(&rules) + rExtra := FromRules(&rules) // extraparams.FromRules() in practice if rExtra != nil { fmt.Printf("%+v\n", rExtra) } } // Output: - // 1234 - // 530003640 + // Chain ID 1234 + // Fork time 530003640 // &{IsMyFork:false} // &{IsMyFork:true} // &{IsMyFork:true} From 25e67045692de1a22f247b214ac1222d811d01a7 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Fri, 23 Aug 2024 15:33:02 +0100 Subject: [PATCH 08/24] doc: `params/config.libevm_test.go` comments and improved readability --- params/config.libevm_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 49e6278f75c..7f74e748a70 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -10,6 +10,9 @@ import ( "github.com/stretchr/testify/require" ) +// testOnlyClearRegisteredExtras SHOULD be called before every call to +// [RegisterExtras] and then defer-called afterwards. This is a workaround for +// the single-call limitation on [RegisterExtras]. func testOnlyClearRegisteredExtras() { registeredExtras = nil } @@ -18,10 +21,10 @@ type rawJSON struct { json.RawMessage } -var ( - _ json.Unmarshaler = (*rawJSON)(nil) - _ json.Marshaler = (*rawJSON)(nil) -) +var _ interface { + json.Marshaler + json.Unmarshaler +} = (*rawJSON)(nil) func TestRegisterExtras(t *testing.T) { type ( From 6d75c120c1cc5601ca74037608e06f48728a3fa5 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 26 Aug 2024 12:19:21 +0100 Subject: [PATCH 09/24] refactor: simplify `params.ChainConfig.UnmarshalJSON()` --- params/config.libevm.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/params/config.libevm.go b/params/config.libevm.go index 8c56f5e0f4a..38acbb7a3b8 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -90,35 +90,29 @@ func notStructMessage[T any]() string { // UnmarshalJSON implements the [json.Unmarshaler] interface. func (c *ChainConfig) UnmarshalJSON(data []byte) error { - // We need to bypass this UnmarshalJSON() method when we again call - // json.Unmarshal(). The `raw` type won't inherit the method. - type raw ChainConfig + type raw ChainConfig // doesn't inherit methods so avoids recursing back here (infinitely) cc := &struct { *raw - Extra json.RawMessage `json:"extra"` - }{raw: (*raw)(c)} - - if err := json.Unmarshal(data, cc); err != nil { - return err - } - if registeredExtras == nil || len(cc.Extra) == 0 { - return nil + Extra *pseudo.Type `json:"extra"` + }{ + raw: (*raw)(c), // embedded to achieve regular JSON unmarshalling + Extra: registeredExtras.nilForChainConfig(), // `c.extra` is otherwise unexported } - extra := registeredExtras.newForChainConfig() - if err := json.Unmarshal(cc.Extra, extra); err != nil { + if err := json.Unmarshal(data, cc); err != nil { return err } - c.extra = extra + c.extra = cc.Extra return nil } // MarshalJSON implements the [json.Marshaler] interface. func (c *ChainConfig) MarshalJSON() ([]byte, error) { + // See UnmarshalJSON() for rationale. type raw ChainConfig cc := &struct { *raw - Extra any `json:"extra"` + Extra *pseudo.Type `json:"extra"` }{raw: (*raw)(c), Extra: c.extra} return json.Marshal(cc) } From a6742fd1ad8012cba56043e10a7231443d6a3f30 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 26 Aug 2024 12:52:00 +0100 Subject: [PATCH 10/24] refactor: abstract new/nil-pointer creation into `pseudo.Constructor`s --- libevm/pseudo/constructor.go | 24 ++++++++ libevm/pseudo/constructor_test.go | 45 +++++++++++++++ params/config.libevm.go | 91 ++++++++++++++----------------- 3 files changed, 110 insertions(+), 50 deletions(-) create mode 100644 libevm/pseudo/constructor.go create mode 100644 libevm/pseudo/constructor_test.go diff --git a/libevm/pseudo/constructor.go b/libevm/pseudo/constructor.go new file mode 100644 index 00000000000..91429240340 --- /dev/null +++ b/libevm/pseudo/constructor.go @@ -0,0 +1,24 @@ +package pseudo + +// A Constructor returns newly constructed [Type] instances for a pre-registered +// concrete type. +type Constructor interface { + Zero() *Type + NewPointer() *Type + NilPointer() *Type +} + +// NewConstructor returns a [Constructor] that builds `T` [Type] instances. +func NewConstructor[T any]() Constructor { + return ctor[T]{} +} + +type ctor[T any] struct{} + +func (ctor[T]) Zero() *Type { return Zero[T]().Type } +func (ctor[T]) NilPointer() *Type { return Zero[*T]().Type } + +func (ctor[T]) NewPointer() *Type { + var x T + return From(&x).Type +} diff --git a/libevm/pseudo/constructor_test.go b/libevm/pseudo/constructor_test.go new file mode 100644 index 00000000000..a28f3dd42de --- /dev/null +++ b/libevm/pseudo/constructor_test.go @@ -0,0 +1,45 @@ +package pseudo + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConstructor(t *testing.T) { + testConstructor[uint](t) + testConstructor[string](t) + testConstructor[struct{ x string }](t) +} + +func testConstructor[T any](t *testing.T) { + var zero T + t.Run(fmt.Sprintf("%T", zero), func(t *testing.T) { + ctor := NewConstructor[T]() + + t.Run("NilPointer()", func(t *testing.T) { + got := get[*T](t, ctor.NilPointer()) + assert.Nil(t, got) + }) + + t.Run("NewPointer()", func(t *testing.T) { + got := get[*T](t, ctor.NewPointer()) + require.NotNil(t, got) + assert.Equal(t, zero, *got) + }) + + t.Run("Zero()", func(t *testing.T) { + got := get[T](t, ctor.Zero()) + assert.Equal(t, zero, got) + }) + }) +} + +func get[T any](t *testing.T, typ *Type) (x T) { + t.Helper() + val, err := NewValue[T](typ) + require.NoError(t, err, "NewValue[%T]()", x) + return val.Get() +} diff --git a/params/config.libevm.go b/params/config.libevm.go index 38acbb7a3b8..66e995f9a6a 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -44,17 +44,45 @@ func RegisterExtras[C any, R any](e Extras[C, R]) ExtraPayloadGetter[C, R] { } mustBeStruct[C]() mustBeStruct[R]() - registeredExtras = &e - return ExtraPayloadGetter[C, R]{} + registeredExtras = &extraConstructors{ + chainConfig: pseudo.NewConstructor[C](), + rules: pseudo.NewConstructor[R](), + newForRules: e.newForRules, + } + return e.getter() +} + +// registeredExtras holds non-generic constructors for the [Extras] types +// registered via [RegisterExtras]. +var registeredExtras *extraConstructors + +type extraConstructors struct { + chainConfig, rules pseudo.Constructor + newForRules func(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type +} + +func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type { + if e.NewRules == nil { + return registeredExtras.rules.NilPointer() + } + rExtra := e.NewRules(c, r, e.getter().FromChainConfig(c), blockNum, isMerge, timestamp) + return pseudo.From(rExtra).Type +} + +func (*Extras[C, R]) getter() (g ExtraPayloadGetter[C, R]) { return } + +// mustBeStruct panics if `T` isn't a struct. +func mustBeStruct[T any]() { + if k := reflect.TypeFor[T]().Kind(); k != reflect.Struct { + panic(notStructMessage[T]()) + } } -// registeredExtras holds the [Extras] registered via [RegisterExtras]. As we -// don't know `C` and `R` at compile time, it must be an interface. -var registeredExtras interface { - nilForChainConfig() *pseudo.Type - nilForRules() *pseudo.Type - newForChainConfig() *pseudo.Type - newForRules(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type +// notStructMessage returns the message with which [mustBeStruct] might panic. +// It exists to avoid change-detector tests should the message contents change. +func notStructMessage[T any]() string { + var x T + return fmt.Sprintf("%T is not a struct", x) } // An ExtraPayloadGettter provides strongly typed access to the extra payloads @@ -74,20 +102,6 @@ func (ExtraPayloadGetter[C, R]) FromRules(r *Rules) *R { return pseudo.MustNewValue[*R](r.extraPayload()).Get() } -func mustBeStruct[T any]() { - var x T - if k := reflect.TypeOf(x).Kind(); k != reflect.Struct { - panic(notStructMessage[T]()) - } -} - -// notStructMessage returns the message with which [mustBeStruct] might panic. -// It exists to avoid change-detector tests should the message contents change. -func notStructMessage[T any]() string { - var x T - return fmt.Sprintf("%T is not a struct", x) -} - // UnmarshalJSON implements the [json.Unmarshaler] interface. func (c *ChainConfig) UnmarshalJSON(data []byte) error { type raw ChainConfig // doesn't inherit methods so avoids recursing back here (infinitely) @@ -95,8 +109,8 @@ func (c *ChainConfig) UnmarshalJSON(data []byte) error { *raw Extra *pseudo.Type `json:"extra"` }{ - raw: (*raw)(c), // embedded to achieve regular JSON unmarshalling - Extra: registeredExtras.nilForChainConfig(), // `c.extra` is otherwise unexported + raw: (*raw)(c), // embedded to achieve regular JSON unmarshalling + Extra: registeredExtras.chainConfig.NilPointer(), // `c.extra` is otherwise unexported } if err := json.Unmarshal(data, cc); err != nil { @@ -143,7 +157,7 @@ func (c *ChainConfig) extraPayload() *pseudo.Type { panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", c)) } if c.extra == nil { - c.extra = registeredExtras.nilForChainConfig() + c.extra = registeredExtras.chainConfig.NilPointer() } return c.extra } @@ -155,30 +169,7 @@ func (r *Rules) extraPayload() *pseudo.Type { panic(fmt.Sprintf("%T.ExtraPayload() called before RegisterExtras()", r)) } if r.extra == nil { - r.extra = registeredExtras.nilForRules() + r.extra = registeredExtras.rules.NilPointer() } return r.extra } - -/** - * Start of Extras implementing the registeredExtras interface. - */ - -func (Extras[C, R]) nilForChainConfig() *pseudo.Type { return pseudo.Zero[*C]().Type } -func (Extras[C, R]) nilForRules() *pseudo.Type { return pseudo.Zero[*R]().Type } - -func (*Extras[C, R]) newForChainConfig() *pseudo.Type { - var x C - return pseudo.From(&x).Type -} - -func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type { - if e.NewRules == nil { - return e.nilForRules() - } - return pseudo.From(e.NewRules(c, r, c.extra.Interface().(*C), blockNum, isMerge, timestamp)).Type -} - -/** - * End of Extras implementing the registeredExtras interface. - */ From fc28b9c0b3dfd89ea697efede4fafa6d34b79ec9 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 2 Sep 2024 14:58:33 +0100 Subject: [PATCH 11/24] feat: precompile override via `params.Extras` hooks --- core/vm/evm.go | 3 + core/vm/precompiles.libevm_test.go | 113 +++++++++++++++++++++++++++++ libevm/interfaces_test.go | 16 ++++ libevm/libevm.go | 9 +++ params/config.libevm.go | 51 ++++++++++++- params/config.libevm_test.go | 41 +++-------- params/example.libevm_test.go | 9 +++ params/hooks.libevm.go | 50 +++++++++++++ 8 files changed, 259 insertions(+), 33 deletions(-) create mode 100644 core/vm/precompiles.libevm_test.go create mode 100644 libevm/interfaces_test.go create mode 100644 libevm/libevm.go create mode 100644 params/hooks.libevm.go diff --git a/core/vm/evm.go b/core/vm/evm.go index 16cc8549080..35a0aa227cb 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -38,6 +38,9 @@ type ( ) func (evm *EVM) precompile(addr common.Address) (PrecompiledContract, bool) { + if p, override := evm.chainRules.PrecompileOverride(addr); override { + return p, p != nil + } var precompiles map[common.Address]PrecompiledContract switch { case evm.chainRules.IsCancun: diff --git a/core/vm/precompiles.libevm_test.go b/core/vm/precompiles.libevm_test.go new file mode 100644 index 00000000000..c3d2018658f --- /dev/null +++ b/core/vm/precompiles.libevm_test.go @@ -0,0 +1,113 @@ +package vm + +import ( + "fmt" + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/rawdb" + "github.com/ethereum/go-ethereum/core/state" + "github.com/ethereum/go-ethereum/libevm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/exp/rand" +) + +// precompileOverrides is a [params.RulesHooks] that overrides precompiles from +// a map of predefined addresses. +type precompileOverrides struct { + contracts map[common.Address]PrecompiledContract + params.NoopHooks // all other hooks +} + +func (o precompileOverrides) PrecompileOverride(_ params.Rules, a common.Address) (libevm.PrecompiledContract, bool) { + c, ok := o.contracts[a] + return c, ok +} + +// A precompileStub is a [PrecompiledContract] that always returns the same +// values. +type precompileStub struct { + requiredGas uint64 + returnData []byte +} + +func (s *precompileStub) RequiredGas([]byte) uint64 { return s.requiredGas } +func (s *precompileStub) Run([]byte) ([]byte, error) { return s.returnData, nil } + +func TestPrecompileOverride(t *testing.T) { + type test struct { + name string + addr common.Address + requiredGas uint64 + stubData []byte + } + + const gasLimit = uint64(1e7) + + tests := []test{ + { + name: "arbitrary values", + addr: common.Address{'p', 'r', 'e', 'c', 'o', 'm', 'p', 'i', 'l', 'e'}, + requiredGas: 314159, + stubData: []byte("the return data"), + }, + } + + rng := rand.New(rand.NewSource(42)) + for _, addr := range PrecompiledAddressesCancun { + tests = append(tests, test{ + name: fmt.Sprintf("existing precompile %v", addr), + addr: addr, + requiredGas: rng.Uint64n(gasLimit), + stubData: addr[:], + }) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + precompile := &precompileStub{ + requiredGas: tt.requiredGas, + returnData: tt.stubData, + } + + params.TestOnlyClearRegisteredExtras() + params.RegisterExtras(params.Extras[params.NoopHooks, precompileOverrides]{ + NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *params.NoopHooks, blockNum *big.Int, isMerge bool, timestamp uint64) *precompileOverrides { + return &precompileOverrides{ + contracts: map[common.Address]PrecompiledContract{ + tt.addr: precompile, + }, + } + }, + }) + + t.Run(fmt.Sprintf("%T.Call([overridden precompile address = %v])", &EVM{}, tt.addr), func(t *testing.T) { + gotData, gotGasLeft, err := newEVM(t).Call(AccountRef{}, tt.addr, nil, gasLimit, uint256.NewInt(0)) + require.NoError(t, err) + assert.Equal(t, tt.stubData, gotData, "contract's return data") + assert.Equal(t, gasLimit-tt.requiredGas, gotGasLeft, "gas left") + }) + }) + } +} + +func newEVM(t *testing.T) *EVM { + t.Helper() + + sdb, err := state.New(common.Hash{}, state.NewDatabase(rawdb.NewMemoryDatabase()), nil) + require.NoError(t, err, "state.New()") + + return NewEVM( + BlockContext{ + Transfer: func(_ StateDB, _, _ common.Address, _ *uint256.Int) {}, + }, + TxContext{}, + sdb, + ¶ms.ChainConfig{}, + Config{}, + ) +} diff --git a/libevm/interfaces_test.go b/libevm/interfaces_test.go new file mode 100644 index 00000000000..13691be27eb --- /dev/null +++ b/libevm/interfaces_test.go @@ -0,0 +1,16 @@ +package libevm_test + +import ( + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/libevm" +) + +// These two interfaces MUST be identical. If this breaks then the libevm copy +// MUST be updated. +var ( + // Each assignment demonstrates that the methods of the LHS interface are a + // (non-strict) subset of the RHS interface's; both being possible + // proves that they are identical. + _ vm.PrecompiledContract = (libevm.PrecompiledContract)(nil) + _ libevm.PrecompiledContract = (vm.PrecompiledContract)(nil) +) diff --git a/libevm/libevm.go b/libevm/libevm.go new file mode 100644 index 00000000000..b48eb1c5fae --- /dev/null +++ b/libevm/libevm.go @@ -0,0 +1,9 @@ +package libevm + +// PrecompiledContract is an exact copy of [vm.PrecompiledContract], mirrored +// here for instances where importing [vm] would result in a circular +// dependency. +type PrecompiledContract interface { + RequiredGas(input []byte) uint64 + Run(input []byte) ([]byte, error) +} diff --git a/params/config.libevm.go b/params/config.libevm.go index 66e995f9a6a..423d2d662e7 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -5,13 +5,15 @@ import ( "fmt" "math/big" "reflect" + "runtime" + "strings" "github.com/ethereum/go-ethereum/libevm/pseudo" ) // Extras are arbitrary payloads to be added as extra fields in [ChainConfig] // and [Rules] structs. See [RegisterExtras]. -type Extras[C any, R any] struct { +type Extras[C ChainConfigHooks, R RulesHooks] struct { // NewRules, if non-nil is called at the end of [ChainConfig.Rules] with the // newly created [Rules] and other context from the method call. Its // returned value will be the extra payload of the [Rules]. If NewRules is @@ -38,18 +40,35 @@ type Extras[C any, R any] struct { // The payloads can be accessed via the [ExtraPayloadGetter.FromChainConfig] and // [ExtraPayloadGetter.FromRules] methods of the getter returned by // RegisterExtras. -func RegisterExtras[C any, R any](e Extras[C, R]) ExtraPayloadGetter[C, R] { +func RegisterExtras[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ExtraPayloadGetter[C, R] { if registeredExtras != nil { panic("re-registration of Extras") } mustBeStruct[C]() mustBeStruct[R]() + + getter := e.getter() registeredExtras = &extraConstructors{ chainConfig: pseudo.NewConstructor[C](), rules: pseudo.NewConstructor[R](), newForRules: e.newForRules, + getter: getter, + } + return getter +} + +// TestOnlyClearRegisteredExtras clears the [Extras] previously passed to +// [RegisterExtras]. It panics if called from a non-test file. +// +// In tests, it SHOULD be called before every call to [RegisterExtras] and then +// defer-called afterwards. This is a workaround for the single-call limitation +// on [RegisterExtras]. +func TestOnlyClearRegisteredExtras() { + _, file, _, ok := runtime.Caller(1 /* 0 would be here, not our caller */) + if !ok || !strings.HasSuffix(file, "_test.go") { + panic("call from non-test file") } - return e.getter() + registeredExtras = nil } // registeredExtras holds non-generic constructors for the [Extras] types @@ -59,6 +78,10 @@ var registeredExtras *extraConstructors type extraConstructors struct { chainConfig, rules pseudo.Constructor newForRules func(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type + getter interface { // use hooksFrom() methods for access + hooksFromChainConfig(*ChainConfig) ChainConfigHooks + hooksFromRules(*Rules) RulesHooks + } } func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type { @@ -88,7 +111,7 @@ func notStructMessage[T any]() string { // An ExtraPayloadGettter provides strongly typed access to the extra payloads // carried by [ChainConfig] and [Rules] structs. The only valid way to construct // a getter is by a call to [RegisterExtras]. -type ExtraPayloadGetter[C any, R any] struct { +type ExtraPayloadGetter[C ChainConfigHooks, R RulesHooks] struct { _ struct{} // make godoc show unexported fields so nobody tries to make their own getter ;) } @@ -97,11 +120,31 @@ func (ExtraPayloadGetter[C, R]) FromChainConfig(c *ChainConfig) *C { return pseudo.MustNewValue[*C](c.extraPayload()).Get() } +// hooksFromChainConfig is equivalent to FromChainConfig(), but returns an +// interface instead of the concrete type implementing it; this allows it to be +// used in non-generic code. If the concrete-type value is nil (typically +// because no [Extras] were registered) a [noopHooks] is returned so it can be +// used without nil checks. +func (e ExtraPayloadGetter[C, R]) hooksFromChainConfig(c *ChainConfig) ChainConfigHooks { + if h := e.FromChainConfig(c); h != nil { + return *h + } + return NoopHooks{} +} + // FromRules returns the Rules' extra payload. func (ExtraPayloadGetter[C, R]) FromRules(r *Rules) *R { return pseudo.MustNewValue[*R](r.extraPayload()).Get() } +// hooksFromRules is the [RulesHooks] equivalent of hooksFromChainConfig(). +func (e ExtraPayloadGetter[C, R]) hooksFromRules(r *Rules) RulesHooks { + if h := e.FromRules(r); h != nil { + return *h + } + return NoopHooks{} +} + // UnmarshalJSON implements the [json.Unmarshaler] interface. func (c *ChainConfig) UnmarshalJSON(data []byte) error { type raw ChainConfig // doesn't inherit methods so avoids recursing back here (infinitely) diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 7f74e748a70..83854c30442 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -10,13 +10,6 @@ import ( "github.com/stretchr/testify/require" ) -// testOnlyClearRegisteredExtras SHOULD be called before every call to -// [RegisterExtras] and then defer-called afterwards. This is a workaround for -// the single-call limitation on [RegisterExtras]. -func testOnlyClearRegisteredExtras() { - registeredExtras = nil -} - type rawJSON struct { json.RawMessage } @@ -30,15 +23,19 @@ func TestRegisterExtras(t *testing.T) { type ( ccExtraA struct { A string `json:"a"` + ChainConfigHooks } rulesExtraA struct { A string + RulesHooks } ccExtraB struct { B string `json:"b"` + ChainConfigHooks } rulesExtraB struct { B string + RulesHooks } ) @@ -79,20 +76,20 @@ func TestRegisterExtras(t *testing.T) { { name: "custom JSON handling honoured", register: func() { - RegisterExtras(Extras[rawJSON, struct{}]{}) + RegisterExtras(Extras[rawJSON, struct{ RulesHooks }]{}) }, ccExtra: pseudo.From(&rawJSON{ RawMessage: []byte(`"hello, world"`), }).Type, - wantRulesExtra: (*struct{})(nil), + wantRulesExtra: (*struct{ RulesHooks })(nil), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - testOnlyClearRegisteredExtras() + TestOnlyClearRegisteredExtras() tt.register() - defer testOnlyClearRegisteredExtras() + defer TestOnlyClearRegisteredExtras() in := &ChainConfig{ ChainID: big.NewInt(142857), @@ -116,22 +113,8 @@ func TestRegisterExtras(t *testing.T) { } func TestExtrasPanic(t *testing.T) { - testOnlyClearRegisteredExtras() - defer testOnlyClearRegisteredExtras() - - assertPanics( - t, func() { - RegisterExtras(Extras[int, struct{}]{}) - }, - notStructMessage[int](), - ) - - assertPanics( - t, func() { - RegisterExtras(Extras[struct{}, bool]{}) - }, - notStructMessage[bool](), - ) + TestOnlyClearRegisteredExtras() + defer TestOnlyClearRegisteredExtras() assertPanics( t, func() { @@ -147,11 +130,11 @@ func TestExtrasPanic(t *testing.T) { "before RegisterExtras", ) - RegisterExtras(Extras[struct{}, struct{}]{}) + RegisterExtras(Extras[struct{ ChainConfigHooks }, struct{ RulesHooks }]{}) assertPanics( t, func() { - RegisterExtras(Extras[struct{}, struct{}]{}) + RegisterExtras(Extras[struct{ ChainConfigHooks }, struct{ RulesHooks }]{}) }, "re-registration", ) diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index c6c99219fa7..4db9fdef8a5 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -15,12 +15,16 @@ import ( "log" "math/big" + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/libevm" "github.com/ethereum/go-ethereum/params" ) // In practice this would be a regular init() function but nuances around the // testing of this package require it to be called in the Example(). func initFn() { + params.TestOnlyClearRegisteredExtras() // not necessary outside of the example // This registration makes *all* [params.ChainConfig] and [params.Rules] // instances respect the payload types. They do not need to be modified to // know about `extraparams`. @@ -52,6 +56,11 @@ type RulesExtra struct { IsMyFork bool } +func (r RulesExtra) PrecompileOverride(params.Rules, common.Address) (libevm.PrecompiledContract, bool) { + var override vm.PrecompiledContract + return override, true +} + // FromChainConfig returns the extra payload carried by the ChainConfig. func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { return getter.FromChainConfig(c) diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go new file mode 100644 index 00000000000..56dbc964da9 --- /dev/null +++ b/params/hooks.libevm.go @@ -0,0 +1,50 @@ +package params + +import ( + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/libevm" +) + +// ChainConfigHooks are... +type ChainConfigHooks interface{} + +// RulesHooks are... +type RulesHooks interface { + PrecompileOverride(Rules, common.Address) (_ libevm.PrecompiledContract, override bool) +} + +func (e *extraConstructors) hooksFromChainConfig(c *ChainConfig) ChainConfigHooks { + if e == nil { + return NoopHooks{} + } + return e.getter.hooksFromChainConfig(c) +} + +func (e *extraConstructors) hooksFromRules(r *Rules) RulesHooks { + if e == nil { + return NoopHooks{} + } + return e.getter.hooksFromRules(r) +} + +// PrecompileOverride... +func (r Rules) PrecompileOverride(addr common.Address) (libevm.PrecompiledContract, bool) { + return registeredExtras.hooksFromRules(&r).PrecompileOverride(r, addr) +} + +// NoopHooks implement both [ChainConfigHooks] and [RulesHooks] such that every +// hook is a no-op. This allows it to be returned instead of nil, which would +// require every usage site to perform a nil check. It can also be embedded in +// structs that only wish to implement a sub-set of hooks. +type NoopHooks struct{} + +var _ interface { + ChainConfigHooks + RulesHooks +} = NoopHooks{} + +// PrecompileOverride always returns `nil, false`; the `false` indicates that +// the `nil` should be ignored. +func (NoopHooks) PrecompileOverride(Rules, common.Address) (libevm.PrecompiledContract, bool) { + return nil, false +} From fb916fe4a941678e5abb192d896c6c650ba979c9 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 2 Sep 2024 17:19:17 +0100 Subject: [PATCH 12/24] doc: flesh out `PrecompileOverride()` in example --- params/example.libevm_test.go | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index 4db9fdef8a5..e779da4ddfa 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -56,11 +56,6 @@ type RulesExtra struct { IsMyFork bool } -func (r RulesExtra) PrecompileOverride(params.Rules, common.Address) (libevm.PrecompiledContract, bool) { - var override vm.PrecompiledContract - return override, true -} - // FromChainConfig returns the extra payload carried by the ChainConfig. func FromChainConfig(c *params.ChainConfig) *ChainConfigExtra { return getter.FromChainConfig(c) @@ -71,6 +66,33 @@ func FromRules(r *params.Rules) *RulesExtra { return getter.FromRules(r) } +// myForkPrecompiledContracts is analogous to the vm.PrecompiledContracts +// maps. Note [RulesExtra.PrecompileOverride] treatment of nil values here. +var myForkPrecompiledContracts = map[common.Address]vm.PrecompiledContract{ + //... + common.BytesToAddress([]byte{0x2}): nil, // i.e disabled + //... +} + +// PrecompileOverride implements the required [params.RuleHooks] method. +func (rex RulesExtra) PrecompileOverride(_ params.Rules, addr common.Address) (_ libevm.PrecompiledContract, override bool) { + if !rex.IsMyFork { + return nil, false + } + p, ok := myForkPrecompiledContracts[addr] + // The returned boolean indicates whether or not [vm.EVMInterpreter] MUST + // override the address, not what it returns as its own `isPrecompile` + // boolean. + // + // Therefore returning `nil, true` here indicates that the precompile will + // be disabled. Returning `false` here indicates that the default precompile + // behaviour will be exhibited. + // + // The same pattern can alternatively be implemented with an explicit + // `disabledPrecompiles` set to make the behaviour clearer. + return p, ok +} + // This example demonstrates how the rest of this file would be used from a // *different* package. func ExampleExtraPayloadGetter() { From 7d1d2069a2170a4b9cb20991221dd11bef4b6ed8 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Mon, 2 Sep 2024 18:20:11 +0100 Subject: [PATCH 13/24] doc: complete commentary and improve readability --- core/vm/precompiles.libevm_test.go | 6 ++-- libevm/libevm.go | 4 +-- params/config.libevm.go | 14 +++++--- params/hooks.libevm.go | 58 +++++++++++++++++++----------- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/core/vm/precompiles.libevm_test.go b/core/vm/precompiles.libevm_test.go index c3d2018658f..7f6a5405ca3 100644 --- a/core/vm/precompiles.libevm_test.go +++ b/core/vm/precompiles.libevm_test.go @@ -20,7 +20,7 @@ import ( // a map of predefined addresses. type precompileOverrides struct { contracts map[common.Address]PrecompiledContract - params.NoopHooks // all other hooks + params.NOOPHooks // all other hooks } func (o precompileOverrides) PrecompileOverride(_ params.Rules, a common.Address) (libevm.PrecompiledContract, bool) { @@ -75,8 +75,8 @@ func TestPrecompileOverride(t *testing.T) { } params.TestOnlyClearRegisteredExtras() - params.RegisterExtras(params.Extras[params.NoopHooks, precompileOverrides]{ - NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *params.NoopHooks, blockNum *big.Int, isMerge bool, timestamp uint64) *precompileOverrides { + params.RegisterExtras(params.Extras[params.NOOPHooks, precompileOverrides]{ + NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *params.NOOPHooks, blockNum *big.Int, isMerge bool, timestamp uint64) *precompileOverrides { return &precompileOverrides{ contracts: map[common.Address]PrecompiledContract{ tt.addr: precompile, diff --git a/libevm/libevm.go b/libevm/libevm.go index b48eb1c5fae..edb54cc3c00 100644 --- a/libevm/libevm.go +++ b/libevm/libevm.go @@ -1,7 +1,7 @@ package libevm -// PrecompiledContract is an exact copy of [vm.PrecompiledContract], mirrored -// here for instances where importing [vm] would result in a circular +// PrecompiledContract is an exact copy of vm.PrecompiledContract, mirrored here +// for instances where importing that package would result in a circular // dependency. type PrecompiledContract interface { RequiredGas(input []byte) uint64 diff --git a/params/config.libevm.go b/params/config.libevm.go index 423d2d662e7..9ddc00359e4 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -39,7 +39,9 @@ type Extras[C ChainConfigHooks, R RulesHooks] struct { // // The payloads can be accessed via the [ExtraPayloadGetter.FromChainConfig] and // [ExtraPayloadGetter.FromRules] methods of the getter returned by -// RegisterExtras. +// RegisterExtras. Where stated in the interface definitions, they will also be +// used as hooks to alter Ethereum behaviour; if this isn't desired then they +// can embed [NOOPHooks] to satisfy either interface. func RegisterExtras[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ExtraPayloadGetter[C, R] { if registeredExtras != nil { panic("re-registration of Extras") @@ -60,7 +62,7 @@ func RegisterExtras[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ExtraPaylo // TestOnlyClearRegisteredExtras clears the [Extras] previously passed to // [RegisterExtras]. It panics if called from a non-test file. // -// In tests, it SHOULD be called before every call to [RegisterExtras] and then +// In tests it SHOULD be called before every call to [RegisterExtras] and then // defer-called afterwards. This is a workaround for the single-call limitation // on [RegisterExtras]. func TestOnlyClearRegisteredExtras() { @@ -78,7 +80,9 @@ var registeredExtras *extraConstructors type extraConstructors struct { chainConfig, rules pseudo.Constructor newForRules func(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type - getter interface { // use hooksFrom() methods for access + // use top-level hooksFrom() functions instead of these as they handle + // instances where no [Extras] were registered. + getter interface { hooksFromChainConfig(*ChainConfig) ChainConfigHooks hooksFromRules(*Rules) RulesHooks } @@ -129,7 +133,7 @@ func (e ExtraPayloadGetter[C, R]) hooksFromChainConfig(c *ChainConfig) ChainConf if h := e.FromChainConfig(c); h != nil { return *h } - return NoopHooks{} + return NOOPHooks{} } // FromRules returns the Rules' extra payload. @@ -142,7 +146,7 @@ func (e ExtraPayloadGetter[C, R]) hooksFromRules(r *Rules) RulesHooks { if h := e.FromRules(r); h != nil { return *h } - return NoopHooks{} + return NOOPHooks{} } // UnmarshalJSON implements the [json.Unmarshaler] interface. diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go index 56dbc964da9..4133f14815d 100644 --- a/params/hooks.libevm.go +++ b/params/hooks.libevm.go @@ -5,46 +5,62 @@ import ( "github.com/ethereum/go-ethereum/libevm" ) -// ChainConfigHooks are... +// ChainConfigHooks are required for all types registered as [Extras] for +// [ChainConfig] payloads. type ChainConfigHooks interface{} -// RulesHooks are... +// RulesHooks are required for all types registered as [Extras] for [Rules] +// payloads. type RulesHooks interface { + // PrecompileOverride signals whether or not the EVM interpreter MUST + // override its treatment of the address when deciding if it is a + // precompiled contract. If PrecompileOverride returns `true` then the + // interpreter will treat the address as a precompile i.f.f the + // [PrecompiledContract] is non-nil. If it returns `false` then the default + // precompile behaviour is honoured. PrecompileOverride(Rules, common.Address) (_ libevm.PrecompiledContract, override bool) } -func (e *extraConstructors) hooksFromChainConfig(c *ChainConfig) ChainConfigHooks { - if e == nil { - return NoopHooks{} +// hooksFromChainConfig returns the registered hooks, or [NOOPHooks] if none +// were registered. +func hooksFromChainConfig(c *ChainConfig) ChainConfigHooks { + if e := registeredExtras; e != nil { + return e.getter.hooksFromChainConfig(c) } - return e.getter.hooksFromChainConfig(c) + return NOOPHooks{} } -func (e *extraConstructors) hooksFromRules(r *Rules) RulesHooks { - if e == nil { - return NoopHooks{} +// hooksFromRules returns the registered hooks, or [NOOPHooks] if none were +// registered. +func hooksFromRules(r *Rules) RulesHooks { + if e := registeredExtras; e != nil { + return e.getter.hooksFromRules(r) } - return e.getter.hooksFromRules(r) + return NOOPHooks{} } -// PrecompileOverride... +// PrecompileOverride is a wrapper for the equivalent method of the registered +// [RulesHooks] carried by the Rules. If no [Extras] were registered, a +// [NOOPHooks] is used instead, such that the default precompile behaviour is +// exhibited. func (r Rules) PrecompileOverride(addr common.Address) (libevm.PrecompiledContract, bool) { - return registeredExtras.hooksFromRules(&r).PrecompileOverride(r, addr) + return hooksFromRules(&r).PrecompileOverride(r, addr) } -// NoopHooks implement both [ChainConfigHooks] and [RulesHooks] such that every -// hook is a no-op. This allows it to be returned instead of nil, which would -// require every usage site to perform a nil check. It can also be embedded in -// structs that only wish to implement a sub-set of hooks. -type NoopHooks struct{} +// NOOPHooks implements both [ChainConfigHooks] and [RulesHooks] such that every +// hook is a no-op. This allows it to be returned instead of a nil interface, +// which would otherwise require every usage site to perform a nil check. It can +// also be embedded in structs that only wish to implement a sub-set of hooks. +// Use of a NOOPHooks is equivalent to default Ethereum behaviour. +type NOOPHooks struct{} var _ interface { ChainConfigHooks RulesHooks -} = NoopHooks{} +} = NOOPHooks{} -// PrecompileOverride always returns `nil, false`; the `false` indicates that -// the `nil` should be ignored. -func (NoopHooks) PrecompileOverride(Rules, common.Address) (libevm.PrecompiledContract, bool) { +// PrecompileOverride instructs the EVM interpreter to use the default +// precompile behaviour. +func (NOOPHooks) PrecompileOverride(Rules, common.Address) (libevm.PrecompiledContract, bool) { return nil, false } From 9dee6b288edb0dfc7acb1ef405387cf6758768d4 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Tue, 3 Sep 2024 21:16:50 +0100 Subject: [PATCH 14/24] refactor: `ChainConfig.Hooks()` + `Rules` equivalent --- core/vm/evm.go | 2 +- core/vm/precompiles.libevm_test.go | 2 +- params/config.libevm.go | 6 ++++-- params/config.libevm_test.go | 8 ++++++++ params/example.libevm_test.go | 4 ++-- params/hooks.libevm.go | 24 ++++++++---------------- 6 files changed, 24 insertions(+), 22 deletions(-) diff --git a/core/vm/evm.go b/core/vm/evm.go index 35a0aa227cb..6cabfccd94c 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -38,7 +38,7 @@ type ( ) func (evm *EVM) precompile(addr common.Address) (PrecompiledContract, bool) { - if p, override := evm.chainRules.PrecompileOverride(addr); override { + if p, override := evm.chainRules.Hooks().PrecompileOverride(addr); override { return p, p != nil } var precompiles map[common.Address]PrecompiledContract diff --git a/core/vm/precompiles.libevm_test.go b/core/vm/precompiles.libevm_test.go index 7f6a5405ca3..b0b27616282 100644 --- a/core/vm/precompiles.libevm_test.go +++ b/core/vm/precompiles.libevm_test.go @@ -23,7 +23,7 @@ type precompileOverrides struct { params.NOOPHooks // all other hooks } -func (o precompileOverrides) PrecompileOverride(_ params.Rules, a common.Address) (libevm.PrecompiledContract, bool) { +func (o precompileOverrides) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, bool) { c, ok := o.contracts[a] return c, ok } diff --git a/params/config.libevm.go b/params/config.libevm.go index 9ddc00359e4..d9312b528fe 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -156,8 +156,10 @@ func (c *ChainConfig) UnmarshalJSON(data []byte) error { *raw Extra *pseudo.Type `json:"extra"` }{ - raw: (*raw)(c), // embedded to achieve regular JSON unmarshalling - Extra: registeredExtras.chainConfig.NilPointer(), // `c.extra` is otherwise unexported + raw: (*raw)(c), // embedded to achieve regular JSON unmarshalling + } + if e := registeredExtras; e != nil { + cc.Extra = e.chainConfig.NilPointer() // `c.extra` is otherwise unexported } if err := json.Unmarshal(data, cc); err != nil { diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 83854c30442..9b17cb42879 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -12,6 +12,7 @@ import ( type rawJSON struct { json.RawMessage + NOOPHooks } var _ interface { @@ -130,6 +131,13 @@ func TestExtrasPanic(t *testing.T) { "before RegisterExtras", ) + assertPanics( + t, func() { + mustBeStruct[int]() + }, + notStructMessage[int](), + ) + RegisterExtras(Extras[struct{ ChainConfigHooks }, struct{ RulesHooks }]{}) assertPanics( diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index e779da4ddfa..0d91f70531a 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -75,8 +75,8 @@ var myForkPrecompiledContracts = map[common.Address]vm.PrecompiledContract{ } // PrecompileOverride implements the required [params.RuleHooks] method. -func (rex RulesExtra) PrecompileOverride(_ params.Rules, addr common.Address) (_ libevm.PrecompiledContract, override bool) { - if !rex.IsMyFork { +func (r RulesExtra) PrecompileOverride(addr common.Address) (_ libevm.PrecompiledContract, override bool) { + if !r.IsMyFork { return nil, false } p, ok := myForkPrecompiledContracts[addr] diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go index 4133f14815d..54b58a798f6 100644 --- a/params/hooks.libevm.go +++ b/params/hooks.libevm.go @@ -18,35 +18,27 @@ type RulesHooks interface { // interpreter will treat the address as a precompile i.f.f the // [PrecompiledContract] is non-nil. If it returns `false` then the default // precompile behaviour is honoured. - PrecompileOverride(Rules, common.Address) (_ libevm.PrecompiledContract, override bool) + PrecompileOverride(common.Address) (_ libevm.PrecompiledContract, override bool) } -// hooksFromChainConfig returns the registered hooks, or [NOOPHooks] if none -// were registered. -func hooksFromChainConfig(c *ChainConfig) ChainConfigHooks { +// Hooks returns the hooks registered with [RegisterExtras], or [NOOPHooks] if +// none were registered. +func (c *ChainConfig) Hooks() ChainConfigHooks { if e := registeredExtras; e != nil { return e.getter.hooksFromChainConfig(c) } return NOOPHooks{} } -// hooksFromRules returns the registered hooks, or [NOOPHooks] if none were -// registered. -func hooksFromRules(r *Rules) RulesHooks { +// Hooks returns the hooks registered with [RegisterExtras], or [NOOPHooks] if +// none were registered. +func (r *Rules) Hooks() RulesHooks { if e := registeredExtras; e != nil { return e.getter.hooksFromRules(r) } return NOOPHooks{} } -// PrecompileOverride is a wrapper for the equivalent method of the registered -// [RulesHooks] carried by the Rules. If no [Extras] were registered, a -// [NOOPHooks] is used instead, such that the default precompile behaviour is -// exhibited. -func (r Rules) PrecompileOverride(addr common.Address) (libevm.PrecompiledContract, bool) { - return hooksFromRules(&r).PrecompileOverride(r, addr) -} - // NOOPHooks implements both [ChainConfigHooks] and [RulesHooks] such that every // hook is a no-op. This allows it to be returned instead of a nil interface, // which would otherwise require every usage site to perform a nil check. It can @@ -61,6 +53,6 @@ var _ interface { // PrecompileOverride instructs the EVM interpreter to use the default // precompile behaviour. -func (NOOPHooks) PrecompileOverride(Rules, common.Address) (libevm.PrecompiledContract, bool) { +func (NOOPHooks) PrecompileOverride(common.Address) (libevm.PrecompiledContract, bool) { return nil, false } From 639aa1cadf528ad9c46975b0a8f91268d6aad372 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Tue, 3 Sep 2024 21:18:43 +0100 Subject: [PATCH 15/24] chore: rename precompiles test file in keeping with geth equivalent --- core/vm/{precompiles.libevm_test.go => contracts.libevm_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename core/vm/{precompiles.libevm_test.go => contracts.libevm_test.go} (100%) diff --git a/core/vm/precompiles.libevm_test.go b/core/vm/contracts.libevm_test.go similarity index 100% rename from core/vm/precompiles.libevm_test.go rename to core/vm/contracts.libevm_test.go From b6427950b2e0067f5a9eeb30ec2f6028c7decb7e Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Tue, 3 Sep 2024 21:30:45 +0100 Subject: [PATCH 16/24] feat: stateful precompiles + allowlist hooks The allowlist hooks are included in this commit because they allow for the same functionality as stateful precompiles in `ava-labs/coreth` and `ava-labs/subnet-evm`. --- core/state_transition.go | 3 ++ core/state_transition.libevm.go | 7 +++ core/vm/contracts.go | 4 +- core/vm/contracts.libevm.go | 77 ++++++++++++++++++++++++++++++++ core/vm/contracts.libevm_test.go | 4 ++ core/vm/evm.go | 15 +++++-- libevm/interfaces_test.go | 8 +++- libevm/libevm.go | 32 +++++++++++++ params/example.libevm_test.go | 31 ++++++++++--- params/hooks.libevm.go | 17 +++++++ 10 files changed, 184 insertions(+), 14 deletions(-) create mode 100644 core/state_transition.libevm.go create mode 100644 core/vm/contracts.libevm.go diff --git a/core/state_transition.go b/core/state_transition.go index 9c4f76d1c58..0be28b4a4e1 100644 --- a/core/state_transition.go +++ b/core/state_transition.go @@ -365,6 +365,9 @@ func (st *StateTransition) preCheck() error { // However if any consensus issue encountered, return the error directly with // nil evm execution result. func (st *StateTransition) TransitionDb() (*ExecutionResult, error) { + if err := st.canExecuteTransaction(); err != nil { + return nil, err + } // First check this message satisfies all consensus rules before // applying the message. The rules include these clauses // diff --git a/core/state_transition.libevm.go b/core/state_transition.libevm.go new file mode 100644 index 00000000000..0057e93e7ee --- /dev/null +++ b/core/state_transition.libevm.go @@ -0,0 +1,7 @@ +package core + +func (st *StateTransition) canExecuteTransaction() error { + bCtx := st.evm.Context + rules := st.evm.ChainConfig().Rules(bCtx.BlockNumber, bCtx.Random != nil, bCtx.Time) + return rules.Hooks().CanExecuteTransaction(st.msg.From, st.msg.From, st.state) +} diff --git a/core/vm/contracts.go b/core/vm/contracts.go index 33a867654e7..ee1308fa708 100644 --- a/core/vm/contracts.go +++ b/core/vm/contracts.go @@ -168,13 +168,13 @@ func ActivePrecompiles(rules params.Rules) []common.Address { // - the returned bytes, // - the _remaining_ gas, // - any error that occurred -func RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) { +func (args *evmCallArgs) RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) { gasCost := p.RequiredGas(input) if suppliedGas < gasCost { return nil, 0, ErrOutOfGas } suppliedGas -= gasCost - output, err := p.Run(input) + output, err := args.run(p, input) return output, suppliedGas, err } diff --git a/core/vm/contracts.libevm.go b/core/vm/contracts.libevm.go new file mode 100644 index 00000000000..95a98588fb7 --- /dev/null +++ b/core/vm/contracts.libevm.go @@ -0,0 +1,77 @@ +package vm + +import ( + "fmt" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" +) + +// evmCallArgs mirrors the parameters of the [EVM] methods Call(), CallCode(), +// DelegateCall() and StaticCall(). Its fields are identical to those of the +// parameters, prepended with the receiver name. As {Delegate,Static}Call don't +// accept a value, they MUST set the respective field to nil. +// +// Instantiation can be achieved by merely copying the parameter names, in +// order, which is trivially achieved with AST manipulation: +// +// func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas uint64, value *uint256.Int) ... { +// ... +// args := &evmCallArgs{evm, caller, addr, input, gas, value} +type evmCallArgs struct { + evm *EVM + caller ContractRef + addr common.Address + input []byte + gas uint64 + value *uint256.Int +} + +func (args *evmCallArgs) run(p PrecompiledContract, input []byte) (ret []byte, err error) { + if p, ok := p.(statefulPrecompile); ok { + return p.run(args.evm.StateDB, &args.evm.chainRules, args.caller.Address(), args.addr, input) + } + return p.Run(input) +} + +// A StatefulPrecompileFunc... +type StatefulPrecompfileFunc func(_ StateDB, _ *params.Rules, caller, self common.Address, input []byte) ([]byte, error) + +// NewStatefulPrecompile... +func NewStatefulPrecompile(run StatefulPrecompfileFunc, requiredGas func([]byte) uint64) PrecompiledContract { + return statefulPrecompile{ + gas: requiredGas, + run: run, + } +} + +type statefulPrecompile struct { + gas func([]byte) uint64 + run StatefulPrecompfileFunc +} + +func (p statefulPrecompile) RequiredGas(input []byte) uint64 { + return p.gas(input) +} + +func (p statefulPrecompile) Run([]byte) ([]byte, error) { + // https://google.github.io/styleguide/go/best-practices.html#when-to-panic + // This would indicate an API misuse and would occur in tests, not in + // production. + panic(fmt.Sprintf("BUG: call to %T.Run(); MUST call %T", p, p.run)) +} + +var ( + // These lock in the assumptions made when implementing [evmCallArgs]. If + // these break then the struct fields SHOULD be changed to match these + // signatures. + _ = [](func(ContractRef, common.Address, []byte, uint64, *uint256.Int) ([]byte, uint64, error)){ + (*EVM)(nil).Call, + (*EVM)(nil).CallCode, + } + _ = [](func(ContractRef, common.Address, []byte, uint64) ([]byte, uint64, error)){ + (*EVM)(nil).DelegateCall, + (*EVM)(nil).StaticCall, + } +) diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index b0b27616282..96c322012eb 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -16,6 +16,10 @@ import ( "golang.org/x/exp/rand" ) +func RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) { + return (*evmCallArgs)(nil).RunPrecompiledContract(p, input, suppliedGas) +} + // precompileOverrides is a [params.RulesHooks] that overrides precompiles from // a map of predefined addresses. type precompileOverrides struct { diff --git a/core/vm/evm.go b/core/vm/evm.go index 6cabfccd94c..e07e32a1291 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -227,7 +227,8 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas } if isPrecompile { - ret, gas, err = RunPrecompiledContract(p, input, gas) + args := &evmCallArgs{evm, caller, addr, input, gas, value} + ret, gas, err = args.RunPrecompiledContract(p, input, gas) } else { // Initialise a new contract and set the code that is to be used by the EVM. // The contract is a scoped environment for this execution context only. @@ -290,7 +291,8 @@ func (evm *EVM) CallCode(caller ContractRef, addr common.Address, input []byte, // It is allowed to call precompiles, even via delegatecall if p, isPrecompile := evm.precompile(addr); isPrecompile { - ret, gas, err = RunPrecompiledContract(p, input, gas) + args := &evmCallArgs{evm, caller, addr, input, gas, value} + ret, gas, err = args.RunPrecompiledContract(p, input, gas) } else { addrCopy := addr // Initialise a new contract and set the code that is to be used by the EVM. @@ -335,7 +337,8 @@ func (evm *EVM) DelegateCall(caller ContractRef, addr common.Address, input []by // It is allowed to call precompiles, even via delegatecall if p, isPrecompile := evm.precompile(addr); isPrecompile { - ret, gas, err = RunPrecompiledContract(p, input, gas) + args := &evmCallArgs{evm, caller, addr, input, gas, nil} + ret, gas, err = args.RunPrecompiledContract(p, input, gas) } else { addrCopy := addr // Initialise a new contract and make initialise the delegate values @@ -384,7 +387,8 @@ func (evm *EVM) StaticCall(caller ContractRef, addr common.Address, input []byte } if p, isPrecompile := evm.precompile(addr); isPrecompile { - ret, gas, err = RunPrecompiledContract(p, input, gas) + args := &evmCallArgs{evm, caller, addr, input, gas, nil} + ret, gas, err = args.RunPrecompiledContract(p, input, gas) } else { // At this point, we use a copy of address. If we don't, the go compiler will // leak the 'contract' to the outer scope, and make allocation for 'contract' @@ -423,6 +427,9 @@ func (c *codeAndHash) Hash() common.Hash { // create creates a new contract using code as deployment code. func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, value *uint256.Int, address common.Address, typ OpCode) ([]byte, common.Address, uint64, error) { + if err := evm.chainRules.Hooks().CanCreateContract(caller.Address(), evm.Origin, evm.StateDB); err != nil { + return nil, common.Address{}, gas, err + } // Depth check execution. Fail if we're trying to execute above the // limit. if evm.depth > int(params.CallCreateDepth) { diff --git a/libevm/interfaces_test.go b/libevm/interfaces_test.go index 13691be27eb..05497ed40ab 100644 --- a/libevm/interfaces_test.go +++ b/libevm/interfaces_test.go @@ -5,8 +5,9 @@ import ( "github.com/ethereum/go-ethereum/libevm" ) -// These two interfaces MUST be identical. If this breaks then the libevm copy -// MUST be updated. +// IMPORTANT: if any of these break then the libevm copy MUST be updated. + +// These two interfaces MUST be identical. var ( // Each assignment demonstrates that the methods of the LHS interface are a // (non-strict) subset of the RHS interface's; both being possible @@ -14,3 +15,6 @@ var ( _ vm.PrecompiledContract = (libevm.PrecompiledContract)(nil) _ libevm.PrecompiledContract = (vm.PrecompiledContract)(nil) ) + +// StateReader MUST be a subset vm.StateDB. +var _ libevm.StateReader = (vm.StateDB)(nil) diff --git a/libevm/libevm.go b/libevm/libevm.go index edb54cc3c00..c1a563416d6 100644 --- a/libevm/libevm.go +++ b/libevm/libevm.go @@ -1,5 +1,10 @@ package libevm +import ( + "github.com/ethereum/go-ethereum/common" + "github.com/holiman/uint256" +) + // PrecompiledContract is an exact copy of vm.PrecompiledContract, mirrored here // for instances where importing that package would result in a circular // dependency. @@ -7,3 +12,30 @@ type PrecompiledContract interface { RequiredGas(input []byte) uint64 Run(input []byte) ([]byte, error) } + +// StateReader is a subset of vm.StateDB, exposing only methods that read from +// but do not modify state. See method comments in vm.StateDB, which aren't +// copied here as they risk becoming outdated. +type StateReader interface { + GetBalance(common.Address) *uint256.Int + GetNonce(common.Address) uint64 + + GetCodeHash(common.Address) common.Hash + GetCode(common.Address) []byte + GetCodeSize(common.Address) int + + GetRefund() uint64 + + GetCommittedState(common.Address, common.Hash) common.Hash + GetState(common.Address, common.Hash) common.Hash + + GetTransientState(addr common.Address, key common.Hash) common.Hash + + HasSelfDestructed(common.Address) bool + + Exist(common.Address) bool + Empty(common.Address) bool + + AddressInAccessList(addr common.Address) bool + SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) +} diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index 0d91f70531a..b8e7d37562f 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -11,9 +11,11 @@ package params_test import ( "encoding/json" + "errors" "fmt" "log" "math/big" + "time" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/vm" @@ -40,7 +42,8 @@ var getter params.ExtraPayloadGetter[ChainConfigExtra, RulesExtra] // [params.Rules] but it MAY also modify the [params.Rules]. func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx *ChainConfigExtra, blockNum *big.Int, isMerge bool, timestamp uint64) *RulesExtra { return &RulesExtra{ - IsMyFork: cEx.MyForkTime != nil && *cEx.MyForkTime <= timestamp, + IsMyFork: cEx.MyForkTime != nil && *cEx.MyForkTime <= timestamp, + timestamp: timestamp, } } @@ -53,7 +56,13 @@ type ChainConfigExtra struct { // RulesExtra can be any struct. It too mirrors a common pattern in // [params.Rules]. type RulesExtra struct { - IsMyFork bool + IsMyFork bool + timestamp uint64 + + // (Optional) If not all hooks are desirable then embedding a [NOOPHooks] + // allows the type to satisfy the [RulesHooks] interface, resulting in + // default Ethereum behaviour. + params.NOOPHooks } // FromChainConfig returns the extra payload carried by the ChainConfig. @@ -93,6 +102,16 @@ func (r RulesExtra) PrecompileOverride(addr common.Address) (_ libevm.Precompile return p, ok } +// CanCreateContract implements the required [params.RuleHooks] method. Access +// to state allows it to be configured on-chain however this is an optional +// implementation detail. +func (r RulesExtra) CanCreateContract(caller, origin common.Address, _ libevm.StateReader) error { + if time.Unix(int64(r.timestamp), 0).UTC().Day() != int(time.Tuesday) { + return errors.New("uh oh!") + } + return nil +} + // This example demonstrates how the rest of this file would be used from a // *different* package. func ExampleExtraPayloadGetter() { @@ -126,14 +145,14 @@ func ExampleExtraPayloadGetter() { rules := config.Rules(nil, false, time) rExtra := FromRules(&rules) // extraparams.FromRules() in practice if rExtra != nil { - fmt.Printf("%+v\n", rExtra) + fmt.Printf("IsMyFork at %v: %t\n", rExtra.timestamp, rExtra.IsMyFork) } } // Output: // Chain ID 1234 // Fork time 530003640 - // &{IsMyFork:false} - // &{IsMyFork:true} - // &{IsMyFork:true} + // IsMyFork at 530003639: false + // IsMyFork at 530003640: true + // IsMyFork at 530003641: true } diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go index 54b58a798f6..3ea88bbce27 100644 --- a/params/hooks.libevm.go +++ b/params/hooks.libevm.go @@ -9,9 +9,18 @@ import ( // [ChainConfig] payloads. type ChainConfigHooks interface{} +// TODO(arr4n): given the choice of whether a hook should be defined on a +// ChainConfig or on the Rules, what are the guiding principles? A ChainConfig +// carries the most general information while Rules benefit from "knowing" the +// block number and timestamp. I am leaning towards the default choice being +// on Rules (as it's trivial to copy information from ChainConfig to Rules in +// [Extras.NewRules]) unless the call site only has access to a ChainConfig. + // RulesHooks are required for all types registered as [Extras] for [Rules] // payloads. type RulesHooks interface { + CanExecuteTransaction(from, to common.Address, _ libevm.StateReader) error + CanCreateContract(caller, origin common.Address, _ libevm.StateReader) error // PrecompileOverride signals whether or not the EVM interpreter MUST // override its treatment of the address when deciding if it is a // precompiled contract. If PrecompileOverride returns `true` then the @@ -51,6 +60,14 @@ var _ interface { RulesHooks } = NOOPHooks{} +func (NOOPHooks) CanExecuteTransaction(_, _ common.Address, _ libevm.StateReader) error { + return nil +} + +func (NOOPHooks) CanCreateContract(_, _ common.Address, _ libevm.StateReader) error { + return nil +} + // PrecompileOverride instructs the EVM interpreter to use the default // precompile behaviour. func (NOOPHooks) PrecompileOverride(common.Address) (libevm.PrecompiledContract, bool) { From 2867af10b87ecf3b199ee7d46fbc7060218f7c6f Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Tue, 3 Sep 2024 21:37:53 +0100 Subject: [PATCH 17/24] fix: `StateTransition.canExecuteTransaction()` used `msg.From` instead of `To` --- core/state_transition.libevm.go | 2 +- params/hooks.libevm.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/state_transition.libevm.go b/core/state_transition.libevm.go index 0057e93e7ee..c76f2cefc10 100644 --- a/core/state_transition.libevm.go +++ b/core/state_transition.libevm.go @@ -3,5 +3,5 @@ package core func (st *StateTransition) canExecuteTransaction() error { bCtx := st.evm.Context rules := st.evm.ChainConfig().Rules(bCtx.BlockNumber, bCtx.Random != nil, bCtx.Time) - return rules.Hooks().CanExecuteTransaction(st.msg.From, st.msg.From, st.state) + return rules.Hooks().CanExecuteTransaction(st.msg.From, st.msg.To, st.state) } diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go index 3ea88bbce27..c4c6e188f5e 100644 --- a/params/hooks.libevm.go +++ b/params/hooks.libevm.go @@ -19,7 +19,7 @@ type ChainConfigHooks interface{} // RulesHooks are required for all types registered as [Extras] for [Rules] // payloads. type RulesHooks interface { - CanExecuteTransaction(from, to common.Address, _ libevm.StateReader) error + CanExecuteTransaction(from common.Address, to *common.Address, _ libevm.StateReader) error CanCreateContract(caller, origin common.Address, _ libevm.StateReader) error // PrecompileOverride signals whether or not the EVM interpreter MUST // override its treatment of the address when deciding if it is a @@ -60,7 +60,7 @@ var _ interface { RulesHooks } = NOOPHooks{} -func (NOOPHooks) CanExecuteTransaction(_, _ common.Address, _ libevm.StateReader) error { +func (NOOPHooks) CanExecuteTransaction(_ common.Address, _ *common.Address, _ libevm.StateReader) error { return nil } From 5d63223c5facd9e098e4fe70e5e655c2dba5a941 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Wed, 4 Sep 2024 18:45:57 +0100 Subject: [PATCH 18/24] test: `params.RulesHooks.CanCreateContract` integration --- core/vm/contracts.libevm_test.go | 138 ++++++++++++++++++++++++------- core/vm/evm.go | 4 +- libevm/hookstest/stub.go | 52 ++++++++++++ libevm/libevm.go | 4 + params/example.libevm_test.go | 2 +- params/hooks.libevm.go | 4 +- 6 files changed, 171 insertions(+), 33 deletions(-) create mode 100644 libevm/hookstest/stub.go diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index 96c322012eb..fbed763c473 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -2,13 +2,14 @@ package vm import ( "fmt" - "math/big" "testing" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/state" + "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/libevm" + "github.com/ethereum/go-ethereum/libevm/hookstest" "github.com/ethereum/go-ethereum/params" "github.com/holiman/uint256" "github.com/stretchr/testify/assert" @@ -16,24 +17,12 @@ import ( "golang.org/x/exp/rand" ) +// The original RunPrecompiledContract was migrated to being a method on +// [evmCallArgs]. We need to replace it for use by regular geth tests. func RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) { return (*evmCallArgs)(nil).RunPrecompiledContract(p, input, suppliedGas) } -// precompileOverrides is a [params.RulesHooks] that overrides precompiles from -// a map of predefined addresses. -type precompileOverrides struct { - contracts map[common.Address]PrecompiledContract - params.NOOPHooks // all other hooks -} - -func (o precompileOverrides) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, bool) { - c, ok := o.contracts[a] - return c, ok -} - -// A precompileStub is a [PrecompiledContract] that always returns the same -// values. type precompileStub struct { requiredGas uint64 returnData []byte @@ -73,21 +62,16 @@ func TestPrecompileOverride(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - precompile := &precompileStub{ - requiredGas: tt.requiredGas, - returnData: tt.stubData, + hooks := &hookstest.Stub{ + PrecompileOverrides: map[common.Address]libevm.PrecompiledContract{ + tt.addr: &precompileStub{ + requiredGas: tt.requiredGas, + returnData: tt.stubData, + }, + }, } - params.TestOnlyClearRegisteredExtras() - params.RegisterExtras(params.Extras[params.NOOPHooks, precompileOverrides]{ - NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *params.NOOPHooks, blockNum *big.Int, isMerge bool, timestamp uint64) *precompileOverrides { - return &precompileOverrides{ - contracts: map[common.Address]PrecompiledContract{ - tt.addr: precompile, - }, - } - }, - }) + hooks.RegisterForRules() t.Run(fmt.Sprintf("%T.Call([overridden precompile address = %v])", &EVM{}, tt.addr), func(t *testing.T) { gotData, gotGasLeft, err := newEVM(t).Call(AccountRef{}, tt.addr, nil, gasLimit, uint256.NewInt(0)) @@ -99,6 +83,101 @@ func TestPrecompileOverride(t *testing.T) { } } +func TestCanCreateContract(t *testing.T) { + // We need to prove end-to-end plumbing of contract-creation addresses, + // state, and any returned error. We therefore condition an error on a state + // value being set, and that error contains the addresses. + makeErr := func(cc *libevm.ContractCreation) error { + return fmt.Errorf("Origin: %v Caller: %v Contract: %v", cc.Origin, cc.Caller, cc.Contract) + } + slot := common.Hash(crypto.Keccak256([]byte("slot"))) + value := common.Hash(crypto.Keccak256([]byte("value"))) + hooks := &hookstest.Stub{ + CanCreateContractFn: func(cc *libevm.ContractCreation, s libevm.StateReader) error { + if s.GetState(common.Address{}, slot).Cmp(value) != 0 { + return makeErr(cc) + } + return nil + }, + } + params.TestOnlyClearRegisteredExtras() + hooks.RegisterForRules() + + origin := common.Address{'o', 'r', 'i', 'g', 'i', 'n'} + caller := common.Address{'c', 'a', 'l', 'l', 'e', 'r'} + create := crypto.CreateAddress(caller, 0) + var ( + code []byte + salt [32]byte + ) + create2 := crypto.CreateAddress2(caller, salt, crypto.Keccak256(code)) + + tests := []struct { + name string + setState bool + wantCreateErr, wantCreate2Err error + }{ + { + name: "no state => return error", + setState: false, + wantCreateErr: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create}), + wantCreate2Err: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create2}), + }, + { + name: "state set => no error", + setState: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evm := newEVM(t) + evm.TxContext.Origin = origin + + if tt.setState { + sdb := evm.StateDB.(*state.StateDB) + sdb.SetState(common.Address{}, slot, value) + } + + methods := []struct { + name string + deploy func() ([]byte, common.Address, uint64, error) + wantErr error + wantAddr common.Address + }{ + { + name: "Create", + deploy: func() ([]byte, common.Address, uint64, error) { + return evm.Create(AccountRef(caller), code, 1e6, uint256.NewInt(0)) + }, + wantErr: tt.wantCreateErr, + wantAddr: create, + }, + { + name: "Create2", + deploy: func() ([]byte, common.Address, uint64, error) { + return evm.Create2(AccountRef(caller), code, 1e6, uint256.NewInt(0), new(uint256.Int).SetBytes(salt[:])) + }, + wantErr: tt.wantCreate2Err, + wantAddr: create2, + }, + } + + for _, m := range methods { + t.Run(m.name, func(t *testing.T) { + _, gotAddr, _, err := m.deploy() + if want := m.wantErr; want == nil { + require.NoError(t, err) + assert.Equal(t, m.wantAddr, gotAddr) + } else { + require.EqualError(t, err, want.Error()) + } + }) + } + }) + } +} + func newEVM(t *testing.T) *EVM { t.Helper() @@ -107,7 +186,8 @@ func newEVM(t *testing.T) *EVM { return NewEVM( BlockContext{ - Transfer: func(_ StateDB, _, _ common.Address, _ *uint256.Int) {}, + CanTransfer: func(_ StateDB, _ common.Address, _ *uint256.Int) bool { return true }, + Transfer: func(_ StateDB, _, _ common.Address, _ *uint256.Int) {}, }, TxContext{}, sdb, diff --git a/core/vm/evm.go b/core/vm/evm.go index e07e32a1291..4db904dbc84 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -23,6 +23,7 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/libevm" "github.com/ethereum/go-ethereum/params" "github.com/holiman/uint256" ) @@ -427,7 +428,8 @@ func (c *codeAndHash) Hash() common.Hash { // create creates a new contract using code as deployment code. func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, value *uint256.Int, address common.Address, typ OpCode) ([]byte, common.Address, uint64, error) { - if err := evm.chainRules.Hooks().CanCreateContract(caller.Address(), evm.Origin, evm.StateDB); err != nil { + cc := &libevm.ContractCreation{Origin: evm.Origin, Caller: caller.Address(), Contract: address} + if err := evm.chainRules.Hooks().CanCreateContract(cc, evm.StateDB); err != nil { return nil, common.Address{}, gas, err } // Depth check execution. Fail if we're trying to execute above the diff --git a/libevm/hookstest/stub.go b/libevm/hookstest/stub.go new file mode 100644 index 00000000000..58ebd55eefd --- /dev/null +++ b/libevm/hookstest/stub.go @@ -0,0 +1,52 @@ +package hookstest + +import ( + "math/big" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/libevm" + "github.com/ethereum/go-ethereum/params" +) + +// A Stub is a test double for [params.ChainConfigHooks] and +// [params.RulesHooks]. +type Stub struct { + PrecompileOverrides map[common.Address]libevm.PrecompiledContract + CanExecuteTransactionFn func(common.Address, *common.Address, libevm.StateReader) error + CanCreateContractFn func(*libevm.ContractCreation, libevm.StateReader) error +} + +func (s *Stub) RegisterForRules() { + params.RegisterExtras(params.Extras[params.NOOPHooks, Stub]{ + NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *params.NOOPHooks, blockNum *big.Int, isMerge bool, timestamp uint64) *Stub { + return s + }, + }) +} + +func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, bool) { + if len(s.PrecompileOverrides) == 0 { + return nil, false + } + p, ok := s.PrecompileOverrides[a] + return p, ok +} + +func (s Stub) CanExecuteTransaction(from common.Address, to *common.Address, sr libevm.StateReader) error { + if f := s.CanExecuteTransactionFn; f != nil { + return f(from, to, sr) + } + return nil +} + +func (s Stub) CanCreateContract(cc *libevm.ContractCreation, sr libevm.StateReader) error { + if f := s.CanCreateContractFn; f != nil { + return f(cc, sr) + } + return nil +} + +var _ interface { + params.ChainConfigHooks + params.RulesHooks +} = Stub{} diff --git a/libevm/libevm.go b/libevm/libevm.go index c1a563416d6..ac669e71a56 100644 --- a/libevm/libevm.go +++ b/libevm/libevm.go @@ -39,3 +39,7 @@ type StateReader interface { AddressInAccessList(addr common.Address) bool SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) } + +type ContractCreation struct { + Origin, Caller, Contract common.Address +} diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index b8e7d37562f..ab323306e7d 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -105,7 +105,7 @@ func (r RulesExtra) PrecompileOverride(addr common.Address) (_ libevm.Precompile // CanCreateContract implements the required [params.RuleHooks] method. Access // to state allows it to be configured on-chain however this is an optional // implementation detail. -func (r RulesExtra) CanCreateContract(caller, origin common.Address, _ libevm.StateReader) error { +func (r RulesExtra) CanCreateContract(*libevm.ContractCreation, libevm.StateReader) error { if time.Unix(int64(r.timestamp), 0).UTC().Day() != int(time.Tuesday) { return errors.New("uh oh!") } diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go index c4c6e188f5e..13085d6f4eb 100644 --- a/params/hooks.libevm.go +++ b/params/hooks.libevm.go @@ -20,7 +20,7 @@ type ChainConfigHooks interface{} // payloads. type RulesHooks interface { CanExecuteTransaction(from common.Address, to *common.Address, _ libevm.StateReader) error - CanCreateContract(caller, origin common.Address, _ libevm.StateReader) error + CanCreateContract(*libevm.ContractCreation, libevm.StateReader) error // PrecompileOverride signals whether or not the EVM interpreter MUST // override its treatment of the address when deciding if it is a // precompiled contract. If PrecompileOverride returns `true` then the @@ -64,7 +64,7 @@ func (NOOPHooks) CanExecuteTransaction(_ common.Address, _ *common.Address, _ li return nil } -func (NOOPHooks) CanCreateContract(_, _ common.Address, _ libevm.StateReader) error { +func (NOOPHooks) CanCreateContract(*libevm.ContractCreation, libevm.StateReader) error { return nil } From 85a3258191491bf76e7a0b520e0639ceb7f2e2c3 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 5 Sep 2024 10:00:01 +0100 Subject: [PATCH 19/24] test: `params.RulesHooks.CanExecuteTransaction` integration --- core/state_transition.libevm_test.go | 49 ++++++++++++++++++++++++++++ core/vm/contracts.libevm_test.go | 47 +++++++------------------- core/vm/libevm_test.go | 7 ++++ libevm/ethtest/evm.go | 31 ++++++++++++++++++ params/config.libevm.go | 18 +++++++--- 5 files changed, 112 insertions(+), 40 deletions(-) create mode 100644 core/state_transition.libevm_test.go create mode 100644 core/vm/libevm_test.go create mode 100644 libevm/ethtest/evm.go diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go new file mode 100644 index 00000000000..c92de89789d --- /dev/null +++ b/core/state_transition.libevm_test.go @@ -0,0 +1,49 @@ +package core + +import ( + "fmt" + "math/rand" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/libevm" + "github.com/ethereum/go-ethereum/libevm/ethtest" + "github.com/ethereum/go-ethereum/libevm/hookstest" + "github.com/ethereum/go-ethereum/params" + "github.com/stretchr/testify/require" +) + +func TestCanExecuteTransaction(t *testing.T) { + rng := rand.New(rand.NewSource(42)) + var ( + from, to common.Address + account common.Address + slot, value common.Hash + ) + rng.Read(from[:]) + rng.Read(to[:]) + rng.Read(account[:]) + rng.Read(slot[:]) + rng.Read(value[:]) + + makeErr := func(from common.Address, to *common.Address, val common.Hash) error { + return fmt.Errorf("From: %v To: %v State: %v", from, to, val) + } + hooks := &hookstest.Stub{ + CanExecuteTransactionFn: func(from common.Address, to *common.Address, s libevm.StateReader) error { + return makeErr(from, to, s.GetState(account, slot)) + }, + } + params.TestOnlyClearRegisteredExtras() + hooks.RegisterForRules() + t.Cleanup(params.TestOnlyClearRegisteredExtras) + + state, evm := ethtest.NewZeroEVM(t) + state.SetState(account, slot, value) + msg := &Message{ + From: from, + To: &to, + } + _, err := ApplyMessage(evm, msg, new(GasPool).AddGas(30e6)) + require.EqualError(t, err, makeErr(from, &to, value).Error()) +} diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index fbed763c473..0e35c5c922f 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -1,14 +1,14 @@ -package vm +package vm_test import ( "fmt" "testing" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/core/rawdb" - "github.com/ethereum/go-ethereum/core/state" + "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/libevm" + "github.com/ethereum/go-ethereum/libevm/ethtest" "github.com/ethereum/go-ethereum/libevm/hookstest" "github.com/ethereum/go-ethereum/params" "github.com/holiman/uint256" @@ -17,12 +17,6 @@ import ( "golang.org/x/exp/rand" ) -// The original RunPrecompiledContract was migrated to being a method on -// [evmCallArgs]. We need to replace it for use by regular geth tests. -func RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) { - return (*evmCallArgs)(nil).RunPrecompiledContract(p, input, suppliedGas) -} - type precompileStub struct { requiredGas uint64 returnData []byte @@ -51,7 +45,7 @@ func TestPrecompileOverride(t *testing.T) { } rng := rand.New(rand.NewSource(42)) - for _, addr := range PrecompiledAddressesCancun { + for _, addr := range vm.PrecompiledAddressesCancun { tests = append(tests, test{ name: fmt.Sprintf("existing precompile %v", addr), addr: addr, @@ -73,8 +67,9 @@ func TestPrecompileOverride(t *testing.T) { params.TestOnlyClearRegisteredExtras() hooks.RegisterForRules() - t.Run(fmt.Sprintf("%T.Call([overridden precompile address = %v])", &EVM{}, tt.addr), func(t *testing.T) { - gotData, gotGasLeft, err := newEVM(t).Call(AccountRef{}, tt.addr, nil, gasLimit, uint256.NewInt(0)) + t.Run(fmt.Sprintf("%T.Call([overridden precompile address = %v])", &vm.EVM{}, tt.addr), func(t *testing.T) { + _, evm := ethtest.NewZeroEVM(t) + gotData, gotGasLeft, err := evm.Call(vm.AccountRef{}, tt.addr, nil, gasLimit, uint256.NewInt(0)) require.NoError(t, err) assert.Equal(t, tt.stubData, gotData, "contract's return data") assert.Equal(t, gasLimit-tt.requiredGas, gotGasLeft, "gas left") @@ -102,6 +97,7 @@ func TestCanCreateContract(t *testing.T) { } params.TestOnlyClearRegisteredExtras() hooks.RegisterForRules() + t.Cleanup(params.TestOnlyClearRegisteredExtras) origin := common.Address{'o', 'r', 'i', 'g', 'i', 'n'} caller := common.Address{'c', 'a', 'l', 'l', 'e', 'r'} @@ -131,12 +127,11 @@ func TestCanCreateContract(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - evm := newEVM(t) + stateDB, evm := ethtest.NewZeroEVM(t) evm.TxContext.Origin = origin if tt.setState { - sdb := evm.StateDB.(*state.StateDB) - sdb.SetState(common.Address{}, slot, value) + stateDB.SetState(common.Address{}, slot, value) } methods := []struct { @@ -148,7 +143,7 @@ func TestCanCreateContract(t *testing.T) { { name: "Create", deploy: func() ([]byte, common.Address, uint64, error) { - return evm.Create(AccountRef(caller), code, 1e6, uint256.NewInt(0)) + return evm.Create(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0)) }, wantErr: tt.wantCreateErr, wantAddr: create, @@ -156,7 +151,7 @@ func TestCanCreateContract(t *testing.T) { { name: "Create2", deploy: func() ([]byte, common.Address, uint64, error) { - return evm.Create2(AccountRef(caller), code, 1e6, uint256.NewInt(0), new(uint256.Int).SetBytes(salt[:])) + return evm.Create2(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0), new(uint256.Int).SetBytes(salt[:])) }, wantErr: tt.wantCreate2Err, wantAddr: create2, @@ -177,21 +172,3 @@ func TestCanCreateContract(t *testing.T) { }) } } - -func newEVM(t *testing.T) *EVM { - t.Helper() - - sdb, err := state.New(common.Hash{}, state.NewDatabase(rawdb.NewMemoryDatabase()), nil) - require.NoError(t, err, "state.New()") - - return NewEVM( - BlockContext{ - CanTransfer: func(_ StateDB, _ common.Address, _ *uint256.Int) bool { return true }, - Transfer: func(_ StateDB, _, _ common.Address, _ *uint256.Int) {}, - }, - TxContext{}, - sdb, - ¶ms.ChainConfig{}, - Config{}, - ) -} diff --git a/core/vm/libevm_test.go b/core/vm/libevm_test.go new file mode 100644 index 00000000000..5c2fbb02a14 --- /dev/null +++ b/core/vm/libevm_test.go @@ -0,0 +1,7 @@ +package vm + +// The original RunPrecompiledContract was migrated to being a method on +// [evmCallArgs]. We need to replace it for use by regular geth tests. +func RunPrecompiledContract(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) { + return (*evmCallArgs)(nil).RunPrecompiledContract(p, input, suppliedGas) +} diff --git a/libevm/ethtest/evm.go b/libevm/ethtest/evm.go new file mode 100644 index 00000000000..092c39e070a --- /dev/null +++ b/libevm/ethtest/evm.go @@ -0,0 +1,31 @@ +package ethtest + +import ( + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/rawdb" + "github.com/ethereum/go-ethereum/core/state" + "github.com/ethereum/go-ethereum/core/vm" + "github.com/ethereum/go-ethereum/params" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" +) + +func NewZeroEVM(tb testing.TB) (*state.StateDB, *vm.EVM) { + tb.Helper() + + sdb, err := state.New(common.Hash{}, state.NewDatabase(rawdb.NewMemoryDatabase()), nil) + require.NoError(tb, err, "state.New()") + + return sdb, vm.NewEVM( + vm.BlockContext{ + CanTransfer: func(_ vm.StateDB, _ common.Address, _ *uint256.Int) bool { return true }, + Transfer: func(_ vm.StateDB, _, _ common.Address, _ *uint256.Int) {}, + }, + vm.TxContext{}, + sdb, + ¶ms.ChainConfig{}, + vm.Config{}, + ) +} diff --git a/params/config.libevm.go b/params/config.libevm.go index d9312b528fe..7e4dd51edf1 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -60,17 +60,25 @@ func RegisterExtras[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ExtraPaylo } // TestOnlyClearRegisteredExtras clears the [Extras] previously passed to -// [RegisterExtras]. It panics if called from a non-test file. +// [RegisterExtras]. It panics if called from a non-testing call stack. // // In tests it SHOULD be called before every call to [RegisterExtras] and then // defer-called afterwards. This is a workaround for the single-call limitation // on [RegisterExtras]. func TestOnlyClearRegisteredExtras() { - _, file, _, ok := runtime.Caller(1 /* 0 would be here, not our caller */) - if !ok || !strings.HasSuffix(file, "_test.go") { - panic("call from non-test file") + pc := make([]uintptr, 10) + runtime.Callers(0, pc) + frames := runtime.CallersFrames(pc) + for { + f, more := frames.Next() + if strings.Contains(f.File, "/testing/") || strings.HasSuffix(f.File, "_test.go") { + registeredExtras = nil + return + } + if !more { + panic("no _test.go file in call stack") + } } - registeredExtras = nil } // registeredExtras holds non-generic constructors for the [Extras] types From 76fcc7e6c0f29d715ac64d21ec8f73dbde659023 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 5 Sep 2024 11:02:52 +0100 Subject: [PATCH 20/24] test: `vm.NewStatefulPrecompile()` integration --- core/state_transition.libevm_test.go | 5 +-- core/vm/contracts.libevm_test.go | 54 +++++++++++++++++++++++++--- libevm/hookstest/stub.go | 5 ++- params/config.libevm.go | 4 +-- 4 files changed, 56 insertions(+), 12 deletions(-) diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go index c92de89789d..86f08fd04bd 100644 --- a/core/state_transition.libevm_test.go +++ b/core/state_transition.libevm_test.go @@ -9,7 +9,6 @@ import ( "github.com/ethereum/go-ethereum/libevm" "github.com/ethereum/go-ethereum/libevm/ethtest" "github.com/ethereum/go-ethereum/libevm/hookstest" - "github.com/ethereum/go-ethereum/params" "github.com/stretchr/testify/require" ) @@ -34,9 +33,7 @@ func TestCanExecuteTransaction(t *testing.T) { return makeErr(from, to, s.GetState(account, slot)) }, } - params.TestOnlyClearRegisteredExtras() - hooks.RegisterForRules() - t.Cleanup(params.TestOnlyClearRegisteredExtras) + hooks.RegisterForRules(t) state, evm := ethtest.NewZeroEVM(t) state.SetState(account, slot, value) diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index 0e35c5c922f..ac7a99754e1 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -64,8 +64,7 @@ func TestPrecompileOverride(t *testing.T) { }, }, } - params.TestOnlyClearRegisteredExtras() - hooks.RegisterForRules() + hooks.RegisterForRules(t) t.Run(fmt.Sprintf("%T.Call([overridden precompile address = %v])", &vm.EVM{}, tt.addr), func(t *testing.T) { _, evm := ethtest.NewZeroEVM(t) @@ -78,6 +77,53 @@ func TestPrecompileOverride(t *testing.T) { } } +func TestNewStatefulPrecompile(t *testing.T) { + var ( + caller, precompile common.Address + input = make([]byte, 8) + slot, value common.Hash + ) + rng := rand.New(rand.NewSource(314159)) + rng.Read(caller[:]) + rng.Read(precompile[:]) + rng.Read(input[:]) + rng.Read(slot[:]) + rng.Read(value[:]) + + const gasLimit = 1e6 + gasCost := rng.Uint64n(gasLimit) + + makeOutput := func(caller, self common.Address, input []byte, stateVal common.Hash) []byte { + return []byte(fmt.Sprintf( + "Caller: %v Precompile: %v State: %v Input: %#x", + caller, self, stateVal, input, + )) + } + hooks := &hookstest.Stub{ + PrecompileOverrides: map[common.Address]libevm.PrecompiledContract{ + precompile: vm.NewStatefulPrecompile( + func(state vm.StateDB, _ *params.Rules, caller, self common.Address, input []byte) ([]byte, error) { + return makeOutput(caller, self, input, state.GetState(precompile, slot)), nil + }, + func(b []byte) uint64 { + return gasCost + }, + ), + }, + } + hooks.RegisterForRules(t) + + state, evm := ethtest.NewZeroEVM(t) + state.SetState(precompile, slot, value) + wantReturnData := makeOutput(caller, precompile, input, value) + wantGasLeft := gasLimit - gasCost + + gotReturnData, gotGasLeft, err := evm.Call(vm.AccountRef(caller), precompile, input, gasLimit, uint256.NewInt(0)) + require.NoError(t, err) + assert.Equal(t, wantReturnData, gotReturnData) + assert.Equal(t, wantGasLeft, gotGasLeft) +} + func TestCanCreateContract(t *testing.T) { // We need to prove end-to-end plumbing of contract-creation addresses, // state, and any returned error. We therefore condition an error on a state @@ -95,9 +141,7 @@ func TestCanCreateContract(t *testing.T) { return nil }, } - params.TestOnlyClearRegisteredExtras() - hooks.RegisterForRules() - t.Cleanup(params.TestOnlyClearRegisteredExtras) + hooks.RegisterForRules(t) origin := common.Address{'o', 'r', 'i', 'g', 'i', 'n'} caller := common.Address{'c', 'a', 'l', 'l', 'e', 'r'} diff --git a/libevm/hookstest/stub.go b/libevm/hookstest/stub.go index 58ebd55eefd..7554e064687 100644 --- a/libevm/hookstest/stub.go +++ b/libevm/hookstest/stub.go @@ -2,6 +2,7 @@ package hookstest import ( "math/big" + "testing" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/libevm" @@ -16,12 +17,14 @@ type Stub struct { CanCreateContractFn func(*libevm.ContractCreation, libevm.StateReader) error } -func (s *Stub) RegisterForRules() { +func (s *Stub) RegisterForRules(tb testing.TB) { + params.TestOnlyClearRegisteredExtras() params.RegisterExtras(params.Extras[params.NOOPHooks, Stub]{ NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *params.NOOPHooks, blockNum *big.Int, isMerge bool, timestamp uint64) *Stub { return s }, }) + tb.Cleanup(params.TestOnlyClearRegisteredExtras) } func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, bool) { diff --git a/params/config.libevm.go b/params/config.libevm.go index 7e4dd51edf1..37665be2106 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -63,8 +63,8 @@ func RegisterExtras[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ExtraPaylo // [RegisterExtras]. It panics if called from a non-testing call stack. // // In tests it SHOULD be called before every call to [RegisterExtras] and then -// defer-called afterwards. This is a workaround for the single-call limitation -// on [RegisterExtras]. +// defer-called afterwards, either directly or via testing.TB.Cleanup(). This is +// a workaround for the single-call limitation on [RegisterExtras]. func TestOnlyClearRegisteredExtras() { pc := make([]uintptr, 10) runtime.Callers(0, pc) From 4891c50b58df0508848bcea894fb71407d76f02f Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 5 Sep 2024 11:45:02 +0100 Subject: [PATCH 21/24] refactor: simplify test of `CanCreateContract` --- core/state_transition.libevm_test.go | 16 ++-- core/vm/contracts.libevm_test.go | 121 +++++++++++---------------- 2 files changed, 60 insertions(+), 77 deletions(-) diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go index 86f08fd04bd..d7c25f51bc4 100644 --- a/core/state_transition.libevm_test.go +++ b/core/state_transition.libevm_test.go @@ -15,15 +15,11 @@ import ( func TestCanExecuteTransaction(t *testing.T) { rng := rand.New(rand.NewSource(42)) var ( - from, to common.Address - account common.Address - slot, value common.Hash + account common.Address + slot common.Hash ) - rng.Read(from[:]) - rng.Read(to[:]) rng.Read(account[:]) rng.Read(slot[:]) - rng.Read(value[:]) makeErr := func(from common.Address, to *common.Address, val common.Hash) error { return fmt.Errorf("From: %v To: %v State: %v", from, to, val) @@ -35,6 +31,14 @@ func TestCanExecuteTransaction(t *testing.T) { } hooks.RegisterForRules(t) + var ( + from, to common.Address + value common.Hash + ) + rng.Read(from[:]) + rng.Read(to[:]) + rng.Read(value[:]) + state, evm := ethtest.NewZeroEVM(t) state.SetState(account, slot, value) msg := &Message{ diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index ac7a99754e1..0bab6fb6eff 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -79,16 +79,12 @@ func TestPrecompileOverride(t *testing.T) { func TestNewStatefulPrecompile(t *testing.T) { var ( - caller, precompile common.Address - input = make([]byte, 8) - slot, value common.Hash + precompile common.Address + slot common.Hash ) rng := rand.New(rand.NewSource(314159)) - rng.Read(caller[:]) rng.Read(precompile[:]) - rng.Read(input[:]) rng.Read(slot[:]) - rng.Read(value[:]) const gasLimit = 1e6 gasCost := rng.Uint64n(gasLimit) @@ -113,6 +109,15 @@ func TestNewStatefulPrecompile(t *testing.T) { } hooks.RegisterForRules(t) + var ( + caller common.Address + input = make([]byte, 8) + value common.Hash + ) + rng.Read(caller[:]) + rng.Read(input) + rng.Read(value[:]) + state, evm := ethtest.NewZeroEVM(t) state.SetState(precompile, slot, value) wantReturnData := makeOutput(caller, precompile, input, value) @@ -125,94 +130,68 @@ func TestNewStatefulPrecompile(t *testing.T) { } func TestCanCreateContract(t *testing.T) { - // We need to prove end-to-end plumbing of contract-creation addresses, - // state, and any returned error. We therefore condition an error on a state - // value being set, and that error contains the addresses. - makeErr := func(cc *libevm.ContractCreation) error { - return fmt.Errorf("Origin: %v Caller: %v Contract: %v", cc.Origin, cc.Caller, cc.Contract) + var ( + account common.Address + slot common.Hash + ) + rng := rand.New(rand.NewSource(142857)) + rng.Read(account[:]) + rng.Read(slot[:]) + + makeErr := func(cc *libevm.ContractCreation, stateVal common.Hash) error { + return fmt.Errorf("Origin: %v Caller: %v Contract: %v State: %v", cc.Origin, cc.Caller, cc.Contract, stateVal) } - slot := common.Hash(crypto.Keccak256([]byte("slot"))) - value := common.Hash(crypto.Keccak256([]byte("value"))) hooks := &hookstest.Stub{ CanCreateContractFn: func(cc *libevm.ContractCreation, s libevm.StateReader) error { - if s.GetState(common.Address{}, slot).Cmp(value) != 0 { - return makeErr(cc) - } - return nil + return makeErr(cc, s.GetState(account, slot)) }, } hooks.RegisterForRules(t) - origin := common.Address{'o', 'r', 'i', 'g', 'i', 'n'} - caller := common.Address{'c', 'a', 'l', 'l', 'e', 'r'} - create := crypto.CreateAddress(caller, 0) var ( - code []byte - salt [32]byte + origin, caller common.Address + value common.Hash + code = make([]byte, 8) + salt [32]byte ) + rng.Read(origin[:]) + rng.Read(caller[:]) + rng.Read(value[:]) + rng.Read(code) + rng.Read(salt[:]) + + create := crypto.CreateAddress(caller, 0) create2 := crypto.CreateAddress2(caller, salt, crypto.Keccak256(code)) tests := []struct { - name string - setState bool - wantCreateErr, wantCreate2Err error + name string + create func(*vm.EVM) ([]byte, common.Address, uint64, error) + wantErr error }{ { - name: "no state => return error", - setState: false, - wantCreateErr: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create}), - wantCreate2Err: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create2}), + name: "Create", + create: func(evm *vm.EVM) ([]byte, common.Address, uint64, error) { + return evm.Create(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0)) + }, + wantErr: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create}, value), }, { - name: "state set => no error", - setState: true, + name: "Create2", + create: func(evm *vm.EVM) ([]byte, common.Address, uint64, error) { + return evm.Create2(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0), new(uint256.Int).SetBytes(salt[:])) + }, + wantErr: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create2}, value), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - stateDB, evm := ethtest.NewZeroEVM(t) + state, evm := ethtest.NewZeroEVM(t) + state.SetState(account, slot, value) evm.TxContext.Origin = origin - if tt.setState { - stateDB.SetState(common.Address{}, slot, value) - } - - methods := []struct { - name string - deploy func() ([]byte, common.Address, uint64, error) - wantErr error - wantAddr common.Address - }{ - { - name: "Create", - deploy: func() ([]byte, common.Address, uint64, error) { - return evm.Create(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0)) - }, - wantErr: tt.wantCreateErr, - wantAddr: create, - }, - { - name: "Create2", - deploy: func() ([]byte, common.Address, uint64, error) { - return evm.Create2(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0), new(uint256.Int).SetBytes(salt[:])) - }, - wantErr: tt.wantCreate2Err, - wantAddr: create2, - }, - } - - for _, m := range methods { - t.Run(m.name, func(t *testing.T) { - _, gotAddr, _, err := m.deploy() - if want := m.wantErr; want == nil { - require.NoError(t, err) - assert.Equal(t, m.wantAddr, gotAddr) - } else { - require.EqualError(t, err, want.Error()) - } - }) - } + _, _, _, err := tt.create(evm) + require.EqualError(t, err, tt.wantErr.Error()) }) } } From 9162508c8b9551960a4095b52e0abf5e88ac5320 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 5 Sep 2024 12:04:47 +0100 Subject: [PATCH 22/24] refactor: abstract generation of random `Address`/`Hash` values --- core/state_transition.libevm_test.go | 25 +++++---------- core/vm/contracts.libevm_test.go | 47 +++++++++------------------- libevm/ethtest/rand.go | 35 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 51 deletions(-) create mode 100644 libevm/ethtest/rand.go diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go index d7c25f51bc4..274e5711002 100644 --- a/core/state_transition.libevm_test.go +++ b/core/state_transition.libevm_test.go @@ -2,7 +2,6 @@ package core import ( "fmt" - "math/rand" "testing" "github.com/ethereum/go-ethereum/common" @@ -13,13 +12,9 @@ import ( ) func TestCanExecuteTransaction(t *testing.T) { - rng := rand.New(rand.NewSource(42)) - var ( - account common.Address - slot common.Hash - ) - rng.Read(account[:]) - rng.Read(slot[:]) + rng := ethtest.NewRand(42) + account := rng.Address() + slot := rng.Hash() makeErr := func(from common.Address, to *common.Address, val common.Hash) error { return fmt.Errorf("From: %v To: %v State: %v", from, to, val) @@ -31,20 +26,14 @@ func TestCanExecuteTransaction(t *testing.T) { } hooks.RegisterForRules(t) - var ( - from, to common.Address - value common.Hash - ) - rng.Read(from[:]) - rng.Read(to[:]) - rng.Read(value[:]) + value := rng.Hash() state, evm := ethtest.NewZeroEVM(t) state.SetState(account, slot, value) msg := &Message{ - From: from, - To: &to, + From: rng.Address(), + To: rng.AddressPtr(), } _, err := ApplyMessage(evm, msg, new(GasPool).AddGas(30e6)) - require.EqualError(t, err, makeErr(from, &to, value).Error()) + require.EqualError(t, err, makeErr(msg.From, msg.To, value).Error()) } diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index 0bab6fb6eff..3893325775e 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -78,13 +78,9 @@ func TestPrecompileOverride(t *testing.T) { } func TestNewStatefulPrecompile(t *testing.T) { - var ( - precompile common.Address - slot common.Hash - ) - rng := rand.New(rand.NewSource(314159)) - rng.Read(precompile[:]) - rng.Read(slot[:]) + rng := ethtest.NewRand(314159) + precompile := rng.Address() + slot := rng.Hash() const gasLimit = 1e6 gasCost := rng.Uint64n(gasLimit) @@ -109,14 +105,9 @@ func TestNewStatefulPrecompile(t *testing.T) { } hooks.RegisterForRules(t) - var ( - caller common.Address - input = make([]byte, 8) - value common.Hash - ) - rng.Read(caller[:]) - rng.Read(input) - rng.Read(value[:]) + caller := rng.Address() + input := rng.Bytes(8) + value := rng.Hash() state, evm := ethtest.NewZeroEVM(t) state.SetState(precompile, slot, value) @@ -130,13 +121,9 @@ func TestNewStatefulPrecompile(t *testing.T) { } func TestCanCreateContract(t *testing.T) { - var ( - account common.Address - slot common.Hash - ) - rng := rand.New(rand.NewSource(142857)) - rng.Read(account[:]) - rng.Read(slot[:]) + rng := ethtest.NewRand(142857) + account := rng.Address() + slot := rng.Hash() makeErr := func(cc *libevm.ContractCreation, stateVal common.Hash) error { return fmt.Errorf("Origin: %v Caller: %v Contract: %v State: %v", cc.Origin, cc.Caller, cc.Contract, stateVal) @@ -148,17 +135,11 @@ func TestCanCreateContract(t *testing.T) { } hooks.RegisterForRules(t) - var ( - origin, caller common.Address - value common.Hash - code = make([]byte, 8) - salt [32]byte - ) - rng.Read(origin[:]) - rng.Read(caller[:]) - rng.Read(value[:]) - rng.Read(code) - rng.Read(salt[:]) + origin := rng.Address() + caller := rng.Address() + value := rng.Hash() + code := rng.Bytes(8) + salt := rng.Hash() create := crypto.CreateAddress(caller, 0) create2 := crypto.CreateAddress2(caller, salt, crypto.Keccak256(code)) diff --git a/libevm/ethtest/rand.go b/libevm/ethtest/rand.go new file mode 100644 index 00000000000..7e4aaec2241 --- /dev/null +++ b/libevm/ethtest/rand.go @@ -0,0 +1,35 @@ +package ethtest + +import ( + "github.com/ethereum/go-ethereum/common" + "golang.org/x/exp/rand" +) + +type Rand struct { + *rand.Rand +} + +func NewRand(seed uint64) *Rand { + return &Rand{rand.New(rand.NewSource(seed))} +} + +func (r *Rand) Address() (a common.Address) { + r.Read(a[:]) + return a +} + +func (r *Rand) AddressPtr() *common.Address { + a := r.Address() + return &a +} + +func (r *Rand) Hash() (h common.Hash) { + r.Read(h[:]) + return h +} + +func (r *Rand) Bytes(n uint) []byte { + b := make([]byte, n) + r.Read(b) + return b +} From 6f7cb2ed6c8a1ef5ca3148f645bcf733ae6deb6e Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Thu, 5 Sep 2024 15:15:28 +0100 Subject: [PATCH 23/24] doc: full documentation + readability refactoring/renaming --- core/state_transition.libevm.go | 2 ++ core/state_transition.libevm_test.go | 2 +- core/vm/contracts.libevm.go | 16 +++++++++++----- core/vm/contracts.libevm_test.go | 14 +++++++------- core/vm/evm.go | 2 +- libevm/ethtest/evm.go | 12 +++++++++--- libevm/ethtest/rand.go | 20 +++++++++++++------- libevm/hookstest/stub.go | 11 ++++++++--- libevm/libevm.go | 11 +++++++++-- params/example.libevm_test.go | 2 +- params/hooks.libevm.go | 14 +++++++++++--- 11 files changed, 73 insertions(+), 33 deletions(-) diff --git a/core/state_transition.libevm.go b/core/state_transition.libevm.go index c76f2cefc10..4d2ee16f020 100644 --- a/core/state_transition.libevm.go +++ b/core/state_transition.libevm.go @@ -1,5 +1,7 @@ package core +// canExecuteTransaction is a convenience wrapper for calling the +// [params.RulesHooks.CanExecuteTransaction] hook. func (st *StateTransition) canExecuteTransaction() error { bCtx := st.evm.Context rules := st.evm.ChainConfig().Rules(bCtx.BlockNumber, bCtx.Random != nil, bCtx.Time) diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go index 274e5711002..856f106d24e 100644 --- a/core/state_transition.libevm_test.go +++ b/core/state_transition.libevm_test.go @@ -12,7 +12,7 @@ import ( ) func TestCanExecuteTransaction(t *testing.T) { - rng := ethtest.NewRand(42) + rng := ethtest.NewPseudoRand(42) account := rng.Address() slot := rng.Hash() diff --git a/core/vm/contracts.libevm.go b/core/vm/contracts.libevm.go index 95a98588fb7..23057d6f9c8 100644 --- a/core/vm/contracts.libevm.go +++ b/core/vm/contracts.libevm.go @@ -28,6 +28,8 @@ type evmCallArgs struct { value *uint256.Int } +// run runs the [PrecompiledContract], differentiating between stateful and +// regular types. func (args *evmCallArgs) run(p PrecompiledContract, input []byte) (ret []byte, err error) { if p, ok := p.(statefulPrecompile); ok { return p.run(args.evm.StateDB, &args.evm.chainRules, args.caller.Address(), args.addr, input) @@ -35,11 +37,15 @@ func (args *evmCallArgs) run(p PrecompiledContract, input []byte) (ret []byte, e return p.Run(input) } -// A StatefulPrecompileFunc... -type StatefulPrecompfileFunc func(_ StateDB, _ *params.Rules, caller, self common.Address, input []byte) ([]byte, error) +// PrecompiledStatefulRun is the stateful equivalent of the Run() method of a +// [PrecompiledContract]. +type PrecompiledStatefulRun func(_ StateDB, _ *params.Rules, caller, self common.Address, input []byte) ([]byte, error) -// NewStatefulPrecompile... -func NewStatefulPrecompile(run StatefulPrecompfileFunc, requiredGas func([]byte) uint64) PrecompiledContract { +// NewStatefulPrecompile constructs a new PrecompiledContract that can be used +// via an [EVM] instance but MUST NOT be called directly; a direct call to Run() +// reserves the right to panic. See other requirements defined in the comments +// on [PrecompiledContract]. +func NewStatefulPrecompile(run PrecompiledStatefulRun, requiredGas func([]byte) uint64) PrecompiledContract { return statefulPrecompile{ gas: requiredGas, run: run, @@ -48,7 +54,7 @@ func NewStatefulPrecompile(run StatefulPrecompfileFunc, requiredGas func([]byte) type statefulPrecompile struct { gas func([]byte) uint64 - run StatefulPrecompfileFunc + run PrecompiledStatefulRun } func (p statefulPrecompile) RequiredGas(input []byte) uint64 { diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index 3893325775e..28bdcde09a5 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -78,7 +78,7 @@ func TestPrecompileOverride(t *testing.T) { } func TestNewStatefulPrecompile(t *testing.T) { - rng := ethtest.NewRand(314159) + rng := ethtest.NewPseudoRand(314159) precompile := rng.Address() slot := rng.Hash() @@ -121,15 +121,15 @@ func TestNewStatefulPrecompile(t *testing.T) { } func TestCanCreateContract(t *testing.T) { - rng := ethtest.NewRand(142857) + rng := ethtest.NewPseudoRand(142857) account := rng.Address() slot := rng.Hash() - makeErr := func(cc *libevm.ContractCreation, stateVal common.Hash) error { - return fmt.Errorf("Origin: %v Caller: %v Contract: %v State: %v", cc.Origin, cc.Caller, cc.Contract, stateVal) + makeErr := func(cc *libevm.AddressContext, stateVal common.Hash) error { + return fmt.Errorf("Origin: %v Caller: %v Contract: %v State: %v", cc.Origin, cc.Caller, cc.Self, stateVal) } hooks := &hookstest.Stub{ - CanCreateContractFn: func(cc *libevm.ContractCreation, s libevm.StateReader) error { + CanCreateContractFn: func(cc *libevm.AddressContext, s libevm.StateReader) error { return makeErr(cc, s.GetState(account, slot)) }, } @@ -154,14 +154,14 @@ func TestCanCreateContract(t *testing.T) { create: func(evm *vm.EVM) ([]byte, common.Address, uint64, error) { return evm.Create(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0)) }, - wantErr: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create}, value), + wantErr: makeErr(&libevm.AddressContext{Origin: origin, Caller: caller, Self: create}, value), }, { name: "Create2", create: func(evm *vm.EVM) ([]byte, common.Address, uint64, error) { return evm.Create2(vm.AccountRef(caller), code, 1e6, uint256.NewInt(0), new(uint256.Int).SetBytes(salt[:])) }, - wantErr: makeErr(&libevm.ContractCreation{Origin: origin, Caller: caller, Contract: create2}, value), + wantErr: makeErr(&libevm.AddressContext{Origin: origin, Caller: caller, Self: create2}, value), }, } diff --git a/core/vm/evm.go b/core/vm/evm.go index 4db904dbc84..7b830a343a7 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -428,7 +428,7 @@ func (c *codeAndHash) Hash() common.Hash { // create creates a new contract using code as deployment code. func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, value *uint256.Int, address common.Address, typ OpCode) ([]byte, common.Address, uint64, error) { - cc := &libevm.ContractCreation{Origin: evm.Origin, Caller: caller.Address(), Contract: address} + cc := &libevm.AddressContext{Origin: evm.Origin, Caller: caller.Address(), Self: address} if err := evm.chainRules.Hooks().CanCreateContract(cc, evm.StateDB); err != nil { return nil, common.Address{}, gas, err } diff --git a/libevm/ethtest/evm.go b/libevm/ethtest/evm.go index 092c39e070a..069a949ac64 100644 --- a/libevm/ethtest/evm.go +++ b/libevm/ethtest/evm.go @@ -1,17 +1,23 @@ +// Package ethtest provides utility functions for use in testing +// Ethereum-related functionality. package ethtest import ( "testing" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/state" "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/params" - "github.com/holiman/uint256" "github.com/stretchr/testify/require" ) +// NewZeroEVM returns a new EVM backed by a [rawdb.NewMemoryDatabase]; all other +// arguments to [vm.NewEVM] are the zero values of their respective types, +// except for the use of [core.CanTransfer] and [core.Transfer] instead of nil +// functions. func NewZeroEVM(tb testing.TB) (*state.StateDB, *vm.EVM) { tb.Helper() @@ -20,8 +26,8 @@ func NewZeroEVM(tb testing.TB) (*state.StateDB, *vm.EVM) { return sdb, vm.NewEVM( vm.BlockContext{ - CanTransfer: func(_ vm.StateDB, _ common.Address, _ *uint256.Int) bool { return true }, - Transfer: func(_ vm.StateDB, _, _ common.Address, _ *uint256.Int) {}, + CanTransfer: core.CanTransfer, + Transfer: core.Transfer, }, vm.TxContext{}, sdb, diff --git a/libevm/ethtest/rand.go b/libevm/ethtest/rand.go index 7e4aaec2241..dfabcfedca1 100644 --- a/libevm/ethtest/rand.go +++ b/libevm/ethtest/rand.go @@ -5,30 +5,36 @@ import ( "golang.org/x/exp/rand" ) -type Rand struct { +// PseudoRand extends [rand.Rand] (*not* crypto/rand). +type PseudoRand struct { *rand.Rand } -func NewRand(seed uint64) *Rand { - return &Rand{rand.New(rand.NewSource(seed))} +// NewPseudoRand returns a new PseudoRand with the given seed. +func NewPseudoRand(seed uint64) *PseudoRand { + return &PseudoRand{rand.New(rand.NewSource(seed))} } -func (r *Rand) Address() (a common.Address) { +// Address returns a pseudorandom address. +func (r *PseudoRand) Address() (a common.Address) { r.Read(a[:]) return a } -func (r *Rand) AddressPtr() *common.Address { +// AddressPtr returns a pointer to a pseudorandom address. +func (r *PseudoRand) AddressPtr() *common.Address { a := r.Address() return &a } -func (r *Rand) Hash() (h common.Hash) { +// Hash returns a pseudorandom hash. +func (r *PseudoRand) Hash() (h common.Hash) { r.Read(h[:]) return h } -func (r *Rand) Bytes(n uint) []byte { +// Bytes returns `n` pseudorandom bytes. +func (r *PseudoRand) Bytes(n uint) []byte { b := make([]byte, n) r.Read(b) return b diff --git a/libevm/hookstest/stub.go b/libevm/hookstest/stub.go index 7554e064687..3dcb1619c3c 100644 --- a/libevm/hookstest/stub.go +++ b/libevm/hookstest/stub.go @@ -1,3 +1,4 @@ +// Package hookstest provides test doubles for testing subsets of libevm hooks. package hookstest import ( @@ -10,13 +11,17 @@ import ( ) // A Stub is a test double for [params.ChainConfigHooks] and -// [params.RulesHooks]. +// [params.RulesHooks]. Each of the fields, if non-nil, back their respective +// hook methods, which otherwise fall back to the default behaviour. type Stub struct { PrecompileOverrides map[common.Address]libevm.PrecompiledContract CanExecuteTransactionFn func(common.Address, *common.Address, libevm.StateReader) error - CanCreateContractFn func(*libevm.ContractCreation, libevm.StateReader) error + CanCreateContractFn func(*libevm.AddressContext, libevm.StateReader) error } +// RegisterForRules clears any registered [params.Extras] and then registers s +// as [params.RulesHooks], which are themselves cleared by the +// [testing.TB.Cleanup] routine. func (s *Stub) RegisterForRules(tb testing.TB) { params.TestOnlyClearRegisteredExtras() params.RegisterExtras(params.Extras[params.NOOPHooks, Stub]{ @@ -42,7 +47,7 @@ func (s Stub) CanExecuteTransaction(from common.Address, to *common.Address, sr return nil } -func (s Stub) CanCreateContract(cc *libevm.ContractCreation, sr libevm.StateReader) error { +func (s Stub) CanCreateContract(cc *libevm.AddressContext, sr libevm.StateReader) error { if f := s.CanCreateContractFn; f != nil { return f(cc, sr) } diff --git a/libevm/libevm.go b/libevm/libevm.go index ac669e71a56..8b7d85bd49e 100644 --- a/libevm/libevm.go +++ b/libevm/libevm.go @@ -40,6 +40,13 @@ type StateReader interface { SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) } -type ContractCreation struct { - Origin, Caller, Contract common.Address +// AddressContext carries addresses available to contexts such as calls and +// contract creation. +// +// With respect to contract creation, the Self address MAY be the predicted +// address of the contract about to be deployed, which may not exist yet. +type AddressContext struct { + Origin common.Address // equivalent to vm.ORIGIN op code + Caller common.Address // equivalent to vm.CALLER op code + Self common.Address // equivalent to vm.ADDRESS op code } diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index ab323306e7d..37e8b2e4b58 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -105,7 +105,7 @@ func (r RulesExtra) PrecompileOverride(addr common.Address) (_ libevm.Precompile // CanCreateContract implements the required [params.RuleHooks] method. Access // to state allows it to be configured on-chain however this is an optional // implementation detail. -func (r RulesExtra) CanCreateContract(*libevm.ContractCreation, libevm.StateReader) error { +func (r RulesExtra) CanCreateContract(*libevm.AddressContext, libevm.StateReader) error { if time.Unix(int64(r.timestamp), 0).UTC().Day() != int(time.Tuesday) { return errors.New("uh oh!") } diff --git a/params/hooks.libevm.go b/params/hooks.libevm.go index 13085d6f4eb..c44cd3f1f05 100644 --- a/params/hooks.libevm.go +++ b/params/hooks.libevm.go @@ -19,8 +19,7 @@ type ChainConfigHooks interface{} // RulesHooks are required for all types registered as [Extras] for [Rules] // payloads. type RulesHooks interface { - CanExecuteTransaction(from common.Address, to *common.Address, _ libevm.StateReader) error - CanCreateContract(*libevm.ContractCreation, libevm.StateReader) error + RulesAllowlistHooks // PrecompileOverride signals whether or not the EVM interpreter MUST // override its treatment of the address when deciding if it is a // precompiled contract. If PrecompileOverride returns `true` then the @@ -30,6 +29,13 @@ type RulesHooks interface { PrecompileOverride(common.Address) (_ libevm.PrecompiledContract, override bool) } +// RulesAllowlistHooks are a subset of [RulesHooks] that gate actions, signalled +// by returning a nil (allowed) or non-nil (blocked) error. +type RulesAllowlistHooks interface { + CanCreateContract(*libevm.AddressContext, libevm.StateReader) error + CanExecuteTransaction(from common.Address, to *common.Address, _ libevm.StateReader) error +} + // Hooks returns the hooks registered with [RegisterExtras], or [NOOPHooks] if // none were registered. func (c *ChainConfig) Hooks() ChainConfigHooks { @@ -60,11 +66,13 @@ var _ interface { RulesHooks } = NOOPHooks{} +// CanExecuteTransaction allows all (otherwise valid) transactions. func (NOOPHooks) CanExecuteTransaction(_ common.Address, _ *common.Address, _ libevm.StateReader) error { return nil } -func (NOOPHooks) CanCreateContract(*libevm.ContractCreation, libevm.StateReader) error { +// CanCreateContract allows all (otherwise valid) contract deployment. +func (NOOPHooks) CanCreateContract(*libevm.AddressContext, libevm.StateReader) error { return nil } From e6840aa7651798f57e80e80c8ebef9f7af7220b2 Mon Sep 17 00:00:00 2001 From: Arran Schlosberg Date: Fri, 6 Sep 2024 14:23:56 +0100 Subject: [PATCH 24/24] fix: remove circular dependency in tests --- core/state_transition.libevm_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go index 856f106d24e..d81e9ce4f76 100644 --- a/core/state_transition.libevm_test.go +++ b/core/state_transition.libevm_test.go @@ -1,10 +1,11 @@ -package core +package core_test import ( "fmt" "testing" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/libevm" "github.com/ethereum/go-ethereum/libevm/ethtest" "github.com/ethereum/go-ethereum/libevm/hookstest" @@ -30,10 +31,10 @@ func TestCanExecuteTransaction(t *testing.T) { state, evm := ethtest.NewZeroEVM(t) state.SetState(account, slot, value) - msg := &Message{ + msg := &core.Message{ From: rng.Address(), To: rng.AddressPtr(), } - _, err := ApplyMessage(evm, msg, new(GasPool).AddGas(30e6)) + _, err := core.ApplyMessage(evm, msg, new(core.GasPool).AddGas(30e6)) require.EqualError(t, err, makeErr(msg.From, msg.To, value).Error()) }