-
Notifications
You must be signed in to change notification settings - Fork 5
refactor!: temporary extras require proof of global lock #238
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
base: main
Are you sure you want to change the base?
Changes from all commits
2a5e630
9f50a13
d6d5935
264c00f
f747f0a
3ee649c
93876ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
// <http://www.gnu.org/licenses/>. | ||
|
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we can omit taking the lock in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean? The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if taking multiple |
||
extrasMu.Lock() | ||
defer func() { | ||
extrasHandle.Add(1) | ||
extrasMu.Unlock() | ||
ceyonur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}() | ||
|
||
v := extrasHandle.Load() | ||
return fn(ExtrasLock{&v}) | ||
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to pass the lock here and defer the lock verification to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
TLDR; IMO locks should be abstracted away. |
||
} | ||
|
||
// 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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
// <http://www.gnu.org/licenses/>. | ||
|
||
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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have this handle as a
uint64
? Or why do we need thehandle
? Aren't this implying if the lock is held or not?