Add flag to disable ArbOwner outside on-chain execution#4591
Add flag to disable ArbOwner outside on-chain execution#4591
Conversation
- When `DisableOffchainArbOwner` is set to `true` in chain config, `OwnerPrecompile.Call` panics - Defaults to `false` — no behavior change unless explicitly opted in Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4591 +/- ##
==========================================
- Coverage 34.20% 34.15% -0.05%
==========================================
Files 494 494
Lines 58926 58943 +17
==========================================
- Hits 20156 20134 -22
- Misses 35230 35264 +34
- Partials 3540 3545 +5 |
❌ 14 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Add `--execution.disable-offchain-arbowner` flag to disable ArbOwner precompile calls outside on-chain execution Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
| var arbOwnerPrecompile *precompiles.OwnerPrecompile | ||
|
|
||
| func GetOwnerPrecompile() *precompiles.OwnerPrecompile { | ||
| return arbOwnerPrecompile | ||
| } | ||
|
|
There was a problem hiding this comment.
The *OwnerPrecompile reference in gethhook is an unexported package-level variable, which we'd normally avoid. Here's why it's necessary:
Precompile instances are created during gethhook.init() — a void function that runs automatically at package load time. Node config (DisableOffchainArbOwner) only becomes available much later in ExecutionNode.Initialize(). Since init() can't return values and has no caller to hand the instance to, the pointer must be held somewhere between creation and configuration. The alternatives we evaluated:
- storing it in chain config (wrong: this is a node-level concern, not consensus state)
- a freestanding global atomic bool (worse: config detached from the object it belongs to)
- or looking it up from the VM precompile maps (fragile: requires unwrapping across multiple layers and picking an arbitrary ArbOS version map)
all the above were all strictly worse. The unexported var with an exported getter is the smallest surface: set exactly once in init(), immutable after that, and the actual config flag (disableOffchain) lives on the OwnerPrecompile struct where it belongs.
In more detail:
gethhook/geth-hook.goinit()— runs at package load time (triggered ingethexec/blockchain.goimporting gethhook). This is whereprecompiles.Precompiles()is called, which creates the*OwnerPrecompileinstance. That instance gets wrapped inArbosPrecompileWrapperand stored in the VM's global precompile maps (vm.PrecompiledContractsBeforeArbOS30, etc.). Afterinit()returns, nobody holds a direct reference to the*OwnerPrecompile— it's buried inside the wrapper inside the VM maps.gethexec.CreateExecutionNode()— runs much later, when the node is actually being set up. This is where configFetcher (which has theDisableOffchainArbOwnerflag) first becomes available.ExecutionNode.Initialize()— called afterCreateExecutionNode. This is where we want to callownerPC.SetDisableOffchain(config.DisableOffchainArbOwner). But we need the*OwnerPrecompilepointer to do that.
The gap: the instance is created in step 1, the config arrives in step 3, and there's no object that naturally carries the pointer from 1 to 3. Hence the global
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
execution/gethexec/node.go
Outdated
| ExposeMultiGas bool `koanf:"expose-multi-gas"` | ||
| RPCServer rpcserver.Config `koanf:"rpc-server"` | ||
| ConsensusRPCClient rpcclient.ClientConfig `koanf:"consensus-rpc-client" reload:"hot"` | ||
| DisableOffchainArbOwner bool `koanf:"disable-offchain-arbowner"` |
There was a problem hiding this comment.
If you check MessageRunContext, you are going to see that NonMutating happens to be the negation of ExecutedOnChain.
So using non mutating here can be more appropriate than using offchain.
There was a problem hiding this comment.
Yeah and furthermore IsNonMutating() and !IsExecutedOnChain() are equivalent:
func (c *MessageRunContext) IsNonMutating() bool {
return c.runMode == messageGasEstimationMode || c.runMode == messageEthcallMode
}
...
func (c *MessageRunContext) IsExecutedOnChain() bool {
return c.IsCommitMode() || c.runMode == messageReplayMode || c.runMode == messageRecordingMode
}@joshuacolvin0 please advice if you had any other plans for DisableOffchainArbOwner as using IsNonMutating seems to be doing exactly what DisableOffchainArbOwner. If we went with IsNonMutating then the following check:
if wrapper.disableOffchain {
txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor)
if !ok || !txProcessor.RunContext().IsExecutedOnChain() {
return nil, gasSupplied, multigas.ZeroGas(), errors.New("ArbOwner precompile is disabled outside on-chain
execution")
}
} would turn into something like:
txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor)
if !ok || txProcessor.MsgIsNonMutating() {
return nil, gasSupplied, multigas.ZeroGas(), errors.New("ArbOwner precompile is disabled outside on-chain
execution")
}There was a problem hiding this comment.
in the internal code - using either IsNonMutating or IsExecutedOnChaon seems to be identical, choose what you will.
Config name is external and should be easy to understand for users. nonmutating seems less understandable than oochain, possibly just disable-arbowner-ethcall or something similar would be best. @joshuacolvin0 your take?
There was a problem hiding this comment.
Updated DisableOffchainArbOwner to DisableArbOwnerEthCall and global from disableOffchain to disableEthCall in 5f7da96
precompiles/wrapper.go
Outdated
| ) ([]byte, uint64, multigas.MultiGas, error) { | ||
| if wrapper.disableOffchain.Load() { | ||
| txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor) | ||
| if !ok || !txProcessor.RunContext().IsExecutedOnChain() { |
There was a problem hiding this comment.
The task mentions to use IsExecutedOnChain.
Check with who created the task if you can use evm.ProcessinHook.MsgIsNonMutating() instead, which today is the negation of IsExecutedOnChain.
This way you avoid casting.
If not possible then split this if in two.
One to return an error due to casting, and another to return the error that is being proposed here.
precompiles/wrapper.go
Outdated
| if wrapper.disableOffchain.Load() { | ||
| txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor) | ||
| if !ok || !txProcessor.RunContext().IsExecutedOnChain() { | ||
| return nil, 0, multigas.ComputationGas(gasSupplied), errors.New("ArbOwner precompile is disabled outside on-chain execution") |
There was a problem hiding this comment.
Why returning 0 and multigas.ComputationGas(gasSupplied)?
I am not sure what is the expected behavior, related to those gas values, for eth_estimateGas if it returns an error.
There was a problem hiding this comment.
I think it's best to follow what the rest of Call does so modifying to return:
return nil, gasSupplied, multigas.ZeroGas(), errors.New("ArbOwner precompile is disabled outside on-chain execution")There was a problem hiding this comment.
obs: I think this likely not matter because this is only used in non mutating calls, but those returned gas values are related to when a arbowner is calling the arbowner precompile.
If another kind of user calls the precompile other gas values are being returned.
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
gethhook/geth-hook.go
Outdated
| if ownerPC, ok := arbosPrecompiles[types.ArbOwnerAddress].(*precompiles.OwnerPrecompile); ok { | ||
| arbOwnerPrecompile = ownerPC | ||
| } else { | ||
| log.Error("ArbOwner precompile is not an *OwnerPrecompile, disable-arbowner-ethcall flag will not work") |
precompiles/wrapper.go
Outdated
| if wrapper.disableEthCall { | ||
| txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor) | ||
| if !ok || !txProcessor.RunContext().IsExecutedOnChain() { |
There was a problem hiding this comment.
Can't this be simplified here?
| if wrapper.disableEthCall { | |
| txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor) | |
| if !ok || !txProcessor.RunContext().IsExecutedOnChain() { | |
| if wrapper.disableEthCall && evm.ProcessingHook.MsgIsNonMutating() { |
There was a problem hiding this comment.
Good point, I went with IsExecutedOnChain because it's more "human readable" but I like the simplification
precompiles/wrapper.go
Outdated
| if wrapper.disableOffchain.Load() { | ||
| txProcessor, ok := evm.ProcessingHook.(*arbos.TxProcessor) | ||
| if !ok || !txProcessor.RunContext().IsExecutedOnChain() { | ||
| return nil, 0, multigas.ComputationGas(gasSupplied), errors.New("ArbOwner precompile is disabled outside on-chain execution") |
There was a problem hiding this comment.
obs: I think this likely not matter because this is only used in non mutating calls, but those returned gas values are related to when a arbowner is calling the arbowner precompile.
If another kind of user calls the precompile other gas values are being returned.
| } | ||
| } | ||
|
|
||
| func TestDisableArbOwnerEthCall(t *testing.T) { |
There was a problem hiding this comment.
It would be nice to also have a test to check that eth_call/eth_estimateGas, with arbowner calls, work fine when DisableArbOwnerEthCall is set to false.
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
5 participants
--execution.disable-offchain-arbownernode config flag (defaultfalse)OwnerPrecompile.Callpanics if the run context is notIsExecutedOnChain()(blocks ethcall and gas estimation modes)atomic.Boolfield on theOwnerPrecompilestruct, configured duringExecutionNode.Initialize()viagethhook.GetOwnerPrecompile()The
*OwnerPrecompilereference is held in an unexported var in thegethhookpackage, exposed viaGetOwnerPrecompile(). This is necessary because precompile instances are created duringgethhook.init()(a void function that runs at package load time), while node config only becomes available later inExecutionNode.Initialize(). Sinceinit()cannot return values, the pointer must be held somewhere to bridge that gap. An unexported var with a getter is the smallest surface area — set once, immutable after init, and the actual config (disableOffchain) lives on theOwnerPrecompilestruct where it belongs.closes NIT-4744