geth: implement sync service spec #477

tynes · 2021-04-16T02:03:11Z

Description
Implements the SyncService spec, still WIP
https://github.com/ethereum-optimism/specs/blob/main/l2-geth/transaction-ingestor.md

The spec was designed to prevent double ingestion or skipping of ingestion of transactions. The function names in the spec should match 1:1 the methods on the SyncService. The actual logic implemented differs slightly from the spec in how it implements its break conditions, but is functionally equivalent.

The sequencer queries for transaction batches and compares the individual transactions to what it has locally (first step to reorgs based on L1)
The verifier syncs using transaction batches
Query param based calls to the DTL to query for L1 or L2 transactions (explicit instead of implicit DTL config)
~~Application level attempt to allow for 0 downtime rollovers of the sequencer. I think this should code should be deleted and moved to the infra layer~~

Additional context
This is the first step to reorgs based on L1 as well as 0 downtime rollovers
This PR builds on top of #479 but is backwards compatible enough for the integration tests to work without it. The verifier really only takes advantage of the query param based syncing currently

changeset-bot · 2021-04-16T02:03:13Z

🦋 Changeset detected

Latest commit: d743630

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@eth-optimism/l2geth	Minor

Not sure what this means? Click here to learn what changesets are.

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

l2geth/rollup/sync_service.go

l2geth/internal/ethapi/api.go

l2geth/rollup/client.go

gakonst · 2021-04-18T07:39:46Z

@tynes please add a changeset with a minor version bump by running "yarn changeset" and going through the interactive dialogue!

tynes · 2021-04-19T18:14:15Z

please add a changeset with a minor version bump by running "yarn changeset" and going through the interactive dialogue!

I've added the changeset with a minor bump. This should be ready for review now

gakonst

A few changes requested internally.

Also, as a process point, I think we should be more diligent in having explicit commits for each change being made inside geth and contracts, since they are critical components for the core protocol.

This PR does a lot of refactoring/renaming, when that should be saved for a separate PR which changes no functionality and simply moves code around.

As another example, this commit should have been split into a few commits:

refactor: use transaction instead of response when parsing dtl response
refactor: rename updateContext to updateEthContext
feat: allow reading and writing verified batch indexes to the db
feat: add backend parameter to DTL queries

Similarly, when removing stuff, e.g. confirmationDepth, you can add a commit that says chore: remove unused confirmationDepth param.

Finally, if the DTL "choose your own backend" changes are NOT needed to implement the basic functionality, then they should be split in a separate PR. It does not make sense to add things to client.go in this PR if they are not present in the DTL as well.

OK if you don't want to do this for this PR, but it'll make reviewing such code changes much easier in the future.

Another tip would be that after you're doing with your PR, git reset master, and then start re-doing the commits individually so that they're logically split with git add -p, to stage the hunks that matter for each logical change.

"fix typos" and "fix broken tests" commits are not useful for the reviewer, since they are changes introduced in this PR. The commit message is not about things you've done related to your previous commits, it's related to the things you've done related to master!

l2geth/eth/api_backend.go

l2geth/internal/ethapi/api.go

gakonst · 2021-04-18T08:10:54Z

l2geth/rollup/client.go

 		return nil, nil
 	}
 	// The queue origin must be either sequencer of l1, otherwise
 	// it is considered an unknown queue origin and will not be processed
 	var queueOrigin types.QueueOrigin
-	if res.Transaction.QueueOrigin == "sequencer" {
+	if res.QueueOrigin == "sequencer" {


Not for this PR, but we should replace strings with enums.

gakonst · 2021-04-20T08:44:02Z

l2geth/cmd/utils/flags.go

@@ -849,6 +849,12 @@ var (
 		Value:  time.Minute * 15,
 		EnvVar: "ROLLUP_TIMESTAMP_REFRESH",
 	}
+	RollupSyncTypeFlag = cli.StringFlag{
+		Name:   "rollup.synctype",
+		Usage:  "Transaction sync source",


Suggested change

Usage: "Transaction sync source",

Usage: "Transaction sync source (\"batched\" or \"sequenced\")",

l2geth/rollup/sync_service.go

gakonst · 2021-04-20T09:26:25Z

l2geth/rollup/sync_service_test.go

-		t.Fatal("verification failed", err)
-	}
-
+	go service.syncTransactionsToTip(SyncTypeSequenced)


Same as above comment on goroutine

gakonst · 2021-04-20T09:27:15Z

l2geth/rollup/sync_service_test.go

-	// Run an iteration of the eloop
-	err = service.sequence()
-	if err != nil {
-		t.Fatal("sequencing failed", err)
-	}
+	go service.syncQueueToTip()


Why was this changed? IMO it is super error prone to use go-routines which we don't know how many iterations they'll run for inside tests. Better to explicitly run a single iteration (or N iterations for N transactions) inside.

syncQueueToTip does less things than sequence and calling it once is a single iteration. This starts a goroutine that runs until syncQueueToTip returns. This was added because if it does not run in a goroutine, it blocks due to the addition of txCh and waiting explicitly for transactions to be added to the blockchain after processing them

l2geth/rollup/sync_service.go

tynes · 2021-04-20T15:37:56Z

l2geth/rollup/sync_service.go

 	txs := types.Transactions{tx}
 	s.txFeed.Send(core.NewTxsEvent{Txs: txs})
+	// Block until the transaction has been added to the chain
+	log.Debug("Waiting for transaction to be added to chain", "hash", tx.Hash().Hex())
+	<-s.chainHeadCh


@gakonst The chainHeadCh is used here. This blocks until the the tx is added to the chain. Without this, events are sent over the txFeed much faster than they can be processed and things start breaking due to the combination of Clique and the miner being used

tynes · 2021-04-22T22:02:12Z

Closed in favor of #552

geth: implement sync service spec

8599671

tynes mentioned this pull request Apr 16, 2021

dtl: add query param based querying for txs #479

Merged

tynes commented Apr 16, 2021

View reviewed changes

l2geth/rollup/sync_service.go Show resolved Hide resolved

tynes commented Apr 16, 2021

View reviewed changes

l2geth/internal/ethapi/api.go Show resolved Hide resolved

tynes commented Apr 16, 2021

View reviewed changes

l2geth/rollup/client.go Outdated Show resolved Hide resolved

tynes added 4 commits April 16, 2021 16:37

geth: clean up syncservice refactor

488c216

geth: remove dead comment

79ceb3d

geth: remove unneeded diff

376a378

geth: fix typos

ecd01fa

tynes marked this pull request as ready for review April 17, 2021 00:03

tynes requested review from karlfloersch and smartcontracts as code owners April 17, 2021 00:03

geth: add some comments

24b102a

tynes self-assigned this Apr 17, 2021

tynes added A-geth C-feature Category: features labels Apr 17, 2021

tynes added 4 commits April 16, 2021 20:27

syncservice: modularize update gas price

de9515f

syncservice: fix broken tests

fd48c5c

syncservice: update test

29701bf

syncservice: add goroutine to test

6a373d6

tynes mentioned this pull request Apr 19, 2021

optimization: add back in the abi caching #492

Merged

tynes force-pushed the geth/safer-syncing branch 2 times, most recently from e9a2245 to d743630 Compare April 19, 2021 18:09

changeset: minor

d743630

gakonst suggested changes Apr 20, 2021

View reviewed changes

tynes commented Apr 20, 2021

View reviewed changes

tynes mentioned this pull request Apr 20, 2021

l2geth: rollup client batch api #516

Merged

tynes closed this Apr 22, 2021

tynes deleted the geth/safer-syncing branch April 22, 2021 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

geth: implement sync service spec #477

geth: implement sync service spec #477

tynes commented Apr 16, 2021 •

edited

Loading

changeset-bot bot commented Apr 16, 2021 •

edited

Loading

gakonst commented Apr 18, 2021

tynes commented Apr 19, 2021

gakonst left a comment

gakonst Apr 18, 2021

tynes Apr 20, 2021

gakonst Apr 20, 2021

gakonst Apr 20, 2021

gakonst Apr 20, 2021

tynes Apr 20, 2021

tynes Apr 20, 2021

tynes commented Apr 22, 2021

	Usage: "Transaction sync source",
	Usage: "Transaction sync source (\"batched\" or \"sequenced\")",

geth: implement sync service spec #477

geth: implement sync service spec #477

Conversation

tynes commented Apr 16, 2021 • edited Loading

changeset-bot bot commented Apr 16, 2021 • edited Loading

🦋 Changeset detected

gakonst commented Apr 18, 2021

tynes commented Apr 19, 2021

gakonst left a comment

Choose a reason for hiding this comment

gakonst Apr 18, 2021

Choose a reason for hiding this comment

tynes Apr 20, 2021

Choose a reason for hiding this comment

gakonst Apr 20, 2021

Choose a reason for hiding this comment

gakonst Apr 20, 2021

Choose a reason for hiding this comment

gakonst Apr 20, 2021

Choose a reason for hiding this comment

tynes Apr 20, 2021

Choose a reason for hiding this comment

tynes Apr 20, 2021

Choose a reason for hiding this comment

tynes commented Apr 22, 2021

tynes commented Apr 16, 2021 •

edited

Loading

changeset-bot bot commented Apr 16, 2021 •

edited

Loading