Skip to content

Safe head fix 2#3382

Merged
mslipper merged 15 commits intodevelopfrom
safe-head-fix-2
Sep 10, 2022
Merged

Safe head fix 2#3382
mslipper merged 15 commits intodevelopfrom
safe-head-fix-2

Conversation

@protolambda
Copy link
Contributor

@protolambda protolambda commented Sep 9, 2022

Description

This replaces #3320, preserving the FindHeads function, with the recently discussed requirements:

  • Off By one fix: full sequence window, beyond the start
  • Prev safe head minimum:
    • if the L1 origins referenced by unsafe L2 blocks are canonical for a full sequence window, then we can accept the previous safehead without going back further.
    • if the above is not the case, we end up selecting a block before the safe head. And since it is one loop, we can still optimize this like the above
  • Finalized head: make sure we never go back further than the finalized head, and that we fall back to genesis if nothing has been finalized yet.
  • Genesis checks: make sure the engine and the L1 source are on the correct chain. Never start syncing between different chain data.
  • Contiguous L1 origin: when we are not ahead of the L1 chain, we should check the L1 origin parent hashes all match. And always check basic consistency between L1 origins in L2 blocks we traverse.
  • Port over tests from original PR. Including adjustments to engine queue finalization PR, since it needs to account for the safe head changes / lavel usage.
  • Change function signature: we have rpc labels to go by, and shouldn't rely on any previous derivation state from the rollup node itself.

Metadata

Fix ENG-2671

@changeset-bot
Copy link

changeset-bot bot commented Sep 9, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 90cda78

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

tynes
tynes reviewed Sep 10, 2022
View reviewed changes
trianglesphere
trianglesphere suggested changes Sep 10, 2022
View reviewed changes
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the off by one error & where is it fixed? I'm having a hard time finding it.

@protolambda
Copy link
Contributor Author

protolambda commented Sep 10, 2022

Where is the off by one error & where is it fixed? I'm having a hard time finding it.

Commit 56c6093 fixes it.

trianglesphere
trianglesphere reviewed Sep 10, 2022
View reviewed changes
@protolambda
op-node: seq window size check with l1 origin, update engine queue te…
c064550 
…st with extra l1 origin, now that off by 1 fix applies properly to chains with multiple L2 blocks per L1 block
@protolambda
op-node: start from parent block before seq nr 0
bbe1c37
trianglesphere
trianglesphere reviewed Sep 10, 2022
View reviewed changes
protolambda and others added 2 commits September 10, 2022 18:57
@protolambda @trianglesphere
Update op-node/rollup/sync/start.go
c1ac3e9 
Co-authored-by: Joshua Gutow <jgutow@optimism.io>
@protolambda
review fixes
8a7846b
trianglesphere
trianglesphere approved these changes Sep 10, 2022
View reviewed changes
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goimports is failing in the mocks

@mslipper mslipper merged commit e38c5f4 into develop Sep 10, 2022
@mslipper mslipper deleted the safe-head-fix-2 branch September 10, 2022 18:21
maurelian pushed a commit that referenced this pull request Sep 15, 2022
@protolambda @trianglesphere
@mslipper @maurelian
Safe head fix 2 (#3382)
754c708 
* op-node: sync start update func signature, port over tests from safe-head-fix pr

* op-node: sync start now starts with current heads by label

* op-node: single loop find-heads

* op-node: sync - clean up go doc

* op-node: use sync fn in engine queue

* op-node: fix engine queue finalization test

* op-node: sync off-by-one fix

* op-node: fix highest l2 block with canon origin, need to reset when l1 reorg

* op-node: handle non-standard safe/finalized not found errors

* op-node: sync start review fixes / comment typos

* op-node: seq window size check with l1 origin, update engine queue test with extra l1 origin, now that off by 1 fix applies properly to chains with multiple L2 blocks per L1 block

* op-node: start from parent block before seq nr 0

* Update op-node/rollup/sync/start.go

Co-authored-by: Joshua Gutow <jgutow@optimism.io>

* review fixes

* fix lint

Co-authored-by: Joshua Gutow <jgutow@optimism.io>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>
sam-goldman pushed a commit that referenced this pull request Sep 15, 2022
@protolambda @trianglesphere
@mslipper @sam-goldman
Safe head fix 2 (#3382)
0fac751 
* op-node: sync start update func signature, port over tests from safe-head-fix pr

* op-node: sync start now starts with current heads by label

* op-node: single loop find-heads

* op-node: sync - clean up go doc

* op-node: use sync fn in engine queue

* op-node: fix engine queue finalization test

* op-node: sync off-by-one fix

* op-node: fix highest l2 block with canon origin, need to reset when l1 reorg

* op-node: handle non-standard safe/finalized not found errors

* op-node: sync start review fixes / comment typos

* op-node: seq window size check with l1 origin, update engine queue test with extra l1 origin, now that off by 1 fix applies properly to chains with multiple L2 blocks per L1 block

* op-node: start from parent block before seq nr 0

* Update op-node/rollup/sync/start.go

Co-authored-by: Joshua Gutow <jgutow@optimism.io>

* review fixes

* fix lint

Co-authored-by: Joshua Gutow <jgutow@optimism.io>
Co-authored-by: Matthew Slipper <me@matthewslipper.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes left review comments

+1 more reviewer

@trianglesphere trianglesphere trianglesphere approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@protolambda @tynes @trianglesphere @mslipper