txmgr: Restructure internals and add TxNotInMempoolTimeout

[trianglesphere]

Description

This restructures a lot of the internal of transaction sending. It adds a new flag and features where it aborts sending the transaction if the transaction is never seen on the network after a default of 2 minutes (configurable via txmgr flag).

The transaction manager's sendState has been modified to track successful publishes and the time of its creation. If it goes for TxNotInMempoolTimeout time without seeing a successful transaction publish (i.e. it believes the tx is not in the mempool) then it will abort.

The goal of this change is to ensure that the transaction manager does not get stuck in state where the transaction will not get confirmed while not accidentally recording transactions as failed when they in fact succeeded.

TODOs

• Author or reviewer has added an entry to the current release notes draft, if appropriate.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9afb543

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	9afb543
🔍 Latest deploy log	https://app.netlify.com/sites/opstack-docs/deploys/6427258303251700085e46ed

[semgrep-app]

Semgrep found 1 unchecked-type-assertion finding:

• op-bindings/bindings/systemconfig.go: L380

Unchecked type assertion.

_{Created by unchecked-type-assertion.}

[codecov]

Codecov Report

Merging #5286 (9afb543) into develop (7354398) will decrease coverage by 3.79%.
The diff coverage is 68.18%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5286      +/-   ##
===========================================
- Coverage    39.93%   36.15%   -3.79%     
===========================================
  Files          382      227     -155     
  Lines        24376    19883    -4493     
  Branches       838        0     -838     
===========================================
- Hits          9734     7188    -2546     
+ Misses       13911    12000    -1911     
+ Partials       731      695      -36     

Flag	Coverage Δ
bedrock-go-tests	36.15% <68.18%> (-0.06%)	⬇️
common-ts-tests	?
contracts-bedrock-tests	?
contracts-tests	?
core-utils-tests	?
dtl-tests	?
fault-detector-tests	?
sdk-tests	?

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

Impacted Files	Coverage Δ
op-service/txmgr/cli.go	43.05% <33.33%> (-0.03%)	⬇️
op-service/txmgr/txmgr.go	78.94% <68.90%> (-3.13%)	⬇️
op-service/txmgr/send_state.go	96.66% <100.00%> (+0.30%)	⬆️

... and 156 files with indirect coverage changes

[ajsutton]

Generally looks good - left a few clarifying comments. Main concern for me is having to sleep in tests - that's a very common source of intermittency and leads to slower tests.

[sebastianst]

I'm wondering, do we even need the async tx sending in send with sendTxAsync via a goroutine and then waiting for the receipt on a channel?

It looks to me like the whole transaction sending loop could also just be a normal synchronous call to publishAndWaitForTx, which would simply return *types.Receipt, error. But maybe I'm just oversimplifying the algorithm here.

[trianglesphere]

I'm wondering, do we even need the async tx sending in send with sendTxAsync via a goroutine and then waiting for the receipt on a channel?

It looks to me like the whole transaction sending loop could also just be a normal synchronous call to publishAndWaitForTx, which would simply return *types.Receipt, error. But maybe I'm just oversimplifying the algorithm here.

It's because when we update the gas price, we are waiting for multiple different transaction hashes. Say a tx gets included in L1, but we then published another transaction with a higher gas price before we realize. If we only waited for the second TX we would miss that the first TX we sent got included on L1.

We could make it synchronous by polling every transaction that has been sent

[ajsutton]

Actually I'm going to dismiss my review and just leave approval to Seb since he has better context. I really just meant to remove my request changes.

I'm happy with it, but leaving final approval to Seb.

[sebastianst]

I'm wondering, do we even need the async tx sending in send with sendTxAsync via a goroutine and then waiting for the receipt on a channel?
It looks to me like the whole transaction sending loop could also just be a normal synchronous call to publishAndWaitForTx, which would simply return *types.Receipt, error. But maybe I'm just oversimplifying the algorithm here.

It's because when we update the gas price, we are waiting for multiple different transaction hashes. Say a tx gets included in L1, but we then published another transaction with a higher gas price before we realize. If we only waited for the second TX we would miss that the first TX we sent got included on L1.

We could make it synchronous by polling every transaction that has been sent

Ah thanks, that makes sense. If we're running into problems with the current async architecture, I'd vote for this refactor. But fine for now since it works.

[sebastianst]

Looks great, just two open questions regarding error strings checking

[trianglesphere]

#5286 (comment)
Can't comment on that for some reason. @sebastianst

ethereum.NotFound is correctly returned by the client.

[mergify]

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

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.

[trianglesphere]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed

[mergify]

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