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

have batcher submit appropriate cancellation transactions when mempool is blocked #10941

Conversation

roberto-bayardo
Copy link
Collaborator

@roberto-bayardo roberto-bayardo commented Jun 18, 2024

Description

Extend batcher to submit appropriate cancellation transactions when the mempool is blocked by a transaction of incompatible type.

Also makes the txmgr use the geth-defined AlreadyReservedErr instead of redefining it, now that op-geth has caught up to the commit that exposes it.

Tests

Extended the E2E tests to submit incompatible pending transactions to the txpool to confirm the new functionality appropriately clears them.

Metadata

  • Fixes #[ethereum-optimism/protocol-quest/issues/102]

Copy link
Contributor

@axelKingsley axelKingsley left a comment

Choose a reason for hiding this comment

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

This solution manages an enumerated State of the Transaction Pool, specifically whether it is unblocked, blocked, or pending(cancel). It does this by detecting a specific error type which sets the state to Blocked. When blocked it sends and empty batch with a special ID, and progresses to Pending. When in Pending, it optimistically advances back to Unblocked.

I left some comments to better understand the behavior and suggest some potential simplifications. This solution seems reasonable given the scope of changes, though it'd be nice to have a more comprehensive handling as part of the architecture at some point down the road.

op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-service/txmgr/send_state.go Show resolved Hide resolved
op-batcher/batcher/driver.go Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jul 6, 2024

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jul 6, 2024
Copy link

codecov bot commented Jul 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.58%. Comparing base (f5221f4) to head (dcfc6c3).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #10941   +/-   ##
========================================
  Coverage    62.58%   62.58%           
========================================
  Files           21       21           
  Lines         1935     1935           
  Branches        71       71           
========================================
  Hits          1211     1211           
  Misses         687      687           
  Partials        37       37           
Flag Coverage Δ
cannon-go-tests 80.21% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
sdk-tests 16.44% <ø> (ø)

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

@roberto-bayardo roberto-bayardo force-pushed the clear-incompatible-tx-from-mempool2 branch from 8f75ba1 to c0baefd Compare July 9, 2024 15:51
@roberto-bayardo
Copy link
Collaborator Author

I have rebased the PR to clear the conflicts.

op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-service/txmgr/txmgr.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
@roberto-bayardo roberto-bayardo force-pushed the clear-incompatible-tx-from-mempool2 branch 9 times, most recently from a4c1835 to 5e2918c Compare July 15, 2024 00:17
@roberto-bayardo roberto-bayardo force-pushed the clear-incompatible-tx-from-mempool2 branch 2 times, most recently from 05e797b to e40069f Compare July 17, 2024 22:29
@roberto-bayardo roberto-bayardo force-pushed the clear-incompatible-tx-from-mempool2 branch from e40069f to 4a7e7cf Compare July 17, 2024 23:02
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.

Looks good! Some minor improvement suggestions.

op-batcher/batcher/driver.go Outdated Show resolved Hide resolved
op-batcher/batcher/service.go Outdated Show resolved Hide resolved
op-e2e/eip4844_test.go Outdated Show resolved Hide resolved
op-e2e/eip4844_test.go Outdated Show resolved Hide resolved
op-service/txmgr/txmgr.go Outdated Show resolved Hide resolved
@roberto-bayardo roberto-bayardo force-pushed the clear-incompatible-tx-from-mempool2 branch 3 times, most recently from 4b00a48 to 2ab3e00 Compare July 18, 2024 16:15
@roberto-bayardo roberto-bayardo force-pushed the clear-incompatible-tx-from-mempool2 branch from 2ab3e00 to dcfc6c3 Compare July 18, 2024 16:40
@sebastianst sebastianst added this pull request to the merge queue Jul 18, 2024
Merged via the queue into ethereum-optimism:develop with commit b7f8188 Jul 18, 2024
60 checks passed
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.

3 participants