op-supervisor: local-safe reorg support by protolambda · Pull Request #13645 · ethereum-optimism/optimism

protolambda · 2025-01-08T23:08:46Z

Description

Start of reorg support. This handles the reorg case where a local-safe block is not actually cross-safe.

When a local-safe block is detected to conflict:

Send update to chains-DB to roll back, and insert a replacement block entry into local/cross-safe DB.
Use feed to have connected sync nodes proactively follow the replacement block, rather than finding cross-safe conflicting later (at which point we then also need to handle replacement)
Sync nodes should insert the replacement block with a reorg, mark it as safe, and then then trigger a reset, so the nodes continues to derive on top of this cross-safe block.
Sync nodes should publish the replacement-block as event. Instead of a derived-from event. This can then be watched by the supervisor, and marked as cross-safe.

Tests

Unit tests for block-replacement tx
Action test for e2e block replacement example
Updated existing test from tyler, that now reorgs out the bad tx, rather than halting cross-safe progression.

Additional context

Metadata

After this we still have two types of reorgs:

L1 reorgs, and we need to roll back the supervisor (simple, already keyed by L1 blocks)
L2 unsafe block conflicts: we won't know what is and isn't valid. We might need some heuristics here, to choose an unsafe chain that can be recovered, and remove the rest asap, so it's not batch-submitted.

codecov · 2025-01-09T21:18:47Z

Codecov Report

Attention: Patch coverage is 38.40399% with 247 lines in your changes missing coverage. Please review.

Project coverage is 45.24%. Comparing base (9580179) to head (4ed049d).
Report is 22 commits behind head on develop.

Files with missing lines	Patch %	Lines
op-supervisor/supervisor/backend/db/update.go	0.00%	67 Missing ⚠️
op-supervisor/supervisor/backend/db/query.go	0.00%	51 Missing ⚠️
op-node/rollup/interop/managed/system.go	0.00%	42 Missing ⚠️
op-supervisor/supervisor/backend/syncnode/node.go	0.00%	25 Missing and 1 partial ⚠️
op-node/rollup/interop/managed/attributes.go	86.90%	7 Missing and 4 partials ⚠️
op-service/eth/types.go	0.00%	7 Missing ⚠️
...-supervisor/supervisor/backend/db/fromda/update.go	36.36%	7 Missing ⚠️
op-supervisor/supervisor/backend/db/fromda/db.go	66.66%	4 Missing and 2 partials ⚠️
...r/supervisor/backend/processors/chain_processor.go	0.00%	5 Missing ⚠️
op-supervisor/supervisor/types/types.go	0.00%	5 Missing ⚠️
... and 10 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #13645      +/-   ##
===========================================
- Coverage    47.04%   45.24%   -1.80%     
===========================================
  Files          973      917      -56     
  Lines        81123    76680    -4443     
  Branches       776      776              
===========================================
- Hits         38161    34693    -3468     
+ Misses       40003    39258     -745     
+ Partials      2959     2729     -230

Flag	Coverage Δ
cannon-go-tests-32	`?`
cannon-go-tests-64	`?`
contracts-bedrock-tests	`88.45% <ø> (-2.37%)`	⬇️

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

Files with missing lines	Coverage Δ
op-node/rollup/derive/deposit_source.go	`100.00% <100.00%> (ø)`
op-node/rollup/engine/engine_reset.go	`86.20% <100.00%> (ø)`
op-node/rollup/engine/payload_success.go	`100.00% <100.00%> (ø)`
op-node/rollup/sequencing/origin_selector.go	`94.35% <100.00%> (ø)`
...pervisor/supervisor/backend/cross/safe_frontier.go	`100.00% <100.00%> (ø)`
op-node/rollup/attributes/attributes.go	`68.38% <0.00%> (ø)`
op-node/rollup/derive/deriver.go	`0.00% <0.00%> (ø)`
...supervisor/supervisor/backend/cross/safe_update.go	`71.00% <96.87%> (+2.86%)`	⬆️
...ervisor/supervisor/backend/l1access/l1_accessor.go	`24.17% <0.00%> (-0.27%)`	⬇️
op-node/rollup/event.go	`0.00% <0.00%> (ø)`
... and 15 more

... and 84 files with indirect coverage changes

axelKingsley · 2025-01-17T19:55:54Z

Discussed synchronously, the API updates will be split out so we can land these separately

…acement work, and add action-test for block replacement

axelKingsley

This PR is more impactful than the title suggests, as it also contains things like AttributesToReplaceInvalidBlock, which is the implementation of block replacement to spec.

The Action testing makes this easy to approve, I can see that the supervisor and node both react correctly when a local-safe block is invalid in the cross-safe context.

I have a handful of comment, but none are blocking, so I've approved and will let you address as many of them as you like.

op-node/rollup/derive/deriver.go

op-node/rollup/interop/managed/attributes.go

op-supervisor/supervisor/backend/cross/safe_update.go

op-supervisor/supervisor/backend/processors/chain_processor.go

op-supervisor/supervisor/backend/syncnode/node.go

op-supervisor/supervisor/types/types.go

op-e2e/actions/interop/interop_test.go

axelKingsley · 2025-01-24T16:16:15Z

Another thing I'm thinking about is:

	// It is called with the candidate, the block that will be invalidated.
	// The replacement of this candidate will effectively be "derived from"
	// the scope that the candidate block was invalidated at.

What I think this means is that, looking the local safe database, a given L2 block L2.A may appear as derived from L1.A, and then eventually when it is discovered to be cross-invalid, we invalidate it at the point it becomes invalid. So, if L2.A isn't found to be invalid until L1.Z appears, the database will feature a locally safe L2 block which is known to be invalid, at which point it is replaced with L2.A'. But link entries L2.A:L1.A, L2.A:L1.B... will still exist.

I think that's okay because it accurately describes the derivation information which is available at each L1 step, but I'm wondering out loud if any of our APIs might find a Local-Safe block L2.A and make some assumptions about the validity of that block. The node will totally forget about L2.A and will only know about L2.A'.

Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com>

zhiqiangxu · 2025-01-25T03:13:49Z

op-node/rollup/interop/managed/attributes.go

+	if tx.Type() != types.DepositTxType {
+		return nil, fmt.Errorf("expected deposit tx type, but got %d", tx.Type())
+	}
+	signer := types.LatestSignerForChainID(tx.ChainId())


Is it necessary to validate tx.ChainId() matches the actual chainid here?

It's implicitly checked below when recovering the sender from the tx.

* interop: local-safe invalidation handling v3 * op-node,op-supervisor: build local-safe invalidation replacement block * op-service,op-e2e,op-supervisor: fix lint * op-supervisor: add new cross-safe-update unit tests * op-supervisor: fromda IsDerived test checks * op-node: test InvalidatedBlockSource * op-node: fix and test replace-block attributes * op-node,op-supervisor: fixes to make invalidate-local-safe block-replacement work, and add action-test for block replacement * op-e2e: fix interop tests * Update op-supervisor/supervisor/backend/syncnode/node.go Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com> --------- Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com>

protolambda requested review from a user and axelKingsley January 8, 2025 23:08

protolambda force-pushed the supervisor-reorg-support-start branch from e96901b to f6fe8e3 Compare January 9, 2025 19:13

protolambda changed the title ~~op-supervisor: start of local-safe reorg support~~ op-supervisor: local-safe reorg support Jan 10, 2025

protolambda force-pushed the supervisor-reorg-support-start branch 2 times, most recently from 0e1d391 to 8bd1dad Compare January 16, 2025 21:29

axelKingsley mentioned this pull request Jan 17, 2025

interop: Reorg Support - Goals and Design #13837

Closed

protolambda mentioned this pull request Jan 17, 2025

op-supervisor: return typed pairs of L1/L2 blocks, and support invalidation of fromda entries #13840

Merged

2 tasks

protolambda force-pushed the supervisor-reorg-support-start branch from 8bd1dad to af475e4 Compare January 21, 2025 14:27

protolambda changed the base branch from develop to supervisor-reorg-interface-changes January 21, 2025 14:28

Base automatically changed from supervisor-reorg-interface-changes to develop January 21, 2025 17:28

protolambda force-pushed the supervisor-reorg-support-start branch from af475e4 to 5dd6fdc Compare January 21, 2025 18:26

protolambda added 3 commits January 22, 2025 17:55

interop: local-safe invalidation handling v3

284f83f

op-node,op-supervisor: build local-safe invalidation replacement block

2a5b8e9

op-service,op-e2e,op-supervisor: fix lint

75e26ac

protolambda force-pushed the supervisor-reorg-support-start branch from 0d02172 to 75e26ac Compare January 22, 2025 17:01

protolambda added 6 commits January 22, 2025 18:18

op-supervisor: add new cross-safe-update unit tests

c806af1

op-supervisor: fromda IsDerived test checks

4513227

op-node: test InvalidatedBlockSource

154ad02

op-node: fix and test replace-block attributes

45961da

op-node,op-supervisor: fixes to make invalidate-local-safe block-repl…

5827d77

…acement work, and add action-test for block replacement

op-e2e: fix interop tests

9aee48f

protolambda marked this pull request as ready for review January 23, 2025 15:39

protolambda requested review from a team as code owners January 23, 2025 15:39

protolambda requested a review from ajsutton January 23, 2025 15:39

protolambda added the H-interop Hardfork: change planned for interop upgrade label Jan 23, 2025

protolambda added this to the Interop: Stable Devnet milestone Jan 23, 2025

axelKingsley approved these changes Jan 24, 2025

View reviewed changes

Update op-supervisor/supervisor/backend/syncnode/node.go

4ed049d

Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com>

protolambda enabled auto-merge January 24, 2025 17:47

protolambda added this pull request to the merge queue Jan 24, 2025

Merged via the queue into develop with commit 82784fa Jan 24, 2025

protolambda deleted the supervisor-reorg-support-start branch January 24, 2025 17:58

zhiqiangxu reviewed Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

op-supervisor: local-safe reorg support#13645

op-supervisor: local-safe reorg support#13645
protolambda merged 10 commits intodevelopfrom
supervisor-reorg-support-start

protolambda commented Jan 8, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 9, 2025 •

edited

Loading

Uh oh!

axelKingsley commented Jan 17, 2025

Uh oh!

axelKingsley left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

axelKingsley commented Jan 24, 2025 •

edited

Loading

Uh oh!

zhiqiangxu Jan 25, 2025

Uh oh!

Inphi Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

protolambda commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

axelKingsley commented Jan 17, 2025

Uh oh!

axelKingsley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

axelKingsley commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhiqiangxu Jan 25, 2025

Choose a reason for hiding this comment

Uh oh!

Inphi Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

protolambda commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

axelKingsley commented Jan 24, 2025 •

edited

Loading