Skip to content

op-node: P2P req-resp alt sync method support#5180

Merged
mergify[bot] merged 4 commits intodevelopfrom
p2p-alt-sync
Apr 3, 2023
Merged

op-node: P2P req-resp alt sync method support#5180
mergify[bot] merged 4 commits intodevelopfrom
p2p-alt-sync

Conversation

@protolambda
Copy link
Contributor

@protolambda protolambda commented Mar 16, 2023

Description

Experimental feature: P2P Req-resp based L2 block syncing, to ensure the tip of the L2 chain is more reliably synced when data is slow to confirm in L1 or L2 gossip is missed.

This PR implements parallel block-fetching, a small quarantine for the results, and a promotion-process to get verify things by parent-hash against blocks with established trust (e.g. parent blocks of requested sync targets, or parent blocks of previously promoted blocks). This is effectively a reverse-header-sync implementation that can balances the work between peers.

Changes:

  • Change request-range interface to use block refs, or zeroed end block ref for open range.
  • Implements p2p/sync.go, including:
    • a sync client that implements the alt-sync interface to fetch blocks by range
    • a sync server that serves the req-resp protocol
    • rate-limiting across server and client
  • Defines this sync req-resp protocol in the p2p-interface specs.
  • Adds a feature-flag to enable it.

This functionality is off by default, since production-use relies on peer-scoring / banning to make sure the new p2p surface cannot be misused (aside from the rate-limits that are already there), and test-running this with a feature-flag on testnets will help harden it too.

Tests

  • Op-e2e test that syncs a new verifier node that joins a bit later, while L1 batch-submission is disabled and gossip is missed by this node
  • Server<>Client P2P Sync unit test
  • Sync test with multiple peers and a temporary gap in available payloads

Metadata

Fix CLI-3644

@changeset-bot
Copy link

changeset-bot bot commented Mar 16, 2023

⚠️ No Changeset found

Latest commit: dee6046

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@protolambda protolambda force-pushed the alt-sync-improvements branch from 009bf1f to 34fec18 Compare March 16, 2023 23:26
@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2023

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify
Copy link
Contributor

mergify bot commented Mar 20, 2023

Hey @protolambda! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 20, 2023
Base automatically changed from alt-sync-improvements to develop March 22, 2023 20:48
@netlify
Copy link

netlify bot commented Mar 22, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit dee6046
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/642b395f6f41900008a2a88b

@protolambda protolambda force-pushed the p2p-alt-sync branch 8 times, most recently from dab2192 to 72ba688 Compare March 29, 2023 06:04
Signed-off-by: protolambda <proto@protolambda.com>
@protolambda protolambda marked this pull request as ready for review March 29, 2023 06:42
@protolambda protolambda requested a review from a team as a code owner March 29, 2023 06:42
@protolambda protolambda requested review from a team and trianglesphere as code owners March 29, 2023 06:42
@protolambda protolambda requested a review from clabby March 29, 2023 06:42
@sebastianst
Copy link
Member

Does this also fix CLI-3117?

@protolambda
Copy link
Contributor Author

@sebastianst no it does not. That issue also covers other LRU caches that we already have, which are still using v1. For the new things I introduce here I figured I should adopt v2 from the start. I added more details to the issue to point to the v1 usages.

@protolambda protolambda changed the title op-node: P2P req-resp alt sync method support (work in progress) op-node: P2P req-resp alt sync method support Mar 29, 2023
@semgrep-app
Copy link
Contributor

semgrep-app bot commented Mar 31, 2023

Semgrep found 1 unchecked-type-assertion finding:

  • op-bindings/bindings/systemconfig.go: L380

Unchecked type assertion.

Created by unchecked-type-assertion.

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #5180 (dee6046) into develop (025e157) will decrease coverage by 2.98%.
The diff coverage is 54.11%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5180      +/-   ##
===========================================
- Coverage    39.32%   36.34%   -2.98%     
===========================================
  Files          389      235     -154     
  Lines        25048    21072    -3976     
  Branches       838        0     -838     
===========================================
- Hits          9849     7658    -2191     
+ Misses       14461    12674    -1787     
- Partials       738      740       +2     
Flag Coverage Δ
bedrock-go-tests 36.34% <54.11%> (+0.75%) ⬆️
common-ts-tests ?
contracts-bedrock-tests ?
contracts-tests ?
core-utils-tests ?
dtl-tests ?
fault-detector-tests ?
sdk-tests ?

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

Impacted Files Coverage Δ
op-node/flags/p2p_flags.go 100.00% <ø> (ø)
op-node/node/node.go 0.00% <0.00%> (ø)
op-node/p2p/cli/load_config.go 0.00% <0.00%> (ø)
op-node/p2p/prepared.go 0.00% <0.00%> (ø)
op-node/rollup/derive/engine_queue.go 44.32% <0.00%> (+0.39%) ⬆️
op-node/rollup/derive/pipeline.go 0.00% <0.00%> (ø)
op-node/rollup/driver/driver.go 0.00% <ø> (ø)
op-node/rollup/driver/state.go 0.00% <0.00%> (ø)
op-node/sources/sync_client.go 0.00% <0.00%> (ø)
op-node/metrics/metrics.go 2.29% <5.35%> (+0.97%) ⬆️
... and 4 more

... and 159 files with indirect coverage changes

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Nice work

@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Apr 3, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants