Skip to content

supervisor: L1 Finality Processor#13274

Merged
protolambda merged 4 commits intodevelopfrom
supervisor-l1-finality-processor
Dec 11, 2024
Merged

supervisor: L1 Finality Processor#13274
protolambda merged 4 commits intodevelopfrom
supervisor-l1-finality-processor

Conversation

@axelKingsley
Copy link
Copy Markdown
Contributor

@axelKingsley axelKingsley commented Dec 5, 2024

What

Extends the L1 Processor from #13206 to include subscription and management of the L1 Finality Signal

Why

We would like for the Supervisor to control the canonical L1 source for "Owned Nodes" in order to have better consistency and control over the data in the system.

How

Introduces:

  • Eth Subscription to finality
  • Update of L1 Finality to the ChainsDB on handle
  • Stopping L1 Finality updates from OP Nodes

Testing

  • Existing tests and checks.
  • New unit tests for basic update/don't-update behavior of the handler
  • Additionally Tested by running the existing E2E tests with a long wait-time and observing the logs:
    l1_processor.go:109:        WARN [12-05|15:47:50.851] Failed to process L1                     role=supervisor service=l1-processor err="failed to fetch header by num 11: not found"
    l1_processor.go:106:        DEBUG[12-05|15:47:56.838] Checking for new L1 block                role=supervisor service=l1-processor current=10
    l1_processor.go:147:        DEBUG[12-05|15:47:56.838] Received new Finalized L1 block          role=supervisor service=l1-processor block=81e1e0..d7c67e:8
    l1_processor.go:160:        DEBUG[12-05|15:47:56.838] Updated finalized L1 block               role=supervisor service=l1-processor block=81e1e0..d7c67e:8
    l1_processor.go:128:        DEBUG[12-05|15:47:56.838] Processing new L1 block                  role=supervisor service=l1-processor block=8dfb55..df45d5:11
    l1_processor.go:106:        DEBUG[12-05|15:47:56.850] Checking for new L1 block                role=supervisor service=l1-processor current=11
    l1_processor.go:109:        WARN [12-05|15:47:56.850] Failed to process L1                     role=supervisor service=l1-processor err="failed to fetch header by num 12: not found"
    l1_processor.go:147:        DEBUG[12-05|15:47:56.850] Received new Finalized L1 block          role=supervisor service=l1-processor block=81e1e0..d7c67e:8
    l1_processor.go:106:        DEBUG[12-05|15:48:02.838] Checking for new L1 block                role=supervisor service=l1-processor current=11
    l1_processor.go:147:        DEBUG[12-05|15:48:02.839] Received new Finalized L1 block          role=supervisor service=l1-processor block=5d5aa2..305c6f:9
    l1_processor.go:160:        DEBUG[12-05|15:48:02.839] Updated finalized L1 block               role=supervisor service=l1-processor block=5d5aa2..305c6f:9

When the OP Nodes were still contributing their L1 Finalized, this function was very rarely able to get an update it. It would discover one, and then find it already set in the database.

@axelKingsley axelKingsley marked this pull request as ready for review December 5, 2024 22:00
@axelKingsley axelKingsley requested review from a team as code owners December 5, 2024 22:00
@axelKingsley axelKingsley changed the base branch from develop to supervisor-l1-processor December 5, 2024 22:04
@axelKingsley axelKingsley changed the title Supervisor l1 finality processor supervisor: L1 Finality Processor Dec 5, 2024
@axelKingsley
Copy link
Copy Markdown
Contributor Author

did a brief review with @protolambda synchronously today:

  • To fix the action tests, directly leverage the L1 Processor's handleFinality function (expose it)

Base automatically changed from supervisor-l1-processor to develop December 9, 2024 15:26
@protolambda protolambda added this to the Interop: Stable Devnet milestone Dec 9, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 42.10526% with 33 lines in your changes missing coverage. Please review.

Project coverage is 42.95%. Comparing base (eabf704) to head (1b8e22c).
Report is 14 commits behind head on develop.

Files with missing lines Patch % Lines
...isor/supervisor/backend/processors/l1_processor.go 55.00% 17 Missing and 1 partial ⚠️
op-service/sources/supervisor_client.go 0.00% 7 Missing ⚠️
op-supervisor/supervisor/backend/backend.go 0.00% 4 Missing ⚠️
op-supervisor/supervisor/backend/db/query.go 0.00% 2 Missing ⚠️
op-supervisor/supervisor/backend/mock.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13274      +/-   ##
===========================================
- Coverage    47.45%   42.95%   -4.50%     
===========================================
  Files          931      762     -169     
  Lines        78188    68462    -9726     
  Branches       849        0     -849     
===========================================
- Hits         37103    29408    -7695     
+ Misses       38361    36554    -1807     
+ Partials      2724     2500     -224     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

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

Files with missing lines Coverage Δ
op-node/rollup/interop/interop.go 56.17% <ø> (+0.20%) ⬆️
op-service/testutils/mock_interop_backend.go 62.74% <ø> (-7.07%) ⬇️
op-supervisor/supervisor/frontend/frontend.go 100.00% <100.00%> (ø)
op-supervisor/supervisor/backend/db/query.go 0.00% <0.00%> (ø)
op-supervisor/supervisor/backend/mock.go 0.00% <0.00%> (ø)
op-supervisor/supervisor/backend/backend.go 35.59% <0.00%> (-0.21%) ⬇️
op-service/sources/supervisor_client.go 0.00% <0.00%> (ø)
...isor/supervisor/backend/processors/l1_processor.go 54.45% <55.00%> (-0.24%) ⬇️

... and 178 files with indirect coverage changes

@axelKingsley axelKingsley force-pushed the supervisor-l1-finality-processor branch from e0adbfe to a47441c Compare December 9, 2024 20:25
@axelKingsley axelKingsley reopened this Dec 9, 2024
@axelKingsley
Copy link
Copy Markdown
Contributor Author

Sorry for the thrash -- I rebased this PR in a way that caused the new commits to become detached from the branch. Then when I pushed it cleared all the changes which auto-closed the PR. I've fixed it and squashed the changes into one commit.

@axelKingsley axelKingsley force-pushed the supervisor-l1-finality-processor branch from 0dcb7bc to 1d54219 Compare December 10, 2024 17:26
@axelKingsley axelKingsley requested a review from a team as a code owner December 10, 2024 17:28
@axelKingsley axelKingsley force-pushed the supervisor-l1-finality-processor branch from 6c07488 to b6a883d Compare December 10, 2024 17:31
@axelKingsley axelKingsley force-pushed the supervisor-l1-finality-processor branch from b6a883d to 43095e3 Compare December 10, 2024 17:43
Copy link
Copy Markdown
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

LGTM, just one type change needed

@protolambda protolambda added this pull request to the merge queue Dec 11, 2024
Merged via the queue into develop with commit 5516508 Dec 11, 2024
@protolambda protolambda deleted the supervisor-l1-finality-processor branch December 11, 2024 23:24
sigma pushed a commit that referenced this pull request Dec 19, 2024
* Add Finality Handling to L1 Processor

* Add E2E Test for Finality Tracking

* Completely remove UpdateFinalizedL1 API

* use BlockRef in Supervisor Client return
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.

2 participants