fix: estimate gas when pending tx in mempool #1024

karlfloersch · 2021-06-06T01:50:31Z

Description
We have observed that when a transaction is stuck in the mempool, the replacement transactions fail to be applied and would return UNPREDICTABLE_GAS_LIMIT with the error that the starting index in the batch is incorrect.

The reason the starting index was seemingly incorrect was because eth_estimateGas has blockTag=pending by default.
That meant that when we attempted to resubmit our transaction, while estimating gas, it would attempt to reapply the same batch on top of the pending transaction. We explicitly do not want this behavior.

This PR changes the batch submitter submission logic in such a way that before submitting the transaction it populates the transaction fields including the gasLimit. This way we explicitly specify a gasLimit, we don't call estimateGas, and the transaction resubmission can occur.

Fixes OP-834

changeset-bot · 2021-06-06T01:50:35Z

🦋 Changeset detected

Latest commit: e2985bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@eth-optimism/batch-submitter	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov-commenter · 2021-06-07T04:46:02Z

Codecov Report

Merging #1024 (082fe1b) into develop (e615144) will not change coverage.
The diff coverage is n/a.

❗ Current head 082fe1b differs from pull request most recent head 3fc6772. Consider uploading reports for the commit 3fc6772 to get more accurate results

@@           Coverage Diff            @@
##           develop    #1024   +/-   ##
========================================
  Coverage    86.05%   86.05%           
========================================
  Files           49       49           
  Lines         1936     1936           
  Branches       307      307           
========================================
  Hits          1666     1666           
  Misses         270      270

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e615144...3fc6772. Read the comment docs.

snario

Questions:

Can you clarify (maybe in the code) how we are getting around the gas estimation error being thrown despite still making a call to this.signer.provider.estimateGas? Does an explicit call to this method not "rebase" on pending transactions?
Does the old appendSequencerBatch override now become unused? If so we should delete this code.
Does this now have the effect of making this code unable to submit multiple batches in series assuming each of them will succeed? Do we want this behaviour, even?

Requests:

We need to add a test case or two for these scenarios that would fail before this branch is merged

snario · 2021-06-07T14:24:35Z

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts

        const nonce = await this.signer.getTransactionCount()
+        // Generate the transaction we will repeatedly submit
+        const tx = await this.chainContract.populateTransaction.appendQueueBatch(
+          99999999


This 9999999 constant really needs a comment or some kind of explanation, it's unclear why this number exists here in the code.

To be honest, we could delete this code path completely since queue batch append isn't currently allowed

I think it's worth keeping around for posterity sake until we split the CTC and can delete it completely. We don't want to forget that in our current design this codepath is absolutely required for the eventual proper functioning of the system.

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts

snario · 2021-06-07T14:26:39Z

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts

      l1tipHeight,
    })

    const nonce = await this.signer.getTransactionCount()


Unrelated to this PR, but why are we even doing this if we are using populateTransaction; shouldn't that automatically be handling the nonce selection for us?

Yes it definitely should. Just removed it.

I left it in to avoid changing lines of code that I didn't have to, but I think removing the manual nonce setting is a good improvement.

packages/batch-submitter/src/transaction-chain-contract.ts

snario · 2021-06-07T14:36:55Z

packages/batch-submitter/src/transaction-chain-contract.ts

-    data: '0x' + methodId + calldata,
+    data: getEncodedCalldata(batch),
    ...options,
  })


Is this method now unused?

Oh wow yes it is! I'm thinking it could make sense to leave it in in case we ever tried to call appendSequencerBatch and it fails because it doesn't accept any parameters. But you can let me know if you want me to just delete it -- I'm happy to

packages/batch-submitter/src/batch-submitter/state-batch-submitter.ts

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts

tynes · 2021-06-08T15:45:49Z

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts

      l1tipHeight,
    })

    const nonce = await this.signer.getTransactionCount()


Getting the nonce both outside and inside of customPopulateTransaction.appendSequencerBatch seems like its prone to a race condition. Note that I do think its very unlikely in practice, but the function does take an optional second argument which may be able to take the nonce.

Yes agreed, I just deleted the nonce logic

tynes · 2021-06-08T15:46:21Z

packages/batch-submitter/src/transaction-chain-contract.ts

+  public customPopulateTransaction = {
+    appendSequencerBatch: async (
+      batch: AppendSequencerBatchParams,
+      options?: TransactionRequest


The options aren't really used at all here?

Ah good catch, removing the options.

tynes · 2021-06-08T15:46:42Z

packages/batch-submitter/src/transaction-chain-contract.ts

+        from: await this.signer.getAddress(),
+        data,
+      })
+      const value = 0


value looks unused?

Yeah I just set it to zero explicitly. Probably could leave it out entirely but was just making zero value explicit. Let me know if you want me to remove that line

Its not returned?

Ah you're right -- deleted it

packages/batch-submitter/src/transaction-chain-contract.ts

snario · 2021-06-14T23:00:58Z

I'd just re-assert that we should add a test case before merging.

tynes · 2021-06-23T19:50:59Z

How important is this? Its been open for awhile and is almost there

snario · 2021-06-23T20:35:56Z

It has caused extended issues on mainnet production in the past. It is a priority

karlfloersch · 2021-06-28T17:33:35Z

@snario how would you suggest testing this? The options I see are as follows:

Refactor _submitAndLogTx to be a library which handles the contractFunction logic, then unit test that functionality independently.
Add an integration test for the tx resubmission

This would be a pretty meaty project so I don't think that this is too feasible. It would require excessive mocking & we would be much better served doing a larger scale refactor that allows us to 1) easily mock, 2) easily unit test, 3) compose unit tested functions together.

This is another example of where the batch submitter's structure is extremely difficult to test. Does option (1) suffice you think? If so I'll go ahead and add it -- should be a pretty quick refactor

snario · 2021-06-28T20:08:57Z

This is another example of where the batch submitter's structure is extremely difficult to test. Does option (1) suffice you think? If so I'll go ahead and add it -- should be a pretty quick refactor

👍

tynes · 2021-06-29T22:32:15Z

This is going to need to have its commits cleaned up now that we do a rebase/merge workflow

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts

karlfloersch · 2021-07-07T22:05:10Z

@snario / @tynes would love re-review on this!

snario · 2021-07-08T14:00:22Z

packages/batch-submitter/src/batch-submitter/state-batch-submitter.ts

      [batch, startBlock]
    )
-    const batchSizeInBytes = remove0x(tx).length / 2
+    const batchSizeInBytes = remove0x(calldata).length / 2


I think there is utility in ethers called hexDataLength we could use here.

Ideally everything was Buffers instead of strings or even uint8 arrays so that we didn't have to deal with constant prefix slicing

snario · 2021-07-08T14:17:23Z

packages/batch-submitter/src/transaction-chain-contract.ts

+const APPEND_SEQUENCER_BATCH_METHOD_ID = keccak256(
+  Buffer.from('appendSequencerBatch()')
+).slice(2, 10)


Also can do

const { Interface, Fragment } = ethers.utils; Interface.getSighash(Fragment.from('function appendSequencerBatch()'));

snario · 2021-07-08T14:19:15Z

packages/batch-submitter/src/transaction-chain-contract.ts

+const getEncodedCalldata = (batch: AppendSequencerBatchParams): string => {
+  const methodId = APPEND_SEQUENCER_BATCH_METHOD_ID
+  const calldata = encodeAppendSequencerBatch(batch)
+  return '0x' + remove0x(methodId) + remove0x(calldata)


ethers.utils.hexConcat(methodId, calldata)

snario · 2021-07-08T14:20:36Z

packages/batch-submitter/test/batch-submitter/batch-submitter.spec.ts


        const receipt = await batchSubmitter.submitNextBatch()
-        expect(sequencer.getGasPrice).to.have.been.calledOnce
+        expect(sequencer.getGasPrice).to.have.been.calledTwice


Why does it call twice now?

snario · 2021-07-08T14:21:38Z

packages/batch-submitter/test/utils/tx-submission.spec.ts

+    expect(called.sendTransaction).to.be.true
+    expect(called.waitForTransaction).to.be.true
+    expect(called.beforeSendTransaction).to.be.true
+    expect(called.onTransactionResponse).to.be.true


👍

You could also do more checks here on the inputs; e.g., is the expected nonce present

snario · 2021-08-02T18:19:18Z

We need to merge this

tynes · 2021-08-02T20:28:22Z

We need to merge this

What can we do to help get this over the line?

style: address pr feedback

karlfloersch requested a review from annieke as a code owner June 6, 2021 01:50

github-actions bot added 2-reviewers A-op-batcher Area: op-batcher labels Jun 6, 2021

karlfloersch force-pushed the fix/broken-estimate-gas-when-pending-tx-in-mempool branch 2 times, most recently from 34cd37a to b979d38 Compare June 6, 2021 02:01

snario suggested changes Jun 7, 2021

View reviewed changes

tynes reviewed Jun 8, 2021

View reviewed changes

packages/batch-submitter/src/batch-submitter/state-batch-submitter.ts Outdated Show resolved Hide resolved

tynes reviewed Jun 8, 2021

View reviewed changes

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts Outdated Show resolved Hide resolved

tynes reviewed Jun 8, 2021

View reviewed changes

packages/batch-submitter/src/transaction-chain-contract.ts Show resolved Hide resolved

tynes reviewed Jun 14, 2021

View reviewed changes

packages/batch-submitter/src/transaction-chain-contract.ts Outdated Show resolved Hide resolved

annieke reviewed Jul 1, 2021

View reviewed changes

packages/batch-submitter/src/batch-submitter/tx-batch-submitter.ts Outdated Show resolved Hide resolved

karlfloersch force-pushed the fix/broken-estimate-gas-when-pending-tx-in-mempool branch from 3fc6772 to 41ea3ad Compare July 2, 2021 19:26

snario reviewed Jul 8, 2021

View reviewed changes

karlfloersch mentioned this pull request Jul 9, 2021

fix fixSkippedDeposits #1240

Closed

karlfloersch added 3 commits August 5, 2021 00:31

fix: estimate gas when pending tx in mempool

3b13297

style: address pr feedback

add: tx submitter library

7b43840

integrate new tx submission lib

e2985bf

karlfloersch force-pushed the fix/broken-estimate-gas-when-pending-tx-in-mempool branch from 41ea3ad to e2985bf Compare August 5, 2021 00:31

snario approved these changes Aug 5, 2021

View reviewed changes

karlfloersch merged commit a8c9cd6 into develop Aug 5, 2021

karlfloersch deleted the fix/broken-estimate-gas-when-pending-tx-in-mempool branch August 5, 2021 15:08

Uh oh!

fix: estimate gas when pending tx in mempool #1024

fix: estimate gas when pending tx in mempool #1024

Uh oh!

Conversation

karlfloersch commented Jun 6, 2021 • edited by snario Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jun 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codecov-commenter commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

snario left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

snario commented Jun 14, 2021

Uh oh!

tynes commented Jun 23, 2021

Uh oh!

snario commented Jun 23, 2021

Uh oh!

karlfloersch commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snario commented Jun 28, 2021

Uh oh!

tynes commented Jun 29, 2021

Uh oh!

Uh oh!

karlfloersch commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snario Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karlfloersch commented Jun 6, 2021 •

edited by snario

Loading

changeset-bot bot commented Jun 6, 2021 •

edited

Loading

codecov-commenter commented Jun 7, 2021 •

edited

Loading

karlfloersch commented Jun 28, 2021 •

edited

Loading

snario Jul 8, 2021 •

edited

Loading