From 1bccf4f2ddb28ff440f9e24f46506a2e98ad2a0d Mon Sep 17 00:00:00 2001 From: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Date: Thu, 16 Oct 2025 15:27:15 +0100 Subject: [PATCH] refactor!: temporary extras require proof of global lock (#238) ## Why this should be merged The `temporary.WithTempRegisteredExtras()` global lock introduced in #234 wasn't fit for purpose when used in `coreth` as it required central coordination of registration, types, and usage of the payload accessor. ## How this works Instead of a central registration point, the new `libevm.WithTemporaryExtrasLock()` function takes out a global lock and provides the caller with a handle that proves the lock is held. All of the override functions, e.g. `params.WithTempRegisteredExtras()` now require a current lock, which will be propagated by the respective `coreth` functions. See https://github.com/ava-labs/coreth/pull/1328 for intended usage in `coreth` and `subnet-evm`. A consumer of both of these can then safely do the following: ```go import ( "github.com/ava-labs/libevm/libevm" coreth "github.com/ava-labs/coreth/plugin/evm" subnet "github.com/ava-labs/subnet-evm/plugin/evm" ) // asCChain calls `fn` while emulating `coreth`. It is safe for concurrent usage with [asSubnetEVM]. func asCChain(fn func() error) error { return libevm.WithTemporaryExtrasLock(func(l libevm.ExtrasLock) error { return coreth.WithTempRegisteredLibEVMExtras(l, fn) }) } // asSubnetEVM calls `fn` while emulating `subnet-evm`. It is safe for concurrent usage with [asCChain]. func asSubnetEVM(fn func() error) error { return libevm.WithTemporaryExtrasLock(func(l libevm.ExtrasLock) error { return subnet.WithTempRegisteredLibEVMExtras(l, fn) }) } ``` ## How this was tested Unit test of the new function plus existing integration tests of all modified code. --- core/state/statedb.libevm.go | 8 +++- core/state/statedb.libevm_test.go | 17 +++++--- core/types/rlp_payload.libevm.go | 8 +++- core/types/tempextras.libevm_test.go | 25 ++++++----- core/vm/evm.libevm_test.go | 11 +++-- core/vm/hooks.libevm.go | 8 +++- libevm/extraslock.go | 63 +++++++++++++++++++++++++++ libevm/extraslock_test.go | 52 ++++++++++++++++++++++ libevm/register/register.go | 13 +++--- libevm/register/register_test.go | 18 ++++++-- libevm/temporary/temporary.go | 65 ---------------------------- params/config.libevm.go | 11 +++-- params/config.libevm_test.go | 28 +++++++----- 13 files changed, 213 insertions(+), 114 deletions(-) create mode 100644 libevm/extraslock.go create mode 100644 libevm/extraslock_test.go delete mode 100644 libevm/temporary/temporary.go diff --git a/core/state/statedb.libevm.go b/core/state/statedb.libevm.go index 5b9d7d1042..327ae1dc5b 100644 --- a/core/state/statedb.libevm.go +++ b/core/state/statedb.libevm.go @@ -21,6 +21,7 @@ import ( "github.com/ava-labs/libevm/common" "github.com/ava-labs/libevm/core/state/snapshot" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/libevm/register" "github.com/ava-labs/libevm/libevm/stateconf" ) @@ -91,8 +92,11 @@ func RegisterExtras(s StateDBHooks) { // consumers that require access to extras. Said consumers SHOULD NOT, however // call this function directly. Use the libevm/temporary.WithRegisteredExtras() // function instead as it atomically overrides all possible packages. -func WithTempRegisteredExtras(s StateDBHooks, fn func()) { - registeredExtras.TempOverride(s, fn) +func WithTempRegisteredExtras(lock libevm.ExtrasLock, s StateDBHooks, fn func() error) error { + if err := lock.Verify(); err != nil { + return err + } + return registeredExtras.TempOverride(s, fn) } // TestOnlyClearRegisteredExtras clears the arguments previously passed to diff --git a/core/state/statedb.libevm_test.go b/core/state/statedb.libevm_test.go index 5cb3c8d5ac..4c3f17827a 100644 --- a/core/state/statedb.libevm_test.go +++ b/core/state/statedb.libevm_test.go @@ -27,6 +27,7 @@ import ( "github.com/ava-labs/libevm/core/state/snapshot" "github.com/ava-labs/libevm/core/types" "github.com/ava-labs/libevm/ethdb" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/libevm/stateconf" "github.com/ava-labs/libevm/trie" "github.com/ava-labs/libevm/trie/trienode" @@ -216,13 +217,17 @@ func TestTransformStateKey(t *testing.T) { assertCommittedEq(t, flippedKey, flippedVal, noTransform) t.Run("WithTempRegisteredExtras", func(t *testing.T) { - WithTempRegisteredExtras(noopHooks{}, func() { - // No-op hooks are equivalent to using the `noTransform` option. - // NOTE this is NOT the intended usage of [WithTempRegisteredExtras] - // and is simply an easy way to test the temporary registration. - assertEq(t, regularKey, regularVal) - assertEq(t, flippedKey, flippedVal) + err := libevm.WithTemporaryExtrasLock(func(lock libevm.ExtrasLock) error { + return WithTempRegisteredExtras(lock, noopHooks{}, func() error { + // No-op hooks are equivalent to using the `noTransform` option. + // NOTE this is NOT the intended usage of [WithTempRegisteredExtras] + // and is simply an easy way to test the temporary registration. + assertEq(t, regularKey, regularVal) + assertEq(t, flippedKey, flippedVal) + return nil + }) }) + require.NoError(t, err) }) updatedVal := common.Hash{'u', 'p', 'd', 'a', 't', 'e', 'd'} diff --git a/core/types/rlp_payload.libevm.go b/core/types/rlp_payload.libevm.go index 82f3da30a8..f72790ad35 100644 --- a/core/types/rlp_payload.libevm.go +++ b/core/types/rlp_payload.libevm.go @@ -21,6 +21,7 @@ import ( "io" "github.com/ava-labs/libevm/common" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/libevm/pseudo" "github.com/ava-labs/libevm/libevm/register" "github.com/ava-labs/libevm/libevm/testonly" @@ -114,9 +115,12 @@ func WithTempRegisteredExtras[ H, B, SA any, HPtr HeaderHooksPointer[H], BPtr BlockBodyHooksPointer[B, BPtr], -](fn func(ExtraPayloads[HPtr, BPtr, SA])) { +](lock libevm.ExtrasLock, fn func(ExtraPayloads[HPtr, BPtr, SA]) error) error { + if err := lock.Verify(); err != nil { + return err + } payloads, ctors := payloadsAndConstructors[H, HPtr, B, BPtr, SA]() - registeredExtras.TempOverride(ctors, func() { fn(payloads) }) + return registeredExtras.TempOverride(ctors, func() error { return fn(payloads) }) } // A HeaderHooksPointer is a type constraint for an implementation of diff --git a/core/types/tempextras.libevm_test.go b/core/types/tempextras.libevm_test.go index eb5bfb4bdf..b01b780213 100644 --- a/core/types/tempextras.libevm_test.go +++ b/core/types/tempextras.libevm_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/rlp" ) @@ -58,19 +59,23 @@ func TestTempRegisteredExtras(t *testing.T) { t.Run("before_temp", testPrimaryExtras) t.Run("WithTempRegisteredExtras", func(t *testing.T) { - WithTempRegisteredExtras(func(extras ExtraPayloads[*NOOPHeaderHooks, *tempBlockBodyHooks, bool]) { - const val = "Hello, world" - b := new(Block) - payload := &tempBlockBodyHooks{X: val} - extras.Block.Set(b, payload) + err := libevm.WithTemporaryExtrasLock(func(lock libevm.ExtrasLock) error { + return WithTempRegisteredExtras(lock, func(extras ExtraPayloads[*NOOPHeaderHooks, *tempBlockBodyHooks, bool]) error { + const val = "Hello, world" + b := new(Block) + payload := &tempBlockBodyHooks{X: val} + extras.Block.Set(b, payload) - got, err := rlp.EncodeToBytes(b) - require.NoErrorf(t, err, "rlp.EncodeToBytes(%T) with %T hooks", b, extras.Block.Get(b)) - want, err := rlp.EncodeToBytes([]string{val}) - require.NoErrorf(t, err, "rlp.EncodeToBytes(%T{%[1]v})", []string{val}) + got, err := rlp.EncodeToBytes(b) + require.NoErrorf(t, err, "rlp.EncodeToBytes(%T) with %T hooks", b, extras.Block.Get(b)) + want, err := rlp.EncodeToBytes([]string{val}) + require.NoErrorf(t, err, "rlp.EncodeToBytes(%T{%[1]v})", []string{val}) - assert.Equalf(t, want, got, "rlp.EncodeToBytes(%T) with %T hooks", b, payload) + assert.Equalf(t, want, got, "rlp.EncodeToBytes(%T) with %T hooks", b, payload) + return nil + }) }) + require.NoError(t, err) }) t.Run("after_temp", testPrimaryExtras) } diff --git a/core/vm/evm.libevm_test.go b/core/vm/evm.libevm_test.go index cc98d4bc1a..c2e6708f72 100644 --- a/core/vm/evm.libevm_test.go +++ b/core/vm/evm.libevm_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/params" ) @@ -72,10 +73,14 @@ func TestOverrideNewEVMArgs(t *testing.T) { assertChainID(t, chainID) t.Run("WithTempRegisteredHooks", func(t *testing.T) { - override := evmArgOverrider{newEVMchainID: 24680} - WithTempRegisteredHooks(&override, func() { - assertChainID(t, override.newEVMchainID) + err := libevm.WithTemporaryExtrasLock(func(lock libevm.ExtrasLock) error { + override := evmArgOverrider{newEVMchainID: 24680} + return WithTempRegisteredHooks(lock, &override, func() error { + assertChainID(t, override.newEVMchainID) + return nil + }) }) + require.NoError(t, err) t.Run("after", func(t *testing.T) { assertChainID(t, chainID) }) diff --git a/core/vm/hooks.libevm.go b/core/vm/hooks.libevm.go index 16e49f3bdc..9c4feb1339 100644 --- a/core/vm/hooks.libevm.go +++ b/core/vm/hooks.libevm.go @@ -17,6 +17,7 @@ package vm import ( + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/libevm/register" "github.com/ava-labs/libevm/params" ) @@ -36,8 +37,11 @@ func RegisterHooks(h Hooks) { // consumers that require access to extras. Said consumers SHOULD NOT, however // call this function directly. Use the libevm/temporary.WithRegisteredExtras() // function instead as it atomically overrides all possible packages. -func WithTempRegisteredHooks(h Hooks, fn func()) { - libevmHooks.TempOverride(h, fn) +func WithTempRegisteredHooks(lock libevm.ExtrasLock, h Hooks, fn func() error) error { + if err := lock.Verify(); err != nil { + return err + } + return libevmHooks.TempOverride(h, fn) } // TestOnlyClearRegisteredHooks clears the [Hooks] previously passed to diff --git a/libevm/extraslock.go b/libevm/extraslock.go new file mode 100644 index 0000000000..4032624126 --- /dev/null +++ b/libevm/extraslock.go @@ -0,0 +1,63 @@ +// Copyright 2025 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 +// . + +package libevm + +import ( + "errors" + "sync" + "sync/atomic" +) + +var ( + extrasMu sync.Mutex + extrasHandle atomic.Uint64 +) + +// An ExtrasLock is a handle that proves a current call to +// [WithTemporaryExtrasLock]. +type ExtrasLock struct { + handle *uint64 +} + +// WithTemporaryExtrasLock takes a global lock and calls `fn` with a handle that +// can be used to prove that the lock is held. All package-specific temporary +// overrides require this proof. +// +// WithTemporaryExtrasLock MUST NOT be used on a live chain. It is solely +// intended for off-chain consumers that require access to extras. +func WithTemporaryExtrasLock(fn func(lock ExtrasLock) error) error { + extrasMu.Lock() + defer func() { + extrasHandle.Add(1) + extrasMu.Unlock() + }() + + v := extrasHandle.Load() + return fn(ExtrasLock{&v}) +} + +// ErrExpiredExtrasLock is returned by [ExtrasLock.Verify] if the lock has been +// persisted beyond the call to [WithTemporaryExtrasLock] that created it. +var ErrExpiredExtrasLock = errors.New("libevm.ExtrasLock expired") + +// Verify verifies that the lock is valid. +func (l ExtrasLock) Verify() error { + if *l.handle != extrasHandle.Load() { + return ErrExpiredExtrasLock + } + return nil +} diff --git a/libevm/extraslock_test.go b/libevm/extraslock_test.go new file mode 100644 index 0000000000..bdd0adeff9 --- /dev/null +++ b/libevm/extraslock_test.go @@ -0,0 +1,52 @@ +// Copyright 2025 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 +// . + +package libevm_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + // Testing from outside the package to guarantee usage of the public API only. + . "github.com/ava-labs/libevm/libevm" +) + +func TestExtrasLock(t *testing.T) { + var zero ExtrasLock + assert.Panics(t, func() { _ = zero.Verify() }, "Verify() method of zero-value ExtrasLock{}") + + testIntegration := func(t *testing.T) { + t.Helper() + require.NoError(t, + WithTemporaryExtrasLock((ExtrasLock).Verify), + "WithTemporaryExtrasLock((ExtrasLock).Verify)", + ) + } + t.Run("initial_usage", testIntegration) + + t.Run("lock_expiration", func(t *testing.T) { + var persisted ExtrasLock + require.NoError(t, WithTemporaryExtrasLock(func(l ExtrasLock) error { + persisted = l + return l.Verify() + })) + assert.ErrorIs(t, persisted.Verify(), ErrExpiredExtrasLock, "Verify() of persisted ExtrasLock") + }) + + t.Run("repeat_usage", testIntegration) +} diff --git a/libevm/register/register.go b/libevm/register/register.go index b8bb246e86..4aedd37010 100644 --- a/libevm/register/register.go +++ b/libevm/register/register.go @@ -72,8 +72,8 @@ func (o *AtMostOnce[T]) TestOnlyClear() { // // It is valid to call this method with or without a prior call to // [AtMostOnce.Register]. -func (o *AtMostOnce[T]) TempOverride(with T, fn func()) { - o.temp(&with, fn) +func (o *AtMostOnce[T]) TempOverride(with T, fn func() error) error { + return o.temp(&with, fn) } // TempClear calls `fn`, clearing any registered `T`, but only for the life of @@ -81,13 +81,14 @@ func (o *AtMostOnce[T]) TempOverride(with T, fn func()) { // // It is valid to call this method with or without a prior call to // [AtMostOnce.Register]. -func (o *AtMostOnce[T]) TempClear(fn func()) { - o.temp(nil, fn) +func (o *AtMostOnce[T]) TempClear(fn func() error) error { + return o.temp(nil, fn) } -func (o *AtMostOnce[T]) temp(with *T, fn func()) { +func (o *AtMostOnce[T]) temp(with *T, fn func() error) error { old := o.v o.v = with - fn() + err := fn() o.v = old + return err } diff --git a/libevm/register/register_test.go b/libevm/register/register_test.go index fa8e1f7071..615a01ea39 100644 --- a/libevm/register/register_test.go +++ b/libevm/register/register_test.go @@ -17,6 +17,7 @@ package register import ( + "errors" "testing" "github.com/stretchr/testify/assert" @@ -56,9 +57,10 @@ func TestAtMostOnce(t *testing.T) { t.Run("TempOverride", func(t *testing.T) { t.Run("during", func(t *testing.T) { - sut.TempOverride(val+1, func() { + require.NoError(t, sut.TempOverride(val+1, func() error { assertRegistered(t, val+1) - }) + return nil + })) }) t.Run("after", func(t *testing.T) { assertRegistered(t, val) @@ -67,12 +69,20 @@ func TestAtMostOnce(t *testing.T) { t.Run("TempClear", func(t *testing.T) { t.Run("during", func(t *testing.T) { - sut.TempClear(func() { + require.NoError(t, sut.TempClear(func() error { assert.False(t, sut.Registered(), "Registered()") - }) + return nil + })) }) t.Run("after", func(t *testing.T) { assertRegistered(t, val) }) }) + + t.Run("error_propagation", func(t *testing.T) { + errFoo := errors.New("foo") + fn := func() error { return errFoo } + assert.ErrorIs(t, sut.TempOverride(0, fn), errFoo, "TempOverride()") //nolint:testifylint // Blindly using require is an anti-pattern!!! + assert.ErrorIs(t, sut.TempClear(fn), errFoo, "TempClear()") + }) } diff --git a/libevm/temporary/temporary.go b/libevm/temporary/temporary.go deleted file mode 100644 index dae3a5b772..0000000000 --- a/libevm/temporary/temporary.go +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright 2025 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 -// . - -// Package temporary provides thread-safe, temporary registration of all libevm -// hooks and payloads. -package temporary - -import ( - "sync" - - "github.com/ava-labs/libevm/core/state" - "github.com/ava-labs/libevm/core/types" - "github.com/ava-labs/libevm/core/vm" - "github.com/ava-labs/libevm/params" -) - -var mu sync.Mutex - -// WithRegisteredExtras takes a global lock and temporarily registers [params], -// [state], [types], and [vm] extras before calling the provided function. It -// can be thought of as an atomic call to all functions equivalent to -// [params.WithTempRegisteredExtras]. -// -// This is the *only* safe way to override libevm functionality. Direct calls to -// the package-specific temporary registration functions are not advised. -// -// WithRegisteredExtras MUST NOT be used on a live chain. It is solely intended -// for off-chain consumers that require access to extras. -func WithRegisteredExtras[ - C params.ChainConfigHooks, R params.RulesHooks, - H, B, SA any, - HPtr types.HeaderHooksPointer[H], - BPtr types.BlockBodyHooksPointer[B, BPtr], -]( - paramsExtras params.Extras[C, R], - sdbHooks state.StateDBHooks, - vmHooks vm.Hooks, - fn func(params.ExtraPayloads[C, R], types.ExtraPayloads[HPtr, BPtr, SA]), -) { - mu.Lock() - defer mu.Unlock() - - params.WithTempRegisteredExtras(paramsExtras, func(paramsPayloads params.ExtraPayloads[C, R]) { - types.WithTempRegisteredExtras(func(typesPayloads types.ExtraPayloads[HPtr, BPtr, SA]) { - state.WithTempRegisteredExtras(sdbHooks, func() { - vm.WithTempRegisteredHooks(vmHooks, func() { - fn(paramsPayloads, typesPayloads) - }) - }) - }) - }) -} diff --git a/params/config.libevm.go b/params/config.libevm.go index 7d05893bb1..0cbae661a2 100644 --- a/params/config.libevm.go +++ b/params/config.libevm.go @@ -21,6 +21,7 @@ import ( "math/big" "reflect" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/libevm/pseudo" "github.com/ava-labs/libevm/libevm/register" "github.com/ava-labs/libevm/log" @@ -106,11 +107,15 @@ func payloadsAndConstructors[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ( // call this function directly. Use the libevm/temporary.WithRegisteredExtras() // function instead as it atomically overrides all possible packages. func WithTempRegisteredExtras[C ChainConfigHooks, R RulesHooks]( + lock libevm.ExtrasLock, e Extras[C, R], - fn func(ExtraPayloads[C, R]), -) { + fn func(ExtraPayloads[C, R]) error, +) error { + if err := lock.Verify(); err != nil { + return err + } payloads, ctors := payloadsAndConstructors(e) - registeredExtras.TempOverride(ctors, func() { fn(payloads) }) + return registeredExtras.TempOverride(ctors, func() error { return fn(payloads) }) } // TestOnlyClearRegisteredExtras clears the [Extras] previously passed to diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index da6f04ce4e..a9b82b446f 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 the libevm authors. +// Copyright 2024-2025 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 @@ -13,6 +13,7 @@ // You should have received a copy of the GNU Lesser General Public License // along with the go-ethereum library. If not, see // . + package params import ( @@ -23,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/ava-labs/libevm/libevm" "github.com/ava-labs/libevm/libevm/pseudo" "github.com/ava-labs/libevm/libevm/register" ) @@ -329,16 +331,20 @@ func TestTempRegisteredExtras(t *testing.T) { t.Run("before_temp", testPrimaryExtras) t.Run("WithTempRegisteredExtras", func(t *testing.T) { - WithTempRegisteredExtras( - override, - func(extras ExtraPayloads[overrideCC, overrideRules]) { // deliberately shadow `extras` - assertRulesCopiedFromChainConfig( - t, extras, "hello, world", - func(cc *overrideCC, x string) { cc.X = x }, - func(r *overrideRules) string { return r.X }, - ) - }, - ) + err := libevm.WithTemporaryExtrasLock(func(lock libevm.ExtrasLock) error { + return WithTempRegisteredExtras( + lock, override, + func(extras ExtraPayloads[overrideCC, overrideRules]) error { // deliberately shadow `extras` + assertRulesCopiedFromChainConfig( + t, extras, "hello, world", + func(cc *overrideCC, x string) { cc.X = x }, + func(r *overrideRules) string { return r.X }, + ) + return nil + }, + ) + }) + require.NoError(t, err) }) t.Run("after_temp", testPrimaryExtras) }