dtl: add query param based querying for txs #479

tynes · 2021-04-16T02:19:30Z

Description
The DTL currently prioritizes transactions sourced from L2 instead of transactions sourced from L1. This was a quick patch required for sequencer maintenance, the source of truth should generally be L1. This allows the client to specify a query param that says to source the transaction from L1 or from L2

Additional context
Related to #477
Previously query params had no impact on the DTL. The DTL has a backing index for both transactions ingested from the L1 contracts (CTC + SCC) and an index for transactions pulled from a sequencer (eth_getBlockByNumber). The DTL has preferred different data sources over time. Before this PR, the config option showUnconfirmedTransactions being set to true would prefer the L2 sourced transactions but then fall back to L1 sourced transactions. After this PR, the client will specify the data source and then the server will only return that transaction sourced from that datasource.

changeset-bot · 2021-04-16T02:19:32Z

🦋 Changeset detected

Latest commit: e86ee09

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/data-transport-layer	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

tynes · 2021-04-16T02:36:17Z

packages/data-transport-layer/src/services/server/service.ts

@@ -156,8 +156,7 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
   * TODO: Link to our API spec.
   */
  private _registerAllRoutes(): void {
-    // TODO: Maybe add doc-like comments to each of these routes?
-
+    // TODO: this needs a backend argument
    this._registerRoute(
      'get',


@smartcontracts Any thoughts on how to reduce complexity in this endpoint? We need to add the backend query param to this so that it can determine whether or not its sync'd L1 or L2. Ideally we refactor the db handlers now that we are moving away from confirmed/not confirmed as concepts in this codebase

is backend in this case defaultSource or a different concept?

Based on the code, I believe it's the same thing @annieke. @tynes may want to use consistent language and call it backend everywhere.

@tynes what do you think is too complex here? This looks OK to me.

what do you think is too complex here? This looks OK to me.

I'm talking about the crazy if/else nesting
https://github.com/ethereum-optimism/optimism/pull/479/files#diff-29a43f33722b902c31ee7224a372262d2b73f170ed0af5be6aca1b54aa505a72L183-L211

packages/data-transport-layer/src/services/main/service.ts

annieke · 2021-04-19T23:41:39Z

packages/data-transport-layer/src/services/run.ts

      sentryDsn: config.str('sentryDsn'),
+      defaultSource: config.str('defaultSource', 'l1'),


do you mean to make the default 'batched' here instead of 'l1'?

Good catch, this is the bug that is causing CI to fail

annieke · 2021-04-19T23:42:39Z

packages/data-transport-layer/src/services/server/service.ts

+  defaultSource: {
+    default: 'batched',
+    validate: (val: string) => {
+      return val === 'batched' || val === 'sequenced'


will we have more sources than batched or sequenced in the future? assuming no since it seems like they directly correspond to l1 and l2 right?

if we anticipate adding more sources

['batched', 'sequenced'].includes(val)

They don't necessarily correspond to L1 and L2 per say, really the abstraction is batched or batched + pre-batched. The batched transactions are currently batched to Ethereum L1 but will be batched to an Eth2 data shard in the future.

Working on unifying the language between the different codebases.

['batched', 'sequenced'].includes(val)

We only have 2 sources right now so I think its fine. I prefer writing JS using a subset of the language that is as C like as possible with Python sprinkled in, see https://github.com/handshake-org/hsd/blob/master/lib/blockchain/chain.js as an example

annieke · 2021-04-19T23:44:56Z

packages/data-transport-layer/src/services/server/service.ts

@@ -156,8 +156,7 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
   * TODO: Link to our API spec.
   */
  private _registerAllRoutes(): void {
-    // TODO: Maybe add doc-like comments to each of these routes?
-
+    // TODO: this needs a backend argument
    this._registerRoute(
      'get',


is backend in this case defaultSource or a different concept?

annieke

lgtm!

gakonst · 2021-04-21T12:54:45Z

.changeset/calm-books-hope.md

+"@eth-optimism/data-transport-layer": patch
+---
+
+Add backwards compatible query params to REST API


Nit: May want to make your message a bit clearer, I do not understand what you changed just by reading this.

Something like: Allow the DTL to provide data from either L1 or L2, configurable via a query param sent by the client.

gakonst · 2021-04-21T12:59:27Z

packages/data-transport-layer/src/services/server/service.ts

@@ -156,8 +156,7 @@ export class L1TransportServer extends BaseService<L1TransportServerOptions> {
   * TODO: Link to our API spec.
   */
  private _registerAllRoutes(): void {
-    // TODO: Maybe add doc-like comments to each of these routes?
-
+    // TODO: this needs a backend argument
    this._registerRoute(
      'get',


Based on the code, I believe it's the same thing @annieke. @tynes may want to use consistent language and call it backend everywhere.

@tynes what do you think is too complex here? This looks OK to me.

* dtl: remove stopL2SyncAtHeight config * dtl: implement query param based backend * dtl: remove dead comment * dtl: add changeset * dtl: standardize on backend query param * changeset: update comment

…) (ethereum-optimism#481)

tynes requested a review from smartcontracts April 16, 2021 02:31

tynes commented Apr 16, 2021

View reviewed changes

packages/data-transport-layer/src/services/main/service.ts Outdated Show resolved Hide resolved

tynes mentioned this pull request Apr 16, 2021

geth: implement sync service spec #477

Closed

tynes force-pushed the dtl/query-params branch from 85086b4 to 2bbaa1f Compare April 17, 2021 00:10

tynes added M-dtl C-feature Category: features labels Apr 17, 2021

tynes force-pushed the dtl/query-params branch 3 times, most recently from e2efd16 to a129eeb Compare April 19, 2021 19:23

tynes marked this pull request as ready for review April 19, 2021 19:45

tynes requested a review from annieke as a code owner April 19, 2021 19:45

annieke reviewed Apr 19, 2021

View reviewed changes

tynes added 2 commits April 20, 2021 16:37

dtl: remove stopL2SyncAtHeight config

0491ff0

dtl: implement query param based backend

5f6cf32

tynes force-pushed the dtl/query-params branch from a129eeb to 5f6cf32 Compare April 20, 2021 23:38

tynes added 2 commits April 20, 2021 16:39

dtl: remove dead comment

79df951

dtl: add changeset

f5b8763

gakonst requested a review from annieke April 21, 2021 12:54

annieke approved these changes Apr 21, 2021

View reviewed changes

gakonst approved these changes Apr 21, 2021

View reviewed changes

tynes added 2 commits April 21, 2021 11:45

dtl: standardize on backend query param

2e92209

changeset: update comment

e86ee09

gakonst merged commit 27f32ca into master Apr 21, 2021

gakonst deleted the dtl/query-params branch April 21, 2021 20:01

bap2pecs added a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

fix: data race in LocalEOTSManager (ethereum-optimism#479)

578e635

bap2pecs added a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

(cherry-pick) fix: data race in LocalEOTSManager (ethereum-optimism#479…

151e507

…) (ethereum-optimism#481)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dtl: add query param based querying for txs #479

dtl: add query param based querying for txs #479

tynes commented Apr 16, 2021 •

edited

Loading

changeset-bot bot commented Apr 16, 2021 •

edited

Loading

tynes Apr 16, 2021 •

edited

Loading

annieke Apr 19, 2021

gakonst Apr 21, 2021

tynes Apr 21, 2021

annieke Apr 19, 2021

tynes Apr 20, 2021

annieke Apr 19, 2021

annieke Apr 19, 2021

tynes Apr 20, 2021

annieke Apr 19, 2021

annieke left a comment

gakonst Apr 21, 2021

gakonst Apr 21, 2021

		sentryDsn: config.str('sentryDsn'),
		defaultSource: config.str('defaultSource', 'l1'),

dtl: add query param based querying for txs #479

dtl: add query param based querying for txs #479

Conversation

tynes commented Apr 16, 2021 • edited Loading

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

🦋 Changeset detected

tynes Apr 16, 2021 • 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

annieke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes commented Apr 16, 2021 •

edited

Loading

changeset-bot bot commented Apr 16, 2021 •

edited

Loading

tynes Apr 16, 2021 •

edited

Loading