From 561161a484946012b639a07f743d7037f05aef91 Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Wed, 10 Nov 2021 15:33:40 -0800 Subject: [PATCH 1/6] test: add erc20 test coverage --- packages/regenesis-surgery/package.json | 5 ++- .../regenesis-surgery/scripts/classifiers.ts | 5 ++- .../regenesis-surgery/scripts/handlers.ts | 5 +++ packages/regenesis-surgery/scripts/types.ts | 1 + packages/regenesis-surgery/scripts/utils.ts | 45 ++++++++++++++++++- packages/regenesis-surgery/test/erc20.spec.ts | 36 +++++++++++++++ .../regenesis-surgery/test/provider.spec.ts | 2 +- packages/regenesis-surgery/test/setup.ts | 9 +++- packages/regenesis-surgery/test/utils.ts | 32 +++++++++++++ yarn.lock | 15 +++++++ 10 files changed, 149 insertions(+), 6 deletions(-) create mode 100644 packages/regenesis-surgery/test/erc20.spec.ts create mode 100644 packages/regenesis-surgery/test/utils.ts diff --git a/packages/regenesis-surgery/package.json b/packages/regenesis-surgery/package.json index 491e13ad6274b..b10631f4bffa0 100644 --- a/packages/regenesis-surgery/package.json +++ b/packages/regenesis-surgery/package.json @@ -17,6 +17,7 @@ "@discoveryjs/json-ext": "^0.5.3", "@eth-optimism/core-utils": "0.6.0", "@ethersproject/abstract-provider": "^5.5.1", + "@ethersproject/abi": "^5.5.0", "@ethersproject/bignumber": "^5.5.0", "@ethersproject/properties": "^5.5.0", "@ethersproject/providers": "^5.5.0", @@ -45,7 +46,7 @@ "mocha": "^9.1.2", "node-fetch": "2.6.5", "solc": "0.8.7-fixed", - "ts-node": "^10.0.0", - "ts-mocha": "^8.0.0" + "ts-mocha": "^8.0.0", + "ts-node": "^10.0.0" } } diff --git a/packages/regenesis-surgery/scripts/classifiers.ts b/packages/regenesis-surgery/scripts/classifiers.ts index 429ccef3437cc..d46ffa19b41bd 100644 --- a/packages/regenesis-surgery/scripts/classifiers.ts +++ b/packages/regenesis-surgery/scripts/classifiers.ts @@ -13,7 +13,7 @@ import { DELETE_CONTRACTS, } from './constants' import { Account, AccountType, SurgeryDataSources } from './types' -import { hexStringEqual } from './utils' +import { hexStringEqual, isBytecodeERC20 } from './utils' export const classifiers: { [key in AccountType]: (account: Account, data: SurgeryDataSources) => boolean @@ -90,6 +90,9 @@ export const classifiers: { [AccountType.VERIFIED]: (account, data) => { return !classifiers[AccountType.UNVERIFIED](account, data) }, + [AccountType.ERC20]: (account) => { + return isBytecodeERC20(account.code) + }, } export const classify = ( diff --git a/packages/regenesis-surgery/scripts/handlers.ts b/packages/regenesis-surgery/scripts/handlers.ts index 4a6ad87e58b7f..972c9851f0b77 100644 --- a/packages/regenesis-surgery/scripts/handlers.ts +++ b/packages/regenesis-surgery/scripts/handlers.ts @@ -434,4 +434,9 @@ export const handlers: { code: bytecode, } }, + [AccountType.ERC20]: async (account) => { + throw new Error( + `Unexpected ERC20 classification, this should never happen: ${account.address}` + ) + }, } diff --git a/packages/regenesis-surgery/scripts/types.ts b/packages/regenesis-surgery/scripts/types.ts index 5e16ec21756b4..281ad6c00ea14 100644 --- a/packages/regenesis-surgery/scripts/types.ts +++ b/packages/regenesis-surgery/scripts/types.ts @@ -59,6 +59,7 @@ export enum AccountType { UNISWAP_V3_OTHER, UNVERIFIED, VERIFIED, + ERC20, } export interface UniswapPoolData { diff --git a/packages/regenesis-surgery/scripts/utils.ts b/packages/regenesis-surgery/scripts/utils.ts index a99381f83718c..d1287849a2fdb 100644 --- a/packages/regenesis-surgery/scripts/utils.ts +++ b/packages/regenesis-surgery/scripts/utils.ts @@ -1,13 +1,14 @@ /* eslint @typescript-eslint/no-var-requires: "off" */ import { ethers } from 'ethers' import { abi as UNISWAP_FACTORY_ABI } from '@uniswap/v3-core/artifacts/contracts/UniswapV3Factory.sol/UniswapV3Factory.json' +import { Interface } from '@ethersproject/abi' import { parseChunked } from '@discoveryjs/json-ext' import { createReadStream } from 'fs' import * as fs from 'fs' import byline from 'byline' import * as dotenv from 'dotenv' import * as assert from 'assert' -import { reqenv, getenv } from '@eth-optimism/core-utils' +import { reqenv, getenv, remove0x } from '@eth-optimism/core-utils' import { Account, EtherscanContract, @@ -114,6 +115,48 @@ export const getMappingKey = (keys: any[], slot: number) => { return key } +// ERC20 interface +const iface = new Interface([ + 'function balanceOf(address)', + 'function name()', + 'function symbol()', + 'function decimals()', + 'function totalSupply()', + 'function transfer(address,uint256)', +]) + +// PUSH4 should prefix any 4 byte selector +const PUSH4 = 0x63 +const erc20Sighashes = new Set() +// Build the set of erc20 4 byte selectors +for (const fn of Object.keys(iface.functions)) { + const sighash = iface.getSighash(fn) + erc20Sighashes.add(sighash) +} + +export const isBytecodeERC20 = (bytecode: string): boolean => { + if (bytecode === '0x' || bytecode === undefined) { + return false + } + const seen = new Set() + const buf = Buffer.from(remove0x(bytecode), 'hex') + for (const [i, byte] of buf.entries()) { + // Track all of the observed 4 byte selectors that follow a PUSH4 + // and are also present in the set of erc20Sighashes + if (byte === PUSH4) { + const sighash = '0x' + buf.slice(i + 1, i + 5).toString('hex') + if (erc20Sighashes.has(sighash)) { + seen.add(sighash) + } + } + } + + // create a set that contains those elements of set + // erc20Sighashes that are not in set seen + const elements = [...erc20Sighashes].filter((x) => !seen.has(x)) + return !elements.length +} + export const getUniswapV3Factory = (signerOrProvider: any): ethers.Contract => { return new ethers.Contract( UNISWAP_V3_FACTORY_ADDRESS, diff --git a/packages/regenesis-surgery/test/erc20.spec.ts b/packages/regenesis-surgery/test/erc20.spec.ts new file mode 100644 index 0000000000000..0f089e09039b6 --- /dev/null +++ b/packages/regenesis-surgery/test/erc20.spec.ts @@ -0,0 +1,36 @@ +import { expect } from '@eth-optimism/core-utils/test/setup' +import { BigNumber } from 'ethers' +import { env } from './setup' + +describe('erc20', () => { + describe('standard ERC20', () => { + before(async () => { + await env.init() + }) + + it('ERC20s', () => { + for (const [i, erc20] of env.erc20s.entries()) { + describe(`erc20 ${i}/${env.erc20s.length} (${erc20.address})`, () => { + it('should have the same storage', async () => { + const account = env.surgeryDataSources.dump.find( + (a) => a.address === erc20.address + ) + if (account.storage) { + for (const key of Object.keys(account.storage)) { + const pre = await env.preL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + const post = await env.postL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + expect(pre).to.deep.eq(post) + } + } + }) + }) + } + }) + }) +}) diff --git a/packages/regenesis-surgery/test/provider.spec.ts b/packages/regenesis-surgery/test/provider.spec.ts index db0970b6598e3..872a326a087d9 100644 --- a/packages/regenesis-surgery/test/provider.spec.ts +++ b/packages/regenesis-surgery/test/provider.spec.ts @@ -49,7 +49,7 @@ const genesis: Genesis = { }, } -describe.only('GenesisJsonProvider', () => { +describe('GenesisJsonProvider', () => { let provider before(() => { provider = new GenesisJsonProvider(genesis) diff --git a/packages/regenesis-surgery/test/setup.ts b/packages/regenesis-surgery/test/setup.ts index c8c388993bc9d..d3680fab2968f 100644 --- a/packages/regenesis-surgery/test/setup.ts +++ b/packages/regenesis-surgery/test/setup.ts @@ -7,7 +7,7 @@ import { getenv, remove0x } from '@eth-optimism/core-utils' import { providers, BigNumber } from 'ethers' import { SurgeryDataSources, Account, AccountType } from '../scripts/types' import { loadSurgeryData } from '../scripts/data' -import { classify } from '../scripts/classifiers' +import { classify, classifiers } from '../scripts/classifiers' import { GenesisJsonProvider } from './provider' // Chai plugins go here. @@ -64,6 +64,9 @@ class TestEnv { // List of typed accounts in the input dump accounts: TypedAccount[] = [] + // List of erc20 contracts in input dump + erc20s: Account[] = [] + constructor(opts: TestEnvConfig) { this.config = opts // If the pre provider url is provided, use a json rpc provider. @@ -138,6 +141,10 @@ class TestEnv { ...account, type: accountType, }) + + if (classifiers[AccountType.ERC20](account, this.surgeryDataSources)) { + this.erc20s.push(account) + } } } } diff --git a/packages/regenesis-surgery/test/utils.ts b/packages/regenesis-surgery/test/utils.ts new file mode 100644 index 0000000000000..4b999ad6d7301 --- /dev/null +++ b/packages/regenesis-surgery/test/utils.ts @@ -0,0 +1,32 @@ +import { expect } from '@eth-optimism/core-utils/test/setup' +import fs from 'fs/promises' +import path from 'path' +import { isBytecodeERC20 } from '../scripts/utils' + +describe('Utils', () => { + // Read in the mock data + const contracts = {} + before(async () => { + const files = await fs.readdir(path.join(__dirname, 'data')) + for (const filename of files) { + const file = await fs.readFile(path.join(__dirname, 'data', filename)) + const name = path.parse(filename).name + const json = JSON.parse(file.toString()) + contracts[name] = { + bytecode: json.bytecode.toString().trim(), + expected: json.expected, + } + } + }) + + it('isBytecodeERC20', () => { + for (const [name, contract] of Object.entries(contracts)) { + describe(`contract ${name}`, () => { + it('should be identified erc20', () => { + const result = isBytecodeERC20((contract as any).bytecode as string) + expect(result).to.eq((contract as any).expected) + }) + }) + } + }) +}) diff --git a/yarn.lock b/yarn.lock index 44eaea9a28305..1d81d2d567e2e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -734,6 +734,21 @@ "@ethersproject/properties" "^5.4.0" "@ethersproject/strings" "^5.4.0" +"@ethersproject/abi@^5.5.0": + version "5.5.0" + resolved "https://registry.yarnpkg.com/@ethersproject/abi/-/abi-5.5.0.tgz#fb52820e22e50b854ff15ce1647cc508d6660613" + integrity sha512-loW7I4AohP5KycATvc0MgujU6JyCHPqHdeoo9z3Nr9xEiNioxa65ccdm1+fsoJhkuhdRtfcL8cfyGamz2AxZ5w== + dependencies: + "@ethersproject/address" "^5.5.0" + "@ethersproject/bignumber" "^5.5.0" + "@ethersproject/bytes" "^5.5.0" + "@ethersproject/constants" "^5.5.0" + "@ethersproject/hash" "^5.5.0" + "@ethersproject/keccak256" "^5.5.0" + "@ethersproject/logger" "^5.5.0" + "@ethersproject/properties" "^5.5.0" + "@ethersproject/strings" "^5.5.0" + "@ethersproject/abstract-provider@5.4.1", "@ethersproject/abstract-provider@^5.0.0", "@ethersproject/abstract-provider@^5.4.0", "@ethersproject/abstract-provider@^5.4.1": version "5.4.1" resolved "https://registry.yarnpkg.com/@ethersproject/abstract-provider/-/abstract-provider-5.4.1.tgz#e404309a29f771bd4d28dbafadcaa184668c2a6e" From b90494062ef91643083586d00364addc3a383dc5 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Fri, 12 Nov 2021 11:47:49 -0500 Subject: [PATCH 2/6] fix: no gas price for static fns in deploy --- .changeset/lovely-pumas-sing.md | 5 +++++ packages/contracts/deploy/000-hardhat-setup.ts | 2 +- ...002-OVM_ChainStorageContainer_ctc_batches.deploy.ts | 2 +- ...003-OVM_ChainStorageContainer_scc_batches.deploy.ts | 2 +- .../deploy/004-OVM_CanonicalTransactionChain.deploy.ts | 2 +- .../deploy/005-OVM_StateCommitmentChain.deploy.ts | 2 +- .../contracts/deploy/006-OVM_BondManager.deploy.ts | 2 +- .../deploy/007-OVM_L1CrossDomainMessenger.deploy.ts | 2 +- .../008-Proxy__OVM_L1CrossDomainMessenger.deploy.ts | 2 +- .../deploy/009-Proxy__OVM_L1StandardBridge.deploy.ts | 2 +- .../contracts/deploy/010-AddressDictator.deploy.ts | 2 +- packages/contracts/deploy/011-set-addresses.ts | 5 +---- .../012-initialize-Proxy__L1CrossDomainMessenger.ts | 2 +- .../contracts/deploy/013-ChugSplashDictator.deploy.ts | 2 +- .../deploy/014-OVM_L1StandardBridge.deploy.ts | 2 +- packages/contracts/deploy/015-finalize.ts | 2 +- packages/contracts/deploy/016-fund-accounts.ts | 5 +---- .../src/{hardhat-deploy-ethers.ts => deploy-utils.ts} | 10 +++++++++- 18 files changed, 30 insertions(+), 23 deletions(-) create mode 100644 .changeset/lovely-pumas-sing.md rename packages/contracts/src/{hardhat-deploy-ethers.ts => deploy-utils.ts} (94%) diff --git a/.changeset/lovely-pumas-sing.md b/.changeset/lovely-pumas-sing.md new file mode 100644 index 0000000000000..fd4f728f3792e --- /dev/null +++ b/.changeset/lovely-pumas-sing.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts': patch +--- + +Use a gas price of zero for static calls in the deploy process diff --git a/packages/contracts/deploy/000-hardhat-setup.ts b/packages/contracts/deploy/000-hardhat-setup.ts index 244026c04e972..0bd9426f22318 100644 --- a/packages/contracts/deploy/000-hardhat-setup.ts +++ b/packages/contracts/deploy/000-hardhat-setup.ts @@ -6,7 +6,7 @@ import { fundAccount, sendImpersonatedTx, BIG_BALANCE, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' import { awaitCondition } from '@eth-optimism/core-utils' diff --git a/packages/contracts/deploy/002-OVM_ChainStorageContainer_ctc_batches.deploy.ts b/packages/contracts/deploy/002-OVM_ChainStorageContainer_ctc_batches.deploy.ts index d2b2237d1ece8..730481a74efed 100644 --- a/packages/contracts/deploy/002-OVM_ChainStorageContainer_ctc_batches.deploy.ts +++ b/packages/contracts/deploy/002-OVM_ChainStorageContainer_ctc_batches.deploy.ts @@ -5,7 +5,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/003-OVM_ChainStorageContainer_scc_batches.deploy.ts b/packages/contracts/deploy/003-OVM_ChainStorageContainer_scc_batches.deploy.ts index 4e842aaf065b0..64fe83da8e419 100644 --- a/packages/contracts/deploy/003-OVM_ChainStorageContainer_scc_batches.deploy.ts +++ b/packages/contracts/deploy/003-OVM_ChainStorageContainer_scc_batches.deploy.ts @@ -5,7 +5,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts b/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts index 00a73871546d9..8ab5e7585b3fb 100644 --- a/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts +++ b/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts @@ -5,7 +5,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/005-OVM_StateCommitmentChain.deploy.ts b/packages/contracts/deploy/005-OVM_StateCommitmentChain.deploy.ts index 9c595ee27b28c..7a4f280e1da31 100644 --- a/packages/contracts/deploy/005-OVM_StateCommitmentChain.deploy.ts +++ b/packages/contracts/deploy/005-OVM_StateCommitmentChain.deploy.ts @@ -5,7 +5,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/006-OVM_BondManager.deploy.ts b/packages/contracts/deploy/006-OVM_BondManager.deploy.ts index ee0fad40b2249..0917c12fdac59 100644 --- a/packages/contracts/deploy/006-OVM_BondManager.deploy.ts +++ b/packages/contracts/deploy/006-OVM_BondManager.deploy.ts @@ -5,7 +5,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts b/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts index 75deb04ebea52..046199d55f150 100644 --- a/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts +++ b/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts @@ -6,7 +6,7 @@ import { hexStringEquals, awaitCondition } from '@eth-optimism/core-utils' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/008-Proxy__OVM_L1CrossDomainMessenger.deploy.ts b/packages/contracts/deploy/008-Proxy__OVM_L1CrossDomainMessenger.deploy.ts index 385358449133b..6e64fcd4c1d63 100644 --- a/packages/contracts/deploy/008-Proxy__OVM_L1CrossDomainMessenger.deploy.ts +++ b/packages/contracts/deploy/008-Proxy__OVM_L1CrossDomainMessenger.deploy.ts @@ -5,7 +5,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/009-Proxy__OVM_L1StandardBridge.deploy.ts b/packages/contracts/deploy/009-Proxy__OVM_L1StandardBridge.deploy.ts index 8cdedb0b14a8e..91f9d3b65bb83 100644 --- a/packages/contracts/deploy/009-Proxy__OVM_L1StandardBridge.deploy.ts +++ b/packages/contracts/deploy/009-Proxy__OVM_L1StandardBridge.deploy.ts @@ -2,7 +2,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' /* Imports: Internal */ -import { deployAndVerifyAndThen } from '../src/hardhat-deploy-ethers' +import { deployAndVerifyAndThen } from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/010-AddressDictator.deploy.ts b/packages/contracts/deploy/010-AddressDictator.deploy.ts index c41c303abd594..c0f963ab62647 100644 --- a/packages/contracts/deploy/010-AddressDictator.deploy.ts +++ b/packages/contracts/deploy/010-AddressDictator.deploy.ts @@ -6,7 +6,7 @@ import { hexStringEquals } from '@eth-optimism/core-utils' import { deployAndVerifyAndThen, getContractFromArtifact, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' import { predeploys } from '../src/predeploys' diff --git a/packages/contracts/deploy/011-set-addresses.ts b/packages/contracts/deploy/011-set-addresses.ts index 3103cecea6ccf..88a46a67fe824 100644 --- a/packages/contracts/deploy/011-set-addresses.ts +++ b/packages/contracts/deploy/011-set-addresses.ts @@ -3,10 +3,7 @@ import { hexStringEquals, awaitCondition } from '@eth-optimism/core-utils' import { DeployFunction } from 'hardhat-deploy/dist/types' /* Imports: Internal */ -import { - getContractFromArtifact, - isHardhatNode, -} from '../src/hardhat-deploy-ethers' +import { getContractFromArtifact, isHardhatNode } from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts b/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts index a40d18d41fa44..815bb96d32284 100644 --- a/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts +++ b/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts @@ -3,7 +3,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { hexStringEquals, awaitCondition } from '@eth-optimism/core-utils' /* Imports: Internal */ -import { getContractFromArtifact } from '../src/hardhat-deploy-ethers' +import { getContractFromArtifact } from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/013-ChugSplashDictator.deploy.ts b/packages/contracts/deploy/013-ChugSplashDictator.deploy.ts index d563d3c9b9400..86cefd71877d9 100644 --- a/packages/contracts/deploy/013-ChugSplashDictator.deploy.ts +++ b/packages/contracts/deploy/013-ChugSplashDictator.deploy.ts @@ -8,7 +8,7 @@ import { getContractDefinition } from '../src/contract-defs' import { getContractFromArtifact, deployAndVerifyAndThen, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/014-OVM_L1StandardBridge.deploy.ts b/packages/contracts/deploy/014-OVM_L1StandardBridge.deploy.ts index 14e13c42a8d3d..bf0ff22ea1240 100644 --- a/packages/contracts/deploy/014-OVM_L1StandardBridge.deploy.ts +++ b/packages/contracts/deploy/014-OVM_L1StandardBridge.deploy.ts @@ -9,7 +9,7 @@ import { getContractFromArtifact, deployAndVerifyAndThen, isHardhatNode, -} from '../src/hardhat-deploy-ethers' +} from '../src/deploy-utils' import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { diff --git a/packages/contracts/deploy/015-finalize.ts b/packages/contracts/deploy/015-finalize.ts index 15a09b2505d2e..9e0d574fff4f4 100644 --- a/packages/contracts/deploy/015-finalize.ts +++ b/packages/contracts/deploy/015-finalize.ts @@ -3,7 +3,7 @@ import { DeployFunction } from 'hardhat-deploy/dist/types' import { hexStringEquals, awaitCondition } from '@eth-optimism/core-utils' /* Imports: Internal */ -import { getContractFromArtifact } from '../src/hardhat-deploy-ethers' +import { getContractFromArtifact } from '../src/deploy-utils' const deployFn: DeployFunction = async (hre) => { const { deployer } = await hre.getNamedAccounts() diff --git a/packages/contracts/deploy/016-fund-accounts.ts b/packages/contracts/deploy/016-fund-accounts.ts index cac809831243f..a259311a57b81 100644 --- a/packages/contracts/deploy/016-fund-accounts.ts +++ b/packages/contracts/deploy/016-fund-accounts.ts @@ -5,10 +5,7 @@ import { defaultHardhatNetworkHdAccountsConfigParams } from 'hardhat/internal/co import { normalizeHardhatNetworkAccountsConfig } from 'hardhat/internal/core/providers/util' /* Imports: Internal */ -import { - getContractFromArtifact, - isHardhatNode, -} from '../src/hardhat-deploy-ethers' +import { getContractFromArtifact, isHardhatNode } from '../src/deploy-utils' import { names } from '../src/address-names' // This is a TEMPORARY way to fund the default hardhat accounts on L2. The better way to do this is diff --git a/packages/contracts/src/hardhat-deploy-ethers.ts b/packages/contracts/src/deploy-utils.ts similarity index 94% rename from packages/contracts/src/hardhat-deploy-ethers.ts rename to packages/contracts/src/deploy-utils.ts index c9b99c4e86821..e7c98f409ac90 100644 --- a/packages/contracts/src/hardhat-deploy-ethers.ts +++ b/packages/contracts/src/deploy-utils.ts @@ -103,8 +103,16 @@ export const getAdvancedContract = (opts: { for (const fnName of Object.keys(contract.functions)) { const fn = contract[fnName].bind(contract) ;(contract as any)[fnName] = async (...args: any) => { + // We want to use the gas price that has been configured at the beginning of the deployment. + // However, if the function being triggered is a "constant" (static) function, then we don't + // want to provide a gas price because we're prone to getting insufficient balance errors. + let gasPrice = opts.hre.deployConfig.gasPrice || undefined + if (contract.interface.getFunction(fnName).constant) { + gasPrice = 0 + } + const tx = await fn(...args, { - gasPrice: opts.hre.deployConfig.gasprice || undefined, + gasPrice, }) if (typeof tx !== 'object' || typeof tx.wait !== 'function') { From 3a87ac26b1193be4b5d244408ff6ee055903374b Mon Sep 17 00:00:00 2001 From: Mark Tyneway Date: Tue, 9 Nov 2021 11:03:38 -0800 Subject: [PATCH 3/6] regenesis-surgery: initial predeploy spec Adds a skeleton for the initial predeploy tests --- .../regenesis-surgery/test/predeploy.spec.ts | 62 +++++++++++++------ 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/packages/regenesis-surgery/test/predeploy.spec.ts b/packages/regenesis-surgery/test/predeploy.spec.ts index 7de7f4b832044..bb5b1fbf61a96 100644 --- a/packages/regenesis-surgery/test/predeploy.spec.ts +++ b/packages/regenesis-surgery/test/predeploy.spec.ts @@ -1,34 +1,56 @@ /* eslint-disable @typescript-eslint/no-empty-function */ -describe.skip('predeploys', () => { - describe('new predeploys that are not ETH', () => { - it('should have the exact state specified in the base genesis file', async () => {}) +import { expect, env } from './setup' +import { AccountType, Account } from '../scripts/types' + +describe('predeploys', () => { + let predeploys = { + eth: [], + newNotEth: [], + noWipe: [], + wipe: [], + weth: [], + } + + before(async () => { + await env.init() + predeploys.eth = env.getAccountsByType(AccountType.PREDEPLOY_ETH) + predeploys.newNotEth = env.getAccountsByType(AccountType.PREDEPLOY_NEW_NOT_ETH) + predeploys.noWipe = env.getAccountsByType(AccountType.PREDEPLOY_NO_WIPE) + predeploys.wipe = env.getAccountsByType(AccountType.PREDEPLOY_WIPE) + predeploys.weth = env.getAccountsByType(AccountType.PREDEPLOY_WETH) }) - describe('predeploys where the old state should be wiped', () => { - it('should have the code and storage of the base genesis file', async () => {}) + it('predeploy tests', () => { + describe.skip('new predeploys that are not ETH', () => { + it('should have the exact state specified in the base genesis file', async () => {}) + }) - it('should have the same nonce and balance as before', async () => {}) - }) + describe.skip('predeploys where the old state should be wiped', () => { + it('should have the code and storage of the base genesis file', async () => {}) - describe('predeploys where the old state should be preserved', () => { - it('should have the code of the base genesis file', async () => {}) + it('should have the same nonce and balance as before', async () => {}) + }) - it('should have the combined storage of the old and new state', async () => {}) + describe.skip('predeploys where the old state should be preserved', () => { + it('should have the code of the base genesis file', async () => {}) - it('should have the same nonce and balance as before', async () => {}) - }) + it('should have the combined storage of the old and new state', async () => {}) - describe('OVM_ETH', () => { - it('should have disabled ERC20 features', async () => {}) + it('should have the same nonce and balance as before', async () => {}) + }) - it('should no recorded balance for the contracts that move to WETH9', async () => {}) + describe.skip('OVM_ETH', () => { + it('should have disabled ERC20 features', async () => {}) - it('should have a new balance for WETH9 equal to the sum of the moved contract balances', async () => {}) - }) + it('should no recorded balance for the contracts that move to WETH9', async () => {}) + + it('should have a new balance for WETH9 equal to the sum of the moved contract balances', async () => {}) + }) - describe('WETH9', () => { - it('should have balances for each contract that should move', async () => {}) + describe.skip('WETH9', () => { + it('should have balances for each contract that should move', async () => {}) - it('should have a balance equal to the sum of all moved balances', async () => {}) + it('should have a balance equal to the sum of all moved balances', async () => {}) + }) }) }) From d5226e853403f4f6abf28bcd2d4b827dfdeddd3c Mon Sep 17 00:00:00 2001 From: Annie Ke Date: Tue, 9 Nov 2021 12:38:45 -0800 Subject: [PATCH 4/6] wip: surgery predeploy tests --- packages/regenesis-surgery/package.json | 5 +- .../regenesis-surgery/test/predeploy.spec.ts | 243 ++++++++++++++++-- packages/regenesis-surgery/test/setup.ts | 5 + .../regenesis-surgery/test/uniswap.spec.ts | 4 +- yarn.lock | 2 +- 5 files changed, 227 insertions(+), 32 deletions(-) diff --git a/packages/regenesis-surgery/package.json b/packages/regenesis-surgery/package.json index 738d287fc9a82..3969b20763100 100644 --- a/packages/regenesis-surgery/package.json +++ b/packages/regenesis-surgery/package.json @@ -39,13 +39,14 @@ "eslint-plugin-prettier": "^3.4.0", "eslint-plugin-react": "^7.24.0", "eslint-plugin-unicorn": "^32.0.1", + "ethereum-waffle": "^3.4.0", "ethereumjs-util": "^7.1.3", "ethers": "^5.4.5", "lint-staged": "11.0.0", "mocha": "^9.1.2", "node-fetch": "2.6.5", "solc": "0.8.7-fixed", - "ts-node": "^10.0.0", - "ts-mocha": "^8.0.0" + "ts-mocha": "^8.0.0", + "ts-node": "^10.0.0" } } diff --git a/packages/regenesis-surgery/test/predeploy.spec.ts b/packages/regenesis-surgery/test/predeploy.spec.ts index bb5b1fbf61a96..4aae248f231af 100644 --- a/packages/regenesis-surgery/test/predeploy.spec.ts +++ b/packages/regenesis-surgery/test/predeploy.spec.ts @@ -1,56 +1,247 @@ -/* eslint-disable @typescript-eslint/no-empty-function */ -import { expect, env } from './setup' -import { AccountType, Account } from '../scripts/types' +import { ethers, BigNumber, Contract } from 'ethers' +import { expect, env, ERC20_ABI } from './setup' +import { AccountType } from '../scripts/types' +import { GenesisJsonProvider } from './provider' describe('predeploys', () => { - let predeploys = { + const predeploys = { eth: [], newNotEth: [], noWipe: [], wipe: [], weth: [], } + // Base genesis file only + let genesisStateProvider: GenesisJsonProvider + // Old sequencer state + let oldStateProvider: GenesisJsonProvider before(async () => { await env.init() predeploys.eth = env.getAccountsByType(AccountType.PREDEPLOY_ETH) - predeploys.newNotEth = env.getAccountsByType(AccountType.PREDEPLOY_NEW_NOT_ETH) + predeploys.newNotEth = env.getAccountsByType( + AccountType.PREDEPLOY_NEW_NOT_ETH + ) predeploys.noWipe = env.getAccountsByType(AccountType.PREDEPLOY_NO_WIPE) predeploys.wipe = env.getAccountsByType(AccountType.PREDEPLOY_WIPE) predeploys.weth = env.getAccountsByType(AccountType.PREDEPLOY_WETH) + + genesisStateProvider = new GenesisJsonProvider( + env.surgeryDataSources.genesis + ) + oldStateProvider = new GenesisJsonProvider( + env.surgeryDataSources.configs.stateDumpFilePath + ) }) - it('predeploy tests', () => { - describe.skip('new predeploys that are not ETH', () => { - it('should have the exact state specified in the base genesis file', async () => {}) - }) + describe('new predeploys that are not ETH', () => { + for (const [i, account] of predeploys.newNotEth.entries()) { + describe(`account ${i}/${predeploys.newNotEth.length} (${account.address})`, () => { + it('should have the exact state specified in the base genesis file', async () => { + const preBytecode = await genesisStateProvider.getCode( + account.address + ) + const postBytecode = await env.postL2Provider.getCode(account.address) + expect(preBytecode).to.eq(postBytecode) - describe.skip('predeploys where the old state should be wiped', () => { - it('should have the code and storage of the base genesis file', async () => {}) + const dumpAccount = env.surgeryDataSources.dump.find( + (a) => a.address === account.address + ) + if (dumpAccount.storage) { + for (const key of Object.keys(dumpAccount.storage)) { + const pre = await env.preL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + const post = await env.postL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + expect(pre).to.deep.eq(post) + } + } - it('should have the same nonce and balance as before', async () => {}) - }) + const preNonce = await genesisStateProvider.getTransactionCount( + account.address, + env.config.stateDumpHeight + ) + const postNonce = await env.postL2Provider.getTransactionCount( + account.address + ) + expect(preNonce).to.deep.eq(postNonce) + + const preBalance = await genesisStateProvider.getBalance( + account.address, + env.config.stateDumpHeight + ) + const postBalance = await env.postL2Provider.getBalance( + account.address + ) + expect(preBalance).to.deep.eq(postBalance) + }) + }) + } + }) - describe.skip('predeploys where the old state should be preserved', () => { - it('should have the code of the base genesis file', async () => {}) + describe('predeploys where the old state should be wiped', () => { + for (const [i, account] of predeploys.wipe.entries()) { + describe(`account ${i}/${predeploys.wipe.length} (${account.address})`, () => { + it('should have the code and storage of the base genesis file', async () => { + const preBytecode = await genesisStateProvider.getCode( + account.address + ) + const postBytecode = await env.postL2Provider.getCode(account.address) + expect(preBytecode).to.eq(postBytecode) - it('should have the combined storage of the old and new state', async () => {}) + const dumpAccount = env.surgeryDataSources.dump.find( + (a) => a.address === account.address + ) + if (dumpAccount.storage) { + for (const key of Object.keys(dumpAccount.storage)) { + const pre = await env.preL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + const post = await env.postL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + expect(pre).to.deep.eq(post) + } + } + }) - it('should have the same nonce and balance as before', async () => {}) - }) + it('should have the same nonce and balance as before', async () => { + const preNonce = await oldStateProvider.getTransactionCount( + account.address, + env.config.stateDumpHeight + ) + const postNonce = await env.postL2Provider.getTransactionCount( + account.address + ) + expect(preNonce).to.deep.eq(postNonce) - describe.skip('OVM_ETH', () => { - it('should have disabled ERC20 features', async () => {}) + const preBalance = await oldStateProvider.getBalance( + account.address, + env.config.stateDumpHeight + ) + const postBalance = await env.postL2Provider.getBalance( + account.address + ) + expect(preBalance).to.deep.eq(postBalance) + }) + }) + } + }) - it('should no recorded balance for the contracts that move to WETH9', async () => {}) + describe('predeploys where the old state should be preserved', () => { + for (const [i, account] of predeploys.noWipe.entries()) { + describe(`account ${i}/${predeploys.noWipe.length} (${account.address})`, () => { + it('should have the code of the base genesis file', async () => { + const preBytecode = await genesisStateProvider.getCode( + account.address + ) + const postBytecode = await env.postL2Provider.getCode(account.address) + expect(preBytecode).to.eq(postBytecode) + }) - it('should have a new balance for WETH9 equal to the sum of the moved contract balances', async () => {}) - }) + it('should have the combined storage of the old and new state', async () => { + const dumpAccount = env.surgeryDataSources.dump.find( + (a) => a.address === account.address + ) + if (dumpAccount.storage) { + for (const key of Object.keys(dumpAccount.storage)) { + const pre = await env.preL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + const post = await env.postL2Provider.getStorageAt( + account.address, + BigNumber.from(key) + ) + expect(pre).to.deep.eq(post) + } + } + }) - describe.skip('WETH9', () => { - it('should have balances for each contract that should move', async () => {}) + it('should have the same nonce and balance as before', async () => { + const preNonce = await oldStateProvider.getTransactionCount( + account.address, + env.config.stateDumpHeight + ) + const postNonce = await env.postL2Provider.getTransactionCount( + account.address + ) + expect(preNonce).to.deep.eq(postNonce) - it('should have a balance equal to the sum of all moved balances', async () => {}) + const preBalance = await oldStateProvider.getBalance( + account.address, + env.config.stateDumpHeight + ) + const postBalance = await env.postL2Provider.getBalance( + account.address + ) + expect(preBalance).to.deep.eq(postBalance) + }) + }) + } + }) + + describe('OVM_ETH', () => { + if (!env.hasLiveProviders()) { + console.log('Cannot run pool contract tests without live provider') + return + } + let OVM_ETH: Contract + before(async () => { + OVM_ETH = new ethers.Contract( + predeploys.eth[0].address, + ERC20_ABI, + env.postL2Provider + ) }) + + for (const [i, account] of predeploys.eth.entries()) { + describe(`account ${i}/${predeploys.eth.length} (${account.address})`, () => { + it('should have disabled ERC20 features', async () => { + await expect( + OVM_ETH.transfer(account.address, 100) + ).to.be.revertedWith( + 'OVM_ETH: transfer is disabled pending further community discussion.' + ) + }) + + it('should have a new balance for WETH9 equal to the sum of the moved contract balances', async () => { + // need live provider for WETH balances + }) + }) + } + }) + + describe('WETH9', () => { + for (const [i, account] of predeploys.weth.entries()) { + describe(`account ${i}/${predeploys.weth.length} (${account.address})`, () => { + it('should no recorded ETH balance', async () => { + const postBalance = await env.postL2Provider.getBalance( + account.address + ) + expect(postBalance.toNumber()).to.eq(0) + }) + + it('should have WETH balances for each contract that should move', async () => { + if (!env.hasLiveProviders()) { + console.log('Cannot run pool contract tests without live provider') + return + } + }) + + it('should have a balance equal to the sum of all moved balances', async () => { + if (!env.hasLiveProviders()) { + console.log('Cannot run pool contract tests without live provider') + return + } + }) + }) + } }) }) diff --git a/packages/regenesis-surgery/test/setup.ts b/packages/regenesis-surgery/test/setup.ts index c8c388993bc9d..cccb7826de20c 100644 --- a/packages/regenesis-surgery/test/setup.ts +++ b/packages/regenesis-surgery/test/setup.ts @@ -5,6 +5,7 @@ import chaiAsPromised from 'chai-as-promised' import * as dotenv from 'dotenv' import { getenv, remove0x } from '@eth-optimism/core-utils' import { providers, BigNumber } from 'ethers' +import { solidity } from 'ethereum-waffle' import { SurgeryDataSources, Account, AccountType } from '../scripts/types' import { loadSurgeryData } from '../scripts/data' import { classify } from '../scripts/classifiers' @@ -12,6 +13,7 @@ import { GenesisJsonProvider } from './provider' // Chai plugins go here. chai.use(chaiAsPromised) +chai.use(solidity) const should = chai.should() const expect = chai.expect @@ -19,6 +21,9 @@ const expect = chai.expect dotenv.config() export const NUM_ACCOUNTS_DIVISOR = 4096 +export const ERC20_ABI = [ + 'function balanceOf(address owner) view returns (uint256)', +] interface TestEnvConfig { preL2ProviderUrl: string | null diff --git a/packages/regenesis-surgery/test/uniswap.spec.ts b/packages/regenesis-surgery/test/uniswap.spec.ts index 492c991576f79..343313f9a534b 100644 --- a/packages/regenesis-surgery/test/uniswap.spec.ts +++ b/packages/regenesis-surgery/test/uniswap.spec.ts @@ -2,11 +2,9 @@ import { ethers } from 'ethers' import { abi as UNISWAP_POOL_ABI } from '@uniswap/v3-core/artifacts/contracts/UniswapV3Pool.sol/UniswapV3Pool.json' import { UNISWAP_V3_NFPM_ADDRESS } from '../scripts/constants' import { getUniswapV3Factory, replaceWETH } from '../scripts/utils' -import { expect, env } from './setup' +import { expect, env, ERC20_ABI } from './setup' import { AccountType } from '../scripts/types' -const ERC20_ABI = ['function balanceOf(address owner) view returns (uint256)'] - describe('uniswap contracts', () => { before(async () => { await env.init() diff --git a/yarn.lock b/yarn.lock index a2d1bf1d485d9..602269aeac587 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6861,7 +6861,7 @@ ethereum-cryptography@^0.1.2, ethereum-cryptography@^0.1.3: secp256k1 "^4.0.1" setimmediate "^1.0.5" -ethereum-waffle@^3.3.0: +ethereum-waffle@^3.3.0, ethereum-waffle@^3.4.0: version "3.4.0" resolved "https://registry.yarnpkg.com/ethereum-waffle/-/ethereum-waffle-3.4.0.tgz#990b3c6c26db9c2dd943bf26750a496f60c04720" integrity sha512-ADBqZCkoSA5Isk486ntKJVjFEawIiC+3HxNqpJqONvh3YXBTNiRfXvJtGuAFLXPG91QaqkGqILEHANAo7j/olQ== From a8b14a7dcac1106fd1836f955a3a18aa784d6e34 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Fri, 12 Nov 2021 12:24:08 -0500 Subject: [PATCH 5/6] feat: transfer messenger ownership during deploy --- .changeset/sharp-ants-act.md | 5 +++++ .../007-OVM_L1CrossDomainMessenger.deploy.ts | 19 ++++++++++++++++++- ...nitialize-Proxy__L1CrossDomainMessenger.ts | 19 +++++++++++++++++-- 3 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 .changeset/sharp-ants-act.md diff --git a/.changeset/sharp-ants-act.md b/.changeset/sharp-ants-act.md new file mode 100644 index 0000000000000..af873cb97412a --- /dev/null +++ b/.changeset/sharp-ants-act.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts': patch +--- + +Adds additional deploy step to transfer messenger ownership diff --git a/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts b/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts index 046199d55f150..c79df6417ef96 100644 --- a/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts +++ b/packages/contracts/deploy/007-OVM_L1CrossDomainMessenger.deploy.ts @@ -25,7 +25,7 @@ const deployFn: DeployFunction = async (hre) => { // a proxy. However, it's best practice to initialize it anyway just in case there's // some unknown security hole. It also prevents another user from appearing like an // official address because it managed to call the initialization function. - console.log(`Initializing L1CrossDomainMessenger...`) + console.log(`Initializing L1CrossDomainMessenger (implementation)...`) await contract.initialize(Lib_AddressManager.address) console.log(`Checking that contract was correctly initialized...`) @@ -39,6 +39,23 @@ const deployFn: DeployFunction = async (hre) => { 5000, 100 ) + + // Same thing as above, we want to transfer ownership of this contract to the owner of the + // AddressManager. Not technically necessary but seems like the right thing to do. + console.log( + `Transferring ownership of L1CrossDomainMessenger (implementation)...` + ) + const owner = (hre as any).deployConfig.ovmAddressManagerOwner + await contract.transferOwnership(owner) + + console.log(`Checking that contract owner was correctly set...`) + await awaitCondition( + async () => { + return hexStringEquals(await contract.owner(), owner) + }, + 5000, + 100 + ) }, }) } diff --git a/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts b/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts index 815bb96d32284..0e918bc4010ca 100644 --- a/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts +++ b/packages/contracts/deploy/012-initialize-Proxy__L1CrossDomainMessenger.ts @@ -9,8 +9,6 @@ import { names } from '../src/address-names' const deployFn: DeployFunction = async (hre) => { const { deployer } = await hre.getNamedAccounts() - console.log(`Initializing Proxy__L1CrossDomainMessenger...`) - // There's a risk that we could get front-run during a fresh deployment, which would brick this // contract and require that the proxy be re-deployed. We will not have this risk once we move // entirely to chugsplash-style deployments. It's unlikely to happen and relatively easy to @@ -29,6 +27,7 @@ const deployFn: DeployFunction = async (hre) => { names.unmanaged.Lib_AddressManager ) + console.log(`Initializing Proxy__OVM_L1CrossDomainMessenger...`) await Proxy__OVM_L1CrossDomainMessenger.initialize(Lib_AddressManager.address) console.log(`Checking that contract was correctly initialized...`) @@ -42,6 +41,22 @@ const deployFn: DeployFunction = async (hre) => { 5000, 100 ) + + console.log(`Setting Proxy__OVM_L1CrossDomainMessenger owner...`) + const owner = (hre as any).deployConfig.ovmAddressManagerOwner + await Proxy__OVM_L1CrossDomainMessenger.transferOwnership(owner) + + console.log(`Checking that the contract owner was correctly set...`) + await awaitCondition( + async () => { + return hexStringEquals( + await Proxy__OVM_L1CrossDomainMessenger.owner(), + owner + ) + }, + 5000, + 100 + ) } deployFn.tags = ['finalize'] From 7f2898ba592122c6d973ca391248768a97d52b6d Mon Sep 17 00:00:00 2001 From: Matthew Slipper Date: Fri, 19 Nov 2021 10:30:24 -0700 Subject: [PATCH 6/6] Fix deadlock on invalid txs - Fixes a bug that causes Geth to deadlock if transactions pass the policy checks but fail in the miner. - Fixes a bug that causes Geth to mine empty blocks, and returns a proper error to users. --- .changeset/spicy-spoons-jog.md | 5 ++++ l2geth/core/events.go | 5 +++- l2geth/core/tx_pool.go | 2 +- l2geth/miner/worker.go | 14 ++++++++-- l2geth/rollup/sync_service.go | 51 +++++++++++++++++++++------------- 5 files changed, 54 insertions(+), 23 deletions(-) create mode 100644 .changeset/spicy-spoons-jog.md diff --git a/.changeset/spicy-spoons-jog.md b/.changeset/spicy-spoons-jog.md new file mode 100644 index 0000000000000..251d2f339da6f --- /dev/null +++ b/.changeset/spicy-spoons-jog.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/l2geth': patch +--- + +Fixes deadlock diff --git a/l2geth/core/events.go b/l2geth/core/events.go index ac935a137f5f6..4a85f02de2b90 100644 --- a/l2geth/core/events.go +++ b/l2geth/core/events.go @@ -22,7 +22,10 @@ import ( ) // NewTxsEvent is posted when a batch of transactions enter the transaction pool. -type NewTxsEvent struct{ Txs []*types.Transaction } +type NewTxsEvent struct { + Txs []*types.Transaction + ErrCh chan error +} // NewMinedBlockEvent is posted when a block has been imported. type NewMinedBlockEvent struct{ Block *types.Block } diff --git a/l2geth/core/tx_pool.go b/l2geth/core/tx_pool.go index 00c5d0c6fde82..9e2fedd9d596c 100644 --- a/l2geth/core/tx_pool.go +++ b/l2geth/core/tx_pool.go @@ -1090,7 +1090,7 @@ func (pool *TxPool) runReorg(done chan struct{}, reset *txpoolResetRequest, dirt for _, set := range events { txs = append(txs, set.Flatten()...) } - pool.txFeed.Send(NewTxsEvent{txs}) + pool.txFeed.Send(NewTxsEvent{Txs: txs}) } } diff --git a/l2geth/miner/worker.go b/l2geth/miner/worker.go index 9af4d39de6e3c..43b01d1a5ae74 100644 --- a/l2geth/miner/worker.go +++ b/l2geth/miner/worker.go @@ -506,7 +506,10 @@ func (w *worker) mainLoop() { } w.pendingMu.Unlock() } else { - log.Debug("Problem committing transaction", "msg", err) + log.Error("Problem committing transaction", "msg", err) + if ev.ErrCh != nil { + ev.ErrCh <- err + } } case ev := <-w.txsCh: @@ -781,6 +784,11 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin } var coalescedLogs []*types.Log + // UsingOVM + // Keep track of the number of transactions being added to the block. + // Blocks should only have a single transaction. This value is used to + // compute a success return value + var txCount int for { // In the following three cases, we will interrupt the execution of the transaction. @@ -814,6 +822,8 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin break } + txCount++ + // Error may be ignored here. The error has already been checked // during transaction acceptance is the transaction pool. // @@ -881,7 +891,7 @@ func (w *worker) commitTransactions(txs *types.TransactionsByPriceAndNonce, coin if interrupt != nil { w.resubmitAdjustCh <- &intervalAdjust{inc: false} } - return false + return txCount == 0 } // commitNewTx is an OVM addition that mines a block with a single tx in it. diff --git a/l2geth/rollup/sync_service.go b/l2geth/rollup/sync_service.go index 63bd5d1fcdcc4..3f01ae37c060f 100644 --- a/l2geth/rollup/sync_service.go +++ b/l2geth/rollup/sync_service.go @@ -806,9 +806,9 @@ func (s *SyncService) applyTransactionToTip(tx *types.Transaction) error { // Note that Ethereum Layer one consensus rules dictate that the timestamp // must be strictly increasing between blocks, so no need to check both the // timestamp and the blocknumber. + ts := s.GetLatestL1Timestamp() + bn := s.GetLatestL1BlockNumber() if tx.L1Timestamp() == 0 { - ts := s.GetLatestL1Timestamp() - bn := s.GetLatestL1BlockNumber() tx.SetL1Timestamp(ts) tx.SetL1BlockNumber(bn) } else if tx.L1Timestamp() > s.GetLatestL1Timestamp() { @@ -816,17 +816,15 @@ func (s *SyncService) applyTransactionToTip(tx *types.Transaction) error { // service's locally maintained timestamp, update the timestamp and // blocknumber to equal that of the transaction's. This should happen // with `enqueue` transactions. - ts := tx.L1Timestamp() - bn := tx.L1BlockNumber() - s.SetLatestL1Timestamp(ts) - s.SetLatestL1BlockNumber(bn.Uint64()) - log.Debug("Updating OVM context based on new transaction", "timestamp", ts, "blocknumber", bn.Uint64(), "queue-origin", tx.QueueOrigin()) + s.SetLatestL1Timestamp(tx.L1Timestamp()) + s.SetLatestL1BlockNumber(tx.L1BlockNumber().Uint64()) + log.Debug("Updating OVM context based on new transaction", "timestamp", ts, "blocknumber", tx.L1BlockNumber().Uint64(), "queue-origin", tx.QueueOrigin()) } else if tx.L1Timestamp() < s.GetLatestL1Timestamp() { log.Error("Timestamp monotonicity violation", "hash", tx.Hash().Hex()) } + index := s.GetLatestIndex() if tx.GetMeta().Index == nil { - index := s.GetLatestIndex() if index == nil { tx.SetIndex(0) } else { @@ -846,21 +844,36 @@ func (s *SyncService) applyTransactionToTip(tx *types.Transaction) error { log.Debug("Applying transaction to tip", "index", *tx.GetMeta().Index, "hash", tx.Hash().Hex(), "origin", tx.QueueOrigin().String()) txs := types.Transactions{tx} - s.txFeed.Send(core.NewTxsEvent{Txs: txs}) + errCh := make(chan error, 1) + s.txFeed.Send(core.NewTxsEvent{ + Txs: txs, + ErrCh: errCh, + }) // Block until the transaction has been added to the chain log.Trace("Waiting for transaction to be added to chain", "hash", tx.Hash().Hex()) - <-s.chainHeadCh - - // Update the cache when the transaction is from the owner - // of the gas price oracle - sender, _ := types.Sender(s.signer, tx) - owner := s.GasPriceOracleOwnerAddress() - if owner != nil && sender == *owner { - if err := s.updateGasPriceOracleCache(nil); err != nil { - return err + + select { + case err := <-errCh: + log.Error("Got error waiting for transaction to be added to chain", "msg", err) + s.SetLatestL1Timestamp(ts) + s.SetLatestL1BlockNumber(bn) + s.SetLatestIndex(index) + return err + case <-s.chainHeadCh: + // Update the cache when the transaction is from the owner + // of the gas price oracle + sender, _ := types.Sender(s.signer, tx) + owner := s.GasPriceOracleOwnerAddress() + if owner != nil && sender == *owner { + if err := s.updateGasPriceOracleCache(nil); err != nil { + s.SetLatestL1Timestamp(ts) + s.SetLatestL1BlockNumber(bn) + s.SetLatestIndex(index) + return err + } } + return nil } - return nil } // applyBatchedTransaction applies transactions that were batched to layer one.