Add flag to disable ArbOwner outside on-chain execution#4591
Merged
joshuacolvin0 merged 11 commits intomasterfrom Apr 7, 2026
Merged
Add flag to disable ArbOwner outside on-chain execution#4591joshuacolvin0 merged 11 commits intomasterfrom
joshuacolvin0 merged 11 commits intomasterfrom
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.14% 34.16% +0.02%
==========================================
Files 495 495
Lines 59024 59041 +17
==========================================
+ Hits 20154 20172 +18
- Misses 35312 35319 +7
+ Partials 3558 3550 -8 |
Contributor
❌ 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>
bragaigor
commented
Apr 1, 2026
Comment on lines
+20
to
+25
| var arbOwnerPrecompile *precompiles.OwnerPrecompile | ||
|
|
||
| func GetOwnerPrecompile() *precompiles.OwnerPrecompile { | ||
| return arbOwnerPrecompile | ||
| } | ||
|
|
Contributor
Author
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>
bragaigor
commented
Apr 1, 2026
diegoximenes
requested changes
Apr 1, 2026
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>
diegoximenes
requested changes
Apr 2, 2026
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
diegoximenes
previously approved these changes
Apr 6, 2026
tsahee
reviewed
Apr 6, 2026
| tx, err := arbOwner.AddChainOwner(&auth, common.HexToAddress("0xdeadbeef")) | ||
| Require(t, err) | ||
| _, err = builder.L2.EnsureTxSucceeded(tx) | ||
| Require(t, err) |
Contributor
There was a problem hiding this comment.
We want a final cleanup here that sets the static to false (default), so that whatever order tests are running other tests will see the same value
Contributor
Author
There was a problem hiding this comment.
that's probably line 1376 where I included gethhook.GetOwnerPrecompile().SetDisableEthCall(false) inside defer, or you meant something else?
joshuacolvin0
requested changes
Apr 6, 2026
| tx, err := arbOwner.AddChainOwner(&auth, common.HexToAddress("0xdeadbeef")) | ||
| Require(t, err) | ||
| _, err = builder.L2.EnsureTxSucceeded(tx) | ||
| Require(t, err) |
tsahee
previously approved these changes
Apr 6, 2026
| tx, err := arbOwner.AddChainOwner(&auth, common.HexToAddress("0xdeadbeef")) | ||
| Require(t, err) | ||
| _, err = builder.L2.EnsureTxSucceeded(tx) | ||
| Require(t, err) |
Contributor
|
@clabot check |
Signed-off-by: Igor Braga <5835477+bragaigor@users.noreply.github.com>
bragaigor
dismissed stale reviews from joshuacolvin0, tsahee, and diegoximenes
via
a77614a
Contributor
Author
|
had to resolve a minor conflict on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--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