Skip to content

feat: simplify ExecutionMode #198

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

varun-doshi
Copy link
Contributor

Fixes #196

Changes made:

  • Removed Fallback mode from ExecutionMode
  • DryRun mode will get both l2_payload and builder_payload, but will always send the l2_payload. This was different previously where in DryRun, the call to builder_payload would return an error.

Copy link

vercel bot commented Apr 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
rollup-boost ⬜️ Ignored (Inspect) Visit Preview May 7, 2025 6:28am

@varun-doshi varun-doshi requested a review from ferranbt May 1, 2025 09:48
@0xKitsune
Copy link
Collaborator

0xKitsune commented May 1, 2025

Thanks for opening this PR! In addition to the comments above can we also ensure that Rollup Boost does not forward Engine API requests to the builder when ExecutionMode is set to Disabled?

@varun-doshi
Copy link
Contributor Author

Latest Commit makes the following changes:

  • In Disabled Mode, dont await the builder payload. Only get the l2_payload and send that
  • In Enabled/ Dry Run mode, get both builder + l2 payload.
  • In dry_run mode:
    • if builder + l2 is okay, send the l2 payload with Healthy signal
    • if builder is none/error and l2 is okay, send l2 payload with PartialContent signal
  • In enabled mode:
    • if builder + l2 is okay, send the builder payload with Healthy signal
    • if builder is none/err and l2 is okay send l2 with PartialContent signal
    • If both are none/err, send ServiceUnavailable signal

@varun-doshi varun-doshi requested a review from 0xKitsune May 2, 2025 08:21
@varun-doshi
Copy link
Contributor Author

varun-doshi commented May 6, 2025

There's a failing test which says Unknown context: "l2" which is weird because there's a match for l2 in the tests/common/mod.rs file

@varun-doshi varun-doshi requested a review from 0xKitsune May 6, 2025 06:14
@0xKitsune
Copy link
Collaborator

0xKitsune commented May 6, 2025

There's a failing test which says Unknown context: "l2" which is weird because there's a match for l2 in the tests/common/mod.rs file

It looks like the test is failing because the way context is injected into the info! call when execution mode is disabled, which differs from when execution mode is enabled. This mismatch causes the test to fail with Unknown context: "l2". Aligning the logging format when execution mode is disabled will resolve the issue.

@varun-doshi varun-doshi requested a review from 0xKitsune May 7, 2025 06:32
@0xKitsune
Copy link
Collaborator

0xKitsune commented May 7, 2025

Thanks for the PR, LGTM!

@ferranbt @avalonche any additional thoughts?

Also it seems the Kurtosis CI keeps failing due to an issue with how Contender is being run. We can open a separate PR to fix this.

@danyalprout
Copy link
Contributor

@0xKitsune opened a PR to fix the stress test CI #204

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.

Simplify ExecutionMode
4 participants