Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[op-batcher] Add support for multiple batcher transactions per L1 block #5398

Merged

Conversation

mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Apr 10, 2023

Description

Adds support for sending multiple transactions from the batcher at once. A new flag has been added: --max-pending-tx, which controls the number of pending transactions sent to the txmgr.

The txmgr was refactored to allow concurrent calls to Send. Some internal nonce tracking code was added that manually increments the nonce for each tx (after initially pulling it from an L1 node). If any of the transaction sends experiences an error, the entire set of pending txs is canceled, and the cached nonce is cleared.

Note: currently the batcher only supports a single pending channel, which means that only when channels have multiple frames will multiple txs be submitted at once. Multiple pending channels will come in a later PR.

Tests

Add tests for txmgr nonce management + reset, as well as tests for the new TxQueue.

Additional context

At times we are experiencing more L2 state than the batcher can keep up with, and our safe chain is falling behind our unsafe chain. This PR will allow a greater throughput of batcher transactions by allowing multiple txs per L1 block.

Here's an example block that has 10 transactions from our batcher (0x53394266fc80e5d4e6d25b3d0b7ca243859b7b09).

TODOs

@changeset-bot
Copy link

changeset-bot bot commented Apr 10, 2023

⚠️ No Changeset found

Latest commit: 26b46ac

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

netlify bot commented Apr 10, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 26b46ac
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/6448243bbce7eb0008a256dd

@mdehoog mdehoog force-pushed the michael/multi-tx-batcher branch 6 times, most recently from fc0c21c to 9c47f84 Compare April 10, 2023 05:26
@tynes
Copy link
Contributor

tynes commented Apr 10, 2023

I feel like the better way to implement this is in the TxManager, having it support concurrent transaction submission. Then any service that uses the TxManager has this ability. It also feels a bit weird to cap the amount for max transactions per block. Does that actually mean "max pending transactions"? I didn't go super in depth into the implementation here to be honest

@mdehoog
Copy link
Contributor Author

mdehoog commented Apr 10, 2023

@tynes totally agree 👍 will refactor

@tynes
Copy link
Contributor

tynes commented Apr 10, 2023

@tynes totally agree +1 will refactor

I think @trianglesphere has been thinking about this

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

I've been thinking about this as well & was going to start work on this as well this week. Some of the refactors we've made to the batcher make it slightly harder to parallelize, but it should still be relatively easy to do async tx sending & async tx confirmation. There might be some slight difficulty with shutdown, but only driver.go should have to be modified in the batcher.

op-batcher/batcher/config.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
@mdehoog mdehoog force-pushed the michael/multi-tx-batcher branch 9 times, most recently from e5df09e to bfbafde Compare April 11, 2023 19:01
@mergify
Copy link
Contributor

mergify bot commented Apr 11, 2023

Hey @mdehoog! This PR has merge conflicts. Please fix them before continuing review.

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

I like the approach. The new functions in driver.go are really nice.
More of an idle thought, I wonder what it’d look like to integrate the concurrency (+ limits into the txmgr or in a layer on top of the txmgr that’s not the in the batch submitter)

op-service/txmgr/txmgr.go Outdated Show resolved Hide resolved
op-batcher/batcher/job_runner.go Outdated Show resolved Hide resolved
op-batcher/batcher/job_runner.go Outdated Show resolved Hide resolved
op-batcher/batcher/job_runner.go Outdated Show resolved Hide resolved
@mergify mergify bot removed the conflict label Apr 11, 2023
@mdehoog mdehoog force-pushed the michael/multi-tx-batcher branch 2 times, most recently from 6cd6b3f to 499417b Compare April 11, 2023 22:16
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

Nice. I really like the Queue interface & how it's used. Inside the queue, I think that code is correct, but I don't think we need the full complexity of sync.Cond there.

op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-service/txmgr/queue.go Outdated Show resolved Hide resolved
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

LGTM now! Just a nit about a redundant check in tests.

op-service/txmgr/queue_test.go Outdated Show resolved Hide resolved
@trianglesphere trianglesphere merged commit af19fae into ethereum-optimism:develop Apr 25, 2023
71 of 81 checks passed
@mdehoog mdehoog deleted the michael/multi-tx-batcher branch April 26, 2023 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants