Skip to content

op-node: Switch L1 Retrieval to pull based#3584

Merged
mslipper merged 7 commits intojg/l1traversalfrom
jg/l1retrieval
Sep 28, 2022
Merged

op-node: Switch L1 Retrieval to pull based#3584
mslipper merged 7 commits intojg/l1traversalfrom
jg/l1retrieval

Conversation

@trianglesphere
Copy link
Contributor

Description

This makes the L1 Retrieval a purely pull based stage. This commit required large modifications to the channel bank stage in order for the channel bank to maintain its own progress.

Tests

L1 Retrieval unit tests pass. I skipped the channel bank unit tests & fixed them up in the next PR (would have been lots of extra work to fix them twice).

Metadata

  • Fixes ENG-2748

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2022

⚠️ No Changeset found

Latest commit: c77bf2b

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

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@trianglesphere
Copy link
Contributor Author

Tests are failing at the check changed script. I think it's because it's not pointing to develop as the base commit.

@trianglesphere trianglesphere marked this pull request as ready for review September 28, 2022 18:41
@mergify mergify bot mentioned this pull request Sep 28, 2022
This makes the L1 Retrieval a purely pull based stage. This commit
required large modifications to the channel bank stage in order
for the channel bank to maintain it's own progress.

// NewChannelBank creates a ChannelBank, which should be Reset(origin) before use.
func NewChannelBank(log log.Logger, cfg *rollup.Config, next ChannelBankOutput) *ChannelBank {
func NewChannelBank(log log.Logger, cfg *rollup.Config, next ChannelBankOutput, prev *L1Retrieval) *ChannelBank {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will focus on the retrieval code for this PR, the channel bank can change later. But would be nice to separate each stage by interface instead of concrete types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi the channel bank takes an interface rather than a concrete type in the next PR

trianglesphere and others added 6 commits September 28, 2022 15:23
This again requires a fair amount of changes to channel_in_reader.go
for the channel in reader to maintain its progress state.
Like the rest of the changes, this also required modifications to
the next stage - the batch queue in order for it to manage the
progress API. This commit required even more changes than usual.
I changed the pipeline to be reset to a common starting point
and now use the L2SafeHead block to filter out adding batches &
L1 blocks to the batch queue.
The attributes queue actually had pretty few modifications to work
with the progress API. The logic of switching the batch queue over
was a bit more complex because the batch queue is very stateful, but
still not the worst.
The progress API is very nearly removed from the engine queue stage.
op-node: Switch batch queue to be pull based
op-node: Switch channel bank and channel in reader to a pull based stage
@mslipper mslipper merged commit d954685 into jg/l1traversal Sep 28, 2022
@mslipper mslipper deleted the jg/l1retrieval branch September 28, 2022 22:55
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