l2geth: update timestamp logic by tynes · Pull Request #1982 · ethereum-optimism/optimism

tynes · 2022-01-05T01:33:52Z

Description
This commit updates the timestamp updating logic
such that time.Now is used instead of relying on
L1 timestamps. This gives a higher fidelity for the
TIMESTAMP opcode as well as makes the time on L2
be closer to the time on L1.

L1 to L2 transactions no longer have the property of
having the same timestamp on L2 as the timestamp
of the L1 block they were included in.

This should be able to be turned on without needing
hardfork logic as replicas should always accept the
timestamp that the sequencer sets. The sequencer is
a trusted entity in the existing implementation and
it is expected that the sequencer will become more
trustless in future iterations of the protocol.

This change is added to improve both UX and devex.
Users are confused by the timestamps on Etherscan
being ~15 minutes behind. This is due to the timestamps
being set based on L1 block numbers, and the system
only pulls L1 data once a secure amount of PoW
is placed on top. Developers would like the timestamps
to have a higher fidelity and be closer to the timestamps
on L1.

Metadata

Fixes INT-379
Fixes INT-445

changeset-bot · 2022-01-05T01:37:11Z

🦋 Changeset detected

Latest commit: dad6fd9

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

This PR includes changesets to release 2 packages

Name	Type
@eth-optimism/l2geth	Patch
@eth-optimism/integration-tests	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

codecov-commenter · 2022-01-05T01:45:58Z

Codecov Report

Merging #1982 (77a81b1) into develop (f393872) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1982   +/-   ##
========================================
  Coverage    74.42%   74.42%           
========================================
  Files           79       79           
  Lines         2545     2545           
  Branches       397      397           
========================================
  Hits          1894     1894           
  Misses         651      651

Flag	Coverage Δ
batch-submitter	`62.50% <ø> (ø)`
contracts	`90.48% <ø> (ø)`
core-utils	`57.50% <ø> (ø)`
data-transport-layer	`38.64% <ø> (ø)`
message-relayer	`70.86% <ø> (ø)`
sdk	`87.06% <ø> (ø)`

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f393872...77a81b1. Read the comment docs.

tynes · 2022-01-06T02:27:34Z

This is now ready for review and includes tests. I need to double check two things before this is ready to merge:

This code can run as a replica on mainnet now without a network split
This code can run as a sequencer on a devnet with an older node (pre this PR) can run as a replica without a network split

l2geth/rollup/sync_service_test.go

tynes · 2022-01-06T17:11:46Z

Try to trigger a timestamp inconsistency by spamming a network

smartcontracts · 2022-01-07T21:04:36Z

integration-tests/test/ovmcontext.spec.ts

      expect(l1BlockNumber.toNumber()).to.deep.equal(l1Block.number)

-      // L1 and L2 blocks will have the same timestamp.
+      // L1 and L2 blocks will have approximately the same timestamp.


Can we make the assertion of "strictly greater than" in addition to "approximately equal to"?

Which do you mean should be greater than? - an L1 block can have at highest a timestamp of time.Now() + 15s which could be larger than the time.Now that the sequencer assigns to a transaction. This only applies if we do not trail L1 by any amount - then the configurable amount that we trail L1 by impacts which is larger

I added another test that asserts that the L1 blocknumber and timestamps are ascending, it runs after the stress tests run

l2geth/rollup/sync_service.go

smartcontracts · 2022-01-07T21:07:09Z

l2geth/rollup/sync_service.go

+		log.Error("Could not update execution context", "error", err)
 	}
 }



Just leaving myself a mental note that we'll need to update the documentation to reflect this new behavior.

smartcontracts · 2022-01-07T21:09:19Z

l2geth/rollup/sync_service.go

+		// If enough time has passed, then assign the
+		// transaction to have the timestamp now. Otherwise,
+		// use the current timestamp
+		if now.Sub(current) > s.timestampRefreshThreshold {


What is this threshold set to? How often can we expect timestamps to refresh?

Why even have a threshold at all?

If we are required to have a threshold, it's worth noting that we're using the same threshold for both this logic and for the L1 block number updating logic. If we set the threshold too low we may have monotonicity issues with the L1 block number (wouldn't be good).

There is a threshold here because otherwise we would blow up our batch context overhead, basically every tx would have its own context. The idea would be to set the threshold to 10-15 seconds and then all transactions that happen within that range would get the same timestamp

smartcontracts · 2022-01-07T21:15:42Z

l2geth/rollup/sync_service.go

+		log.Error("No tx timestamp found when running as verifier", "hash", tx.Hash().Hex())
 	} else if tx.L1Timestamp() < s.GetLatestL1Timestamp() {
+		// This should never happen, but sometimes does
 		log.Error("Timestamp monotonicity violation", "hash", tx.Hash().Hex())


Unrelated but since I'm seeing it, do we keep track of these errors @optimisticben?

We do not track these errors, but that is a good idea

l2geth/rollup/sync_service.go

smartcontracts · 2022-01-07T21:17:37Z

l2geth/rollup/sync_service.go

+	}
+
+	// Store the latest timestamp value
+	s.SetLatestL1Timestamp(tx.L1Timestamp())


Are we sure we want to set this in all cases? Because we still get to this point even if tx.L1Timestamp() == 0 && s.verifier or tx.L1Timestamp() < s.GetLatestL1Timestamp() gets triggered. Maybe we only want to set if tx.L1Timestamp() > ts.

So tx.L1Timestamp() == 0 && s.verifier should never happen. The only time when tx.L1Timestamp() == 0 is when a transaction is fresh from a user coming in via RPC

I've noted the above via a comment

Still wondering if we should gate on tx.L1Timestamp() > s.GetLatestL1Timestamp().

l2geth/rollup/sync_service.go

tynes · 2022-01-08T01:45:35Z

l2geth/rollup/sync_service.go

The latest L1 blocknumber logic is now updated, it simply will see if the remote tip is greater than the known latest L1 block number and update it if so

This commit updates the timestamp updating logic such that `time.Now` is used instead of relying on L1 timestamps. This gives a higher fidelity for the `TIMESTAMP` opcode as well as makes the time on L2 be closer to the time on L1. L1 to L2 transactions no longer have the property of having the same timestamp on L2 as the timestamp of the L1 block they were included in. This should be able to be turned on without needing hardfork logic as replicas should always accept the timestamp that the sequencer sets. The sequencer is a trusted entity in the existing implementation and it is expected that the sequencer will become more trustless in future iterations of the protocol. This change is added to improve both UX and devex. Users are confused by the timestamps on Etherscan being ~15 minutes behind. This is due to the timestamps being set based on L1 block numbers, and the system only pulls L1 data once a secure amount of PoW is placed on top. Developers would like the timestamps to have a higher fidelity and be closer to the timestamps on L1.

Update the assertion to not expect that the timestamps are exactly equal but instead assert that the values are within 5%. This is because now the timestamps in L2 no longer correspond to L1 timestamps and may be updated arbitrarily by the sequencer.

smartcontracts

LGTM

0xged · 2022-01-18T21:36:40Z

Any ETA for this release on mainnet ? 😇 🙏

Madalosso · 2022-01-19T13:00:26Z

Any ETA for this release on mainnet ? innocent pray

ETA for kovan as well?

vinta · 2022-01-20T03:34:57Z

@0xged @Madalosso It's already live on Optimism Kovan. As far as I know, it will be live on mainnet on 25th if everything goes well on testnet.

Madalosso · 2022-01-21T12:13:30Z

My bad @vinta I thought this would make the timestamps unique until I saw the comment (#1982 (comment)).
As I saw multiple orders with the same timestamp I thought it wasn't live on kovan yet. My bad.

### Description Sets up the first set of metrics for `kona-derive`. Closes #1981 Closes #1982

github-actions bot added 2-reviewers A-cannon Area: cannon labels Jan 5, 2022

tynes force-pushed the feat/sequencer-timestamp branch 3 times, most recently from 0ef4e81 to 9d30e0e Compare January 6, 2022 01:32

github-actions bot added the A-integration Area: integration tests label Jan 6, 2022

tynes force-pushed the feat/sequencer-timestamp branch from e8e9e8d to 77a81b1 Compare January 6, 2022 02:24

tynes requested a review from smartcontracts January 6, 2022 02:25

tynes commented Jan 6, 2022

View reviewed changes

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

smartcontracts reviewed Jan 7, 2022

View reviewed changes

tynes commented Jan 7, 2022

View reviewed changes

l2geth/rollup/sync_service.go Show resolved Hide resolved

tynes force-pushed the feat/sequencer-timestamp branch 2 times, most recently from e65889b to 7c27306 Compare January 8, 2022 01:37

tynes commented Jan 8, 2022

View reviewed changes

tynes force-pushed the feat/sequencer-timestamp branch from 7c27306 to fb59ed4 Compare January 8, 2022 02:28

tynes added 4 commits January 10, 2022 12:39

chore: add changeset

3a77bbc

chore: add changeset for integration-tests

dad6fd9

tynes force-pushed the feat/sequencer-timestamp branch from fb59ed4 to dad6fd9 Compare January 10, 2022 20:40

smartcontracts approved these changes Jan 10, 2022

View reviewed changes

tynes merged commit 89eab8f into develop Jan 10, 2022

tynes deleted the feat/sequencer-timestamp branch January 10, 2022 21:11

theochap pushed a commit that referenced this pull request Dec 10, 2025

feat(derive): Initial Metrics (#2007)

52b8e0e

### Description Sets up the first set of metrics for `kona-derive`. Closes #1981 Closes #1982

theochap pushed a commit that referenced this pull request Jan 14, 2026

feat(derive): Initial Metrics (op-rs/kona#2007)

e92d286

### Description Sets up the first set of metrics for `kona-derive`. Closes #1981 Closes #1982

Conversation

tynes commented Jan 5, 2022 • edited by snario Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codecov-commenter commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

tynes commented Jan 6, 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!

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!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smartcontracts left a comment

Choose a reason for hiding this comment

Uh oh!

0xged commented Jan 18, 2022

Uh oh!

Madalosso commented Jan 19, 2022

Uh oh!

vinta commented Jan 20, 2022

Uh oh!

Madalosso commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tynes commented Jan 5, 2022 •

edited by snario

Loading

changeset-bot bot commented Jan 5, 2022 •

edited

Loading

codecov-commenter commented Jan 5, 2022 •

edited

Loading

tynes commented Jan 6, 2022 •

edited

Loading