Refactor the SyncService to more closely implement the specification #552

tynes · 2021-04-22T01:02:47Z

Description
Replaces #477

Description
Implements the SyncService spec
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)
Ended up adding an additional db index for the latest BatchIndex because that was more simple and less bug prone

Adds a new config option:

ROLLUP_BACKEND - it must be set to L1 or L2. This sets the query param that is used by the RollupClient when querying the DTL

Additional context
#477 was broken up and this is the PR that includes most of the logic around the SyncService refactor

changeset-bot · 2021-04-22T01:02:49Z

🦋 Changeset detected

Latest commit: 112f0e8

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	Patch

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

gakonst

I've rebased this to master and resolved the conflicts.

High level logic looks good - I think we need more unit tests before we merge this, e.g. checks that batches get correctly applied, that the tip gets synced, that the timestamps are correct after a mix of batch/queue transactions becoming available

l2geth/cmd/utils/flags.go

l2geth/rollup/sync_service.go

gakonst · 2021-04-29T11:29:04Z

l2geth/rollup/sync_service.go

+		if err != nil {
+			return fmt.Errorf("Canot get enqueue transaction; %w", err)
+		}
+		if err := s.applyTransaction(tx); err != nil {


Should there be any validation on the transaction here, similar to the ValidateAndApplySequencerTransaction?

The enqueue transactions are validated by the L1 contracts so we can assume that they have already been validated by this point

Got it, let's add this as a comment for clarity. Something like "We do not need to do validation checks here because they've already been done in the CTC, link to code"

l2geth/rollup/types.go

gakonst · 2021-04-29T11:35:58Z

l2geth/rollup/sync_service.go

+		s.SetLatestL1Timestamp(ts)
+		s.SetLatestL1BlockNumber(bn.Uint64())


Is this desired behavior? Why? Maybe obvious, but should be noted in the comment that there's side effects to the blocknumber and timestamp.

If the incoming transaction's timestamp is greater than the latest timestamp, increase the latest timestamp to the incoming transaction's timestamp. This is to prevent monotonicity errors

l2geth/rollup/sync_service.go

gakonst · 2021-04-29T11:39:54Z

l2geth/rollup/sync_service.go

 	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


Just to clarify: This is the reason transactions go through the full txpool / miner codepath?

They do not go through the txpool but they do go through the miner. This is to block until the transaction sent over the feed is added to the chain. Without this, calling reorganize breaks with an unknown ancestor error. This is due to many transactions sitting in the channel - a reorg would need to empty the channel before reorg'ing if we went with that approach. There may be other complications, for example when shutting down we would need to wait for the channel to fully empty, otherwise transactions may be skipped. This PR does not include the reorganize logic but it needs to be added in the future

* l2geth: fix get last confirmed enqueue * chore: add changeset * client: return error correctly

* batch-submitter: backwards compatible configuration * chore: add changeset * deps: update * js: move bcfg interface to core-utils * batch-submitter: parse USE_SENTRY and add to env example * chore: add changeset * batch-submitter: parse as float instead of int * batch-submitter: better error logging

Co-authored-by: Georgios Konstantopoulos <[email protected]>

l2geth/cmd/geth/main.go

l2geth/cmd/utils/flags.go

smartcontracts · 2021-05-13T22:54:16Z

l2geth/core/rawdb/rollup_indexes.go

@@ -69,3 +69,24 @@ func WriteHeadVerifiedIndex(db ethdb.KeyValueWriter, index uint64) {
 		log.Crit("Failed to store verifier index", "err", err)
 	}
 }
+
+// ReadHeadBatchIndex will read the known tip of the processed batches
+func ReadHeadBatchIndex(db ethdb.KeyValueReader) *uint64 {


The name (Read/Write)HeadBatchIndex is a bit confusing, might consider updating it.

{Read/Write}HeadTransactionBatchIndex ?
Open to a suggestion

l2geth/core/rawdb/rollup_indexes.go

l2geth/rollup/sync_service.go

smartcontracts · 2021-05-13T23:13:49Z

l2geth/rollup/sync_service.go

+		// If the timestamp of the transaction is greater than the sync
+		// service's locally maintained timestamp, update the timestamp and
+		// blocknumber to equal that of the transaction's. This should happen
+		// with `enqueue` transactions.


You say that this should happen with enqueue transactions. Should we add a sanity check that the tx.QueueOrigin().Uint64() == uint64(types.QueueOriginL1ToL2) in this case then? Or is it guaranteed that tx.L1Timestamp() != 0 only in the case that the transaction is an enqueue?

It is guaranteed that tx.L1Timestamp() == 0 only for types.QueueOriginSequencer transactions, assuming no bugs in the upstream software

smartcontracts · 2021-05-13T23:19:53Z

l2geth/rollup/sync_service.go

+	if tx.GetMeta().Index == nil {
+		index := s.GetLatestIndex()
+		if index == nil {
+			tx.SetIndex(0)


TBH I'm somewhat averse to having the client mutate transactions directly like this. It feels prone to error. For instance, verifiers should never be mutating transactions, right? Could that be an invariant that we try to enforce?

The transaction index must be set by the SyncService (both sequencer and verifier) as the index is dependent on the current blockchain. It is set and then indexed

smartcontracts · 2021-05-13T23:21:09Z

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.Trace("Waiting for transaction to be added to chain", "hash", tx.Hash().Hex())
+	<-s.chainHeadCh


Will this cause sendTransaction RPC requests to hang until all previous transactions have been fully executed and included in the chain?

Good question - I believe it will, which is not ideal at all but we need to implement a queue asap that transactions are added to as they come in. We will currently block once the txFeed channel is full and on shutdown, all of the txs are lost that are waiting to get into the channel or are in the channel. Short term we need a queue that txs are added to from the RPC handler and the tx hash is returned to the user and then the sync service pulls from that queue continuously. Our longer term plan is to have a single database be the source of truth where transactions are indexed and pulled in one by one.

Right now there isn't a good way to get around having this synchronization mechanism if we want to do reorgs and I do have a follow up PR that removes this by removing the miner from the hot code path and using a custom consensus.Engine

I'm OK with this change (removed here) as long as we include the miner change ASAP.

gakonst · 2021-05-17T12:23:01Z

l2geth/rollup/sync_service.go

+	// the L1 block that holds the transaction. This should never happen but is
+	// a sanity check to prevent fraudulent execution.


Should the be a panic if it should never happen?

We cannot panic here - it would be ideal to but we need a db healer after it panics

l2geth/core/rawdb/rollup_indexes.go

gakonst

LGTM. Next steps to make this code better are to 1) remove the miner codepath (#875) and introduce the sequencer FIFO queue (like a mempool for the sequencer txs), in separate PRs

snario · 2021-05-26T16:25:53Z

🥳🥳🥳🥳

tynes added the P-DO-NOT-MERGE label Apr 22, 2021

tynes force-pushed the l2geth/sync-spec branch from 61e4190 to 895cb81 Compare April 22, 2021 01:05

tynes added A-geth C-feature Category: features and removed P-DO-NOT-MERGE labels Apr 22, 2021

tynes marked this pull request as ready for review April 22, 2021 21:53

tynes requested review from karlfloersch and smartcontracts as code owners April 22, 2021 21:53

tynes changed the title ~~WIP: implement syncservice spec~~ geth: implement sync service spec Apr 22, 2021

tynes commented Apr 22, 2021

View reviewed changes

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

tynes mentioned this pull request Apr 22, 2021

geth: implement sync service spec #477

Closed

tynes added the P-high label Apr 23, 2021

snario removed the P-high label Apr 26, 2021

gakonst self-assigned this Apr 28, 2021

tynes added 10 commits April 29, 2021 13:06

l2geth: add Backend enums and config parsing

656cbe3

l2geth: move OVMContext to types file

97fd394

l2geth: implement syncservice spec

60f3a2e

l2geth: fix error handling for get tx batch

d8359ce

l2geth: update tests to compile and pass

87742d9

l2geth: add sync range functions

82aea42

l2geth: add batch index indexing

c0c3561

l2geth: update syncservice hot path logging

6aba901

l2geth: use indexed batch index

c902766

chore: add changeset

570b79a

gakonst force-pushed the l2geth/sync-spec branch from 2480b2d to 570b79a Compare April 29, 2021 11:17

gakonst suggested changes Apr 29, 2021

View reviewed changes

gakonst assigned tynes and unassigned gakonst Apr 29, 2021

snario changed the title ~~geth: implement sync service spec~~ Refactor the SyncService to more closely implement the specification Apr 29, 2021

github-actions bot added the M-dtl label May 13, 2021

tynes and others added 6 commits May 13, 2021 13:49

fix: get last confirmed enqueue (#846)

cac3daf

* l2geth: fix get last confirmed enqueue * chore: add changeset * client: return error correctly

l2geth: update rawdb logline

9d91c2c

Co-authored-by: Georgios Konstantopoulos <[email protected]>

l2geth: more robust testing

1b83b4b

Co-authored-by: Georgios Konstantopoulos <[email protected]>

l2geth: add sanity check for L1ToL2 timestamps

9ada2d0

l2geth: handle error in single place

4c9ceb7

github-actions bot removed A-op-batcher Area: op-batcher A-ts-packages A-pkg-core-utils Area: packages/core-utils labels May 13, 2021

Merge branch 'develop' into l2geth/sync-spec

1f86620

smartcontracts reviewed May 13, 2021

View reviewed changes

tynes added 3 commits May 13, 2021 17:15

l2geth: fix test tx queue origin

435e25d

l2geth: add new arg to start.sh

f7d18c1

l2geth: return error in the SyncService.Start()

c38ec87

tynes mentioned this pull request May 14, 2021

l2geth: remove the miner from the hot code path #875

Closed

gakonst reviewed May 17, 2021

View reviewed changes

tynes mentioned this pull request May 19, 2021

feat[integration-tests]: add verifier sync test #913

Merged

Merge branch 'develop' into l2geth/sync-spec

ae02108

github-actions bot added the 2-reviewers label May 26, 2021

chore: go fmt

112f0e8

gakonst approved these changes May 26, 2021

View reviewed changes

snario approved these changes May 26, 2021

View reviewed changes

tynes merged commit a25acbb into develop May 26, 2021

tynes deleted the l2geth/sync-spec branch May 26, 2021 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the SyncService to more closely implement the specification #552

Refactor the SyncService to more closely implement the specification #552

tynes commented Apr 22, 2021 •

edited

Loading

changeset-bot bot commented Apr 22, 2021 •

edited

Loading

gakonst left a comment •

edited

Loading

gakonst Apr 29, 2021

tynes Apr 30, 2021

gakonst May 13, 2021

gakonst Apr 29, 2021

tynes Apr 30, 2021

gakonst Apr 29, 2021

tynes Apr 30, 2021

smartcontracts May 13, 2021

tynes May 14, 2021

smartcontracts May 13, 2021

tynes May 13, 2021

smartcontracts May 13, 2021

tynes May 13, 2021

smartcontracts May 13, 2021

tynes May 14, 2021

gakonst May 17, 2021

gakonst May 17, 2021

tynes May 24, 2021

gakonst left a comment •

edited

Loading

snario commented May 26, 2021

		s.SetLatestL1Timestamp(ts)
		s.SetLatestL1BlockNumber(bn.Uint64())

		// the L1 block that holds the transaction. This should never happen but is
		// a sanity check to prevent fraudulent execution.

Refactor the SyncService to more closely implement the specification #552

Refactor the SyncService to more closely implement the specification #552

Conversation

tynes commented Apr 22, 2021 • edited Loading

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

🦋 Changeset detected

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

snario commented May 26, 2021

tynes commented Apr 22, 2021 •

edited

Loading

changeset-bot bot commented Apr 22, 2021 •

edited

Loading

gakonst left a comment •

edited

Loading

gakonst left a comment •

edited

Loading