Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: CheckConfig{Compatible,ForkOrder} + Description hooks #29

Merged
merged 6 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions libevm/hookstest/stub.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,27 +14,30 @@ import (
// Register clears any registered [params.Extras] and then registers `extras`
// for the lifetime of the current test, clearing them via tb's
// [testing.TB.Cleanup].
func Register[C params.ChainConfigHooks, R params.RulesHooks](tb testing.TB, extras params.Extras[C, R]) {
func Register[C params.ChainConfigHooks, R params.RulesHooks](tb testing.TB, extras params.Extras[C, R]) params.ExtraPayloads[C, R] {
tb.Helper()
params.TestOnlyClearRegisteredExtras()
tb.Cleanup(params.TestOnlyClearRegisteredExtras)
params.RegisterExtras(extras)
return params.RegisterExtras(extras)
}

// A Stub is a test double for [params.ChainConfigHooks] and
// [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 {
CheckConfigForkOrderFn func() error
CheckConfigCompatibleFn func(*params.ChainConfig, *big.Int, uint64) *params.ConfigCompatError
DescriptionSuffix string
PrecompileOverrides map[common.Address]libevm.PrecompiledContract
CanExecuteTransactionFn func(common.Address, *common.Address, libevm.StateReader) error
CanCreateContractFn func(*libevm.AddressContext, uint64, libevm.StateReader) (uint64, error)
}

// Register is a convenience wrapper for registering s as both the
// [params.ChainConfigHooks] and [params.RulesHooks] via [Register].
func (s *Stub) Register(tb testing.TB) {
func (s *Stub) Register(tb testing.TB) params.ExtraPayloads[*Stub, *Stub] {
tb.Helper()
Register(tb, params.Extras[*Stub, *Stub]{
return Register(tb, params.Extras[*Stub, *Stub]{
NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *Stub, blockNum *big.Int, isMerge bool, timestamp uint64) *Stub {
return s
},
Expand All @@ -52,6 +55,29 @@ func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract,
return p, ok
}

// CheckConfigForkOrder proxies arguments to the s.CheckConfigForkOrderFn
// function if non-nil, otherwise it acts as a noop.
func (s Stub) CheckConfigForkOrder() error {
if f := s.CheckConfigForkOrderFn; f != nil {
return f()
}
return nil
}

// CheckConfigCompatible proxies arguments to the s.CheckConfigCompatibleFn
// function if non-nil, otherwise it acts as a noop.
func (s Stub) CheckConfigCompatible(newcfg *params.ChainConfig, headNumber *big.Int, headTimestamp uint64) *params.ConfigCompatError {
if f := s.CheckConfigCompatibleFn; f != nil {
return f(newcfg, headNumber, headTimestamp)
}
return nil
}

// Description returns s.DescriptionSuffix.
func (s Stub) Description() string {
return s.DescriptionSuffix
}

// CanExecuteTransaction proxies arguments to the s.CanExecuteTransactionFn
// function if non-nil, otherwise it acts as a noop.
func (s Stub) CanExecuteTransaction(from common.Address, to *common.Address, sr libevm.StateReader) error {
Expand Down
6 changes: 3 additions & 3 deletions params/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ func (c *ChainConfig) Description() string {
if c.VerkleTime != nil {
banner += fmt.Sprintf(" - Verkle: @%-10v\n", *c.VerkleTime)
}
return banner
return banner + c.Hooks().Description()
}

// IsHomestead returns whether num is either equal to the homestead block or greater.
Expand Down Expand Up @@ -671,7 +671,7 @@ func (c *ChainConfig) CheckConfigForkOrder() error {
lastFork = cur
}
}
return nil
return c.Hooks().CheckConfigForkOrder()
}

func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, headNumber *big.Int, headTimestamp uint64) *ConfigCompatError {
Expand Down Expand Up @@ -742,7 +742,7 @@ func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, headNumber *big.Int,
if isForkTimestampIncompatible(c.VerkleTime, newcfg.VerkleTime, headTimestamp) {
return newTimestampCompatError("Verkle fork timestamp", c.VerkleTime, newcfg.VerkleTime)
}
return nil
return c.Hooks().CheckConfigCompatible(newcfg, headNumber, headTimestamp)
}

// BaseFeeChangeDenominator bounds the amount the base fee can change between blocks.
Expand Down
8 changes: 5 additions & 3 deletions params/example.libevm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ func constructRulesExtra(c *params.ChainConfig, r *params.Rules, cEx ChainConfig
// the standard [params.ChainConfig] struct.
type ChainConfigExtra struct {
MyForkTime *uint64 `json:"myForkTime"`

// (Optional) If not all hooks are desirable then embedding a [NOOPHooks]
// allows the type to satisfy the [ChainConfigHooks] interface, resulting in
// default Ethereum behaviour.
params.NOOPHooks
}

// RulesExtra can be any struct. It too mirrors a common pattern in
Expand All @@ -59,9 +64,6 @@ type RulesExtra struct {
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.
Comment on lines -62 to -64
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we keep this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just been moved to ChainConfigExtra. I didn't think it was necessary to include it twice in the example, and that it should be at the first appearance of NOOPHooks.

params.NOOPHooks
}

Expand Down
23 changes: 22 additions & 1 deletion params/hooks.libevm.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
package params

import (
"math/big"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/libevm"
)

// ChainConfigHooks are required for all types registered as [Extras] for
// [ChainConfig] payloads.
type ChainConfigHooks interface{}
type ChainConfigHooks interface {
CheckConfigForkOrder() error
CheckConfigCompatible(newcfg *ChainConfig, headNumber *big.Int, headTimestamp uint64) *ConfigCompatError
Description() string
}

// 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
Expand Down Expand Up @@ -68,6 +74,21 @@ var _ interface {
RulesHooks
} = NOOPHooks{}

// CheckConfigForkOrder verifies all (otherwise valid) fork orders.
func (NOOPHooks) CheckConfigForkOrder() error {
return nil
}

// CheckConfigCompatible verifies all (otherwise valid) new configs.
func (NOOPHooks) CheckConfigCompatible(*ChainConfig, *big.Int, uint64) *ConfigCompatError {
return nil
}

// Description returns the empty string.
func (NOOPHooks) Description() string {
return ""
}

// CanExecuteTransaction allows all (otherwise valid) transactions.
func (NOOPHooks) CanExecuteTransaction(_ common.Address, _ *common.Address, _ libevm.StateReader) error {
return nil
Expand Down
63 changes: 63 additions & 0 deletions params/hooks.libevm_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package params_test

import (
"errors"
"fmt"
"math/big"
"testing"

"github.com/stretchr/testify/require"

"github.com/ethereum/go-ethereum/libevm/ethtest"
"github.com/ethereum/go-ethereum/libevm/hookstest"
"github.com/ethereum/go-ethereum/params"
)

func TestChainConfigHooks_Description(t *testing.T) {
const suffix = "Arran was here"
c := new(params.ChainConfig)
want := c.Description() + suffix

hooks := &hookstest.Stub{
DescriptionSuffix: "Arran was here",
}
hooks.Register(t).SetOnChainConfig(c, hooks)
require.Equal(t, want, c.Description(), "ChainConfigHooks.Description() is appended to non-extras equivalent")
}

func TestChainConfigHooks_CheckConfigForkOrder(t *testing.T) {
err := errors.New("uh oh")

c := new(params.ChainConfig)
require.NoError(t, c.CheckConfigForkOrder(), "CheckConfigForkOrder() with no hooks")

hooks := &hookstest.Stub{
CheckConfigForkOrderFn: func() error { return err },
}
hooks.Register(t).SetOnChainConfig(c, hooks)
require.Equal(t, err, c.CheckConfigForkOrder(), "CheckConfigForkOrder() with error-producing hook")
}

func TestChainConfigHooks_CheckConfigCompatible(t *testing.T) {
rng := ethtest.NewPseudoRand(1234567890)
newcfg := &params.ChainConfig{
ChainID: rng.BigUint64(),
}
headNumber := rng.Uint64()
headTimestamp := rng.Uint64()

c := new(params.ChainConfig)
require.Nil(t, c.CheckCompatible(newcfg, headNumber, headTimestamp), "CheckCompatible() with no hooks")

makeCompatErr := func(newcfg *params.ChainConfig, headNumber *big.Int, headTimestamp uint64) *params.ConfigCompatError {
return &params.ConfigCompatError{
What: fmt.Sprintf("ChainID: %v Head #: %v Head Time: %d", newcfg.ChainID, headNumber, headTimestamp),
}
}
hooks := &hookstest.Stub{
CheckConfigCompatibleFn: makeCompatErr,
}
hooks.Register(t).SetOnChainConfig(c, hooks)
want := makeCompatErr(newcfg, new(big.Int).SetUint64(headNumber), headTimestamp)
require.Equal(t, want, c.CheckCompatible(newcfg, headNumber, headTimestamp), "CheckCompatible() with error-producing hook")
}