From 66cafc00aa22095efa52aafb233115088875cf2e Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Thu, 16 Feb 2023 16:44:08 -0800 Subject: [PATCH 1/4] migration: update migrated withdrawal gas limit More accurately computes the gas limit for a migrated withdrawal. This is useful to prevent the gaslimit for large transactions from going over the consensus level block gas limit. It is unlikely for a transaction to go over the block gas limit, but this is a preventative fix just in case. --- .changeset/poor-buses-hug.md | 5 +++++ op-chain-ops/crossdomain/migrate.go | 13 ++++++++++++- packages/sdk/src/cross-chain-messenger.ts | 8 ++++---- 3 files changed, 21 insertions(+), 5 deletions(-) create mode 100644 .changeset/poor-buses-hug.md diff --git a/.changeset/poor-buses-hug.md b/.changeset/poor-buses-hug.md new file mode 100644 index 0000000000000..66db062bbf533 --- /dev/null +++ b/.changeset/poor-buses-hug.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/sdk': patch +--- + +Update migrated withdrawal gaslimit calculation diff --git a/op-chain-ops/crossdomain/migrate.go b/op-chain-ops/crossdomain/migrate.go index b88074d212b79..747d617ad8c95 100644 --- a/op-chain-ops/crossdomain/migrate.go +++ b/op-chain-ops/crossdomain/migrate.go @@ -11,6 +11,7 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/log" + "github.com/ethereum/go-ethereum/params" ) var ( @@ -82,8 +83,18 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err) } + // Compute the cost of the calldata + dataCost := uint64(0) + for _, b := range data { + if b == 0 { + dataCost += params.TxDataZeroGas + } else { + dataCost += params.TxDataNonZeroGasEIP2028 + } + } + // Set the outer gas limit. This cannot be zero - gasLimit := uint64(len(data)*16 + 200_000) + gasLimit := dataCost + 200_000 w := NewWithdrawal( versionedNonce, diff --git a/packages/sdk/src/cross-chain-messenger.ts b/packages/sdk/src/cross-chain-messenger.ts index 59379b15a1264..6b37b90020a8a 100644 --- a/packages/sdk/src/cross-chain-messenger.ts +++ b/packages/sdk/src/cross-chain-messenger.ts @@ -18,16 +18,15 @@ import { sleep, remove0x, toHexString, - fromHexString, toRpcHexString, hashCrossDomainMessage, encodeCrossDomainMessageV0, encodeCrossDomainMessageV1, - L2OutputOracleParameters, BedrockOutputData, BedrockCrossChainMessageProof, decodeVersionedNonce, encodeVersionedNonce, + calldataCost, } from '@eth-optimism/core-utils' import { getContractInterface, predeploys } from '@eth-optimism/contracts' import * as rlp from 'rlp' @@ -352,12 +351,13 @@ export class CrossChainMessenger { } } - const minGasLimit = fromHexString(resolved.message).length * 16 + 200_000 + const dataCost = calldataCost(resolved.message) + const minGasLimit = dataCost.add(200_000) return { ...resolved, value, - minGasLimit: BigNumber.from(minGasLimit), + minGasLimit, messageNonce: encodeVersionedNonce( BigNumber.from(1), resolved.messageNonce From 7af32bdae90168417a54e15ab1dafbf19c4bf002 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Tue, 21 Feb 2023 10:10:39 -0800 Subject: [PATCH 2/4] migration: cap withdrawal gaslimit to 25mil --- op-chain-ops/crossdomain/migrate.go | 5 +++++ packages/sdk/src/cross-chain-messenger.ts | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/op-chain-ops/crossdomain/migrate.go b/op-chain-ops/crossdomain/migrate.go index 747d617ad8c95..29fb5ec6f1ecf 100644 --- a/op-chain-ops/crossdomain/migrate.go +++ b/op-chain-ops/crossdomain/migrate.go @@ -95,6 +95,11 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com // Set the outer gas limit. This cannot be zero gasLimit := dataCost + 200_000 + // Cap the gas limit to be 25 million to prevent creating withdrawals + // that go over the block gas limit. + if gasLimit > 25_000_000 { + gasLimit = 25_000_000 + } w := NewWithdrawal( versionedNonce, diff --git a/packages/sdk/src/cross-chain-messenger.ts b/packages/sdk/src/cross-chain-messenger.ts index 6b37b90020a8a..ff7e67a666f0d 100644 --- a/packages/sdk/src/cross-chain-messenger.ts +++ b/packages/sdk/src/cross-chain-messenger.ts @@ -351,8 +351,12 @@ export class CrossChainMessenger { } } + // Compute the gas limit and cap at 25 million const dataCost = calldataCost(resolved.message) - const minGasLimit = dataCost.add(200_000) + let minGasLimit = dataCost.add(200_000) + if (minGasLimit.gt(25_000_000)) { + minGasLimit = BigNumber.from(25_000_000) + } return { ...resolved, From c7ec576c4169fa03917e87e0908d58da0bb1b345 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Wed, 8 Mar 2023 18:34:47 -0800 Subject: [PATCH 3/4] op-chain-ops: migrated withdrawal gas limit tests --- op-chain-ops/crossdomain/migrate.go | 24 +++++++---- op-chain-ops/crossdomain/migrate_test.go | 54 ++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/op-chain-ops/crossdomain/migrate.go b/op-chain-ops/crossdomain/migrate.go index 29fb5ec6f1ecf..741f353fb9e00 100644 --- a/op-chain-ops/crossdomain/migrate.go +++ b/op-chain-ops/crossdomain/migrate.go @@ -83,6 +83,20 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err) } + gasLimit := MigrateWithdrawalGasLimit(data) + + w := NewWithdrawal( + versionedNonce, + &predeploys.L2CrossDomainMessengerAddr, + l1CrossDomainMessenger, + value, + new(big.Int).SetUint64(gasLimit), + data, + ) + return w, nil +} + +func MigrateWithdrawalGasLimit(data []byte) uint64 { // Compute the cost of the calldata dataCost := uint64(0) for _, b := range data { @@ -101,13 +115,5 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com gasLimit = 25_000_000 } - w := NewWithdrawal( - versionedNonce, - &predeploys.L2CrossDomainMessengerAddr, - l1CrossDomainMessenger, - value, - new(big.Int).SetUint64(gasLimit), - data, - ) - return w, nil + return gasLimit } diff --git a/op-chain-ops/crossdomain/migrate_test.go b/op-chain-ops/crossdomain/migrate_test.go index bcd3711ed8899..45a604eaeaae1 100644 --- a/op-chain-ops/crossdomain/migrate_test.go +++ b/op-chain-ops/crossdomain/migrate_test.go @@ -2,6 +2,7 @@ package crossdomain_test import ( "fmt" + "math/big" "testing" "github.com/ethereum-optimism/optimism/op-bindings/predeploys" @@ -11,6 +12,8 @@ import ( "github.com/stretchr/testify/require" ) +var big25Million = big.NewInt(25_000_000) + func TestMigrateWithdrawal(t *testing.T) { withdrawals := make([]*crossdomain.LegacyWithdrawal, 0) @@ -31,6 +34,57 @@ func TestMigrateWithdrawal(t *testing.T) { require.Equal(t, legacy.XDomainNonce.Uint64(), withdrawal.Nonce.Uint64()) require.Equal(t, *withdrawal.Sender, predeploys.L2CrossDomainMessengerAddr) require.Equal(t, *withdrawal.Target, l1CrossDomainMessenger) + // Always equal to or lower than the cap + require.True(t, withdrawal.GasLimit.Cmp(big25Million) <= 0) }) } } + +// TestMigrateWithdrawalGasLimitMax computes the migrated withdrawal +// gas limit with a very large amount of data. The max value for a migrated +// withdrawal's gas limit is 25 million. +func TestMigrateWithdrawalGasLimitMax(t *testing.T) { + size := 300_000_000 / 16 + data := make([]byte, size) + for _, i := range data { + data[i] = 0xff + } + + result := crossdomain.MigrateWithdrawalGasLimit(data) + require.Equal(t, result, big25Million.Uint64()) +} + +// TestMigrateWithdrawalGasLimit tests an assortment of zero and non zero +// bytes when computing the migrated withdrawal's gas limit. +func TestMigrateWithdrawalGasLimit(t *testing.T) { + tests := []struct { + input []byte + output uint64 + }{ + { + input: []byte{}, + output: 200_000, + }, + { + input: []byte{0xff}, + output: 200_000 + 16, + }, + { + input: []byte{0xff, 0x00}, + output: 200_000 + 16 + 4, + }, + { + input: []byte{0x00}, + output: 200_000 + 4, + }, + { + input: []byte{0x00, 0x00, 0x00}, + output: 200_000 + 4 + 4 + 4, + }, + } + + for _, test := range tests { + result := crossdomain.MigrateWithdrawalGasLimit(test.input) + require.Equal(t, test.output, result) + } +} From 43c4158fb4b390663bfbb16fdd6c19045a532526 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Wed, 8 Mar 2023 18:51:37 -0800 Subject: [PATCH 4/4] sdk: add tests for min gas util --- packages/sdk/src/cross-chain-messenger.ts | 9 ++---- packages/sdk/src/utils/message-utils.ts | 16 +++++++++- packages/sdk/test/utils/message-utils.spec.ts | 29 +++++++++++++++++++ 3 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 packages/sdk/test/utils/message-utils.spec.ts diff --git a/packages/sdk/src/cross-chain-messenger.ts b/packages/sdk/src/cross-chain-messenger.ts index ff7e67a666f0d..6287c809cf371 100644 --- a/packages/sdk/src/cross-chain-messenger.ts +++ b/packages/sdk/src/cross-chain-messenger.ts @@ -26,7 +26,6 @@ import { BedrockCrossChainMessageProof, decodeVersionedNonce, encodeVersionedNonce, - calldataCost, } from '@eth-optimism/core-utils' import { getContractInterface, predeploys } from '@eth-optimism/contracts' import * as rlp from 'rlp' @@ -66,6 +65,7 @@ import { makeMerkleTreeProof, makeStateTrieProof, hashLowLevelMessage, + migratedWithdrawalGasLimit, DEPOSIT_CONFIRMATION_BLOCKS, CHAIN_BLOCK_TIMES, } from './utils' @@ -351,12 +351,7 @@ export class CrossChainMessenger { } } - // Compute the gas limit and cap at 25 million - const dataCost = calldataCost(resolved.message) - let minGasLimit = dataCost.add(200_000) - if (minGasLimit.gt(25_000_000)) { - minGasLimit = BigNumber.from(25_000_000) - } + const minGasLimit = migratedWithdrawalGasLimit(resolved.message) return { ...resolved, diff --git a/packages/sdk/src/utils/message-utils.ts b/packages/sdk/src/utils/message-utils.ts index 210bf88296e8e..323734b1fdf0d 100644 --- a/packages/sdk/src/utils/message-utils.ts +++ b/packages/sdk/src/utils/message-utils.ts @@ -1,4 +1,5 @@ -import { hashWithdrawal } from '@eth-optimism/core-utils' +import { hashWithdrawal, calldataCost } from '@eth-optimism/core-utils' +import { BigNumber } from 'ethers' import { LowLevelMessage } from '../interfaces' @@ -18,3 +19,16 @@ export const hashLowLevelMessage = (message: LowLevelMessage): string => { message.message ) } + +/** + * Compute the min gas limit for a migrated withdrawal. + */ +export const migratedWithdrawalGasLimit = (data: string): BigNumber => { + // Compute the gas limit and cap at 25 million + const dataCost = calldataCost(data) + let minGasLimit = dataCost.add(200_000) + if (minGasLimit.gt(25_000_000)) { + minGasLimit = BigNumber.from(25_000_000) + } + return minGasLimit +} diff --git a/packages/sdk/test/utils/message-utils.spec.ts b/packages/sdk/test/utils/message-utils.spec.ts new file mode 100644 index 0000000000000..718b7f6d4a59f --- /dev/null +++ b/packages/sdk/test/utils/message-utils.spec.ts @@ -0,0 +1,29 @@ +import { BigNumber } from 'ethers' + +import { expect } from '../setup' +import { migratedWithdrawalGasLimit } from '../../src/utils/message-utils' + +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) + expect(result).to.eq(BigNumber.from(25_000_000)) + }) + + it('should work for mixes of zeros and ones', () => { + const tests = [ + { input: '0x', result: BigNumber.from(200_000) }, + { input: '0xff', result: BigNumber.from(200_000 + 16) }, + { input: '0xff00', result: BigNumber.from(200_000 + 16 + 4) }, + { input: '0x00', result: BigNumber.from(200_000 + 4) }, + { input: '0x000000', result: BigNumber.from(200_000 + 4 + 4 + 4) }, + ] + + for (const test of tests) { + const result = migratedWithdrawalGasLimit(test.input) + expect(result).to.eq(test.result) + } + }) + }) +})