Skip to content

batch-submitter: ENG-1688 fix bug causing Kovan to fall behind#1799

Merged
tynes merged 1 commit intoethereum-optimism:developfrom
mslipper:bugfix/ctc-kovan-behind
Nov 23, 2021
Merged

batch-submitter: ENG-1688 fix bug causing Kovan to fall behind#1799
tynes merged 1 commit intoethereum-optimism:developfrom
mslipper:bugfix/ctc-kovan-behind

Conversation

@mslipper
Copy link
Copy Markdown
Contributor

@mslipper mslipper commented Nov 22, 2021

YNATM was retrying transactions with bad nonces for up to an hour on Kovan. This PR causes YNATM to immediately reject when bad nonce errors are encountered.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 22, 2021

🦋 Changeset detected

Latest commit: 85f68bd

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
Copy link
Copy Markdown

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #1799 (85f68bd) into develop (d59341a) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1799      +/-   ##
===========================================
+ Coverage    71.81%   71.90%   +0.08%     
===========================================
  Files           69       69              
  Lines         2303     2310       +7     
  Branches       344      345       +1     
===========================================
+ Hits          1654     1661       +7     
  Misses         649      649              
Flag Coverage Δ
batch-submitter 62.07% <100.00%> (+0.50%) ⬆️
contracts 87.96% <ø> (ø)
core-utils 56.53% <ø> (ø)
data-transport-layer 38.23% <ø> (ø)
message-relayer 70.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ackages/batch-submitter/src/utils/tx-submission.ts 90.90% <100.00%> (+2.44%) ⬆️
...ontracts/contracts/L1/verification/BondManager.sol 100.00% <0.00%> (ø)
...tracts/contracts/L1/messaging/L1StandardBridge.sol 97.05% <0.00%> (ø)
...racts/contracts/L1/rollup/StateCommitmentChain.sol 87.50% <0.00%> (ø)
...acts/contracts/L1/rollup/ChainStorageContainer.sol 70.00% <0.00%> (ø)
.../contracts/L1/messaging/L1CrossDomainMessenger.sol 96.77% <0.00%> (ø)
.../contracts/L1/rollup/CanonicalTransactionChain.sol 86.66% <0.00%> (ø)
.../contracts/libraries/bridge/CrossDomainEnabled.sol 100.00% <0.00%> (ø)

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 d59341a...85f68bd. Read the comment docs.

@smartcontracts
Copy link
Copy Markdown
Contributor

Left the same comment in our other thread but why did a nonce error trigger on the batch submitter?

maxGasPrice: ynatm.toGwei(config.maxGasPriceInGwei),
gasPriceScalingFunction: ynatm.LINEAR(config.gasRetryIncrement),
delay: config.resubmissionTimeout,
rejectImmediatelyOnCondition: ynatmRejectOn,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that the batch submitter will catch this exception and try again?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the test, that looks like it is the case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will abort the retry loop on either reverts or nonce errors so that we can try submitting the full batch again. This will update the nonce.

Copy link
Copy Markdown
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get this deployed on kovan asap

@mslipper
Copy link
Copy Markdown
Contributor Author

Left the same comment in our other thread but why did a nonce error trigger on the batch submitter?

I don't have the root cause of the nonce error. It may be related to the race condition I mentioned on Slack, but I'm not sure. I can dig deeper, but I think we should get this PR out first because it'll at the very least allow the batch submitter to keep submitting batches while we investigate.

@tynes tynes merged commit c1a8253 into ethereum-optimism:develop Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-op-batcher Area: op-batcher

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants