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: introduce PREFER_LOCAL_SAFE_L2 config var #14587

Merged
merged 4 commits into from
Mar 3, 2025

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Feb 28, 2025

Workaround for #14576 .

This flag is turned off by default, restoring the batcher behaviour we had before #14265 but enabling the newer behaviour through configuration if desired.

@geoknee geoknee requested review from a team as code owners February 28, 2025 16:48
@geoknee geoknee requested review from mds1 and protolambda February 28, 2025 16:48
@geoknee geoknee changed the title op-batcher: introduce PREFER_LOCAL_SAFE_L2 config var op-batcher: introduce PREFER_LOCAL_SAFE_L2 config var Feb 28, 2025
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 42.11%. Comparing base (6aa2ca5) to head (be5d71c).
Report is 7 commits behind head on develop.

Files with missing lines Patch % Lines
op-batcher/batcher/service.go 0.00% 2 Missing ⚠️
op-batcher/batcher/config.go 0.00% 1 Missing ⚠️
op-batcher/batcher/driver.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #14587      +/-   ##
===========================================
- Coverage    46.65%   42.11%   -4.55%     
===========================================
  Files         1037      866     -171     
  Lines        89068    79135    -9933     
===========================================
- Hits         41554    33324    -8230     
+ Misses       44428    42897    -1531     
+ Partials      3086     2914     -172     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

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

Files with missing lines Coverage Δ
op-batcher/batcher/sync_actions.go 100.00% <100.00%> (ø)
op-batcher/flags/flags.go 63.33% <ø> (ø)
op-batcher/batcher/config.go 35.95% <0.00%> (-0.41%) ⬇️
op-batcher/batcher/driver.go 4.28% <0.00%> (ø)
op-batcher/batcher/service.go 0.00% <0.00%> (ø)

... and 178 files with indirect coverage changes

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, just minor test improvement suggestions.

Co-authored-by: Sebastian Stammler <seb@oplabs.co>
@geoknee geoknee enabled auto-merge March 3, 2025 13:18
@geoknee geoknee added this pull request to the merge queue Mar 3, 2025
Merged via the queue into develop with commit c46bb16 Mar 3, 2025
46 checks passed
@geoknee geoknee deleted the gk/batcher-interop-flag branch March 3, 2025 13:51
karlb pushed a commit to celo-org/optimism that referenced this pull request Mar 4, 2025
…imism#14587)

* op-batcher: introduce PREFER_LOCAL_SAFE_L2 config var

* lint

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* lint

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>
karlb pushed a commit to celo-org/optimism that referenced this pull request Mar 5, 2025
…imism#14587)

* op-batcher: introduce PREFER_LOCAL_SAFE_L2 config var

* lint

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* lint

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>
palango pushed a commit to celo-org/optimism that referenced this pull request Mar 6, 2025
* op-batcher: prevent `SpanChannelOut` RLP bytes overflowing `MaxRLPBytesPerChannel` (ethereum-optimism#14310)

* fix op-batcher pack over MaxRLPBytesChannel

* add test cases from different CompressionAlgo

* add fresh compression logic and corresponding comments

* refactor: enhance MaxRLPBytesPerChannel test

* refactor: rename variable and add required messages

* op-batcher: use local-safe to reduce lag during interop sync (ethereum-optimism#14265)

* op-batcher: remove `ThrottleInterval` and split block loading and batch publishing into separate goroutines (ethereum-optimism#14244)

* op-batcher: overhaul throttling, reading and writing loops

* no longer evaluate throttling conditions on a ticker
* break main loop into "reading" and "writing"
* reading loop signals to throttling and writing when ALL blocks are loaded (this could be done in a more fine grained way, even when each block is loaded)
* these run concurrently

* improvements

* readloop unblocks writing loop once, and then writing loop goes forever (pausing when it runs out of data)

tests pass but timeout due to bad shutdown

* rename to prevent shadowing

* allow for receipt handling when receipt and err are both nil

* split shutdownCtx into producers and consumers

* throttling loop ranges over a channel, does not take a context

* processReceiptsLoop ranges over a channel, does not take a context

* unify wait groups

* unify contexts

* writeLoop uses a ticker instead of a sleep

* WIP: add throughput test for batcher

* make txQueue local, not global state

* reduce diff

* reduce diff further

* make pendingBytesUpdated a local, not global

* read and write loop communicate with a channel

no more ticker required

* rename

* rename

* abstract promptLoop

* rename

* update readme

* add diagram to readme

* remove test

I'm not sure it is adding any value. May return to it in future

* sendToThrottlingLoop uses mutex

* harmonize "*Loop returning" logs

* tidy

* remove TODO

* rip out ThrottleInterval config var

* add activeSequencerChanged channel and wire it up to onActiveSequencerChanged hook in active rollup provider

* set callback at runtime, not in constructor

* reinstate startup order

* tidy

* remove dead code

* remove unintentional change

* rename readLoop to blockLoadingLoop and writeLoop to publishingLoop

* improve diagram

* replace ThrottleInterval with ThrottleThreshold as enabling var

* remove onActiveProviderChanged arg from constructors

* protect callback invocation with nullity check

* lint

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* remove ThrottelInterval from flags

* docs: mention active sequencer signal

* only attach callback if throttling is enabled

* increase buffer of activeSequencerChanged

means that we buffer a single event if the throttlingLoop is busy, even when using a try-send

* add buffer to pendingBytesUpdated

see previous commit

* reduce indentation

* remove dangling ref

* pass killCtx to publishingLoop and avoid accessing this context globally

* remove continue and always signal publishing loop

* remove unecessary mutex wrangle

* replace signalPublishingLoop method with trySignal fn

* extend TestRollupProvider_FailoverOnInactiveSequencer to cover callback fn

* use retryTimer in throttlingLoop

if RPC calls fail, they will be retried after retryInterval (or before, if another event triggers updateParams)

* op-batcher: improve active-seq-changed signalling setup

* remove unused state var

* rename blocksLoaded channel to publishSignal channel

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-batcher: add `TestBatchSubmitter_sendTx_FloorDataGas` and patch `driver.sendTx` (ethereum-optimism#14517)

* op-batcher: add TestBatchSubmitter_sendTx_FloorDataGas

* op-batcher: use floorDataGas for transactions if greater than intrinsicGas

* Update op-batcher/batcher/driver.go

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* log error instead of ignoring

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-batcher: always `updateCursorAndMetrics` when returning from `processBlocks()` (ethereum-optimism#14520)

* op-batcher: always updateCursorAndMetrics when returning from processBlocks()

* Update op-batcher/batcher/channel_manager.go

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-batcher: remove `ChannelManager.CheckExpectedProgress()` and add channel timeout log (ethereum-optimism#14553)

* op-batcher: remove ChannelManager.CheckExpectedProgress

* op-batcher: add warning log when a channel times out on chain

* op-batcher: correctly track block metrics in `handleChannelInvalidated()` (ethereum-optimism#14561)

* op-batcher: correctly track block metrics in handleChannelInvalidated

Includes test which fails without the fix.

* op-batcher: log out channels which are dropped during handleChannelInvalidated()

* change to warn log for dropped channels

* op-batcher: improve `computeSyncActions()` logging (ethereum-optimism#14563)

* improve computeSyncActions logging

* fixup test and make sure all cases run (!)

* use more friendly format for structured logger

* op-batcher: introduce `PREFER_LOCAL_SAFE_L2` config var (ethereum-optimism#14587)

* op-batcher: introduce PREFER_LOCAL_SAFE_L2 config var

* lint

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* lint

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* batcher: Wait for DA write before shutdown

---------

Co-authored-by: olga yang <ya1994ng@gmail.com>
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: George Knee <georgeknee@googlemail.com>
Co-authored-by: Sebastian Stammler <seb@oplabs.co>
QuentinI pushed a commit to EspressoSystems/optimism-espresso-integration that referenced this pull request Mar 7, 2025
* op-batcher: prevent `SpanChannelOut` RLP bytes overflowing `MaxRLPBytesPerChannel` (ethereum-optimism#14310)

* fix op-batcher pack over MaxRLPBytesChannel

* add test cases from different CompressionAlgo

* add fresh compression logic and corresponding comments

* refactor: enhance MaxRLPBytesPerChannel test

* refactor: rename variable and add required messages

* op-batcher: use local-safe to reduce lag during interop sync (ethereum-optimism#14265)

* op-batcher: remove `ThrottleInterval` and split block loading and batch publishing into separate goroutines (ethereum-optimism#14244)

* op-batcher: overhaul throttling, reading and writing loops

* no longer evaluate throttling conditions on a ticker
* break main loop into "reading" and "writing"
* reading loop signals to throttling and writing when ALL blocks are loaded (this could be done in a more fine grained way, even when each block is loaded)
* these run concurrently

* improvements

* readloop unblocks writing loop once, and then writing loop goes forever (pausing when it runs out of data)

tests pass but timeout due to bad shutdown

* rename to prevent shadowing

* allow for receipt handling when receipt and err are both nil

* split shutdownCtx into producers and consumers

* throttling loop ranges over a channel, does not take a context

* processReceiptsLoop ranges over a channel, does not take a context

* unify wait groups

* unify contexts

* writeLoop uses a ticker instead of a sleep

* WIP: add throughput test for batcher

* make txQueue local, not global state

* reduce diff

* reduce diff further

* make pendingBytesUpdated a local, not global

* read and write loop communicate with a channel

no more ticker required

* rename

* rename

* abstract promptLoop

* rename

* update readme

* add diagram to readme

* remove test

I'm not sure it is adding any value. May return to it in future

* sendToThrottlingLoop uses mutex

* harmonize "*Loop returning" logs

* tidy

* remove TODO

* rip out ThrottleInterval config var

* add activeSequencerChanged channel and wire it up to onActiveSequencerChanged hook in active rollup provider

* set callback at runtime, not in constructor

* reinstate startup order

* tidy

* remove dead code

* remove unintentional change

* rename readLoop to blockLoadingLoop and writeLoop to publishingLoop

* improve diagram

* replace ThrottleInterval with ThrottleThreshold as enabling var

* remove onActiveProviderChanged arg from constructors

* protect callback invocation with nullity check

* lint

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* remove ThrottelInterval from flags

* docs: mention active sequencer signal

* only attach callback if throttling is enabled

* increase buffer of activeSequencerChanged

means that we buffer a single event if the throttlingLoop is busy, even when using a try-send

* add buffer to pendingBytesUpdated

see previous commit

* reduce indentation

* remove dangling ref

* pass killCtx to publishingLoop and avoid accessing this context globally

* remove continue and always signal publishing loop

* remove unecessary mutex wrangle

* replace signalPublishingLoop method with trySignal fn

* extend TestRollupProvider_FailoverOnInactiveSequencer to cover callback fn

* use retryTimer in throttlingLoop

if RPC calls fail, they will be retried after retryInterval (or before, if another event triggers updateParams)

* op-batcher: improve active-seq-changed signalling setup

* remove unused state var

* rename blocksLoaded channel to publishSignal channel

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-batcher: add `TestBatchSubmitter_sendTx_FloorDataGas` and patch `driver.sendTx` (ethereum-optimism#14517)

* op-batcher: add TestBatchSubmitter_sendTx_FloorDataGas

* op-batcher: use floorDataGas for transactions if greater than intrinsicGas

* Update op-batcher/batcher/driver.go

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* log error instead of ignoring

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-batcher: always `updateCursorAndMetrics` when returning from `processBlocks()` (ethereum-optimism#14520)

* op-batcher: always updateCursorAndMetrics when returning from processBlocks()

* Update op-batcher/batcher/channel_manager.go

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* op-batcher: remove `ChannelManager.CheckExpectedProgress()` and add channel timeout log (ethereum-optimism#14553)

* op-batcher: remove ChannelManager.CheckExpectedProgress

* op-batcher: add warning log when a channel times out on chain

* op-batcher: correctly track block metrics in `handleChannelInvalidated()` (ethereum-optimism#14561)

* op-batcher: correctly track block metrics in handleChannelInvalidated

Includes test which fails without the fix.

* op-batcher: log out channels which are dropped during handleChannelInvalidated()

* change to warn log for dropped channels

* op-batcher: improve `computeSyncActions()` logging (ethereum-optimism#14563)

* improve computeSyncActions logging

* fixup test and make sure all cases run (!)

* use more friendly format for structured logger

* op-batcher: introduce `PREFER_LOCAL_SAFE_L2` config var (ethereum-optimism#14587)

* op-batcher: introduce PREFER_LOCAL_SAFE_L2 config var

* lint

* Apply suggestions from code review

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* lint

---------

Co-authored-by: Sebastian Stammler <seb@oplabs.co>

* batcher: Wait for DA write before shutdown

---------

Co-authored-by: olga yang <ya1994ng@gmail.com>
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: George Knee <georgeknee@googlemail.com>
Co-authored-by: Sebastian Stammler <seb@oplabs.co>
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.

2 participants