Skip to content
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

fix: withdrawal gas limit #5501

Merged
merged 5 commits into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rare-moose-itch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@eth-optimism/sdk': patch
---

Update the migrated withdrawal gas limit for non goerli networks
1 change: 1 addition & 0 deletions op-chain-ops/cmd/check-migration/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ func main() {
migrationData,
&config.L1CrossDomainMessengerProxy,
config.L1ChainID,
config.L2ChainID,
config.FinalSystemOwner,
config.ProxyAdminOwner,
&derive.L1BlockInfo{
Expand Down
1 change: 1 addition & 0 deletions op-chain-ops/cmd/op-migrate/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ func main() {
migrationData,
&config.L1CrossDomainMessengerProxy,
config.L1ChainID,
config.L2ChainID,
config.FinalSystemOwner,
config.ProxyAdminOwner,
&derive.L1BlockInfo{
Expand Down
6 changes: 5 additions & 1 deletion op-chain-ops/cmd/withdrawals/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ func main() {
if err != nil {
return err
}
l2ChainID, err := clients.L2Client.ChainID(context.Background())
if err != nil {
return err
}

// create the set of withdrawals
wds, err := newWithdrawals(ctx, l1ChainID)
Expand Down Expand Up @@ -212,7 +216,7 @@ func main() {
log.Info("Processing withdrawal", "index", i)

// migrate the withdrawal
withdrawal, err := crossdomain.MigrateWithdrawal(wd, &l1xdmAddr)
withdrawal, err := crossdomain.MigrateWithdrawal(wd, &l1xdmAddr, l2ChainID)
if err != nil {
return err
}
Expand Down
30 changes: 24 additions & 6 deletions op-chain-ops/crossdomain/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@ var (
)

// MigrateWithdrawals will migrate a list of pending withdrawals given a StateDB.
func MigrateWithdrawals(withdrawals SafeFilteredWithdrawals, db vm.StateDB, l1CrossDomainMessenger *common.Address, noCheck bool) error {
func MigrateWithdrawals(
withdrawals SafeFilteredWithdrawals,
db vm.StateDB,
l1CrossDomainMessenger *common.Address,
noCheck bool,
chainID *big.Int,
) error {
for i, legacy := range withdrawals {
legacySlot, err := legacy.StorageSlot()
if err != nil {
Expand All @@ -34,7 +40,7 @@ func MigrateWithdrawals(withdrawals SafeFilteredWithdrawals, db vm.StateDB, l1Cr
}
}

withdrawal, err := MigrateWithdrawal(legacy, l1CrossDomainMessenger)
withdrawal, err := MigrateWithdrawal(legacy, l1CrossDomainMessenger, chainID)
if err != nil {
return err
}
Expand All @@ -52,7 +58,11 @@ func MigrateWithdrawals(withdrawals SafeFilteredWithdrawals, db vm.StateDB, l1Cr

// MigrateWithdrawal will turn a LegacyWithdrawal into a bedrock
// style Withdrawal.
func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *common.Address) (*Withdrawal, error) {
func MigrateWithdrawal(
withdrawal *LegacyWithdrawal,
l1CrossDomainMessenger *common.Address,
chainID *big.Int,
) (*Withdrawal, error) {
// Attempt to parse the value
value, err := withdrawal.Value()
if err != nil {
Expand Down Expand Up @@ -83,7 +93,7 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com
return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err)
}

gasLimit := MigrateWithdrawalGasLimit(data)
gasLimit := MigrateWithdrawalGasLimit(data, chainID)

w := NewWithdrawal(
versionedNonce,
Expand All @@ -97,13 +107,21 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com
}

// MigrateWithdrawalGasLimit computes the gas limit for the migrated withdrawal.
func MigrateWithdrawalGasLimit(data []byte) uint64 {
// The chain id is used to determine the overhead.
func MigrateWithdrawalGasLimit(data []byte, chainID *big.Int) uint64 {
// Compute the upper bound on the gas limit. This could be more
// accurate if individual 0 bytes and non zero bytes were accounted
// for.
dataCost := uint64(len(data)) * params.TxDataNonZeroGasEIP2028

// Goerli has a lower gas limit than other chains.
overhead := uint64(200_000)
if chainID.Cmp(big.NewInt(420)) != 0 {
mslipper marked this conversation as resolved.
Show resolved Hide resolved
overhead = 1_000_000
}

// Set the outer gas limit. This cannot be zero
gasLimit := dataCost + 200_000
gasLimit := dataCost + overhead
// Cap the gas limit to be 25 million to prevent creating withdrawals
// that go over the block gas limit.
if gasLimit > 25_000_000 {
Expand Down
11 changes: 7 additions & 4 deletions op-chain-ops/crossdomain/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import (
"github.com/stretchr/testify/require"
)

var big25Million = big.NewInt(25_000_000)
var (
big25Million = big.NewInt(25_000_000)
bigGoerliChainID = big.NewInt(420)
)

func TestMigrateWithdrawal(t *testing.T) {
withdrawals := make([]*crossdomain.LegacyWithdrawal, 0)
Expand All @@ -27,7 +30,7 @@ func TestMigrateWithdrawal(t *testing.T) {
l1CrossDomainMessenger := common.HexToAddress("0x25ace71c97B33Cc4729CF772ae268934F7ab5fA1")
for i, legacy := range withdrawals {
t.Run(fmt.Sprintf("test%d", i), func(t *testing.T) {
withdrawal, err := crossdomain.MigrateWithdrawal(legacy, &l1CrossDomainMessenger)
withdrawal, err := crossdomain.MigrateWithdrawal(legacy, &l1CrossDomainMessenger, bigGoerliChainID)
require.Nil(t, err)
require.NotNil(t, withdrawal)

Expand All @@ -50,7 +53,7 @@ func TestMigrateWithdrawalGasLimitMax(t *testing.T) {
data[i] = 0xff
}

result := crossdomain.MigrateWithdrawalGasLimit(data)
result := crossdomain.MigrateWithdrawalGasLimit(data, bigGoerliChainID)
require.Equal(t, result, big25Million.Uint64())
}

Expand Down Expand Up @@ -84,7 +87,7 @@ func TestMigrateWithdrawalGasLimit(t *testing.T) {
}

for _, test := range tests {
result := crossdomain.MigrateWithdrawalGasLimit(test.input)
result := crossdomain.MigrateWithdrawalGasLimit(test.input, bigGoerliChainID)
require.Equal(t, test.output, result)
}
}
7 changes: 4 additions & 3 deletions op-chain-ops/genesis/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ func PostCheckMigratedDB(
migrationData crossdomain.MigrationData,
l1XDM *common.Address,
l1ChainID uint64,
l2ChainID uint64,
finalSystemOwner common.Address,
proxyAdminOwner common.Address,
info *derive.L1BlockInfo,
Expand Down Expand Up @@ -163,7 +164,7 @@ func PostCheckMigratedDB(
}
log.Info("checked legacy eth")

if err := CheckWithdrawalsAfter(db, migrationData, l1XDM); err != nil {
if err := CheckWithdrawalsAfter(db, migrationData, l1XDM, new(big.Int).SetUint64(l2ChainID)); err != nil {
return err
}
log.Info("checked withdrawals")
Expand Down Expand Up @@ -557,7 +558,7 @@ func PostCheckL1Block(db *state.StateDB, info *derive.L1BlockInfo) error {
return nil
}

func CheckWithdrawalsAfter(db *state.StateDB, data crossdomain.MigrationData, l1CrossDomainMessenger *common.Address) error {
func CheckWithdrawalsAfter(db *state.StateDB, data crossdomain.MigrationData, l1CrossDomainMessenger *common.Address, l2ChainID *big.Int) error {
wds, invalidMessages, err := data.ToWithdrawals()
if err != nil {
return err
Expand All @@ -570,7 +571,7 @@ func CheckWithdrawalsAfter(db *state.StateDB, data crossdomain.MigrationData, l1
wdsByOldSlot := make(map[common.Hash]*crossdomain.LegacyWithdrawal)
invalidMessagesByOldSlot := make(map[common.Hash]crossdomain.InvalidMessage)
for _, wd := range wds {
migrated, err := crossdomain.MigrateWithdrawal(wd, l1CrossDomainMessenger)
migrated, err := crossdomain.MigrateWithdrawal(wd, l1CrossDomainMessenger, l2ChainID)
if err != nil {
return err
}
Expand Down
3 changes: 2 additions & 1 deletion op-chain-ops/genesis/db_migration.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ func MigrateDB(ldb ethdb.Database, config *DeployConfig, l1Block *types.Block, m
// the LegacyMessagePasser contract. Here we operate on the list of withdrawals that we
// previously filtered and verified.
log.Info("Starting to migrate withdrawals", "no-check", noCheck)
err = crossdomain.MigrateWithdrawals(filteredWithdrawals, db, &config.L1CrossDomainMessengerProxy, noCheck)
l2ChainID := new(big.Int).SetUint64(config.L2ChainID)
err = crossdomain.MigrateWithdrawals(filteredWithdrawals, db, &config.L1CrossDomainMessengerProxy, noCheck, l2ChainID)
if err != nil {
return nil, fmt.Errorf("cannot migrate withdrawals: %w", err)
}
Expand Down
4 changes: 3 additions & 1 deletion packages/sdk/src/cross-chain-messenger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
BedrockCrossChainMessageProof,
decodeVersionedNonce,
encodeVersionedNonce,
getChainId,
} from '@eth-optimism/core-utils'
import { getContractInterface, predeploys } from '@eth-optimism/contracts'
import * as rlp from 'rlp'
Expand Down Expand Up @@ -403,7 +404,8 @@ export class CrossChainMessenger {
let gasLimit: BigNumber
let messageNonce: BigNumber
if (version.eq(0)) {
gasLimit = migratedWithdrawalGasLimit(encoded)
const chainID = await getChainId(this.l2Provider)
gasLimit = migratedWithdrawalGasLimit(encoded, chainID)
messageNonce = resolved.messageNonce
} else {
const receipt = await this.l2Provider.getTransactionReceipt(
Expand Down
11 changes: 9 additions & 2 deletions packages/sdk/src/utils/message-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,17 @@ export const hashMessageHash = (messageHash: string): string => {
/**
* Compute the min gas limit for a migrated withdrawal.
*/
export const migratedWithdrawalGasLimit = (data: string): BigNumber => {
export const migratedWithdrawalGasLimit = (
data: string,
chainID: number
): BigNumber => {
// Compute the gas limit and cap at 25 million
const dataCost = BigNumber.from(hexDataLength(data)).mul(16)
let minGasLimit = dataCost.add(200_000)
let overhead = 200_000
if (chainID !== 420) {
overhead = 1_000_000
}
let minGasLimit = dataCost.add(overhead)
if (minGasLimit.gt(25_000_000)) {
minGasLimit = BigNumber.from(25_000_000)
}
Expand Down
6 changes: 4 additions & 2 deletions packages/sdk/test/utils/message-utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import {
hashMessageHash,
} from '../../src/utils/message-utils'

const goerliChainID = 420

describe('Message Utils', () => {
describe('migratedWithdrawalGasLimit', () => {
it('should have a max of 25 million', () => {
const data = '0x' + 'ff'.repeat(15_000_000)
const result = migratedWithdrawalGasLimit(data)
const result = migratedWithdrawalGasLimit(data, goerliChainID)
expect(result).to.eq(BigNumber.from(25_000_000))
})

Expand All @@ -25,7 +27,7 @@ describe('Message Utils', () => {
]

for (const test of tests) {
const result = migratedWithdrawalGasLimit(test.input)
const result = migratedWithdrawalGasLimit(test.input, goerliChainID)
expect(result).to.eq(test.result)
}
})
Expand Down