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-program: refactor driver to use events derivers #10971

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Jun 20, 2024

Description

This refactors the op-program driver to run through the state-transition via a synchronous events loop, rather than the old steps loop. This allows it to use the new deriver interface, which then unblocks us from dropping the legacy driver approach.

The op-program starts with an engine-reset, and then events turn into more events until the forkchoice-updated (i.e. event when forkchoice has been applied to engine) to the target block number has been reached.

The op-program is split into two parts:

  • The "driver" that just loops through events, and only abstractly knows about an end-condition. It composes a "program-deriver" with some op-node derivers (engine and derivation) to build the default state-transition deriver to run. It fires an engine-reset request event, to ready the state.
  • The "program-deriver" (yes, naming is hard) that maps events between different derivers, and monitors the events for the end condition of reaching a block number.

Extracted from PR #10947

This temporarily duplicates some of the attributes-handling code, as that is essential to go from derived attributes to engine API interactions.

Tests

  • op-program "driver" has new unit tests, to check what happens to events processing
  • program-deriver has new unit tests, to verify it matches the conceptual steps that should lead to correct program completion / error-exit.

Additional context

Add any other context about the problem you're solving.

Metadata

Fix #10969

@protolambda protolambda requested review from a team and ajsutton as code owners June 20, 2024 21:47
Copy link
Contributor

semgrep-app bot commented Jun 20, 2024

Semgrep found 1 sol-style-malformed-revert finding:

  • packages/contracts-bedrock/src/cannon/MIPS.sol

Malformed revert statement style.

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

@protolambda
Copy link
Contributor Author

protolambda commented Jun 20, 2024

Hmm, the op-program compat test fails, but it does log:

t=2024-06-20T21:52:43+0000 lvl=info msg="Program Bootstrapped" bootInfo="&{
L1Head:0x935428728bcfcfeb2e5ba9175fd2890e52831dae221aa4d5dcffed8320edc001 
L2OutputRoot:0x4a0ac5d9c009abefba838033400b6a40a8e79eecf140969c3997cab8d17b4e5a 
L2Claim:0xf2ae122ac5b3a4a4780674c3d14fbb4975c613d1dfe201356a026695cb65c50b 
L2ClaimBlockNumber:8728320 L2ChainID:11155420 L2ChainConfig:0xc0004ac400 
RollupConfig:0xc0004d3a00}"

...

t=2024-06-20T21:52:43+0000 lvl=info msg="Loaded current L2 heads" 
unsafe=0x9ebb4f9422145591dba9e75d7f278ce6db20d28fdfcf6ca6f7cb6f3897a6dc5a:8728200 
safe=0x9ebb4f9422145591dba9e75d7f278ce6db20d28fdfcf6ca6f7cb6f3897a6dc5a:8728200 
finalized=0x9ebb4f9422145591dba9e75d7f278ce6db20d28fdfcf6ca6f7cb6f3897a6dc5a:8728200 
unsafe_origin=0x0a65a23ad07f18edb07532f4f73c82be616b41823730192420c874772803926b:5391372 
safe_origin=0x0a65a23ad07f18edb07532f4f73c82be616b41823730192420c874772803926b:5391372


...

t=2024-06-20T21:52:54+0000 lvl=info msg="Derivation complete: reached L2 block" 
head=0x49256bf418fb8a75a5295c677716d7d740112b6702311ecb7b97305e222ecc51:8728320

...

t=2024-06-20T21:52:54+0000 lvl=info msg="Validating claim" 
head=0xa6386d31236e5663553623dad83ba67c00e59b04976c282c07156f166cd60f1a:8728286 
output=0x92ef92a32085b0ca90eb52eaf9c3e7e713e2b34d34c69c4710f0003b9144fb69 
claim=0xf2ae122ac5b3a4a4780674c3d14fbb4975c613d1dfe201356a026695cb65c50b

t=2024-06-20T21:52:54+0000 lvl=error msg="Claim is invalid" err="invalid claim:
claim: 0xf2ae122ac5b3a4a4780674c3d14fbb4975c613d1dfe201356a026695cb65c50b 
actual: 0x92ef92a32085b0ca90eb52eaf9c3e7e713e2b34d34c69c4710f0003b9144fb69"

Which is the correct block at the target block number.

@protolambda
Copy link
Contributor Author

I think it has something to do with final forkchoice updates not being applied, and the validate-claim uses the safe-block from the chain, not the unapplied engine-controller forkchoice. So the exit-trigger happens before the state is there that will be validated.

@protolambda
Copy link
Contributor Author

Turns out I stopped execution on unsafe==target, but it needed to be stopped only after safe==target (because span batch backup mechanism)

@ajsutton
Copy link
Contributor

Code looks good to me, but the failing op-e2e-HTTP-tests-l2oo are interesting:

goroutine 1732305 [running]:
github.com/ethereum-optimism/optimism/op-node/rollup/clsync.(*CLSync).onInvalidPayload(0xc0221bb9f0, {0x0?})
        /root/project/op-node/rollup/clsync/clsync.go:84 +0x1f
github.com/ethereum-optimism/optimism/op-node/rollup/clsync.(*CLSync).OnEvent(0xc0221bba90?, {0x3128be0?, 0x0?})
        /root/project/op-node/rollup/clsync/clsync.go:73 +0x105
github.com/ethereum-optimism/optimism/op-node/rollup.(*SynchronousDerivers).OnEvent(0xc028d9a000?, {0x3128be0, 0x0})
        /root/project/op-node/rollup/events.go:57 +0x3f
github.com/ethereum-optimism/optimism/op-node/rollup.(*SynchronousEvents).Drain(0xc0221bb950)
        /root/project/op-node/rollup/synchronous.go:72 +0x27
github.com/ethereum-optimism/optimism/op-node/rollup/driver.(*Driver).eventLoop(0xc0221bf400)
        /root/project/op-node/rollup/driver/state.go:225 +0x2fa
created by github.com/ethereum-optimism/optimism/op-node/rollup/driver.(*Driver).Start in goroutine 186395
        /root/project/op-node/rollup/driver/state.go:129 +0x185
panic: failed to read pre-image hint ack: EOF

I don't think I've seen tests fail like that before.

@ajsutton
Copy link
Contributor

Ah, we're emitting an InvalidPayloadEvent in

e.emitter.Emit(InvalidPayloadEvent{})
with no payload and the new debug call (
eq.log.Debug("CL sync received invalid-payload report", x.Envelope.ExecutionPayload.ID())
) then panics.

@protolambda
Copy link
Contributor Author

@ajsutton thanks for the pointers. I changed it so the engine now differentiates between invalid payload and invalid payload attributes. The invalid-attributes case shouldn't trigger an invalid event (nil payload) that gets picked up by the CL sync.

@protolambda protolambda added this pull request to the merge queue Jun 21, 2024
Merged via the queue into develop with commit db31f84 Jun 21, 2024
58 checks passed
@protolambda protolambda deleted the op-program-events branch June 21, 2024 23:48
rdovgan pushed a commit to rdovgan/optimism that referenced this pull request Jun 24, 2024
…#10971)

* op-program: refactor to use events derivers

* op-node: differentiate between invalid payload and invalid payload attributes
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.

Interop: Update op-program driver to use only events
2 participants