feat: L1 state manager(reduce bridge latency milestone) #1018
Conversation
Conflicts: cmd/geth/main.go core/state_processor_test.go core/txpool/legacypool/legacypool.go eth/backend.go eth/ethconfig/config.go eth/gasprice/gasprice_test.go eth/handler.go eth/protocols/eth/broadcast.go eth/protocols/eth/handlers.go go.mod go.sum miner/miner.go miner/miner_test.go miner/scroll_worker.go miner/scroll_worker_test.go params/config.go params/version.go rollup/rollup_sync_service/rollup_sync_service_test.go
|
Semgrep found 6
Risk: Affected versions of golang.org/x/net, golang.org/x/net/http2, and net/http are vulnerable to Uncontrolled Resource Consumption. An attacker may cause an HTTP/2 endpoint to read arbitrary amounts of header data by sending an excessive number of CONTINUATION frames. Fix: Upgrade this library to at least version 0.23.0 at go-ethereum/go.mod:144. Reference(s): GHSA-4v7x-pqxf-cx7m, CVE-2023-45288 Ignore this finding from ssc-46663897-ab0c-04dc-126b-07fe2ce42fb2. |
rollup/l1_state_tracker/l1_reader.go
Outdated
| } | ||
|
|
||
| // FetchRollupEventsInRange retrieves and parses commit/revert/finalize rollup events between block numbers: [from, to]. | ||
| func (r *L1Reader) FetchRollupEventsInRange(from, to uint64) ([]types.Log, error) { |
There was a problem hiding this comment.
This is probably going to be the main function of the L1Reader.
- it should perform queries in batches (like rollup verifier does iirc)
L1Readershould be as easy to use as possible. That means no handling oftypes.Log. It would be much nicer if all that stuff is done within this method. Specifically, I think we should do this in here and return instances ofRollupEventinterface or something. Each of these should be identifiable by type and then be able to used as such. There are already events here which we can repurpose and extend:
| return | ||
| case <-syncTicker.C: | ||
| err := t.syncLatestHead() | ||
| if err != nil { |
There was a problem hiding this comment.
Here we should probably retry with exponential backoff or something (see syncing pipeline) instead of just skipping and waiting for the next tick.
jonastheis
force-pushed
the
feat/l1-state-tracker
branch
from
95027f5
to
27c5d63
Compare
rollup/l1/tracker.go
Outdated
| if reorg && sub.lastSentHeader != nil { | ||
| // The subscriber is subscribed to a deeper ConfirmationRule than the reorg depth -> this reorg doesn't affect the subscriber. | ||
| // Since the subscribers are sorted by ConfirmationRule, we can return here. | ||
| if sub.lastSentHeader.Number.Uint64() < headerToNotify.Number.Uint64() { |
There was a problem hiding this comment.
reorg doesn't affect subscriber but new header still should be sent I think
There was a problem hiding this comment.
I don't understand what you mean here.
Let's assume the reorg depth is 3 but the subscriber is only interested in 5 confirmations and the tip of L1 is at 103.
Then this reorg at block 100 doesn't affect the subscriber nor do we need to notify again for block 95 as we already did that when we first processed block 100 (the one that got reorged).
The test should cover all those scenarios in detail as it is indeed quite tricky
There was a problem hiding this comment.
Let's assume that we called notifyLatest with newHeader on height 103 and reorg set to true. Then for subscriber with depth 5 headerToNotifyNumber will be 98. And even if last send number for that subscriber was less then 98 and reorg is true, we can inform subscriber that there is a new block on depth 5. Reorg doesn't affect this subscriber but there is still new block for that subscriber
There was a problem hiding this comment.
You are right. I'll need to implement the reorg handling differently.
|
|
||
| // NewReader initializes a new Reader instance | ||
| func NewReader(ctx context.Context, config Config, l1Client Client) (*Reader, error) { | ||
| if config.ScrollChainAddress == (common.Address{}) { |
There was a problem hiding this comment.
should also check for L1MessageQueueAddress
jonastheis
force-pushed
the
feat/l1-state-tracker
branch
from
cd0eb14
to
79842f8
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
1. Purpose or design rationale of this PR
task page
milestone link
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?