require fee increase & gas re-estimation for retried transactions

[roberto-bayardo]

Description

Currently the txmgr retry logic will not bump a transaction's tip and max-fee in the case the latest suggested values are lower than the original values. When a transaction is stuck in the mempool (e.g. due to too low fees or bad gaslimit estimate) this regularly results in retry failures with error message "resubmitted already known transaction" or "replacement transaction underpriced".

This PR imposes a mandatory fee increase with each retry (up to a point) to ensure a retried tx isn't immediately rejected in this manner.

We also see txs get stuck in the mempool because of extremely high gaslimit estimation (e.g. gaslimit nearly equals block gas limit). In these cases, increasing fees alone does not solve the problem. To fix this scenario, this change makes the txmgr re-estimate gas with each retry. We have also created a geth PR to fix what we suspect is causing the high gaslimit estimates. (Fee re-estimation also seems to be helpful in detecting if resubmitting the tx would just fail anyway due to a state change.)

Tests

Updated unit tests to capture the new behavior.

Additional context

The issues addressed here have caused the Base goerli proposer to get stuck multiple times. Historically we have resorted to unjamming the proposer by manually submitting a transaction with a higher fee and lower gaslimit to replace the stuck transaction.

This issue does not happen with the batcher because batcher txs are simple sends, and we believe the underlying cause of the bad gas usage estimation only arises for reverting contract txs. Proposer txs revert quite frequently with the error message "block hash does not match the hash at the expected height".

[changeset-bot]

⚠️ No Changeset found

Latest commit: e1810d8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[netlify]

✅ Deploy Preview for opstack-docs canceled.

Name	Link
🔨 Latest commit	e1810d8
🔍 Latest deploy log	https://app.netlify.com/sites/opstack-docs/deploys/64936a84b868960008211f4b

[tynes]

Generally looks good to me so far but tagging @trianglesphere who wrote this code

[sebastianst]

decreasing bump to 10% from 15% also makes sense to me 👍🏻

[roberto-bayardo]

I think this PR is ready to go. Running since yesterday proposing to goerli and in the last 12 hours we see this from the proposer transactions:

• 0 tx underpriced / already known errors
• 1 case of super-high gas limit corrected by gas-re-estimation
• 0-4 cases of retrying a tx with higher fees every hour. Almost all txs that were retried were retried only once. One tx was retried 3 times but fee suggestions were extremely low at the time. The 3rd retry cleared with feecap of 600 wei and a tip of 400 wei (note that's wei not gwei!)
• 4 cases of tx retry resulted in "gas estimation failure", causing the retry to abort and allowing the original tx to confirm successfully. The remaining ones resulted in successful tx replacement.
• other than the tx re-estimation related aborts above, only other reason for a tx failing without receipt was because it was sucessfully replaced

Let me know if there's anything else I can do?

[sebastianst]

LGTM thanks!

I'd also be fine with just adjusting the tests to the 10% price bump, instead of doing

func init() {
	// fix price bump here (15%) so tests won't break if the default changes
	priceBumpPercent = big.NewInt(100 + 15)
}

[roberto-bayardo]

LGTM thanks!

I'd also be fine with just adjusting the tests to the 10% price bump, instead of doing

Tests adjusted for a 10% price bump, and added a require guard on priceBump for tests that rely on it so it's easy to know why things break if priceBump does get modified.

[trianglesphere]

lgtm. Thanks for these changes

[mergify]

This PR has been added to the merge queue, and will be merged soon.

[mergify]

Hey @roberto-bayardo, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with @mergifyio requeue.
More details can be found on the Queue: Embarked in merge train check-run.

[roberto-bayardo]

@Mergifyio requeue

[mergify]

requeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

• sender-permission>=write