From 7bf4d54d56053f2a0d7bce1b98805f6af11bbaa9 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Mon, 17 Apr 2023 10:47:50 -0700 Subject: [PATCH 1/4] op-chain-ops: legacy withdrawal script refactoring Clean up the script that is used to migrate legacy withdrawals. This script is very important for testing the legacy withdrawal migration. A bug was introduced with https://github.com/ethereum-optimism/optimism/pull/4911 which was a sherlock audit fix. This was a bug because the change resulted in the withdrawal hashes changing, meaning that the storage slots computed client side also changed. This resulted in breaking the script. This commit reverts this change. The changes landing in https://github.com/ethereum-optimism/optimism/pull/5470 will cause the buffer in the gas limit for the migrated withdrawals to be too small, so we can reintroduce this code behind a switch with the network. Its not ideal that the migration code isn't going to match 1:1 with goerli but thats ok because we will be able to thoroughly test it. --- op-chain-ops/cmd/withdrawals/main.go | 91 +++++++++++++++++-- op-chain-ops/crossdomain/migrate.go | 15 +-- op-chain-ops/crossdomain/migrate_test.go | 6 +- packages/sdk/src/utils/message-utils.ts | 8 +- packages/sdk/test/utils/message-utils.spec.ts | 6 +- 5 files changed, 99 insertions(+), 27 deletions(-) diff --git a/op-chain-ops/cmd/withdrawals/main.go b/op-chain-ops/cmd/withdrawals/main.go index 3e5b55b3bce71..d6e03a75753b7 100644 --- a/op-chain-ops/cmd/withdrawals/main.go +++ b/op-chain-ops/cmd/withdrawals/main.go @@ -22,11 +22,14 @@ import ( "github.com/ethereum/go-ethereum/accounts/abi/bind" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/ethereum/go-ethereum/core/state" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/eth" "github.com/ethereum/go-ethereum/eth/tracers" "github.com/ethereum/go-ethereum/ethclient" "github.com/ethereum/go-ethereum/log" + "github.com/ethereum/go-ethereum/rpc" ) // abiTrue represents the storage representation of the boolean @@ -116,6 +119,11 @@ func main() { Value: "bad-withdrawals.json", Usage: "Path to write JSON file of bad withdrawals to manually inspect", }, + &cli.StringFlag{ + Name: "storage-out", + Value: "storage.txt", + Usage: "Path to write JSON file of L2ToL1MessagePasser storage", + }, }, Action: func(ctx *cli.Context) error { clients, err := util.NewClients(ctx) @@ -163,10 +171,11 @@ func main() { } outfile := ctx.String("bad-withdrawals-out") - f, err := os.OpenFile(outfile, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o755) + f, err := os.OpenFile(outfile, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o644) if err != nil { return err } + defer f.Close() // create a transactor opts, err := newTransactor(ctx) @@ -177,6 +186,28 @@ func main() { // Need this to compare in event parsing l1StandardBridgeAddress := common.HexToAddress(ctx.String("l1-standard-bridge-address")) + if storageOutfile := ctx.String("storage-out"); storageOutfile != "" { + ff, err := os.OpenFile(storageOutfile, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o644) + if err != nil { + return err + } + defer ff.Close() + + log.Info("Fetching storage for L2ToL1MessagePasser") + if storageRange, err := callStorageRange(clients, predeploys.L2ToL1MessagePasserAddr); err != nil { + log.Info("error getting storage range", "err", err) + } else { + str := "" + for key, value := range storageRange { + str += fmt.Sprintf("%s: %s\n", key.Hex(), value.Hex()) + } + _, err = ff.WriteString(str) + if err != nil { + return err + } + } + } + // iterate over all of the withdrawals and submit them for i, wd := range wds { log.Info("Processing withdrawal", "index", i) @@ -234,7 +265,7 @@ func main() { // successful messages can be skipped, received messages failed // their execution and should be replayed if isSuccessNew { - log.Info("Message already relayed", "index", i, "hash", hash, "slot", slot) + log.Info("Message already relayed", "index", i, "hash", hash.Hex(), "slot", slot.Hex()) continue } @@ -248,7 +279,9 @@ func main() { // the value should be set to a boolean in storage if !bytes.Equal(storageValue, abiTrue.Bytes()) { - return fmt.Errorf("storage slot %x not found in state", slot) + log.Info("slot not found in state", "slot", slot.Hex()) + continue + //return fmt.Errorf("storage slot %x not found in state", slot) } legacySlot, err := wd.StorageSlot() @@ -443,10 +476,48 @@ func callTrace(c *util.Clients, receipt *types.Receipt) (callFrame, error) { Tracer: &tracer, } err := c.L1RpcClient.Call(&finalizationTrace, "debug_traceTransaction", receipt.TxHash, traceConfig) + return finalizationTrace, err +} + +func callStorageRangeAt( + client *rpc.Client, + blockHash common.Hash, + txIndex int, + addr common.Address, + keyStart hexutil.Bytes, + maxResult int, +) (*eth.StorageRangeResult, error) { + var storageRange *eth.StorageRangeResult + err := client.Call(&storageRange, "debug_storageRangeAt", blockHash, txIndex, addr, keyStart, maxResult) + return storageRange, err +} + +func callStorageRange(c *util.Clients, addr common.Address) (state.Storage, error) { + header, err := c.L2Client.HeaderByNumber(context.Background(), nil) if err != nil { - return finalizationTrace, err + return nil, err } - return finalizationTrace, err + hash := header.Hash() + keyStart := hexutil.Bytes(common.Hash{}.Bytes()) + maxResult := 1000 + + ret := make(state.Storage) + + for { + result, err := callStorageRangeAt(c.L2RpcClient, hash, 0, addr, keyStart, maxResult) + if err != nil { + return nil, err + } + for key, value := range result.Storage { + ret[key] = value.Value + } + if result.NextKey == nil { + break + } else { + keyStart = hexutil.Bytes(result.NextKey.Bytes()) + } + } + return ret, nil } // handleFinalizeETHWithdrawal will ensure that the calldata is correct @@ -709,9 +780,13 @@ func newWithdrawals(ctx *cli.Context, l1ChainID *big.Int) ([]*crossdomain.Legacy witnessFile := ctx.String("witness-file") log.Debug("Migration data", "ovm-path", ovmMsgs, "evm-messages", evmMsgs, "witness-file", witnessFile) - ovmMessages, err := crossdomain.NewSentMessageFromJSON(ovmMsgs) - if err != nil { - return nil, err + var ovmMessages []*crossdomain.SentMessage + var err error + if ovmMsgs != "" { + ovmMessages, err = crossdomain.NewSentMessageFromJSON(ovmMsgs) + if err != nil { + return nil, err + } } // use empty ovmMessages if its not mainnet. The mainnet messages are diff --git a/op-chain-ops/crossdomain/migrate.go b/op-chain-ops/crossdomain/migrate.go index 741f353fb9e00..f0ebfae1681dc 100644 --- a/op-chain-ops/crossdomain/migrate.go +++ b/op-chain-ops/crossdomain/migrate.go @@ -96,17 +96,12 @@ func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *com return w, nil } +// MigrateWithdrawalGasLimit computes the gas limit for the migrated withdrawal. func MigrateWithdrawalGasLimit(data []byte) uint64 { - // Compute the cost of the calldata - dataCost := uint64(0) - for _, b := range data { - if b == 0 { - dataCost += params.TxDataZeroGas - } else { - dataCost += params.TxDataNonZeroGasEIP2028 - } - } - + // 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 // 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 diff --git a/op-chain-ops/crossdomain/migrate_test.go b/op-chain-ops/crossdomain/migrate_test.go index 45a604eaeaae1..c14fadc8cdbae 100644 --- a/op-chain-ops/crossdomain/migrate_test.go +++ b/op-chain-ops/crossdomain/migrate_test.go @@ -71,15 +71,15 @@ func TestMigrateWithdrawalGasLimit(t *testing.T) { }, { input: []byte{0xff, 0x00}, - output: 200_000 + 16 + 4, + output: 200_000 + 16 + 16, }, { input: []byte{0x00}, - output: 200_000 + 4, + output: 200_000 + 16, }, { input: []byte{0x00, 0x00, 0x00}, - output: 200_000 + 4 + 4 + 4, + output: 200_000 + 16 + 16 + 16, }, } diff --git a/packages/sdk/src/utils/message-utils.ts b/packages/sdk/src/utils/message-utils.ts index 323734b1fdf0d..ab23e627cc42a 100644 --- a/packages/sdk/src/utils/message-utils.ts +++ b/packages/sdk/src/utils/message-utils.ts @@ -1,8 +1,10 @@ -import { hashWithdrawal, calldataCost } from '@eth-optimism/core-utils' -import { BigNumber } from 'ethers' +import { hashWithdrawal } from '@eth-optimism/core-utils' +import { BigNumber, utils } from 'ethers' import { LowLevelMessage } from '../interfaces' +const { hexDataLength } = utils + /** * Utility for hashing a LowLevelMessage object. * @@ -25,7 +27,7 @@ export const hashLowLevelMessage = (message: LowLevelMessage): string => { */ export const migratedWithdrawalGasLimit = (data: string): BigNumber => { // Compute the gas limit and cap at 25 million - const dataCost = calldataCost(data) + const dataCost = BigNumber.from(hexDataLength(data)) let minGasLimit = dataCost.add(200_000) if (minGasLimit.gt(25_000_000)) { minGasLimit = BigNumber.from(25_000_000) diff --git a/packages/sdk/test/utils/message-utils.spec.ts b/packages/sdk/test/utils/message-utils.spec.ts index 718b7f6d4a59f..b1c2179499797 100644 --- a/packages/sdk/test/utils/message-utils.spec.ts +++ b/packages/sdk/test/utils/message-utils.spec.ts @@ -15,9 +15,9 @@ describe('Message Utils', () => { 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) }, + { input: '0xff00', result: BigNumber.from(200_000 + 16 + 16) }, + { input: '0x00', result: BigNumber.from(200_000 + 16) }, + { input: '0x000000', result: BigNumber.from(200_000 + 16 + 16 + 16) }, ] for (const test of tests) { From 6c539cd4c38065c77a815e25ad402ab67400b7f4 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Mon, 17 Apr 2023 14:42:07 -0700 Subject: [PATCH 2/4] op-chain-ops: don't write storage to disk by default --- op-chain-ops/cmd/withdrawals/main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/op-chain-ops/cmd/withdrawals/main.go b/op-chain-ops/cmd/withdrawals/main.go index d6e03a75753b7..94312743fc1f2 100644 --- a/op-chain-ops/cmd/withdrawals/main.go +++ b/op-chain-ops/cmd/withdrawals/main.go @@ -121,8 +121,7 @@ func main() { }, &cli.StringFlag{ Name: "storage-out", - Value: "storage.txt", - Usage: "Path to write JSON file of L2ToL1MessagePasser storage", + Usage: "Path to write text file of L2ToL1MessagePasser storage", }, }, Action: func(ctx *cli.Context) error { From 2d3424e1d446eacfa7333090e6d1d89df37429f9 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Mon, 17 Apr 2023 14:43:27 -0700 Subject: [PATCH 3/4] op-chain-ops: clean up debugging code --- op-chain-ops/cmd/withdrawals/main.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/op-chain-ops/cmd/withdrawals/main.go b/op-chain-ops/cmd/withdrawals/main.go index 94312743fc1f2..5163ca758b803 100644 --- a/op-chain-ops/cmd/withdrawals/main.go +++ b/op-chain-ops/cmd/withdrawals/main.go @@ -278,9 +278,7 @@ func main() { // the value should be set to a boolean in storage if !bytes.Equal(storageValue, abiTrue.Bytes()) { - log.Info("slot not found in state", "slot", slot.Hex()) - continue - //return fmt.Errorf("storage slot %x not found in state", slot) + return fmt.Errorf("storage slot %x not found in state", slot.Hex()) } legacySlot, err := wd.StorageSlot() From 83187b02d5ebdbb33c786951a08f142f5e970a9e Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Mon, 17 Apr 2023 15:12:06 -0700 Subject: [PATCH 4/4] sdk: fix gas calculation --- packages/sdk/src/utils/message-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sdk/src/utils/message-utils.ts b/packages/sdk/src/utils/message-utils.ts index ab23e627cc42a..63f17635303e7 100644 --- a/packages/sdk/src/utils/message-utils.ts +++ b/packages/sdk/src/utils/message-utils.ts @@ -27,7 +27,7 @@ export const hashLowLevelMessage = (message: LowLevelMessage): string => { */ export const migratedWithdrawalGasLimit = (data: string): BigNumber => { // Compute the gas limit and cap at 25 million - const dataCost = BigNumber.from(hexDataLength(data)) + const dataCost = BigNumber.from(hexDataLength(data)).mul(16) let minGasLimit = dataCost.add(200_000) if (minGasLimit.gt(25_000_000)) { minGasLimit = BigNumber.from(25_000_000)