Merge pull request #2172 from cfromknecht/bss-estimate-gas-clear-pending-tx

bss: properly set gas limit on clearing txs

.changeset/young-keys-travel.md

+---
+'@eth-optimism/batch-submitter-service': patch
+---
+
+Fixes a bug where clearing txs are rejected on startup due to missing gas limit

.github/workflows/bss-core.yml

+name: bss-core unit tests
+
+on:
+  push:
+    paths:
+      - 'go/bss-core/**'
+    branches:
+      - 'master'
+      - 'develop'
+      - '*rc'
+      - 'regenesis/*'
+  pull_request:
+    paths:
+      - 'go/bss-core/*'
+  workflow_dispatch:
+
+defaults:
+  run:
+    working-directory: './go/bss-core'
+
+jobs:
+  tests:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Install Go
+        uses: actions/setup-go@v2
+        with:
+          go-version: 1.16.x
+
+      - name: Checkout code
+        uses: actions/checkout@v2
+
+      - name: Test
+        run: go test -v ./...

.github/workflows/golangci-lint.yml

@@ -29,3 +29,8 @@ jobs:
         with:
           version: v1.29
           working-directory: go/batch-submitter
+      - name: golangci-lint bss-core
+        uses: golangci/golangci-lint-action@v2
+        with:
+          version: v1.29
+          working-directory: go/bss-core

go/bss-core/crypto.go

@@ -140,36 +140,3 @@ func ParseWalletPrivKeyAndContractAddr(
 
 	return privKey, contractAddress, nil
 }
-
-// parseWalletPrivKeyAndContractAddr returns the wallet private key to use for
-// sending transactions as well as the contract address to send to for a
-// particular sub-service.
-func parseWalletPrivKeyAndContractAddr(
-	name string,
-	mnemonic string,
-	hdPath string,
-	privKeyStr string,
-	contractAddrStr string,
-) (*ecdsa.PrivateKey, common.Address, error) {
-
-	// Parse wallet private key from either privkey string or BIP39 mnemonic
-	// and BIP32 HD derivation path.
-	privKey, err := GetConfiguredPrivateKey(mnemonic, hdPath, privKeyStr)
-	if err != nil {
-		return nil, common.Address{}, err
-	}
-
-	// Parse the target contract address the wallet will send to.
-	contractAddress, err := ParseAddress(contractAddrStr)
-	if err != nil {
-		return nil, common.Address{}, err
-	}
-
-	// Log wallet address rather than private key...
-	walletAddress := crypto.PubkeyToAddress(privKey.PublicKey)
-
-	log.Info(name+" wallet params parsed successfully", "wallet_address",
-		walletAddress, "contract_address", contractAddress)
-
-	return privKey, contractAddress, nil
-}

go/bss-core/drivers/clear_pending_tx.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/ethereum-optimism/optimism/go/bss-core/txmgr"
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
@@ -159,7 +160,20 @@ func SignClearingTx(
 	}
 
 	gasFeeCap := txmgr.CalcGasFeeCap(head.BaseFee, gasTipCap)
-	tx := CraftClearingTx(walletAddr, nonce, gasFeeCap, gasTipCap)
+
+	gasLimit, err := l1Client.EstimateGas(ctx, ethereum.CallMsg{
+		From:      walletAddr,
+		To:        &walletAddr,
+		GasFeeCap: gasFeeCap,
+		GasTipCap: gasTipCap,
+		Value:     nil,
+		Data:      nil,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	tx := CraftClearingTx(walletAddr, nonce, gasFeeCap, gasTipCap, gasLimit)
 
 	return types.SignTx(
 		tx, types.LatestSignerForChainID(chainID), privKey,
@@ -173,11 +187,13 @@ func CraftClearingTx(
 	nonce uint64,
 	gasFeeCap *big.Int,
 	gasTipCap *big.Int,
+	gasLimit uint64,
 ) *types.Transaction {
 
 	return types.NewTx(&types.DynamicFeeTx{
 		To:        &walletAddr,
 		Nonce:     nonce,
+		Gas:       gasLimit,
 		GasFeeCap: gasFeeCap,
 		GasTipCap: gasTipCap,
 		Value:     nil,

go/bss-core/drivers/clear_pending_tx_test.go

@@ -8,9 +8,10 @@ import (
 	"testing"
 	"time"
 
-	"github.com/ethereum-optimism/optimism/go/batch-submitter/drivers"
+	"github.com/ethereum-optimism/optimism/go/bss-core/drivers"
 	"github.com/ethereum-optimism/optimism/go/bss-core/mock"
 	"github.com/ethereum-optimism/optimism/go/bss-core/txmgr"
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
@@ -36,16 +37,18 @@ var (
 	testGasTipCap   = big.NewInt(4)
 	testBlockNumber = uint64(5)
 	testBaseFee     = big.NewInt(6)
+	testGasLimit    = uint64(7)
 )
 
 // TestCraftClearingTx asserts that CraftClearingTx produces the expected
 // unsigned clearing transaction.
 func TestCraftClearingTx(t *testing.T) {
 	tx := drivers.CraftClearingTx(
-		testWalletAddr, testNonce, testGasFeeCap, testGasTipCap,
+		testWalletAddr, testNonce, testGasFeeCap, testGasTipCap, testGasLimit,
 	)
 	require.Equal(t, &testWalletAddr, tx.To())
 	require.Equal(t, testNonce, tx.Nonce())
+	require.Equal(t, testGasLimit, tx.Gas())
 	require.Equal(t, testGasFeeCap, tx.GasFeeCap())
 	require.Equal(t, testGasTipCap, tx.GasTipCap())
 	require.Equal(t, new(big.Int), tx.Value())
@@ -64,6 +67,9 @@ func TestSignClearingTxEstimateGasSuccess(t *testing.T) {
 		SuggestGasTipCap: func(_ context.Context) (*big.Int, error) {
 			return testGasTipCap, nil
 		},
+		EstimateGas: func(_ context.Context, _ ethereum.CallMsg) (uint64, error) {
+			return testGasLimit, nil
+		},
 	})
 
 	expGasFeeCap := new(big.Int).Add(
@@ -131,6 +137,33 @@ func TestSignClearingTxHeaderByNumberFail(t *testing.T) {
 	require.Nil(t, tx)
 }
 
+// TestSignClearingTxEstimateGasFail asserts that signing a clearing
+// transaction will fail if the underlying call to EstimateGas fails.
+func TestSignClearingTxEstimateGasFail(t *testing.T) {
+	errEstimateGas := errors.New("estimate gas")
+
+	l1Client := mock.NewL1Client(mock.L1ClientConfig{
+		EstimateGas: func(_ context.Context, _ ethereum.CallMsg) (uint64, error) {
+			return 0, errEstimateGas
+		},
+		HeaderByNumber: func(_ context.Context, _ *big.Int) (*types.Header, error) {
+			return &types.Header{
+				BaseFee: testBaseFee,
+			}, nil
+		},
+		SuggestGasTipCap: func(_ context.Context) (*big.Int, error) {
+			return testGasTipCap, nil
+		},
+	})
+
+	tx, err := drivers.SignClearingTx(
+		"TEST", context.Background(), testWalletAddr, testNonce, l1Client,
+		testPrivKey, testChainID,
+	)
+	require.Equal(t, errEstimateGas, err)
+	require.Nil(t, tx)
+}
+
 type clearPendingTxHarness struct {
 	l1Client *mock.L1Client
 	txMgr    txmgr.TxManager
@@ -163,6 +196,11 @@ func newClearPendingTxHarnessWithNumConfs(
 			return testGasTipCap, nil
 		}
 	}
+	if l1ClientConfig.EstimateGas == nil {
+		l1ClientConfig.EstimateGas = func(_ context.Context, _ ethereum.CallMsg) (uint64, error) {
+			return testGasLimit, nil
+		}
+	}
 
 	l1Client := mock.NewL1Client(l1ClientConfig)
 	txMgr := txmgr.NewSimpleTxManager("test", txmgr.Config{

go/bss-core/drivers/interface.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"math/big"
 
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
 )
@@ -11,6 +12,13 @@ import (
 // L1Client is an abstraction over an L1 Ethereum client functionality required
 // by the batch submitter.
 type L1Client interface {
+	// EstimateGas tries to estimate the gas needed to execute a specific
+	// transaction based on the current pending state of the backend blockchain.
+	// There is no guarantee that this is the true gas limit requirement as
+	// other transactions may be added or removed by miners, but it should
+	// provide a basis for setting a reasonable default.
+	EstimateGas(context.Context, ethereum.CallMsg) (uint64, error)
+
 	// HeaderByNumber returns a block header from the current canonical chain.
 	// If number is nil, the latest known header is returned.
 	HeaderByNumber(context.Context, *big.Int) (*types.Header, error)

go/bss-core/mock/l1client.go

@@ -5,6 +5,7 @@ import (
 	"math/big"
 	"sync"
 
+	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
 )
@@ -15,6 +16,13 @@ type L1ClientConfig struct {
 	// BlockNumber returns the most recent block number.
 	BlockNumber func(context.Context) (uint64, error)
 
+	// EstimateGas tries to estimate the gas needed to execute a specific
+	// transaction based on the current pending state of the backend blockchain.
+	// There is no guarantee that this is the true gas limit requirement as
+	// other transactions may be added or removed by miners, but it should
+	// provide a basis for setting a reasonable default.
+	EstimateGas func(context.Context, ethereum.CallMsg) (uint64, error)
+
 	// HeaderByNumber returns a block header from the current canonical chain.
 	// If number is nil, the latest known header is returned.
 	HeaderByNumber func(context.Context, *big.Int) (*types.Header, error)
@@ -61,6 +69,18 @@ func (c *L1Client) BlockNumber(ctx context.Context) (uint64, error) {
 	return c.cfg.BlockNumber(ctx)
 }
 
+// EstimateGas tries to estimate the gas needed to execute a specific
+// transaction based on the current pending state of the backend blockchain.
+// There is no guarantee that this is the true gas limit requirement as other
+// transactions may be added or removed by miners, but it should provide a basis
+// for setting a reasonable default.
+func (c *L1Client) EstimateGas(ctx context.Context, msg ethereum.CallMsg) (uint64, error) {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+
+	return c.cfg.EstimateGas(ctx, msg)
+}
+
 // HeaderByNumber returns a block header from the current canonical chain. If
 // number is nil, the latest known header is returned.
 func (c *L1Client) HeaderByNumber(ctx context.Context, blockNumber *big.Int) (*types.Header, error) {
@@ -113,6 +133,16 @@ func (c *L1Client) SetBlockNumberFunc(
 	c.cfg.BlockNumber = f
 }
 
+// SetEstimateGasFunc overwrites the mock EstimateGas method.
+func (c *L1Client) SetEstimateGasFunc(
+	f func(context.Context, ethereum.CallMsg) (uint64, error)) {
+
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	c.cfg.EstimateGas = f
+}
+
 // SetHeaderByNumberFunc overwrites the mock HeaderByNumber method.
 func (c *L1Client) SetHeaderByNumberFunc(
 	f func(ctx context.Context, blockNumber *big.Int) (*types.Header, error)) {