ref impl: find starting point of sync, given L1 and L2 interfaces #130

protolambda · 2022-01-10T23:20:37Z

Part of #119: staging -> main migration

This:

Implements a "sync reference" interface, and implementation that implements it based on common RPC source interfaces. To mock chain status easily for sync starting-point testing.
Implements algorithm from @karlfloersch (with missing edge cases handled) to find the starting point for sync, statelessly (i.e. no rollup-node storage requirement, nor in-memory chain/tree)

Depends on #129

Review: any team.

Spec: update reference how we find the starting point of sync.

Testing: ~~different starting point edge cases~~ done

protolambda · 2022-01-12T20:50:20Z

opnode/l2/sync_start.go

@@ -59,7 +59,7 @@ func FindSyncStart(ctx context.Context, reference SyncReference, genesis *Genesi
 	// Search back: linear walk back from engine head. Should only be as deep as the reorg.
 	for refL2.Number > 0 {
 		// remember the canonical L1 block that builds on top of the L1 source block of the L2 parent block.
-		nextRefL1 = refL1
+		nextRefL1 = currentL1


This was a bug, found with new tests, fixed now. When traversing back, remember the actual L1 hash as canonical, not the L1 hash from the engine block

Clarifying for further reviewers: the version in this PR is the fixed version.

maurelian · 2022-01-13T16:54:07Z

I'm having trouble running this branch. I think it's likely an issue with the CLI config, as it doesn't recognize the flags specified here.

My steps:

Checkout this branch.
From the root dir:

go mod download
go build -o rollupnode ./opnode/cmd

when I run rollupnode run -h, the only output is

(command)

By contrast, on the staging branch I get a proper help menu when I run ./rollupnode run -h.

protolambda · 2022-01-13T17:26:32Z

@maurelian it's missing the code from PRs that build on top of this to complete the rollup node implementation. Most notably the driver and node PRs. If you need to run the latest changes, then run #132 and merge the commits made on top of these feature branches.

norswap

The function really needs a comment explaining its behaviour.

I'm also currently a bit puzzled by the fact it will sometimes return an old L2 block, but it might dawn on me why that makes sense when I read the rest of the sync logic!

opnode/l2/sync_reference.go

opnode/l2/sync_start.go

norswap · 2022-01-17T16:16:01Z

opnode/l2/sync_start.go

@@ -59,7 +59,7 @@ func FindSyncStart(ctx context.Context, reference SyncReference, genesis *Genesi
 	// Search back: linear walk back from engine head. Should only be as deep as the reorg.
 	for refL2.Number > 0 {
 		// remember the canonical L1 block that builds on top of the L1 source block of the L2 parent block.
-		nextRefL1 = refL1
+		nextRefL1 = currentL1


Clarifying for further reviewers: the version in this PR is the fixed version.

norswap · 2022-01-17T16:17:11Z

opnode/l2/sync_start.go

+		nextRefL1 = currentL1
+		refL1, refL2, parentL2, err = reference.RefByL2Hash(ctx, parentL2, genesis)
+		if err != nil {
+			// TODO: re-attempt look-up, now that we already traversed previous history?


What does this TODO mean?

If the reorg is deep, it may have to traverse far to find the common prefix. If we exit on the first API failure along the way it has to start over again. Retrying the first few failures would improve that worst-case.

opnode/l2/sync_start.go

maurelian · 2022-01-18T19:12:29Z

I think we might need to rebase this on main in order to get a coverage report on CodeCov.

protolambda · 2022-01-19T16:29:23Z

*edit: commented on wrong PR

protolambda · 2022-01-19T18:19:00Z

Rebased to main:

build on latest ref impl, fix merge conflicts
resolves rollup-node spec updates (main also changed rollup-node spec)

protolambda · 2022-01-20T16:27:07Z

@norswap please re-review or approve if the updates seem good to you.

tynes · 2022-01-20T16:40:44Z

opnode/l2/sync_reference.go

+	L2 eth.BlockSource
+}
+
+// RefByL1Num fetches the canonical L1 block hash and the parent for the given L1 block height.


Just wondering if it was intentional to make l1Num in RefByL1Num a uint64 while l2Num in RefByL2Num is a *big.Int

I'm guessing it is because we never care for querying the "latest" L1 block?

uint64 is better and more simple, but the standard RPC uses *big.Int (also because a nil argument is a special API value for requesting the head).

tynes · 2022-01-20T16:55:37Z

specs/rollup-node.md

 rollup driver should not result in errors assuming conformity with the specification. Said otherwise, all errors are
 implementation concerns and it is up to them to handle them (e.g. by retrying, or by stopping the chain derivation and
 requiring manual user intervention).

 The following scenarios are assimilated to errors:

- [`engine_forkchoiceUpdatedOPV1`] returning a `status` of `"SYNCING"` instead of `"SUCCESS"` whenever passed a
+- [`engine_forkchoiceUpdatedV1`] returning a `status` of `"SYNCING"` instead of `"SUCCESS"` whenever passed a


There will no longer be an optimism specific set of engine RPCs but instead the ones that need to be modified will be modified?

The change is backwards compatible, going to try and upstream it before we commit to an optimism-specific version

karlfloersch · 2022-01-20T19:11:39Z

specs/rollup-node.md

+A [transaction deposit][transaction deposits] is an L2 transaction that has been submitted on L1, via a call to the
+[deposit feed contract].
+
+Refer to the[**deposit feed contract specification**][deposit-feed-spec] for details on how


Suggested change

Refer to the[**deposit feed contract specification**][deposit-feed-spec] for details on how

Refer to the [**deposit feed contract specification**][deposit-feed-spec] for details on how

karlfloersch · 2022-01-20T19:19:58Z

Awesome @protolambda - just reviewed & looked through the reorg logic and it LGTM! I remember the algorithm that you described & it seems what you wrote up is pretty much the same as that--the main complexity coming from needing to hold on to both the current block and next block on both L1 and L2. Anyway it looks great to me!!!

The one thing to highlight is when we move to sequencer rollup we will need to start our reorgs from the latest safe L2 block as opposed to the tip block for L2. This is because the reorg logic needs to be able to tell what the last block it has checked against L1 is. This is so we can preserve the property that all L2 blocks are checked by the driver against L1 even if the sequencer propagates unsubmitted blocks to the verifiers.

More details--in sequencer rollup the driver needs to know what L2 block it last checked against L1 if we are to allow the sequencer to propagate unsubmitted blocks. This attack is possible because the sequencer would be able to lie about the L1 block hash in the unsubmitted blocks, and by lying it could convince the driver that it doesn't need to re-execute the L2 block even if it is invalid. Let me know if that is clear, I don't yet know how to clearly explain this property.

Anywho this doesn't apply to deposit rollup and it's suuuper exciting to have a stateless driver!!!!!

protolambda added the C-ref-impl label Jan 10, 2022

protolambda added this to the Deposit Rollup Release Candidate milestone Jan 10, 2022

protolambda self-assigned this Jan 10, 2022

This was referenced Jan 10, 2022

EPIC CHECKLIST: Review/Integrate staged deposit-only ref impl work #119

Closed

ref impl: driver agent: respond to head events and stay in sync #131

Merged

protolambda commented Jan 12, 2022

View reviewed changes

norswap suggested changes Jan 17, 2022

View reviewed changes

maurelian requested a review from karlfloersch January 18, 2022 17:38

protolambda force-pushed the impl-driver-step branch from 6d40a12 to b9bd939 Compare January 19, 2022 17:54

Base automatically changed from impl-driver-step to main January 19, 2022 17:57

protolambda added 11 commits January 19, 2022 19:01

ref impl: find starting point of sync, given L1 and L2 interfaces

8b667dd

ref impl: test FindSyncStart

ab7a778

ref impl: sync start genesis and diff height edge cases, more tests

92bfa20

ref impl: sync start genesis L1 check test fixed

11048aa

ref impl: test sync start with offset L2 genesis

90c8a1b

ref impl: no timeouts, called responsible instead

576b370

ref impl: clarify sync-start and sync-ref comments

ef80361

ref impl: sync start finding - lint imports

ae271a5

specs/rollup-node: update & de-duplicate driver process description

4074ef8

specs/rollup-node: update sync algorithm

3be1d41

specs/exec-engine: clarify sync section, refer to rollup node spec

55426b4

protolambda force-pushed the impl-sync-start branch from 03eec94 to ad0b2c8 Compare January 19, 2022 18:16

ref impl: return both refL1 and nextRefL1 for driver

f5d7631

protolambda force-pushed the impl-sync-start branch from ad0b2c8 to f5d7631 Compare January 19, 2022 18:17

protolambda mentioned this pull request Jan 20, 2022

[Do not merge] Merge staging to main #98

Closed

protolambda requested a review from norswap January 20, 2022 16:26

norswap approved these changes Jan 20, 2022

View reviewed changes

protolambda merged commit 03a75b5 into main Jan 20, 2022

protolambda deleted the impl-sync-start branch January 20, 2022 18:42

tynes approved these changes Jan 20, 2022

View reviewed changes

karlfloersch reviewed Jan 20, 2022

View reviewed changes

trianglesphere added implementation and removed C-ref-impl labels Mar 9, 2022

	Refer to the[deposit feed contract specification][deposit-feed-spec] for details on how
	Refer to the [deposit feed contract specification][deposit-feed-spec] for details on how

ref impl: find starting point of sync, given L1 and L2 interfaces #130

ref impl: find starting point of sync, given L1 and L2 interfaces #130

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurelian commented Jan 13, 2022

Uh oh!

protolambda commented Jan 13, 2022

Uh oh!

norswap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maurelian commented Jan 18, 2022

Uh oh!

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

Uh oh!

Uh oh!

protolambda commented Jan 19, 2022

Uh oh!

protolambda commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karlfloersch commented Jan 20, 2022

Uh oh!

Uh oh!

protolambda commented Jan 10, 2022 •

edited

Loading

protolambda commented Jan 19, 2022 •

edited

Loading