op-batcher: remove ThrottleInterval and split block loading and batch publishing into separate goroutines
#14244
+255
−142
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #14244 +/- ##
============================================
- Coverage 45.89% 0 -45.90%
============================================
Files 1008 0 -1008
Lines 86419 0 -86419
============================================
- Hits 39663 0 -39663
+ Misses 43758 0 -43758
+ Partials 2998 0 -2998
Flags with carried forward coverage won't be shown. Click here to find out more. |
geoknee
commented
Feb 11, 2025
* 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
…er (pausing when it runs out of data) tests pass but timeout due to bad shutdown
no more ticker required
28de46b to
5bb0f65
Compare
I'm not sure it is adding any value. May return to it in future
Co-authored-by: Sebastian Stammler <seb@oplabs.co>
means that we buffer a single event if the throttlingLoop is busy, even when using a try-send
see previous commit
if RPC calls fail, they will be retried after retryInterval (or before, if another event triggers updateParams)
sebastianst
reviewed
Feb 20, 2025
op-batcher: improve active-seq-changed signalling setup
sebastianst
approved these changes
Feb 21, 2025
Rjected
pushed a commit
to paradigmxyz/optimism
that referenced
this pull request
Feb 25, 2025
…ch 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>
karlb
pushed a commit
to celo-org/optimism
that referenced
this pull request
Mar 4, 2025
…ch 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>
karlb
pushed a commit
to celo-org/optimism
that referenced
this pull request
Mar 5, 2025
…ch 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>
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves the hybrid "event + interval" model for DA throttling to a purely event driven model. There are two events which can trigger throttling: (1) a change in the amount of pending data building up in the batcher and (2) a change in the active sequencer which the batcher is pointing at.
A happy side effect is that the batcher's loading, publishing and throttling workloads are broken up and now work fully asynchronously. Previously the batcher would load, and then publish for up to some maximum amount of time (blocking block loading and throttling), then return to loading again in order to trigger throttling.
for / selectloop withctx.Done()branch with a simplerrangeover a channel. This makes shutdown behaviour easier to reason about and makes deadlocks less likely.TODO