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-node/rollup: Implement Holocene invalid payload attributes handling #12621

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

sebastianst
Copy link
Member

@sebastianst sebastianst commented Oct 24, 2024

Description

Implements the remaining Holocene derivation feature: invalid payload attribute replacement with a deposits-only (DO) version.

The current approach is to request a DO version of the invalid payload attributes from the engine deriver, by having the BuildInvalidEvent handler emit a new DepositsOnlyPayloadAttributesRequestEvent that is directly processed by the pipeline deriver. It doesn't reset the pending safe, as before, but gives this DO version a second chance. The DO request event is handled by the pipeline as following:

  • The pipeline deriver requests the DO version of the last attributes from the attributes queue stage.
    • The attributes queue performs a few sanity checks,
    • flushes the current channel of the previous stages (this is a new Holocene derivation rule),
    • and then returns the DO-version of the last attributes.
  • The pipeline deriver emits a regular DerivedAttributesEvent with the DO-version of the attributes, which is then processed by the AttributesHandler, as before, and so enters the regular attributes derivation flow.
  • These new attributes land in the engine deriver via the usual PendingSafeRequestEvent->PendingSafeUpdateEvent->BuildStartEvent flow.
  • If the DO-version is still invalid, as before, this is a critical pipeline error and the program stops.
  • Because the new DepositsOnlyPayloadAttributesRequestEvent is directly processed by the pipeline deriver, the ProgramDeriver of the op-program just works™️.

This approach breaks with the current invariant to not have the pipeline and engine derivers talk to each other directly, only via the attributes deriver, or the program deriver in the case of the fault proof program. The reason this approach was taken is that this makes it work automatically with the op-program's program deriver. The program deriver otherwise has to reimplement the attributes handler deriver logic, which felt like a less scalable and brittle approach. Since the DO-attributes request is also directly addressed to the pipeline, this seems clean.

Changes to pending safe/safe promotion are done in follow-up work.

Tests

Added an action test TestHoloceneInvalidPayload that shows the correct derivation behavior upon finding an invalid payload.

Additional context

I started a flow diagram on how the different derivers interact with each other here.

Metadata

Fixes #11316

op-node/rollup/engine/build_invalid.go Outdated Show resolved Hide resolved
op-node/rollup/engine/payload_process.go Outdated Show resolved Hide resolved
op-node/rollup/attributes/attributes.go Outdated Show resolved Hide resolved
Copy link
Contributor

semgrep-app bot commented Oct 25, 2024

Semgrep found 3 sol-style-malformed-revert findings:

  • packages/contracts-bedrock/test/universal/Proxy.t.sol
  • packages/contracts-bedrock/test/mocks/Callers.sol
  • packages/contracts-bedrock/test/invariants/FaultDisputeGame.t.sol

Malformed revert statement style.

Ignore this finding from sol-style-malformed-revert.

Semgrep found 9 sol-style-malformed-require findings:

  • packages/contracts-bedrock/test/safe-tools/CompatibilityFallbackHandler_1_3_0.sol
  • packages/contracts-bedrock/test/invariants/OptimismSuperchainERC20/helpers/MockL2ToL2CrossDomainMessenger.t.sol
  • packages/contracts-bedrock/test/invariants/OptimismSuperchainERC20/handlers/Protocol.t.sol
  • packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol
  • packages/contracts-bedrock/test/dispute/AnchorStateRegistry.t.sol
  • packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol
  • packages/contracts-bedrock/test/L1/SystemConfigInterop.t.sol

"Hash not approved" Malformed require statement style.

Ignore this finding from sol-style-malformed-require.

@sebastianst sebastianst linked an issue Oct 28, 2024 that may be closed by this pull request
@sebastianst sebastianst changed the title op-node/rollup: Implement Holocene invalid payload handling op-node/rollup: Implement Holocene invalid payload attributes handling Oct 28, 2024
Copy link
Contributor

semgrep-app bot commented Oct 28, 2024

Semgrep found 1 sol-style-require-reason finding:

  • packages/contracts-bedrock/scripts/ops/FeeVaultWithdrawal.s.sol

require() must include a reason string

Ignore this finding from sol-style-require-reason.

Semgrep found 6 sol-style-input-arg-fmt findings:

Inputs to functions must be prepended with an underscore (_)

Ignore this finding from sol-style-input-arg-fmt.

Semgrep found 1 golang_fmt_errorf_no_params finding:

  • packages/contracts-bedrock/scripts/checks/semver-natspec/main.go

No fmt.Errorf invocations without fmt arguments allowed

Ignore this finding from golang_fmt_errorf_no_params.

Base automatically changed from seb/holocene-pipeline to develop October 28, 2024 20:59
@sebastianst sebastianst marked this pull request as ready for review October 28, 2024 21:09
@sebastianst sebastianst self-assigned this Oct 29, 2024
@sebastianst sebastianst added this to the Holocene: Derivation milestone Oct 29, 2024
Copy link
Contributor

@protolambda protolambda 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 a few minor things to resolve

op-e2e/actions/upgrades/holocene_fork_test.go Outdated Show resolved Hide resolved
op-e2e/actions/upgrades/holocene_fork_test.go Outdated Show resolved Hide resolved
op-node/rollup/derive/deriver.go Outdated Show resolved Hide resolved
op-node/rollup/engine/build_invalid.go Outdated Show resolved Hide resolved
op-node/rollup/engine/payload_process.go Outdated Show resolved Hide resolved
@tynes tynes added this pull request to the merge queue Oct 31, 2024
Merged via the queue into develop with commit 47da14a Oct 31, 2024
48 checks passed
@tynes tynes deleted the seb/holocene-invalid-payloads branch October 31, 2024 07:14
maurelian pushed a commit that referenced this pull request Oct 31, 2024
#12621)

* op-node/rollup: Implement Holocene invalid payload handling

* op-node: update comment about block-processing errors

---------

Co-authored-by: protolambda <proto@protolambda.com>
axelKingsley pushed a commit that referenced this pull request Oct 31, 2024
#12621)

* op-node/rollup: Implement Holocene invalid payload handling

* op-node: update comment about block-processing errors

---------

Co-authored-by: protolambda <proto@protolambda.com>
samlaf pushed a commit to samlaf/optimism that referenced this pull request Nov 10, 2024
ethereum-optimism#12621)

* op-node/rollup: Implement Holocene invalid payload handling

* op-node: update comment about block-processing errors

---------

Co-authored-by: protolambda <proto@protolambda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Holocene-D: Derive invalid payloads as deposit-only blocks
4 participants