feat(permissionless batches): batch production toolkit and operator recovery #1555
Conversation
WalkthroughThis change introduces the "permissionless batches" feature for the Scroll rollup system, adding new CLI tools, Docker configurations, orchestration scripts, configuration files, and extensive Go implementations for permissionless batch production, recovery, and submission. It also implements robust recovery controllers, new ORM methods, and updates to public interfaces for improved batch and bundle management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (permissionless-batches)
participant DB
participant L2Watcher
participant ChunkProposer
participant BatchProposer
participant BundleProposer
participant Submitter
participant L1
User->>CLI (permissionless-batches): Run recovery or batch production
CLI (permissionless-batches)->>DB: Connect and initialize
CLI (permissionless-batches)->>L2Watcher: Initialize watcher
CLI (permissionless-batches)->>ChunkProposer: Initialize
CLI (permissionless-batches)->>BatchProposer: Initialize
CLI (permissionless-batches)->>BundleProposer: Initialize
CLI (permissionless-batches)->>DB: Check recovery needed
alt Recovery Needed
CLI (permissionless-batches)->>MinimalRecovery: Run recovery
MinimalRecovery->>DB: Reset and restore state
MinimalRecovery->>L2Watcher: Fetch missing blocks
MinimalRecovery->>ChunkProposer: Propose chunks
MinimalRecovery->>BatchProposer: Propose batch
MinimalRecovery->>BundleProposer: Propose bundle
MinimalRecovery->>DB: Update statuses
else No Recovery Needed
CLI (permissionless-batches)->>Submitter: Submit batch (with/without proof)
Submitter->>DB: Fetch latest bundle/batch
Submitter->>L1: Submit batch/proof
Submitter->>DB: Update statuses
end
CLI (permissionless-batches)->>User: Log status and wait for confirmation/exit
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
can you add more description about this pr |
Will do so once it's in a better shape. This is still a draft PR and and such many things are still changing. Once ready, I'll provide a high-level description + how it relates to the changes made in this PR :) |
… message amount. this is mainly for testing purposes
| @@ -149,6 +149,10 @@ func NewBatchProposer(ctx context.Context, cfg *config.BatchProposerConfig, chai | |||
| return p | |||
| } | |||
|
|
|||
| func (p *BatchProposer) BatchORM() *orm.Batch { | |||
There was a problem hiding this comment.
as our layout purpose, the orm instance should be exposed to app layer.
| return db, nil | ||
| } | ||
|
|
||
| func fetchL2Blocks(ctx context.Context, cfg *config.Config, genesis *core.Genesis, db *gorm.DB, registry prometheus.Registerer, fromBlock uint64, l2BlockHeightLimit uint64) (uint64, error) { |
There was a problem hiding this comment.
I suggest that move these logics to controller layer
|
|
||
| // TODO: make these parameters -> part of genesis config? | ||
| scrollChainAddress := common.HexToAddress("0x2D567EcE699Eabe5afCd141eDB7A4f2D0D6ce8a0") | ||
| l1MessageQueueAddress := common.HexToAddress("0xF0B2293F5D834eAe920c6974D50957A1732de763") |
…erriding and instead switching to batch submission mode
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
permissionless-batches/README.md (1)
77-77:⚠️ Potential issueRemove the stray
--cccflag that will cause l2geth to abort.This flag does not exist in l2geth and will cause the node to abort with an unknown flag error.
--miner.gaslimit 1 --miner.gasprice 1 --miner.maxaccountsnum 100 --rpc.gascap 0 --gpo.ignoreprice 1 \ --miner.etherbase '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' --mine \ ---ccc \ --verbosity 3rollup/internal/controller/permissionless_batches/minimal_recovery.go (1)
116-133: 🛠️ Refactor suggestionAdd safeguards against infinite loop in chunk proposing.
While the exit condition exists, if
ProposeChunk()consistently fails to advanceEndBlockNumber, this could loop indefinitely. Consider adding progress tracking.var latestChunk *orm.Chunk var count int +maxAttempts := 1000 // Reasonable upper bound for { + if count >= maxAttempts { + return fmt.Errorf("exceeded maximum chunk proposal attempts (%d)", maxAttempts) + } + if err = r.chunkProposer.ProposeChunk(); err != nil { return fmt.Errorf("failed to propose chunk: %w", err) } count++rollup/internal/controller/relayer/full_recovery.go (2)
75-82: Handle empty DB scenario before dereferencing.
91-98: Fix undefined method compilation error.
🧹 Nitpick comments (5)
permissionless-batches/README.md (1)
174-177: Add language specification to fenced code blocks.The JSON configuration blocks should specify the language for proper syntax highlighting.
-``` +```json "recovery_config": { "enable": true } -``` +``` ... -``` +```json "recovery_config": { "enable": false } -``` +```Also applies to: 182-186
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
174-174: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
permissionless-batches/docker-compose.yml (2)
106-106: Fix excessive spacing after hyphen in environment section.YAML formatting requires consistent spacing after hyphens.
environment: - - N_WORKERS=1 + - N_WORKERS=1🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 106-106: too many spaces after hyphen
(hyphens)
115-115: Fix indentation and add missing newline at end of file.The volumes section should be indented 2 spaces, and files should end with a newline.
volumes: - db_data: + db_data:Also add a newline at the end of the file.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 115-115: wrong indentation: expected 2 but found 4
(indentation)
[error] 115-115: no new line character at the end of file
(new-line-at-end-of-file)
rollup/internal/controller/permissionless_batches/minimal_recovery.go (2)
258-258: Remove debug print statement.The
fmt.Printlnappears to be leftover debug code that should be removed.- fmt.Println(daBatch, daBlobPayload)
447-466: Document the risks of database reset while other components are active.The
resetDBfunction performs destructive operations that could cause issues if other components are concurrently using the database connection.Add a comment documenting this limitation:
+// resetDB performs a destructive reset of the database schema. +// WARNING: This should only be called when no other components are actively +// using the database connection, as it invalidates existing ORM metadata and connections. func (r *MinimalRecovery) resetDB() error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
permissionless-batches/Makefile(1 hunks)permissionless-batches/README.md(1 hunks)permissionless-batches/conf/coordinator/config.json(1 hunks)permissionless-batches/conf/coordinator/coordinator_run.sh(1 hunks)permissionless-batches/conf/proving-service/cloud-prover/config.json(1 hunks)permissionless-batches/conf/proving-service/local-prover/config.json(1 hunks)permissionless-batches/conf/proving-service/local-prover/prover_run.sh(1 hunks)permissionless-batches/conf/relayer/config.json(1 hunks)permissionless-batches/docker-compose.yml(1 hunks)rollup/abi/bridge_abi_test.go(3 hunks)rollup/cmd/permissionless_batches/app/app.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(2 hunks)rollup/go.mod(1 hunks)rollup/internal/config/l1.go(1 hunks)rollup/internal/config/recovery.go(1 hunks)rollup/internal/controller/permissionless_batches/minimal_recovery.go(1 hunks)rollup/internal/controller/permissionless_batches/submitter.go(1 hunks)rollup/internal/controller/relayer/full_recovery.go(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(4 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(2 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(4 hunks)rollup/internal/orm/chunk.go(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- rollup/go.mod
- rollup/internal/config/l1.go
- rollup/internal/controller/relayer/l2_relayer.go
- permissionless-batches/conf/proving-service/cloud-prover/config.json
- permissionless-batches/conf/proving-service/local-prover/config.json
- permissionless-batches/Makefile
🚧 Files skipped from review as they are similar to previous changes (13)
- rollup/abi/bridge_abi_test.go
- rollup/internal/controller/watcher/bundle_proposer.go
- rollup/internal/controller/watcher/chunk_proposer.go
- rollup/cmd/rollup_relayer/app/app.go
- rollup/internal/orm/bundle.go
- rollup/internal/config/recovery.go
- rollup/internal/controller/watcher/l2_watcher.go
- permissionless-batches/conf/coordinator/config.json
- rollup/internal/orm/chunk.go
- rollup/cmd/permissionless_batches/app/app.go
- permissionless-batches/conf/relayer/config.json
- rollup/internal/orm/batch.go
- rollup/internal/controller/permissionless_batches/submitter.go
🧰 Additional context used
🪛 Shellcheck (0.10.0)
permissionless-batches/conf/proving-service/local-prover/prover_run.sh
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
permissionless-batches/conf/coordinator/coordinator_run.sh
[warning] 68-68: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 103-103: Quote this to prevent word splitting.
(SC2046)
🪛 YAMLlint (1.37.1)
permissionless-batches/docker-compose.yml
[warning] 106-106: too many spaces after hyphen
(hyphens)
[warning] 115-115: wrong indentation: expected 2 but found 4
(indentation)
[error] 115-115: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
permissionless-batches/README.md
[grammar] ~1-~1: This phrase is duplicated. You should probably use “Permissionless Batches” only once.
Context: # Permissionless Batches Permissionless batches aka enforced batches is a feature that ...
(PHRASE_REPETITION)
[duplication] ~14-~14: Possible typo: you repeated a word.
Context: ... state recovery from L1 2. l2geth block production 3. production, proving and submission of batch with `...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...ery from L1 Once permissionless mode is activated there's no blocks being produced and pr...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~26-~26: Did you mean “there are no blocks”?
Context: ...1 Once permissionless mode is activated there's no blocks being produced and propagated on L2. Th...
(THERE_S_MANY)
[style] ~36-~36: To form a complete sentence, be sure to include a subject.
Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https...
(MISSING_IT_THERE)
[typographical] ~55-~55: It seems that a comma is missing.
Context: ... production to work. Once you generated blocks you can safely discard it. Running l2g...
(IF_COMMA)
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...2geth in block production mode requires following configuration: - --scroll or `--scrol...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~86-~86: Possible missing comma found.
Context: .... #### Producing a batch To produce a batch you need to run the `batch-production-s...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~95-~95: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...tes that everything is working correctly and the batch is ready to be proven. #### ...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~98-~98: Possible missing comma found.
Context: ...ve the chunk, batch and bundle you just generated you need to run the local-prover or `...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~102-~102: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... 36+ core, 128G memory GPU: 24G memory (eg. Rtx 3090/3090Ti/4090/A10/L4) 2. Make su...
(E_G)
[uncategorized] ~118-~118: Possible missing comma found.
Context: ...bundle. Run make check_proving_status Success! You're ready to submit permissionless batch and proof! indicates that everything i...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~119-~119: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...tes that everything is working correctly and the batch is ready to be submit. ####...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~119-~119: This verb may not be in the correct form. Consider using a different form for this context.
Context: ... correctly and the batch is ready to be submit. #### Batch submission To submit the ...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~123-~123: Possible missing comma found.
Context: .... #### Batch submission To submit the batch you need to run the `batch-production-s...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~125-~125: You might be missing the article “the” here.
Context: ...le in docker-compose.yml. 1. Fill in required fields in conf/relayer/config.json fo...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~131-~131: A comma might be missing here.
Context: ...ubleshooting** - in case the submission fails it will print the calldata for the tran...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~132-~132: Loose punctuation mark.
Context: ...0x4df567b9: ErrorNotInEnforcedBatchMode`: permissionless batch mode is not activa...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~133-~133: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... call`, permissionless mode is activated but you didn't provide a blob in the transa...
(COMMA_COMPOUND_SENTENCE)
[grammar] ~135-~135: This phrase is duplicated. You should probably use “Operator recovery” only once.
Context: ... provide a blob in the transaction. ## Operator recovery Operator recovery needs to be run by the rollup operator ...
(PHRASE_REPETITION)
[style] ~156-~156: To form a complete sentence, be sure to include a subject.
Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
permissionless-batches/README.md
130-130: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
174-174: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
182-182: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
permissionless-batches/conf/proving-service/local-prover/prover_run.sh
Outdated
Show resolved
Hide resolved
permissionless-batches/conf/proving-service/local-prover/prover_run.sh
Outdated
Show resolved
Hide resolved
| ### Prerequisites | ||
| - Unix-like OS, 32GB RAM | ||
| - Docker | ||
| - [l2geth](https://github.com/scroll-tech/go-ethereum/) or [Docker image](https://hub.docker.com/r/scrolltech/l2geth) of corresponding version [TODO link list with versions](#batch-production-toolkit). |
There was a problem hiding this comment.
A TODO here. There are also some other version-related TODOs in this readme. Should they be addressed, or can they be kept?
There was a problem hiding this comment.
Yes that should be addressed before we merge. We will need to create a page in our docs that contains all the necessary information about network versions and such
Co-authored-by: colin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
permissionless-batches/README.md (2)
76-78: Remove stray--cccflag (repeat issue)
l2gethstill has no--cccoption; keeping it will abort startup.---ccc \
20-24: Resolve TODO placeholders before publishingSeveral bullet points still contain
[TODO …]. Leaving these in a public guide will confuse operators and block automation scripts that scrape the doc for version numbers.
Please replace the placeholders with concrete links/versions or add a note that they will be updated in a follow-up commit.
🧹 Nitpick comments (6)
permissionless-batches/README.md (6)
2-3: Pluralise subject & add missing article“Permissionless batches … is” mixes plural subject with singular verb and omits the article before “guarantee”.
-Permissionless batches aka enforced batches is a feature that provides guarantee to users that they can exit Scroll even if the operator is down or censoring. +Permissionless batches (also known as enforced batches) are a feature that provide a guarantee to users that they can exit Scroll even if the operator is down or censoring.
35-37: Add missing subject to option descriptionBoth option descriptions start with “batch where to start recovery from. Can be found …”, which is an incomplete sentence.
-`--da.recovery.initialbatch` - batch where to start recovery from. Can be found on … +`--da.recovery.initialbatch` – batch from which to start recovery. This value can be found on …Do the same for the duplicated description further down.
Also applies to: 143-145
39-41: Placeholder--scroll<-sepolia>is unclearThe angle-bracket syntax isn’t valid shell and may be copy-pasted verbatim.
Either provide two concrete examples or instruct readers to choose one of--scroll(mainnet) or--scroll-sepolia:-./build/bin/geth --scroll<-sepolia> \ +## Mainnet +./build/bin/geth --scroll \ +# or Sepolia +# ./build/bin/geth --scroll-sepolia \Also applies to: 68-70
118-118: Use a proper heading instead of bold emphasisMarkdown-lint flags
**Troubleshooting**.-**Troubleshooting** +### Troubleshooting
100-106: Fix duplicate list numberingThe section has two “2.” items; Markdown auto-numbers but plaintext readers will mis-parse.
Consider letting Markdown handle numbering or adjust manually (1/2/3/4).
162-166: Specify language for fenced JSON blocksAdd
jsonafter the back-ticks to satisfy MD040 and enable syntax highlighting.-``` +```jsonAlso applies to: 171-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
permissionless-batches/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
permissionless-batches/README.md (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1645
File: rollup/proposer-tool-config.json:34-40
Timestamp: 2025-04-15T08:52:44.176Z
Learning: In configuration files like `rollup/proposer-tool-config.json`, placeholders such as `<mainnet read db config>` are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.
🪛 LanguageTool
permissionless-batches/README.md
[style] ~36-~36: To form a complete sentence, be sure to include a subject.
Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https...
(MISSING_IT_THERE)
[style] ~144-~144: To form a complete sentence, be sure to include a subject.
Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
permissionless-batches/README.md
118-118: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
162-162: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Co-authored-by: colin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
permissionless-batches/README.md (1)
64-66: Triple-hyphen flag still present – will breakgethstartup
The CLI option list still shows---miner.gaslimit, butgethonly accepts the double-hyphen form.- - `---miner.gaslimit 1 --miner.gasprice 1 --miner.maxaccountsnum 100 --rpc.gascap 0 --gpo.ignoreprice 1` - gas limits for block production + - `--miner.gaslimit 1 --miner.gasprice 1 --miner.maxaccountsnum 100 --rpc.gascap 0 --gpo.ignoreprice 1` - gas limits for block production
🧹 Nitpick comments (5)
permissionless-batches/README.md (5)
30-38: Minor grammar & readability nits
“Can be found …” lacks a subject; also consider active voice.- - `--da.recovery.initialbatch` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https://scrollscan.com/batches). + - `--da.recovery.initialbatch` – batch number from which recovery should start (see [Scrollscan Explorer](https://scrollscan.com/batches)).
101-104: Duplicate list numbering
Markdown renders both items as “2.” – re-number or switch to unordered bullets.-2. Make sure `verifier` and `high_version_circuit` ... -2. Set the `SCROLL_ZKVM_VERSION` environment variable ... +2. Make sure `verifier` and `high_version_circuit` ... +3. Set the `SCROLL_ZKVM_VERSION` environment variable ...
117-121: Use a proper heading instead of bold emphasis
Markdown-lint flags this; promotes consistent structure.-**Troubleshooting** +### Troubleshooting
161-165: Add language specifier to fenced code block
Helps with syntax highlighting and linting.-``` +```json
170-173: Second JSON block also missing language tag-``` +```json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
permissionless-batches/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
permissionless-batches/README.md (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1645
File: rollup/proposer-tool-config.json:34-40
Timestamp: 2025-04-15T08:52:44.176Z
Learning: In configuration files like `rollup/proposer-tool-config.json`, placeholders such as `<mainnet read db config>` are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.
🪛 LanguageTool
permissionless-batches/README.md
[style] ~36-~36: To form a complete sentence, be sure to include a subject.
Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https...
(MISSING_IT_THERE)
[style] ~143-~143: To form a complete sentence, be sure to include a subject.
Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
permissionless-batches/README.md
117-117: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (1)
permissionless-batches/README.md (1)
92-96: Verify thatmake batch_production_submissiontarget exists
If the Makefile lives outside this PR, ensure CI will not fail for new users.
Co-authored-by: colin <[email protected]>
Co-authored-by: colin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
permissionless-batches/conf/coordinator/config.json (2)
18-20: Hard-coded DSN usessslmode=disableand lacks a password
Same concern raised in earlier reviews: the connection string disables encryption and exposes the super-user without credentials. In production this is a critical security gap.- "dsn": "postgres://db/scroll?sslmode=disable&user=postgres", + // Use TLS (`require`), move credentials to env-vars / secrets manager. + "dsn": "postgres://db/scroll?sslmode=require&user=${POSTGRES_USER}&password=${POSTGRES_PASSWORD}",
26-28: Long-lived secret committed to VCS – flagged by gitleaks
A 64-byte token is embedded directly in the file; this is effectively an API key. Rotate it immediately and load it from an environment variable instead.- "secret": "e788b62d39254928a821ac1c76b274a8c835aa1e20ecfb6f50eb10e87847de44", + // inject via `AUTH_SECRET` env var + "secret": "${AUTH_SECRET}",
🧹 Nitpick comments (2)
permissionless-batches/conf/coordinator/config.json (2)
19-20: Inconsistent naming style (maxOpenNum,maxIdleNum)
Keys elsewhere use snake_case; mixed casing hampers grepping & tooling. Considermax_open_conns/max_idle_conns.
11-12: Absoluteassets_pathreduces portability
Hard-coding/verifier/...ties the config to a specific container layout; prefer a path relative to an env var (e.g.,${ASSETS_PATH}) so the same config works across dev & prod images.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/version/version.go(1 hunks)permissionless-batches/conf/coordinator/config.json(1 hunks)rollup/internal/orm/chunk.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/orm/chunk.go
🧰 Additional context used
🧠 Learnings (1)
permissionless-batches/conf/coordinator/config.json (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1645
File: rollup/proposer-tool-config.json:34-40
Timestamp: 2025-04-15T08:52:44.176Z
Learning: In configuration files like `rollup/proposer-tool-config.json`, placeholders such as `<mainnet read db config>` are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.
🪛 Gitleaks (8.26.0)
permissionless-batches/conf/coordinator/config.json
26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
permissionless-batches/conf/proving-service/prover_run.sh (1)
35-43: Quote the nested command substitution to avoid word-splittingUnquoted
$(dirname "$url")can break when path components contain spaces (Shellcheck SC2046).- dest_subdir="$BASE_DOWNLOAD_DIR/$(basename $(dirname "$url"))" + dest_subdir="$BASE_DOWNLOAD_DIR/$(basename "$(dirname "$url")")"permissionless-batches/conf/coordinator/coordinator_run.sh (1)
50-58: Quote the nested command substitution (SC2046)Same word-splitting risk as in
prover_run.sh.- dest_subdir="$OPENVM_DIR/$(basename $(dirname "$url"))" + dest_subdir="$OPENVM_DIR/$(basename "$(dirname "$url")")"permissionless-batches/README.md (2)
65-66: Triple hyphen---miner.gaslimitis a typoOnly two dashes are valid.
- - `---miner.gaslimit 1 --miner.gasprice 1 --miner.maxaccountsnum 100 --rpc.gascap 0 --gpo.ignoreprice 1` - gas limits for block production + - `--miner.gaslimit 1 --miner.gasprice 1 --miner.maxaccountsnum 100 --rpc.gascap 0 --gpo.ignoreprice 1` - gas limits for block production
40-48: Placeholder--scroll<-sepolia>renders an invalid CLI flagGeth will treat the angle brackets literally and abort. Provide two explicit examples or use a clearly marked placeholder.
-./build/bin/geth --scroll<-sepolia> \ +## Mainnet +./build/bin/geth --scroll \ +# or for Sepolia +./build/bin/geth --scroll-sepolia \
🧹 Nitpick comments (6)
permissionless-batches/Makefile (2)
12-14: Reuse thePG_URLvariable to avoid duplication and make maintenance easier
psqltarget hard-codes the connection string instead of re-using thePG_URLvariable you already declared. Any future change (host, port, db name) now needs to be done in two places.- psql 'postgres://postgres@localhost:5432/scroll' + psql '$(PG_URL)'
1-4: Consider adding standardall,clean,testtargetsStatic-analysis flagged the absence of common phony targets. Even stub implementations help IDEs and CI tooling.
.PHONY: all clean test all: batch_production_submission clean: @docker compose down -v test: @echo "no tests for Makefile"permissionless-batches/conf/proving-service/prover_run.sh (2)
35-43: Comment and behaviour disagree – currently always re-downloads filesThe loop downloads every asset unconditionally while the comment says “skips if file exists.” Add a simple existence check:
- echo "Downloading $filepath..." - curl -o "$filepath" -L "$url" + if [ ! -f "$filepath" ]; then + echo "Downloading $filepath…" + curl -o "$filepath" -L "$url" + else + echo "Skipping $filepath (already present)" + fi
45-47: Symlink creation may fail on second run
ln -swithout-fwill error if$HOME/.openvm/paramsalready exists. Add-sf(force) or check before linking.permissionless-batches/docker-compose.yml (1)
46-52: Environment variable${SCROLL_ZKVM_VERSION}must be exported beforedocker compose upCompose will substitute at parse time. Consider adding a default or documenting that
export SCROLL_ZKVM_VERSION=<version>is required before running any Makefile target.permissionless-batches/README.md (1)
161-169: Specify language for fenced code blocksMarkdown-lint warns (MD040). Add
jsonfor clarity.```json "recovery_config": { "enable": true }</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b221d42995f5801b094d24f1aec24aaa52fcc88e and f102d18d59c452ca1ee5338ae98a4711ef3f69f9. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `permissionless-batches/Makefile` (1 hunks) * `permissionless-batches/README.md` (1 hunks) * `permissionless-batches/conf/coordinator/coordinator_run.sh` (1 hunks) * `permissionless-batches/conf/proving-service/config.json` (1 hunks) * `permissionless-batches/conf/proving-service/prover_run.sh` (1 hunks) * `permissionless-batches/docker-compose.yml` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * permissionless-batches/conf/proving-service/config.json </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>permissionless-batches/README.md (1)</summary>Learnt from: colinlyguo
PR: #1645
File: rollup/proposer-tool-config.json:34-40
Timestamp: 2025-04-15T08:52:44.176Z
Learning: In configuration files likerollup/proposer-tool-config.json, placeholders such as<mainnet read db config>are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.</details> </details><details> <summary>🪛 Shellcheck (0.10.0)</summary> <details> <summary>permissionless-batches/conf/proving-service/prover_run.sh</summary> [warning] 37-37: Quote this to prevent word splitting. (SC2046) </details> <details> <summary>permissionless-batches/conf/coordinator/coordinator_run.sh</summary> [warning] 52-52: Quote this to prevent word splitting. (SC2046) </details> </details> <details> <summary>🪛 YAMLlint (1.37.1)</summary> <details> <summary>permissionless-batches/docker-compose.yml</summary> [warning] 98-98: wrong indentation: expected 2 but found 4 (indentation) --- [error] 98-98: no new line character at the end of file (new-line-at-end-of-file) </details> </details> <details> <summary>🪛 checkmake (0.2.2)</summary> <details> <summary>permissionless-batches/Makefile</summary> [warning] 1-1: Missing required phony target "all" (minphony) --- [warning] 1-1: Missing required phony target "clean" (minphony) --- [warning] 1-1: Missing required phony target "test" (minphony) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>permissionless-batches/README.md</summary> [style] ~36-~36: To form a complete sentence, be sure to include a subject. Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https... (MISSING_IT_THERE) --- [style] ~143-~143: To form a complete sentence, be sure to include a subject. Context: ...` - batch where to start recovery from. Can be found on [Scrollscan Explorer](https... (MISSING_IT_THERE) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>permissionless-batches/README.md</summary> 117-117: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 161-161: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 169-169: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)</summary> * GitHub Check: tests * GitHub Check: check * GitHub Check: tests </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…l-tech/scroll into jt/permissionless-batches-recovery
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
rollup/internal/controller/permissionless_batches/minimal_recovery.go (2)
64-90: Error handling masks real database issues.The current implementation treats all database errors as "recovery needed", which could hide connectivity problems and lead to unnecessary destructive recovery operations.
116-133: Potential infinite loop in chunk creation.The chunk creation loop may run indefinitely if
ProposeChunk()fails to advanceEndBlockNumber, as identified in previous reviews.
🧹 Nitpick comments (1)
rollup/internal/controller/permissionless_batches/minimal_recovery.go (1)
258-258: Remove debug print statement.The
fmt.Println(daBatch, daBlobPayload)appears to be leftover debug code that should be removed before production deployment.- fmt.Println(daBatch, daBlobPayload)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
permissionless-batches/conf/relayer/config.json(1 hunks)rollup/abi/bridge_abi_test.go(1 hunks)rollup/internal/config/recovery.go(1 hunks)rollup/internal/controller/permissionless_batches/minimal_recovery.go(1 hunks)rollup/internal/orm/batch.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- rollup/abi/bridge_abi_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- permissionless-batches/conf/relayer/config.json
- rollup/internal/config/recovery.go
- rollup/internal/orm/batch.go
🧰 Additional context used
🧠 Learnings (1)
rollup/internal/controller/permissionless_batches/minimal_recovery.go (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1530
File: rollup/internal/controller/watcher/batch_proposer.go:291-294
Timestamp: 2024-10-20T16:13:20.397Z
Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
🧬 Code Graph Analysis (1)
rollup/internal/controller/permissionless_batches/minimal_recovery.go (8)
rollup/internal/orm/chunk.go (3)
Chunk(23-67)Chunk(75-77)NewChunk(70-72)rollup/internal/orm/batch.go (3)
Batch(25-75)Batch(83-85)NewBatch(78-80)rollup/internal/controller/watcher/chunk_proposer.go (1)
ChunkProposer(22-52)rollup/internal/controller/watcher/batch_proposer.go (1)
BatchProposer(24-54)rollup/internal/controller/watcher/bundle_proposer.go (1)
BundleProposer(22-42)rollup/internal/controller/watcher/l2_watcher.go (1)
L2WatcherClient(22-38)rollup/internal/config/recovery.go (1)
RecoveryConfig(3-14)common/types/db.go (2)
ProvingTaskVerified(152-152)RollupFinalized(239-239)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (6)
rollup/internal/controller/permissionless_batches/minimal_recovery.go (6)
26-31: Constants are well-defined and documented.The fake indices serve as placeholders for recovery state tracking and are appropriately documented.
33-62: Struct design and constructor are well-implemented.The MinimalRecovery struct contains all necessary dependencies and the constructor properly initializes all fields.
194-325: State restoration logic is comprehensive and well-implemented.The method properly validates configuration, fetches L1 data, and uses database transactions for atomic operations. Error handling is thorough throughout.
327-411: Batch decoding implementation is robust.The method properly handles codec versioning, validates batch hashes, and includes comprehensive error handling for blob fetching operations.
413-445: Block range validation is present but could be enhanced.The method includes basic validation for block height limits. The implementation appears correct with proper error handling.
447-458: Database reset implementation follows best practices.The method correctly uses
migrate.ResetDB()for a clean database reset, which is the recommended approach over manual rollback/migrate cycles.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
rollup/internal/controller/permissionless_batches/submitter.go (1)
228-234: Add safety check for empty chunks slice.The code accesses
chunks[len(chunks)-1]without verifying thatchunksis not empty, which could cause a panic ifGetChunksInRangereturns an empty slice.Add a guard before accessing the slice:
chunks, err := s.chunkOrm.GetChunksInRange(s.ctx, batch.StartChunkIndex, batch.EndChunkIndex) if err != nil { return nil, nil, fmt.Errorf("failed to get chunks in range for batch %d: %w", batch.Index, err) } +if len(chunks) == 0 { + return nil, nil, fmt.Errorf("no chunks found for batch %d", batch.Index) +} if chunks[len(chunks)-1].Index != batch.EndChunkIndex {rollup/internal/controller/relayer/full_recovery.go (4)
75-83: Add nil-safety check for empty database.If the local DB does not yet contain any batch,
GetLatestBatchwill return a "record not found" error, and the code doesn't handle this case properly.Add explicit handling for the case when no batches exist:
latestDBBatch, err := f.batchORM.GetLatestBatch(f.ctx) if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + // Start from genesis batch (index -1) + latestDBBatch = &orm.Batch{Index: 0} // or appropriate genesis value + } else { return fmt.Errorf("failed to get latest batch from DB: %w", err) + } }
91-98: Fix undefined method call.The method
f.l1Reader.LatestFinalizedBatchIndexis undefined. This will cause a compilation error.Use the correct method name:
-latestFinalizedBatchContract, err := f.l1Reader.LatestFinalizedBatchIndex(latestFinalizedL1Block) +latestFinalizedBatchContract, err := f.l1Reader.GetLatestFinalizedBatchIndex(latestFinalizedL1Block)Or check the
l1.Readerinterface for the correct method name.
160-164: Return error instead of silently stopping on type assertion failure.When the type assertion fails, the code logs an error and returns false, which stops event processing silently. This could leave the recovery in an incomplete state.
Consider modifying the callback to return an error instead of a boolean, or at least provide more context about the failure:
revertBatch, ok := event.(*l1.RevertBatchEventV7) if !ok { - log.Error(fmt.Sprintf("unexpected type of revert event: %T, expected RevertEventV7Type", event)) - return false + return fmt.Errorf("unexpected type of revert event: %T, expected RevertEventV7Type", event) }Note: This would require modifying the callback signature to support error returns.
266-274: Fix incorrect batch references in error messages.The error messages reference
firstBatchbut should reference the current batchbbeing processed.-return fmt.Errorf("failed to fetch commit tx data of batch %d, tx hash: %v, err: %w", firstBatch.commit.BatchIndex().Uint64(), firstBatch.commit.TxHash().Hex(), err) +return fmt.Errorf("failed to fetch commit tx data of batch %d, tx hash: %v, err: %w", b.commit.BatchIndex().Uint64(), b.commit.TxHash().Hex(), err)-return fmt.Errorf("unsupported codec version: %v, batch index: %v, tx hash: %s", args.Version, firstBatch.commit.BatchIndex().Uint64(), firstBatch.commit.TxHash().Hex()) +return fmt.Errorf("unsupported codec version: %v, batch index: %v, tx hash: %s", args.Version, b.commit.BatchIndex().Uint64(), b.commit.TxHash().Hex())
🧹 Nitpick comments (4)
rollup/internal/controller/permissionless_batches/submitter.go (2)
178-200: Consider improving error handling for proving status updates.The proving status update logic is marked as "not necessary" and placed outside the main transaction. However, if these updates fail, the error is only logged without any retry mechanism or alerting. This could lead to inconsistent state in the database.
Consider one of these approaches:
- Include these updates in the main transaction if they are important for consistency
- Add a retry mechanism for failed proving status updates
- Use structured logging with appropriate log levels for monitoring
if txErr != nil { - log.Error("Updating chunk and batch proving status when finalizing without proof failure", "bundleHash", bundle.Hash, "err", txErr) + log.Warn("Failed to update proving status when finalizing without proof - this may require manual intervention", "bundleHash", bundle.Hash, "err", txErr) }
46-51: Consider adding context cancellation handling.The constructor creates a sender but doesn't handle potential context cancellation during initialization. While not critical, this could improve graceful shutdown behavior.
Consider adding context cancellation check:
func NewSubmitter(ctx context.Context, db *gorm.DB, cfg *config.RelayerConfig, chainCfg *params.ChainConfig) (*Submitter, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + registry := prometheus.DefaultRegisterer finalizeSender, err := sender.NewSender(ctx, cfg.SenderConfig, cfg.FinalizeSenderSignerConfig, "permissionless_batches_submitter", "finalize_sender", types.SenderTypeFinalizeBatch, db, registry)rollup/internal/controller/relayer/full_recovery.go (2)
242-244: Consider more descriptive error message.The error message "no finalized batches to process" might be misleading if this is a valid state during recovery.
Consider a more descriptive message:
-return fmt.Errorf("no finalized batches to process") +return fmt.Errorf("no finalized batches found between DB latest batch and L1 latest batch - recovery may be up to date")
464-466: Use more descriptive panic message.The panic message could be more descriptive about what went wrong and why this is unexpected.
-panic(fmt.Sprintf("commit and finalize batch index mismatch: %d != %d", commit.BatchIndex().Uint64(), finalize.BatchIndex().Uint64())) +panic(fmt.Sprintf("invalid batch events: commit batch index %d is greater than finalize batch index %d", commit.BatchIndex().Uint64(), finalize.BatchIndex().Uint64()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rollup/cmd/permissionless_batches/app/app.go(1 hunks)rollup/internal/controller/permissionless_batches/submitter.go(1 hunks)rollup/internal/controller/relayer/full_recovery.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/cmd/permissionless_batches/app/app.go
🧰 Additional context used
🧠 Learnings (2)
rollup/internal/controller/permissionless_batches/submitter.go (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1530
File: rollup/internal/controller/watcher/batch_proposer.go:291-294
Timestamp: 2024-10-20T16:13:20.397Z
Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
rollup/internal/controller/relayer/full_recovery.go (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1530
File: rollup/internal/controller/watcher/batch_proposer.go:291-294
Timestamp: 2024-10-20T16:13:20.397Z
Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1555 +/- ##
===========================================
- Coverage 39.69% 37.59% -2.11%
===========================================
Files 237 242 +5
Lines 18935 19999 +1064
===========================================
+ Hits 7516 7518 +2
- Misses 10680 11741 +1061
- Partials 739 740 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Purpose or design rationale of this PR
This PR is part of the permissionless batches (aka enforced batches) feature. It implements the permissionless batch production, operator recovery and provides instructions how to run these in the readme.
permissionless batch production:
rollup/cmd/permissionless_batches/app/app.gothe main tool in conjunction withpermissionless-batches/docker-compose.yml(usage will be explained in readme later) to create batches in a permissionless way and submit them to L1. It requires the recovery and potentially block production without signature in l2geth before.operator recovery:
rollup/cmd/rollup_relayer/app/app.gowithcfg.RecoveryConfig.Enable == true. Willrestore all batches between the latest finalized batch in the DB and the latest finalized batch on L1 based on L1 data. It requires the recovery of l2geth before.
Other parts of this feature are implemented in following PRs:
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:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.dockerignorefiles across multiple services.