-
Notifications
You must be signed in to change notification settings - Fork 2
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: read-only stateful precompiles #20
Conversation
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.
Pretty sure this is OK, it will force the precompiles to have to decide when they are using read-only vs modifying state but that seems a minor change + good for safety to me.
@@ -69,6 +80,69 @@ func (p statefulPrecompile) Run([]byte) ([]byte, error) { | |||
panic(fmt.Sprintf("BUG: call to %T.Run(); MUST call %T", p, p.run)) | |||
} | |||
|
|||
// A PrecompileEnvironment provides information about the context in which a | |||
// precompiled contract is being run. | |||
type PrecompileEnvironment interface { |
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.
Question: in general is it preferable to define interfaces like this (no param getters) or to have a struct like:
type PrecompileEnvironment interface { | |
type PrecompileEnvironment struct { | |
Rules params.Rules | |
ReadOnly bool | |
StateDB StateDB | |
ReadOnlyState libevm.StateReader | |
Addresses *libevm.AddressContext | |
} |
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.
Leaving this unresolved to reply to it later because it's a nuanced answer.
That was entirely by design. I figured that since there would already be read-only checks that would have to be refactored to use the |
Why this should be merged
Adds support for signalling to a stateful precompile that it's running in a read-only context.
How this works
The
vm.PrecompiledStatefulRun
function signature was becoming bloated so everything except forinput []byte
has been collapsed into avm.PrecompileEnvironment
. Using a single argument also means that future features won't be breaking changes. AReadOnly() bool
method is available from theenv
. I called it an environment and not a context so that the idiomatic variable name can beenv
and notctx
.Both a
StateDB() vm.StateDB
and aReadOnlyState() libevm.StateReader
are available from anenv
. The read-only version never returns nil while the fullStateDB()
method returns a non-nil state iff not in a read-only state (i.e. whenReadOnly()
returnsfalse
). Without this, it would be too easy for a precompile implementation to forget to checkReadOnly()
before writing to state.How this was tested
A precompile is in a read-only state when there is at least one
STATICCALL
in the callstack that led to it being run. This can happen when either (a) there is a directvm.EVM.StaticCall()
to the precompile or; (b) there is aStaticCall()
to some other contract that later calls the precompile via any*CALL*
op code.Scenario (a) is tested by extending the existing
TestNewStatefulPrecompile
test. Scenario (b) is more complex but thoroughly documented inTestPrecompileFromReadOnlyState
.