-
Notifications
You must be signed in to change notification settings - Fork 718
Add flag to disable ArbOwner outside on-chain execution #4591
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: master
Are you sure you want to change the base?
Changes from all commits
d626876
8f346ce
8503d01
dce1326
a12d4d6
d7f2e7e
0870413
5f7da96
062f3d6
5060a63
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,2 @@ | ||
| ### Configuration | ||
| - Add `--execution.disable-arbowner-ethcall` flag to disable ArbOwner precompile calls outside on-chain execution |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,11 +10,13 @@ import ( | |
| "math/big" | ||
| "slices" | ||
| "sort" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
|
|
||
| "github.com/ethereum/go-ethereum" | ||
| "github.com/ethereum/go-ethereum/accounts/abi/bind" | ||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/core/types" | ||
|
|
@@ -26,6 +28,7 @@ import ( | |
| "github.com/offchainlabs/nitro/arbos/burn" | ||
| "github.com/offchainlabs/nitro/arbos/l1pricing" | ||
| "github.com/offchainlabs/nitro/cmd/chaininfo" | ||
| "github.com/offchainlabs/nitro/gethhook" | ||
| "github.com/offchainlabs/nitro/precompiles" | ||
| "github.com/offchainlabs/nitro/solgen/go/localgen" | ||
| "github.com/offchainlabs/nitro/solgen/go/precompilesgen" | ||
|
|
@@ -1357,3 +1360,85 @@ func TestArbDebugOverwriteContractCode(t *testing.T) { | |
| t.Fatal("expected code B to be", testCodeB, "got", code) | ||
| } | ||
| } | ||
|
|
||
| func TestDisableArbOwnerEthCall(t *testing.T) { | ||
|
Contributor
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. 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.
Contributor
Author
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. added in 5060a63 |
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| builder := NewNodeBuilder(ctx).DefaultConfig(t, false).DontParalellise() | ||
| builder.execConfig.DisableArbOwnerEthCall = true | ||
| cleanup := builder.Build(t) | ||
| // The DisableArbOwnerEthCall flag is set on the global OwnerPrecompile singleton, | ||
| // so it persists across tests running in the same process. We must: | ||
| // 1. Not run in parallel (DontParalellise above) to avoid affecting concurrent tests | ||
| // 2. Reset the flag on cleanup to avoid affecting subsequent tests | ||
| defer func() { | ||
| gethhook.GetOwnerPrecompile().SetDisableEthCall(false) | ||
| cleanup() | ||
| }() | ||
|
|
||
| arbOwnerABI, err := precompilesgen.ArbOwnerMetaData.GetAbi() | ||
| Require(t, err) | ||
| calldata, err := arbOwnerABI.Pack("getAllChainOwners") | ||
| Require(t, err) | ||
| arbOwnerAddr := types.ArbOwnerAddress | ||
|
|
||
| expectedErrMsg := "ArbOwner precompile is disabled outside on-chain execution" | ||
|
|
||
| // eth_call should fail | ||
| _, err = builder.L2.Client.CallContract(ctx, ethereum.CallMsg{ | ||
| To: &arbOwnerAddr, | ||
| Data: calldata, | ||
| }, nil) | ||
| if err == nil || !strings.Contains(err.Error(), expectedErrMsg) { | ||
| Fatal(t, "eth_call to ArbOwner expected error containing", expectedErrMsg, "got", err) | ||
| } | ||
|
|
||
| // eth_estimateGas should fail | ||
| _, err = builder.L2.Client.EstimateGas(ctx, ethereum.CallMsg{ | ||
| To: &arbOwnerAddr, | ||
| Data: calldata, | ||
| }) | ||
| if err == nil || !strings.Contains(err.Error(), expectedErrMsg) { | ||
| Fatal(t, "eth_estimateGas to ArbOwner expected error containing", expectedErrMsg, "got", err) | ||
| } | ||
|
|
||
| // On-chain transaction should still succeed | ||
| auth := builder.L2Info.GetDefaultTransactOpts("Owner", ctx) | ||
| auth.GasLimit = 32_000_000 | ||
| arbOwner, err := precompilesgen.NewArbOwner(arbOwnerAddr, builder.L2.Client) | ||
| Require(t, err) | ||
| tx, err := arbOwner.AddChainOwner(&auth, common.HexToAddress("0xdeadbeef")) | ||
| Require(t, err) | ||
| _, err = builder.L2.EnsureTxSucceeded(tx) | ||
| Require(t, err) | ||
| } | ||
|
|
||
| func TestArbOwnerEthCallEnabled(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| builder := NewNodeBuilder(ctx).DefaultConfig(t, false).DontParalellise() | ||
| cleanup := builder.Build(t) | ||
| defer cleanup() | ||
|
|
||
| arbOwnerABI, err := precompilesgen.ArbOwnerMetaData.GetAbi() | ||
| Require(t, err) | ||
| calldata, err := arbOwnerABI.Pack("getAllChainOwners") | ||
| Require(t, err) | ||
| arbOwnerAddr := types.ArbOwnerAddress | ||
|
|
||
| // eth_call should succeed when DisableArbOwnerEthCall is false (default) | ||
| _, err = builder.L2.Client.CallContract(ctx, ethereum.CallMsg{ | ||
| To: &arbOwnerAddr, | ||
| Data: calldata, | ||
| }, nil) | ||
| Require(t, err) | ||
|
|
||
| // eth_estimateGas should succeed when DisableArbOwnerEthCall is false (default) | ||
| _, err = builder.L2.Client.EstimateGas(ctx, ethereum.CallMsg{ | ||
| To: &arbOwnerAddr, | ||
| Data: calldata, | ||
| }) | ||
| Require(t, err) | ||
| } | ||
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.
The
*OwnerPrecompilereference 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 inExecutionNode.Initialize(). Sinceinit()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: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 theOwnerPrecompilestruct 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
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.
Nice solve!