refactor!: temporary extras require proof of global lock

[ARR4N]

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 ava-labs/coreth#1328 for intended usage in coreth and subnet-evm. A consumer of both of these can then safely do the following:

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.

[ceyonur]

mostly questions

[ceyonur]

I wonder if we can omit taking the lock in the fn

[ARR4N]

What do you mean? The fn doesn't take it; it waits for it to be taken before being called.

[ceyonur]

https://github.com/ava-labs/libevm/pull/238/files#r2426110219

[ceyonur]

Why do we need to pass the lock here and defer the lock verification to the fn? Can't we verify that here? Are we expecting fn to be called with different locks?

[ARR4N]

Nope, we're expecting fn to pass the lock to every extras override, each of which does its own verification. This is an example of an fn and this is a test that demonstrates it being used to temporarily emulate all C-Chain behaviour via this package.

[ceyonur]

1- This pattern could be difficult for a consumer. I wonder if we can move the complexity either to libevm or to coreth/subnet-evm so that the consumer can use something like this

evm.WithTempRegisteredLibEVMExtras(func() error { 
	t.Run("payloads", func(t *testing.T) {
		for pkg, fn := range payloadTests {
			t.Run(pkg, fn)
		}
	})
	return nil
})

TLDR; IMO locks should be abstracted away.

[ceyonur]

why do we have this handle as a uint64? Or why do we need the handle? Aren't this implying if the lock is held or not?

[ceyonur]

1- This pattern could be difficult for a consumer. I wonder if we can move the complexity either to libevm or to coreth/subnet-evm so that the consumer can use something like this

evm.WithTempRegisteredLibEVMExtras(func() error { 
	t.Run("payloads", func(t *testing.T) {
		for pkg, fn := range payloadTests {
			t.Run(pkg, fn)
		}
	})
	return nil
})

TLDR; IMO locks should be abstracted away.

[ceyonur]

https://github.com/ava-labs/libevm/pull/238/files#r2426110219

[ceyonur]

I wonder if taking multiple fns (i.e WithTemporaryExtrasLock(fns ...func(lock ExtrasLock) error)` would help cleaning this nested calls.