op-node: Gossip Before Import

[axelKingsley]

What

Adds "Async Gossiper" component to the derivation pipeline, and wires it into the state loop and related interfaces.

Why

There is interest in gossiping earlier in the derivation pipeline. Currently, the flow is:

• Engine Controller receives a block
• Engine Controller reinserts that block
• State Loop Continues
• Block is gossiped

We would like to insert the gossiping of the block between the receiving and reinserting of the block to avoid waiting any longer than needed before gossiping.

How

A new component has been written and added to the Driver. This component, the AsyncGossiper, is an asynchronous worker with a synchronized interface for Gossiping. The Driver passes this component down through the layers, and is used during the internal confirmPayload work.

It also stores and can return the most recently gossiped payload. This feature allows for the derivation pipeline to reuse a payload which was created during confirmPayload, if the insertion failed. This is done because we want to avoid regenerating a new block at the same height if we've already begun gossiping this one. As the component is passed down through the layers, it is used by RunNextSequencerAction to decide if there's already a usable block, and by confirmPayload to potentially get a cached block instead of asking the engine to rebuild one.

When the confirmPayload function is able to reinsert the payload, it calls Clear on this component, which wipes out the stored payload. This is because the AsyncGossiper only needs to know about the payload from the time it exists, to the time it has been inserted, and any re-attempts that may live in between those two points.

Testing

• unit test was added demonstrating the basic functionality of the component
• unit test was added demonstrating that repeated calls to the component work
• unit test was added demonstrating that if the publishing fails, the payload is not saved

Notes, Nuance, Concerns

• Adding this component created a lot of interface changes. That may indicate that it's integrated suboptimally, but because it's used in multiple locations in the pipeline, having it owned by the Driver and passed down made the most sense to me.
• Code paths which don't need or use an AsyncGossiper supply nil as needed. There's a nil check prior to use which should guard against NPEs
• The component lives in its own package rollout/async due to import cycle issues.
• This PR may need to rebase or refactor onto changes being actively made by @trianglesphere and changes related to the development of EIP4844

[coderabbitai]

Walkthrough

Walkthrough

The overall change introduces an asynchronous gossiping mechanism into a blockchain node's operation. The AsyncGossiper interface and its implementation are integrated into various components of the system, affecting methods related to payload confirmation and block building. This aims to enhance the network's efficiency by allowing execution payloads to be gossiped asynchronously, thus decoupling the gossiping process from the main execution flow.

Changes

File Path	Change Summary
.../l2_sequencer.go	Added async.NoOpGossiper{} as an argument to CompleteBuildingBlock in L2Sequencer.
.../async/asyncgossiper.go	Introduced AsyncGossiper interface and SimpleAsyncGossiper struct for asynchronous payload handling.
.../async/asyncgossiper_test.go	Added test cases for AsyncGossiper to verify asynchronous functionality and network resilience.
.../derive/engine_controller.go, .../derive/engine_queue.go, .../derive/engine_update.go, .../driver/metered_engine.go, .../driver/sequencer.go, .../driver/sequencer_test.go	Updated ConfirmPayload method and related functions to include async.AsyncGossiper parameter for async gossiping.
.../rollup/driver/driver.go	Imported async package, modified SequencerIface interface, and instantiated asyncGossiper in NewDriver.
.../rollup/driver/state.go	Added asyncGossiper to Driver struct and updated Start and Close methods to manage the gossiping loop.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (db28a83) 28.61% compared to head (3bb2202) 0.69%.
Report is 4 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #8905       +/-   ##
===========================================
- Coverage    28.61%   0.69%   -27.93%     
===========================================
  Files          166      87       -79     
  Lines         7216    2155     -5061     
  Branches      1235     500      -735     
===========================================
- Hits          2065      15     -2050     
+ Misses        5030    2140     -2890     
+ Partials       121       0      -121     

Flag	Coverage Δ
cannon-go-tests	?
chain-mon-tests	?
common-ts-tests	?
contracts-bedrock-tests	0.69% <ø> (ø)
contracts-ts-tests	?
core-utils-tests	?
sdk-next-tests	?
sdk-tests	?

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

see 79 files with indirect coverage changes

[trianglesphere]

Really nice changes. I don't think this will interfere much with the engine queue / controller changes. Maybe after stuff has settled down we'll want to change the Start/Confirm payload APIs.

[semgrep-app]

Semgrep found 10 sol-style-return-arg-fmt findings:

• packages/contracts-bedrock/scripts/libraries/LibSort.sol: L506, L387, L370, L353, L348, L331, L314, L279, L273, L267

Named return arguments to functions must be appended with an underscore (_)

_{Ignore this finding from sol-style-return-arg-fmt.}

Semgrep found 39 sol-style-notice-over-dev-natspec findings:

• packages/contracts-bedrock/scripts/libraries/LibSort.sol: L622, L581, L535, L497, L484, L476, L468, L457, L449, L439, L433, L427, L421, L415, L409, L403, L397, L391, L386, L369, L352, L347, L330, L313, L308, L303, L283, L277, L271, L265, L260, L255, L233, L221, L214, L64, L51, L44, L16

Prefer @notice over @dev in natspec comments

_{Ignore this finding from sol-style-notice-over-dev-natspec.}

[ajsutton]

I think we're close - only real issue I think is making sure we clear invalid payloads in all cases.

I must admit I'm not entirely confident in my knowledge of all the logic flows in this area so would really like @trianglesphere to review before approval given he's been working in this area more recently. And to be clear, if @trianglesphere is happy to merge I'm ok with trusting him on that - no need to wait for my approval as well.

[trianglesphere]

And we have the merge conflict. I think the clear logic is correct

[trianglesphere]

LGTM