refactor!: consolidate params and types payload access (#84)

## Why this should be merged

There are 3 places at which we perform the same, sensitive logic to
access registered payloads and as we modify more types this is likely to
expand. (e.g. `types.Header`).

## How this works

Introduces `pseudo.Accessor` to abstract the reusable code.

## How this was tested

Existing unit tests. Note that the `types.StateAccount` tests needed a
minor refactor to provide the assertions with access to the
`ExtraPayloads[T]` without introducing generic types anywhere.
This commit is contained in:
Arran Schlosberg 2024-12-09 18:32:15 +00:00 committed by GitHub
parent d71677f141
commit 380aa319f9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 135 additions and 135 deletions

View file

@ -27,7 +27,7 @@ import (
func GetExtra[SA any](s *StateDB, p types.ExtraPayloads[SA], addr common.Address) SA {
stateObject := s.getStateObject(addr)
if stateObject != nil {
return p.FromPayloadCarrier(&stateObject.data)
return p.StateAccount.Get(&stateObject.data)
}
var zero SA
return zero
@ -45,9 +45,9 @@ func setExtraOnObject[SA any](s *stateObject, p types.ExtraPayloads[SA], addr co
s.db.journal.append(extraChange[SA]{
payloads: p,
account: &addr,
prev: p.FromPayloadCarrier(&s.data),
prev: p.StateAccount.Get(&s.data),
})
p.SetOnPayloadCarrier(&s.data, extra)
p.StateAccount.Set(&s.data, extra)
}
// extraChange is a [journalEntry] for [SetExtra] / [setExtraOnObject].
@ -60,5 +60,5 @@ type extraChange[SA any] struct {
func (e extraChange[SA]) dirtied() *common.Address { return e.account }
func (e extraChange[SA]) revert(s *StateDB) {
e.payloads.SetOnPayloadCarrier(&s.getStateObject(*e.account).data, e.prev)
e.payloads.StateAccount.Set(&s.getStateObject(*e.account).data, e.prev)
}

View file

@ -87,7 +87,7 @@ func TestGetSetExtra(t *testing.T) {
Root: types.EmptyRootHash,
CodeHash: types.EmptyCodeHash[:],
}
payloads.SetOnPayloadCarrier(want, extra)
payloads.StateAccount.Set(want, extra)
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("types.FullAccount(%T.Account()) diff (-want +got):\n%s", iter, diff)

View file

@ -46,7 +46,7 @@ func TestStateObjectEmpty(t *testing.T) {
{
name: "explicit false bool",
registerAndSet: func(acc *types.StateAccount) {
types.RegisterExtras[bool]().SetOnPayloadCarrier(acc, false)
types.RegisterExtras[bool]().StateAccount.Set(acc, false)
},
wantEmpty: true,
},
@ -60,7 +60,7 @@ func TestStateObjectEmpty(t *testing.T) {
{
name: "true bool",
registerAndSet: func(acc *types.StateAccount) {
types.RegisterExtras[bool]().SetOnPayloadCarrier(acc, true)
types.RegisterExtras[bool]().StateAccount.Set(acc, true)
},
wantEmpty: false,
},

View file

@ -38,7 +38,12 @@ import (
// The payload can be accessed via the [ExtraPayloads.FromPayloadCarrier] method
// of the accessor returned by RegisterExtras.
func RegisterExtras[SA any]() ExtraPayloads[SA] {
var extra ExtraPayloads[SA]
extra := ExtraPayloads[SA]{
StateAccount: pseudo.NewAccessor[ExtraPayloadCarrier, SA](
func(a ExtraPayloadCarrier) *pseudo.Type { return a.extra().payload() },
func(a ExtraPayloadCarrier, t *pseudo.Type) { a.extra().t = t },
),
}
registeredExtras.MustRegister(&extraConstructors{
stateAccountType: func() string {
var x SA
@ -81,7 +86,7 @@ func (e *StateAccountExtra) clone() *StateAccountExtra {
// [StateAccount] and [SlimAccount] structs. The only valid way to construct an
// instance is by a call to [RegisterExtras].
type ExtraPayloads[SA any] struct {
_ struct{} // make godoc show unexported fields so nobody tries to make their own instance ;)
StateAccount pseudo.Accessor[ExtraPayloadCarrier, SA] // Also provides [SlimAccount] access.
}
func (ExtraPayloads[SA]) cloneStateAccount(s *StateAccountExtra) *StateAccountExtra {
@ -103,27 +108,6 @@ var _ = []ExtraPayloadCarrier{
(*SlimAccount)(nil),
}
// FromPayloadCarrier returns the carriers's payload.
func (ExtraPayloads[SA]) FromPayloadCarrier(a ExtraPayloadCarrier) SA {
return pseudo.MustNewValue[SA](a.extra().payload()).Get()
}
// PointerFromPayloadCarrier returns a pointer to the carriers's extra payload.
// This is guaranteed to be non-nil.
//
// Note that copying a [StateAccount] or [SlimAccount] by dereferencing a
// pointer will result in a shallow copy and that the *SA returned here will
// therefore be shared by all copies. If this is not the desired behaviour, use
// [StateAccount.Copy] or [ExtraPayloads.SetOnPayloadCarrier].
func (ExtraPayloads[SA]) PointerFromPayloadCarrier(a ExtraPayloadCarrier) *SA {
return pseudo.MustPointerTo[SA](a.extra().payload()).Value.Get()
}
// SetOnPayloadCarrier sets the carriers's payload.
func (ExtraPayloads[SA]) SetOnPayloadCarrier(a ExtraPayloadCarrier, val SA) {
a.extra().t = pseudo.From(val).Type
}
// A StateAccountExtra carries the extra payload, if any, registered with
// [RegisterExtras]. It SHOULD NOT be used directly; instead use the
// [ExtraPayloads] accessor returned by RegisterExtras.

View file

@ -51,72 +51,68 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) {
arbitrary = common.HexToHash("0x94eecff1444ab69437636630918c15596e001b30b973f03e06006ae20aa6e307")
)
// An assertion is the actual test to be performed. It is returned upon
// registration, instead of being a standalone field in the test, because
// each one uses a different generic parameter.
type assertion func(*testing.T, *types.StateAccount)
tests := []struct {
name string
registerAndSetExtra func(*types.StateAccount) *types.StateAccount
assertExtra func(*testing.T, *types.StateAccount)
registerAndSetExtra func(*types.StateAccount) (*types.StateAccount, assertion)
wantTrieHash common.Hash
}{
{
name: "vanilla geth",
registerAndSetExtra: func(a *types.StateAccount) *types.StateAccount {
return a
},
assertExtra: func(t *testing.T, a *types.StateAccount) {
t.Helper()
assert.Truef(t, a.Extra.Equal(nil), "%T.%T.IsEmpty()", a, a.Extra)
registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) {
//nolint:thelper // It's more useful if this test failure shows this line instead of its call site
return a, func(t *testing.T, got *types.StateAccount) {
assert.Truef(t, a.Extra.Equal(nil), "%T.%T.IsEmpty()", a, a.Extra)
}
},
wantTrieHash: vanillaGeth,
},
{
name: "true-boolean payload",
registerAndSetExtra: func(a *types.StateAccount) *types.StateAccount {
types.RegisterExtras[bool]().SetOnPayloadCarrier(a, true)
return a
},
assertExtra: func(t *testing.T, sa *types.StateAccount) {
t.Helper()
assert.Truef(t, types.ExtraPayloads[bool]{}.FromPayloadCarrier(sa), "")
registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) {
e := types.RegisterExtras[bool]()
e.StateAccount.Set(a, true)
return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper
assert.Truef(t, e.StateAccount.Get(got), "")
}
},
wantTrieHash: trueBool,
},
{
name: "explicit false-boolean payload",
registerAndSetExtra: func(a *types.StateAccount) *types.StateAccount {
p := types.RegisterExtras[bool]()
p.SetOnPayloadCarrier(a, false) // the explicit part
return a
},
assertExtra: func(t *testing.T, sa *types.StateAccount) {
t.Helper()
assert.Falsef(t, types.ExtraPayloads[bool]{}.FromPayloadCarrier(sa), "")
registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) {
e := types.RegisterExtras[bool]()
e.StateAccount.Set(a, false) // the explicit part
return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper
assert.Falsef(t, e.StateAccount.Get(got), "")
}
},
wantTrieHash: falseBool,
},
{
name: "implicit false-boolean payload",
registerAndSetExtra: func(a *types.StateAccount) *types.StateAccount {
types.RegisterExtras[bool]()
registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) {
e := types.RegisterExtras[bool]()
// Note that `a` is reflected, unchanged (the implicit part).
return a
},
assertExtra: func(t *testing.T, sa *types.StateAccount) {
t.Helper()
assert.Falsef(t, types.ExtraPayloads[bool]{}.FromPayloadCarrier(sa), "")
return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper
assert.Falsef(t, e.StateAccount.Get(got), "")
}
},
wantTrieHash: falseBool,
},
{
name: "arbitrary payload",
registerAndSetExtra: func(a *types.StateAccount) *types.StateAccount {
registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) {
e := types.RegisterExtras[arbitraryPayload]()
p := arbitraryPayload{arbitraryData}
types.RegisterExtras[arbitraryPayload]().SetOnPayloadCarrier(a, p)
return a
},
assertExtra: func(t *testing.T, sa *types.StateAccount) {
t.Helper()
got := types.ExtraPayloads[arbitraryPayload]{}.FromPayloadCarrier(sa)
assert.Equalf(t, arbitraryPayload{arbitraryData}, got, "")
e.StateAccount.Set(a, p)
return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper
assert.Equalf(t, arbitraryPayload{arbitraryData}, e.StateAccount.Get(got), "")
}
},
wantTrieHash: arbitrary,
},
@ -127,7 +123,7 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) {
types.TestOnlyClearRegisteredExtras()
t.Cleanup(types.TestOnlyClearRegisteredExtras)
acct := tt.registerAndSetExtra(&types.StateAccount{
acct, asserter := tt.registerAndSetExtra(&types.StateAccount{
Nonce: 42,
Balance: uint256.NewInt(314159),
Root: types.EmptyRootHash,
@ -147,7 +143,7 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) {
if diff := cmp.Diff(acct, got); diff != "" {
t.Errorf("%T.GetAccount() not equal to value passed to %[1]T.UpdateAccount(); diff (-want +got):\n%s", state, diff)
}
tt.assertExtra(t, got)
asserter(t, got)
})
}
}

49
libevm/pseudo/accessor.go Normal file
View file

@ -0,0 +1,49 @@
// Copyright 2024 the libevm authors.
//
// The libevm additions to go-ethereum are free software: you can redistribute
// them and/or modify them under the terms of the GNU Lesser General Public License
// as published by the Free Software Foundation, either version 3 of the License,
// or (at your option) any later version.
//
// The libevm additions are distributed in the hope that they will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser
// General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see
// <http://www.gnu.org/licenses/>.
package pseudo
// An Accessor provides access to T values held in other types.
type Accessor[Container any, T any] struct {
get func(Container) *Type
set func(Container, *Type)
}
// NewAccessor constructs a new [Accessor]. The `get` function MUST return a
// [Type] holding a T.
func NewAccessor[C any, T any](get func(C) *Type, set func(C, *Type)) Accessor[C, T] {
return Accessor[C, T]{get, set}
}
// Get returns the T held by the Container.
func (a Accessor[C, T]) Get(from C) T {
return MustNewValue[T](a.get(from)).Get()
}
// Get returns a pointer to the T held by the Container, which is guaranteed to
// be non-nil. However, if T is itself a pointer, no guarantees are provided.
//
// Note that copying a Container might result in a shallow copy and that the *T
// returned here will therefore be shared by all copies. If this is not the
// desired behaviour, use [Accessor.Set].
func (a Accessor[C, T]) GetPointer(from C) *T {
return MustPointerTo[T](a.get(from)).Value.Get()
}
// Set sets the T carried by the Container.
func (a Accessor[C, T]) Set(on C, val T) {
a.set(on, From(val).Type)
}

View file

@ -112,11 +112,22 @@ func (e *Extras[C, R]) newForRules(c *ChainConfig, r *Rules, blockNum *big.Int,
if e.NewRules == nil {
return registeredExtras.Get().newRules()
}
rExtra := e.NewRules(c, r, e.payloads().FromChainConfig(c), blockNum, isMerge, timestamp)
rExtra := e.NewRules(c, r, e.payloads().ChainConfig.Get(c), blockNum, isMerge, timestamp)
return pseudo.From(rExtra).Type
}
func (*Extras[C, R]) payloads() (g ExtraPayloads[C, R]) { return }
func (*Extras[C, R]) payloads() ExtraPayloads[C, R] {
return ExtraPayloads[C, R]{
ChainConfig: pseudo.NewAccessor[*ChainConfig, C](
(*ChainConfig).extraPayload,
func(c *ChainConfig, t *pseudo.Type) { c.extra = t },
),
Rules: pseudo.NewAccessor[*Rules, R](
(*Rules).extraPayload,
func(r *Rules, t *pseudo.Type) { r.extra = t },
),
}
}
// mustBeStructOrPointerToOne panics if `T` isn't a struct or a *struct.
func mustBeStructOrPointerToOne[T any]() {
@ -144,60 +155,20 @@ func notStructMessage[T any]() string {
// [ChainConfig] and [Rules] structs. The only valid way to construct an
// instance is by a call to [RegisterExtras].
type ExtraPayloads[C ChainConfigHooks, R RulesHooks] struct {
_ struct{} // make godoc show unexported fields so nobody tries to make their own instance ;)
}
// FromChainConfig returns the ChainConfig's extra payload.
func (ExtraPayloads[C, R]) FromChainConfig(c *ChainConfig) C {
return pseudo.MustNewValue[C](c.extraPayload()).Get()
}
// PointerFromChainConfig returns a pointer to the ChainConfig's extra payload.
// This is guaranteed to be non-nil.
//
// Note that copying a ChainConfig by dereferencing a pointer will result in a
// shallow copy and that the *C returned here will therefore be shared by all
// copies. If this is not the desired behaviour, use
// [ExtraPayloads.SetOnChainConfig].
func (ExtraPayloads[C, R]) PointerFromChainConfig(c *ChainConfig) *C {
return pseudo.MustPointerTo[C](c.extraPayload()).Value.Get()
}
// SetOnChainConfig sets the ChainConfig's extra payload.
func (e ExtraPayloads[C, R]) SetOnChainConfig(cc *ChainConfig, val C) {
cc.extra = pseudo.From(val).Type
ChainConfig pseudo.Accessor[*ChainConfig, C]
Rules pseudo.Accessor[*Rules, R]
}
// 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.
func (e ExtraPayloads[C, R]) hooksFromChainConfig(c *ChainConfig) ChainConfigHooks {
return e.FromChainConfig(c)
}
// FromRules returns the Rules' extra payload.
func (ExtraPayloads[C, R]) FromRules(r *Rules) R {
return pseudo.MustNewValue[R](r.extraPayload()).Get()
}
// PointerFromRules returns a pointer to the Rules's extra payload. This is
// guaranteed to be non-nil.
//
// Note that copying a Rules by dereferencing a pointer will result in a shallow
// copy and that the *R returned here will therefore be shared by all copies. If
// this is not the desired behaviour, use [ExtraPayloads.SetOnRules].
func (ExtraPayloads[C, R]) PointerFromRules(r *Rules) *R {
return pseudo.MustPointerTo[R](r.extraPayload()).Value.Get()
}
// SetOnRules sets the Rules' extra payload.
func (e ExtraPayloads[C, R]) SetOnRules(r *Rules, val R) {
r.extra = pseudo.From(val).Type
return e.ChainConfig.Get(c)
}
// hooksFromRules is the [RulesHooks] equivalent of hooksFromChainConfig().
func (e ExtraPayloads[C, R]) hooksFromRules(r *Rules) RulesHooks {
return e.FromRules(r)
return e.Rules.Get(r)
}
// addRulesExtra is called at the end of [ChainConfig.Rules]; it exists to

View file

@ -160,30 +160,30 @@ func TestModificationOfZeroExtras(t *testing.T) {
// closure is demonstrably over the original zero values.
assertChainConfigExtra := func(t *testing.T, want ccExtra, msg string) {
t.Helper()
assert.Equalf(t, want, extras.FromChainConfig(config), "%T: "+msg, &config)
assert.Equalf(t, want, extras.ChainConfig.Get(config), "%T: "+msg, &config)
}
assertRulesExtra := func(t *testing.T, want rulesExtra, msg string) {
t.Helper()
assert.Equalf(t, want, extras.FromRules(rules), "%T: "+msg, &rules)
assert.Equalf(t, want, extras.Rules.Get(rules), "%T: "+msg, &rules)
}
assertChainConfigExtra(t, ccExtra{}, "zero value")
assertRulesExtra(t, rulesExtra{}, "zero value")
const answer = 42
extras.PointerFromChainConfig(config).X = answer
extras.ChainConfig.GetPointer(config).X = answer
assertChainConfigExtra(t, ccExtra{X: answer}, "after setting via pointer field")
const pi = 314159
extras.PointerFromRules(rules).X = pi
extras.Rules.GetPointer(rules).X = pi
assertRulesExtra(t, rulesExtra{X: pi}, "after setting via pointer field")
ccReplace := ccExtra{X: 142857}
extras.SetOnChainConfig(config, ccReplace)
extras.ChainConfig.Set(config, ccReplace)
assertChainConfigExtra(t, ccReplace, "after replacement of entire extra via `*pointer = x`")
rulesReplace := rulesExtra{X: 18101986}
extras.SetOnRules(rules, rulesReplace)
extras.Rules.Set(rules, rulesReplace)
assertRulesExtra(t, rulesReplace, "after replacement of entire extra via `*pointer = x`")
if t.Failed() {
@ -199,27 +199,27 @@ func TestModificationOfZeroExtras(t *testing.T) {
ccCopy := *config
t.Run("ChainConfig", func(t *testing.T) {
assert.Equal(t, extras.FromChainConfig(&ccCopy), ccReplace, "extras copied")
assert.Equal(t, extras.ChainConfig.Get(&ccCopy), ccReplace, "extras copied")
extras.PointerFromChainConfig(&ccCopy).X = seqUp
extras.ChainConfig.GetPointer(&ccCopy).X = seqUp
assertChainConfigExtra(t, ccExtra{X: seqUp}, "original changed via copied.PointerFromChainConfig because copy only shallow")
ccReplace = ccExtra{X: seqDown}
extras.SetOnChainConfig(&ccCopy, ccReplace)
assert.Equal(t, extras.FromChainConfig(&ccCopy), ccReplace, "SetOnChainConfig effect")
extras.ChainConfig.Set(&ccCopy, ccReplace)
assert.Equal(t, extras.ChainConfig.Get(&ccCopy), ccReplace, "SetOnChainConfig effect")
assertChainConfigExtra(t, ccExtra{X: seqUp}, "original unchanged after copied.SetOnChainConfig")
})
rCopy := *rules
t.Run("Rules", func(t *testing.T) {
assert.Equal(t, extras.FromRules(&rCopy), rulesReplace, "extras copied")
assert.Equal(t, extras.Rules.Get(&rCopy), rulesReplace, "extras copied")
extras.PointerFromRules(&rCopy).X = seqUp
extras.Rules.GetPointer(&rCopy).X = seqUp
assertRulesExtra(t, rulesExtra{X: seqUp}, "original changed via copied.PointerFromRuels because copy only shallow")
rulesReplace = rulesExtra{X: seqDown}
extras.SetOnRules(&rCopy, rulesReplace)
assert.Equal(t, extras.FromRules(&rCopy), rulesReplace, "SetOnRules effect")
extras.Rules.Set(&rCopy, rulesReplace)
assert.Equal(t, extras.Rules.Get(&rCopy), rulesReplace, "SetOnRules effect")
assertRulesExtra(t, rulesExtra{X: seqUp}, "original unchanged after copied.SetOnRules")
})
})

View file

@ -85,12 +85,12 @@ type RulesExtra struct {
// FromChainConfig returns the extra payload carried by the ChainConfig.
func FromChainConfig(c *params.ChainConfig) ChainConfigExtra {
return payloads.FromChainConfig(c)
return payloads.ChainConfig.Get(c)
}
// FromRules returns the extra payload carried by the Rules.
func FromRules(r *params.Rules) RulesExtra {
return payloads.FromRules(r)
return payloads.Rules.Get(r)
}
// myForkPrecompiledContracts is analogous to the vm.PrecompiledContracts<Fork>

View file

@ -36,7 +36,7 @@ func TestChainConfigHooks_Description(t *testing.T) {
hooks := &hookstest.Stub{
DescriptionSuffix: "Arran was here",
}
hooks.Register(t).SetOnChainConfig(c, hooks)
hooks.Register(t).ChainConfig.Set(c, hooks)
require.Equal(t, want, c.Description(), "ChainConfigHooks.Description() is appended to non-extras equivalent")
}
@ -49,7 +49,7 @@ func TestChainConfigHooks_CheckConfigForkOrder(t *testing.T) {
hooks := &hookstest.Stub{
CheckConfigForkOrderFn: func() error { return err },
}
hooks.Register(t).SetOnChainConfig(c, hooks)
hooks.Register(t).ChainConfig.Set(c, hooks)
require.Equal(t, err, c.CheckConfigForkOrder(), "CheckConfigForkOrder() with error-producing hook")
}
@ -72,7 +72,7 @@ func TestChainConfigHooks_CheckConfigCompatible(t *testing.T) {
hooks := &hookstest.Stub{
CheckConfigCompatibleFn: makeCompatErr,
}
hooks.Register(t).SetOnChainConfig(c, hooks)
hooks.Register(t).ChainConfig.Set(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")
}