feat(permissionless batches): batch production toolkit and operator recovery #1555
Conversation
WalkthroughThis change introduces a comprehensive permissionless batch production and recovery toolkit for a rollup system. It adds new CLI tools, Dockerfiles, configuration files, scripts, and documentation to support permissionless batch submission, proving, and operator recovery. The codebase is extended with controllers for minimal and full recovery, a submitter for permissionless batches, and related ORM/database enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (permissionless_batches)
participant DB
participant L1Client
participant L2Client
participant Prover
participant Coordinator
participant Relayer
User->>CLI (permissionless_batches): Run with config
CLI (permissionless_batches)->>DB: Connect and migrate
CLI (permissionless_batches)->>L1Client: Validate recovery params
CLI (permissionless_batches)->>L2Client: Connect to L2
CLI (permissionless_batches)->>Coordinator: Initialize proposers
CLI (permissionless_batches)->>Relayer: Check if recovery needed
alt Recovery Needed
CLI (permissionless_batches)->>Relayer: Run recovery process
Relayer->>DB: Reset and restore state from L1
Relayer->>L1Client: Fetch finalized batch data
Relayer->>L2Client: Fetch missing L2 blocks
Relayer->>DB: Insert recovered chunks, batches, bundles
else No Recovery Needed
CLI (permissionless_batches)->>Prover: Optionally generate proof
CLI (permissionless_batches)->>Relayer: Submit batch (with/without proof)
Relayer->>L1Client: Submit transaction
Relayer->>DB: Update batch/bundle status
end
CLI (permissionless_batches)->>User: Log completion/confirmation
Suggested labels
Suggested reviewers
Poem
✨ 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 :) |
| // 2. Make sure that the specified batch is indeed finalized on the L1 rollup contract and is the latest finalized batch. | ||
| // TODO: enable check | ||
| //latestFinalizedBatch, err := reader.LatestFinalizedBatch(latestFinalizedL1Block) | ||
| //if cfg.RecoveryConfig.LatestFinalizedBatch != latestFinalizedBatch { |
There was a problem hiding this comment.
curious about why setting cfg.RecoveryConfig.LatestFinalizedBatch instead of using reader.LatestFinalizedBatch as the start point.
There was a problem hiding this comment.
you're right, can probably remove it and make the configuration a bit easier. initially, I wanted the user to specify L1 block and the latest finalized batch so that the user knows where the (minimal) recovery process is starting from and there's no "magic" happening (e.g. if there's another batch committed in the meantime).
There was a problem hiding this comment.
I introduced another config parameter for testing purposes to override this check force_latest_finalized_batch. The user now still needs to specify the L1 block height and latest finalized batch to not have any surprises/magic.
… 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
…ssionless-batches-recovery
|
Mark it ready for review temporarily for fixing the ci failing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Nitpick comments (19)
rollup/abi/bridge_abi_test.go (2)
177-178: Use testing.T logger instead of fmt.Println
In Go tests, prefert.Logso that logs are captured by the testing framework (and only shown with-vor on failures).- fmt.Println("methods") + t.Log("methods")
189-190: Log errors header using t.Log
Replace the directfmt.Printlncall witht.Logto avoid polluting standard output.- fmt.Println("errors") + t.Log("errors")rollup/internal/config/config.go (1)
21-24: Add validation or defaults for RecoveryConfig
You've introducedRecoveryConfiginto the mainConfigstruct. To avoid nil-pointer issues and enforce correct usage:
- Default
RecoveryConfig.Enabletofalseif omitted (backward compatibility).- Validate required fields (e.g., L1 beacon endpoint, latest batch) and error out in
NewConfigwhenEnable == truebut config is missing or invalid.permissionless-batches/conf/proving-service/config.json (2)
6-6: Consider using HTTPS for coordinator communication.The coordinator URL uses HTTP, which could expose sensitive data if not in an isolated network. Consider using HTTPS for secure communication.
- "base_url": "http://coordinator:8390", + "base_url": "https://coordinator:8390",
18-24: Consider more resilient retry settings for cloud service.The current retry settings (3 retries with 5 second wait) might not be sufficient for a cloud service that could experience temporary outages. Consider increasing the retry count or implementing exponential backoff.
"cloud": { "base_url": "https://sindri.app/api/v1/", "api_key": "<API key>", - "retry_count": 3, - "retry_wait_time_sec": 5, + "retry_count": 5, + "retry_wait_time_sec": 10, "connection_timeout_sec": 60 }rollup/cmd/rollup_relayer/app/app.go (1)
114-124: Consider checking if L1 reader was created successfully.There's no validation that the L1 reader was created successfully before using it in
NewFullRecovery. While there is error handling after thel1.NewReadercall, adding a nil check before passing it toNewFullRecoverywould be safer.if err != nil { return fmt.Errorf("failed to create L1 reader: %w", err) } +if reader == nil { + return fmt.Errorf("L1 reader is nil after creation") +} + fullRecovery := relayer.NewFullRecovery(subCtx, cfg, genesis, db, chunkProposer, batchProposer, bundleProposer, l2watcher, l1Client, reader)permissionless-batches/conf/coordinator/config.json (1)
35-36: Consider shorter token expiration times.The current challenge and login expiration durations are 3600 seconds (1 hour), which might be longer than necessary for security best practices.
- "challenge_expire_duration_sec": 3600, - "login_expire_duration_sec": 3600 + "challenge_expire_duration_sec": 1800, + "login_expire_duration_sec": 1800rollup/internal/config/recovery.go (1)
6-7: Consider adding validation for endpointThe L1BeaconNodeEndpoint field could benefit from validation to ensure it's a valid URL.
Consider adding a validation method to the RecoveryConfig struct:
func (c *RecoveryConfig) Validate() error { if c.Enable { if c.L1BeaconNodeEndpoint != "" { if _, err := url.Parse(c.L1BeaconNodeEndpoint); err != nil { return fmt.Errorf("invalid L1BeaconNodeEndpoint URL: %w", err) } } // Add more validations as needed } return nil }build/dockerfiles/recovery_permissionless_batches.Dockerfile (2)
17-19: Confusing binary naming conventionThe source code is from
/src/rollup/cmd/permissionless_batches/but the binary is namedrollup_relayer. This naming mismatch could lead to confusion.Consider renaming the binary to match its source directory or add a comment explaining the naming convention:
RUN --mount=target=. \ --mount=type=cache,target=/root/.cache/go-build \ - cd /src/rollup/cmd/permissionless_batches/ && CGO_LDFLAGS="-ldl" go build -v -p 4 -o /bin/rollup_relayer + cd /src/rollup/cmd/permissionless_batches/ && CGO_LDFLAGS="-ldl" go build -v -p 4 -o /bin/permissionless_batchesThen update the entrypoint accordingly:
-COPY --from=builder /bin/rollup_relayer /bin/ +COPY --from=builder /bin/permissionless_batches /bin/ WORKDIR /app -ENTRYPOINT ["rollup_relayer"] +ENTRYPOINT ["permissionless_batches"]
28-30: Missing default arguments or configurationThe ENTRYPOINT is set to the binary name without any CMD or default arguments, which could make it less user-friendly.
Consider adding a default CMD that provides help information:
COPY --from=builder /bin/rollup_relayer /bin/ WORKDIR /app ENTRYPOINT ["rollup_relayer"] +CMD ["--help"]This would make the container more user-friendly by displaying help information when run without arguments.
permissionless-batches/docker-compose.yml (1)
120-121: Missing trailing newline & wrong indentation forvolumesYAML best practice (and many linters) expects a newline at EOF and two-space indentation of the root-level
volumes:key. This prevents CI lint failures.-volumes: - db_data: +volumes: + db_data: +🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 121-121: wrong indentation: expected 2 but found 4
(indentation)
[error] 121-121: no new line character at the end of file
(new-line-at-end-of-file)
permissionless-batches/README.md (3)
1-4: Avoid duplicated heading phrase and tighten introduction
# Permissionless Batches Permissionless batches …repeats the phrase “Permissionless batches”. Remove the second occurrence to avoid redundancy and comply with Markdown-lint (PHRASE_REPETITION).-# Permissionless Batches -Permissionless batches aka enforced batches is ... +# Permissionless Batches +Permissionless (aka “enforced”) batches is ...🧰 Tools
🪛 LanguageTool
[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)
26-28: Grammar: use plural agreement
there's no blocks→there are no blocks.🧰 Tools
🪛 LanguageTool
[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)
160-168: Specify language on fenced JSON blocks
markdownlintcomplains (MD040). Addjsonafter the back-ticks so syntax highlighting and linting work:-``` +```json🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
160-160: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
168-168: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
rollup/internal/orm/batch.go (1)
406-427: Unconditional timestamp/tx-hash updates may overwrite correct data
UpdateRollupStatusCommitAndFinalizeTxHashsets both commit and finalize timestamps/hashes regardless ofstatus.
If the caller wants to update only one of them (e.g.RollupFinalizedafter commit), earlier commit information will be silently re-written.Consider:
• Make the fields optional parameters (pointers) and update only when non-empty,
or
• Split into two focused helpers (UpdateCommit*andUpdateFinalize*) to mirror existing API.rollup/internal/controller/relayer/full_recovery.go (1)
99-104: Generic container helper may clash withgo-ethereum/commonimport
common.NewHeapMap[...]relies on an extension that is not present in upstreamgo-ethereum/common.
If your fork defines it, consider aliasing the import to avoid confusion:import ( ethcommon "github.com/scroll-tech/go-ethereum/common" "github.com/scroll-tech/go-ethereum/log" ... )Then adjust usages (
ethcommon.NewHeapMap[...]) for clarity and to prevent accidental shadowing with the widely-knowncommonpackage.rollup/internal/controller/permissionless_batches/minimal_recovery.go (2)
47-61: Constructor name leaks abstraction & complicates test discoveryThe exported constructor is called
NewRecovery, yet it returns a pointer toMinimalRecovery.
a) From an API-design perspective this is misleading – the consumer does not know which flavour of recovery is being instantiated.
b) It also breaks the common Go idiomNew<Type>(), making grepping for constructors or writing table-driven tests harder.-func NewRecovery(ctx context.Context, cfg *config.Config, genesis *core.Genesis, db *gorm.DB, chunkProposer *watcher.ChunkProposer, batchProposer *watcher.BatchProposer, bundleProposer *watcher.BundleProposer, l2Watcher *watcher.L2WatcherClient) *MinimalRecovery { +func NewMinimalRecovery(ctx context.Context, cfg *config.Config, genesis *core.Genesis, db *gorm.DB, + chunkProposer *watcher.ChunkProposer, batchProposer *watcher.BatchProposer, + bundleProposer *watcher.BundleProposer, l2Watcher *watcher.L2WatcherClient) *MinimalRecovery {Renaming keeps public APIs self-describing and avoids potential clashes with any future “full” recovery constructor.
257-258: Left-over debug print pollutes stdout
fmt.Println(daBatch, daBlobPayload)sneaked into production code. This clutters logs and may dump large binary objects to the console.- fmt.Println(daBatch, daBlobPayload) + // fmt.Println removed – use structured log if necessaryrollup/internal/controller/permissionless_batches/submitter.go (1)
151-156: Logging raw calldata can expose sensitive dataOn failure the error path logs the entire ABI-packed calldata as hex.
Depending on chain configuration this may leak user keys or proof bytes.Consider trimming or hashing the payload before logging, e.g.:
log.Error("commitAndFinalize failed", "err", err, "calldataHash", crypto.Keccak256Hash(calldata), )[security]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
build/dockerfiles/coordinator-api.Dockerfile.dockerignore(1 hunks)build/dockerfiles/coordinator-cron.Dockerfile.dockerignore(1 hunks)build/dockerfiles/db_cli.Dockerfile.dockerignore(1 hunks)build/dockerfiles/gas_oracle.Dockerfile.dockerignore(1 hunks)build/dockerfiles/recovery_permissionless_batches.Dockerfile(1 hunks)build/dockerfiles/recovery_permissionless_batches.Dockerfile.dockerignore(1 hunks)build/dockerfiles/rollup_relayer.Dockerfile.dockerignore(1 hunks)permissionless-batches/.gitignore(1 hunks)permissionless-batches/README.md(1 hunks)permissionless-batches/conf/coordinator/config.json(1 hunks)permissionless-batches/conf/genesis.json(1 hunks)permissionless-batches/conf/proving-service/config.json(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/permissionless_batches/main.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(2 hunks)rollup/internal/config/config.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)
🧰 Additional context used
🧬 Code Graph Analysis (5)
rollup/cmd/permissionless_batches/main.go (1)
rollup/cmd/permissionless_batches/app/app.go (1)
Run(137-142)
rollup/cmd/rollup_relayer/app/app.go (5)
rollup/internal/config/recovery.go (1)
RecoveryConfig(3-13)rollup/internal/config/config.go (1)
Config(20-25)rollup/internal/controller/relayer/full_recovery.go (1)
NewFullRecovery(40-58)common/utils/utils.go (1)
LoopWithContext(34-45)rollup/internal/utils/confirmation.go (1)
GetLatestConfirmedBlockNumber(18-56)
rollup/internal/controller/watcher/bundle_proposer.go (1)
rollup/internal/orm/batch.go (2)
Batch(25-75)Batch(83-85)
rollup/internal/orm/bundle.go (3)
coordinator/internal/orm/bundle.go (2)
Bundle(17-45)Bundle(53-55)common/types/db.go (2)
RollupStatus(225-225)RollupFinalized(239-239)common/utils/timezone.go (1)
NowUTC(6-9)
rollup/internal/orm/batch.go (5)
coordinator/internal/orm/batch.go (2)
Batch(18-72)Batch(80-82)tests/integration-test/orm/batch.go (2)
Batch(17-60)Batch(68-70)rollup/internal/orm/chunk.go (2)
Chunk(23-67)Chunk(75-77)common/types/db.go (5)
ProvingStatus(140-140)ProvingTaskVerified(152-152)RollupStatus(225-225)RollupFinalized(239-239)GasOracleStatus(9-9)common/utils/timezone.go (1)
NowUTC(6-9)
🪛 Biome (1.9.4)
permissionless-batches/conf/genesis.json
[error] 1-1: unexpected character <
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: unexpected character .
(parse)
[error] 1-1: String values must be double quoted.
(parse)
[error] 1-1: unexpected character >
(parse)
permissionless-batches/conf/relayer/config.json
[error] 49-49: unexpected character <
(parse)
[error] 49-49: expected , but instead found commit
Remove commit
(parse)
[error] 49-49: expected : but instead found tx
Remove tx
(parse)
[error] 49-49: expected , but instead found of
Remove of
(parse)
[error] 49-49: expected : but instead found last
Remove last
(parse)
[error] 49-49: expected , but instead found finalized
Remove finalized
(parse)
[error] 49-49: expected : but instead found batch
Remove batch
(parse)
[error] 49-49: expected , but instead found on
Remove on
(parse)
[error] 49-49: expected : but instead found L1
Remove L1
(parse)
[error] 49-49: unexpected character >
(parse)
[error] 50-50: unexpected character <
(parse)
[error] 50-50: expected , but instead found last
Remove last
(parse)
[error] 50-50: expected : but instead found finalized
Remove finalized
(parse)
[error] 50-50: expected , but instead found batch
Remove batch
(parse)
[error] 50-50: expected : but instead found on
Remove on
(parse)
[error] 50-50: expected , but instead found L1
Remove L1
(parse)
[error] 50-50: unexpected character >
(parse)
[error] 51-51: unexpected character <
(parse)
[error] 51-51: expected , but instead found L2
Remove L2
(parse)
[error] 51-51: expected : but instead found block
Remove block
(parse)
[error] 51-51: expected , but instead found up
Remove up
(parse)
[error] 51-51: expected : but instead found to
Remove to
(parse)
[error] 51-51: expected , but instead found which
Remove which
(parse)
[error] 51-51: expected : but instead found to
Remove to
(parse)
[error] 51-51: expected , but instead found produce
Remove produce
(parse)
[error] 51-51: expected : but instead found batch
Remove batch
(parse)
[error] 51-51: unexpected character >
(parse)
🪛 golangci-lint (1.64.8)
rollup/cmd/permissionless_batches/app/app.go
120-120: undefined: signal
(typecheck)
🪛 YAMLlint (1.35.1)
permissionless-batches/docker-compose.yml
[warning] 76-76: wrong indentation: expected 6 but found 10
(indentation)
[warning] 121-121: wrong indentation: expected 2 but found 4
(indentation)
[error] 121-121: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: Rollup
rollup/internal/controller/relayer/full_recovery.go
[error] 83-83: Compilation error: 'f.l1Reader.LatestFinalizedBatchIndex' undefined (type *l1.Reader has no field or method LatestFinalizedBatchIndex)
🪛 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)
[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] ~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 proving profile i...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~109-~109: 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] ~117-~117: Possible missing comma found.
Context: ...ubleshooting** - in case the submission fails it will print the calldata for the tran...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~118-~118: Loose punctuation mark.
Context: ...0x4df567b9: ErrorNotInEnforcedBatchMode`: permissionless batch mode is not activa...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~119-~119: 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] ~121-~121: 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] ~142-~142: 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
103-103: Bare URL used
null
(MD034, no-bare-urls)
116-116: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
160-160: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
168-168: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (31)
rollup/abi/bridge_abi_test.go (1)
8-9: No action required for import reordering
Thegorm.io/gormimport was moved but this has no functional impact.rollup/internal/controller/relayer/l2_relayer.go (1)
15-16: Import reordering only
The change moves thegorm.io/gormimport earlier; no behavioral change.build/dockerfiles/coordinator-cron.Dockerfile.dockerignore (1)
8-8: Exclude permissionless-batches configuration from coordinator-cron build
Addingpermissionless-batches/conf/to.dockerignoreensures environment-specific configs aren’t sent to the Docker daemon, slimming the build context and avoiding accidental inclusion of sensitive files. This aligns with other Docker ignore patterns in the repo.build/dockerfiles/coordinator-api.Dockerfile.dockerignore (1)
8-8: Exclude permissionless-batches configuration from coordinator-api build
Addingpermissionless-batches/conf/to.dockerignoreprevents the inclusion of configuration files for permissionless batches in the coordinator-api image, reducing context size and potential misconfiguration. This mirrors the change in other service builds.build/dockerfiles/gas_oracle.Dockerfile.dockerignore (1)
2-2: Exclude unnecessary directories from gas_oracle build context
The updates addcontracts/andpermissionless-batches/conf/to the ignore list (alongside the existing*target/*), which helps shrink the Docker build context by omitting large or irrelevant folders. This is consistent with other service Dockerfiles.Also applies to: 6-6, 8-8
build/dockerfiles/db_cli.Dockerfile.dockerignore (1)
8-8: Exclude permissionless-batches configuration from db_cli build
Addingpermissionless-batches/conf/to.dockerignorekeeps configuration files out of the db_cli Docker context, improving build performance and security. This change is in line with the patterns applied to other components.build/dockerfiles/rollup_relayer.Dockerfile.dockerignore (1)
2-2: Exclude contracts and permissionless-batches configuration from rollup_relayer build
Ignoringcontracts/andpermissionless-batches/conf/(and existing*target/*) streamlines the Docker context for rollup_relayer, preventing extraneous files from being packaged. This matches updates in the other service Dockerfiles.Also applies to: 6-6, 8-8
rollup/cmd/permissionless_batches/main.go (1)
1-7: LGTM - Simple and clean entry point for permissionless batches command.The implementation follows Go's standard pattern for command-line applications by delegating to the app package. This creates a clear separation between the entry point and the actual implementation logic.
build/dockerfiles/recovery_permissionless_batches.Dockerfile.dockerignore (1)
1-8: LGTM - Appropriate Docker ignore patterns.The file properly excludes unnecessary directories and files from the Docker build context, following best practices to reduce the build context size and speed up the build process.
rollup/internal/controller/watcher/chunk_proposer.go (2)
191-191: Method name changed to be publicly accessible for permissionless batches.The method
proposeChunkwas renamed toProposeChunk(capitalized) to make it publicly accessible, and the call site inTryProposeChunkwas updated accordingly. This change enables the new permissionless batch production system to use this method.
271-430: Existing method now exposed as public API.The method logic remains unchanged, but it's now publicly accessible with the capitalized name
ProposeChunk. This is a key component for enabling the permissionless batches feature.rollup/internal/controller/watcher/bundle_proposer.go (4)
12-13: Import reorganization for consistency.The import for "gorm.io/gorm" was moved up to group with other imports, improving code organization.
103-127: Method name changed to be publicly accessible for permissionless batches.The method
updateDBBundleInfowas renamed toUpdateDBBundleInfo(capitalized) to make it publicly accessible. This change enables the new permissionless batch production system to update bundle information in the database.
198-198: Call site updated to use new public method name.Updated to call the newly renamed
UpdateDBBundleInfomethod.
212-212: Call site updated to use new public method name.Updated to call the newly renamed
UpdateDBBundleInfomethod.rollup/cmd/rollup_relayer/app/app.go (2)
141-142:✅ Verification successful
Updated code to ignore errors logged internally.
The change to ignore errors from
TryFetchRunningMissingBlockswith a clarifying comment is appropriate if the method already handles error logging.Let's check if the
TryFetchRunningMissingBlocksmethod properly logs errors internally:
🏁 Script executed:
#!/bin/bash # Check implementation of TryFetchRunningMissingBlocks to verify it logs errors internally grep -A 25 "func.*TryFetchRunningMissingBlocks" rollup/internal/controller/watcher/l2_watcher.go | grep -B 3 -A 3 "log\.Error"Length of output: 670
Approve ignoring errors from TryFetchRunningMissingBlocks
TryFetchRunningMissingBlocksalready logs all error cases internally, so discarding its returned error here is safe and doesn’t lose any diagnostic information.
- In
rollup/internal/controller/watcher/l2_watcher.go, the method logs failures for bothGetL2BlocksLatestHeightandGetAndStoreBlocksbefore returning an error.
111-132:✅ Verification successful
Recovery mode implementation looks good.
The recovery mode implementation follows a good pattern, initializing necessary components and exiting early after completion. The error handling is appropriate.
Let's check if the
FullRecoverycomponent properly handles all expected recovery scenarios:
🏁 Script executed:
#!/bin/bash # Check implementation of FullRecovery.RestoreFullPreviousState method grep -A 30 "func.*RestoreFullPreviousState" rollup/internal/controller/relayer/full_recovery.goLength of output: 1539
To review the full recovery logic, let’s extract the entire
RestoreFullPreviousStateimplementation:
🏁 Script executed:
#!/bin/bash grep -A200 "func (f \*FullRecovery) RestoreFullPreviousState" rollup/internal/controller/relayer/full_recovery.goLength of output: 7709
Recovery mode and FullRecovery logic verified
TheRestoreFullPreviousStatemethod comprehensively handles all recovery steps—fetching the latest DB batch, querying finalized L1 blocks and batches, streaming and grouping rollup events, reproducing chunks, replaying L2 blocks, and rebuilding bundles. Error paths at each stage return meaningful errors, and no gaps in the recovery flow were identified.permissionless-batches/conf/coordinator/config.json (1)
8-22:❓ Verification inconclusive
Verifier configuration has identical paths for different circuit versions.
Both the low_version_circuit and high_version_circuit configurations point to the same parameter and asset paths. Make sure this is intentional.
If the different circuit versions require different parameters or assets, update the paths to be version-specific.
🏁 Script executed:
#!/bin/bash # Check if the params and assets directories have version-specific subdirectories ls -la permissionless-batches/conf/params/ ls -la permissionless-batches/conf/assets/Length of output: 255
Let’s locate where any
paramsandassetsfolders actually live and see all usages of those paths:#!/bin/bash # Search for any directories named 'params' or 'assets' find . -type d | sed 's#^\./##' | grep -E 'params|assets' # Show where and how params_path and assets_path are referenced rg -n 'params_path' -A1 rg -n 'assets_path' -A1
Let’s locate every
paramsandassetsdirectory in the repo and inspect their contents to see if there are version‐specific subfolders:#!/usr/bin/env bash # Find all 'params' and 'assets' dirs find . -type d \( -name 'params' -o -name 'assets' \) | sed 's#^\./##' # For each found directory, list its contents while read -r dir; do echo "Contents of $dir:" ls -la "$dir" done < <(find . -type d \( -name 'params' -o -name 'assets' \))
Confirm circuit-specific parameter and asset paths
I wasn’t able to locate
conf/paramsorconf/assetsunderpermissionless-batches/conf. Bothlow_version_circuitandhigh_version_circuitreference./conf/paramsand./conf/assets, but these directories don’t seem to exist. Please verify:
- Are the
paramsandassetsfolders present at a different path or injected at build/runtime?- If each circuit version needs its own files, update the paths to something like
./conf/params/v4.4.55and./conf/params/v4.4.56.rollup/internal/orm/chunk.go (1)
303-303: Good explanation for TotalL1MessagesPoppedInChunk value.The comment explaining why
TotalL1MessagesPoppedInChunkis set to 0 is very helpful and clarifies the intent.rollup/internal/controller/watcher/l2_watcher.go (4)
62-63: Better error handling and propagationThe changes properly update
TryFetchRunningMissingBlocksto return errors instead of silently logging them, which improves error propagation and enables callers to handle failures appropriately.Also applies to: 67-68
78-81: Method made public and error propagation improvedThe method rename from private
getAndStoreBlocksto publicGetAndStoreBlocksmakes this functionality available to other packages, which is essential for the recovery workflows. The error handling is also appropriately improved.
85-87: Explicit return valueAdding an explicit
return nilimproves code clarity and maintains consistent error-returning behavior.
89-90: Renamed method to publicThe method is now correctly exported with a capital letter, maintaining the same functionality while making it accessible to external packages.
permissionless-batches/conf/relayer/config.json (2)
1-47: Configuration structure looks appropriateThe configuration structure for the L1, L2, and database settings is well-organized and includes the necessary parameters for the permissionless batches relayer.
3-4: Document required RPC endpointsThe configuration contains placeholders for RPC endpoints but doesn't provide information about required permissions or capabilities.
Please ensure that the documentation clearly states:
- Required API methods that must be exposed by these endpoints
- Any specific permissions needed
- Performance requirements for these endpoints to handle the expected load
Also applies to: 7-8
rollup/internal/config/recovery.go (1)
3-13: Well-structured recovery configurationThe RecoveryConfig struct is well-defined with appropriate fields and JSON tags. The comments provide good context for the fields' purposes.
build/dockerfiles/recovery_permissionless_batches.Dockerfile (1)
1-30: Well-structured multi-stage DockerfileThe Dockerfile uses multi-stage builds appropriately to minimize the final image size while including necessary dependencies.
rollup/internal/orm/bundle.go (1)
62-76: Exported name ✔ – implementation looks goodRenaming the method to
GetLatestBundleand updating the doc-comment makes the API consistent with Go’s exported-identifier conventions. Query ordering andErrRecordNotFoundhandling are both correct.permissionless-batches/README.md (1)
18-24: Replace remaining TODO placeholders with concrete URLs before mergeThe prerequisites list still contains several
TODO link list with versionsplaceholders. Leaving these unresolved blocks readers and automation from locating the correct assets.Action: supply final links (e.g. specific git-tag or docs permalink) or remove the bullet until ready.
rollup/internal/controller/permissionless_batches/minimal_recovery.go (1)
381-389: Verify that a blob client is always configured
blob_client.NewBlobClients()returns an aggregator with no underlying clients unlessAddBlobClientis called.
IfL1BeaconNodeEndpointis empty the subsequentGetBlobByVersionedHashAndBlockTimewill likely error out.Please make sure at least one client is registered or fail fast with a clear message, e.g.:
blobs := blob_client.NewBlobClients() if r.cfg.RecoveryConfig.L1BeaconNodeEndpoint == "" { return nil, nil, fmt.Errorf("no blob client configured – set L1BeaconNodeEndpoint") }rollup/internal/controller/permissionless_batches/submitter.go (1)
181-203: Non-proof finalisation updates proving status outside the main DB txWhen
withProof == falsethe code updates proving status in a second transaction.
If the first tx (marking rollup finalising) commits but the second one fails, bundle and batches become stuck in an inconsistent state (finalising but not verified).Consider merging both updates into the primary transaction or adding a compensation mechanism.
Signed-off-by: noelwei <[email protected]> Co-authored-by: jonastheis <[email protected]> Co-authored-by: georgehao <[email protected]> Co-authored-by: colin <[email protected]> Co-authored-by: colinlyguo <[email protected]> Co-authored-by: Rohit Narurkar <[email protected]> Co-authored-by: kunxian xia <[email protected]> Co-authored-by: Ömer Faruk Irmak <[email protected]> Co-authored-by: Ho <[email protected]> Co-authored-by: Velaciela <[email protected]> Co-authored-by: colinlyguo <[email protected]>
…s and bundles to DB
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
|
|
||
| # Download OpenVM files (No checksum verification, but skips if file exists) | ||
| for url in "${OPENVM_URLS[@]}"; do | ||
| dest_subdir="$BASE_DOWNLOAD_DIR/$(basename $(dirname "$url"))" |
There was a problem hiding this comment.
Fix unquoted command substitution to prevent word splitting.
The unquoted command substitution can cause issues if directory names contain spaces.
- dest_subdir="$BASE_DOWNLOAD_DIR/$(basename $(dirname "$url"))"
+ dest_subdir="$BASE_DOWNLOAD_DIR/$(basename "$(dirname "$url")")"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In permissionless-batches/conf/proving-service/local-prover/prover_run.sh at
line 37, the command substitution inside the assignment to dest_subdir is
unquoted, which can cause word splitting if directory names contain spaces. To
fix this, add double quotes around the command substitution so that the entire
output is treated as a single word, preventing unintended splitting.
| for url in "${OPENVM_URLS[@]}"; do | ||
| dest_subdir="$BASE_DOWNLOAD_DIR/$(basename $(dirname "$url"))" | ||
| mkdir -p "$dest_subdir" | ||
|
|
||
| filepath="$dest_subdir/$(basename "$url")" | ||
| echo "Downloading $filepath..." | ||
| curl -o "$filepath" -L "$url" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider skipping existing files.
The download loop lacks error handling and always overwrites files. Consider adding error checking and optionally skip existing files for efficiency.
# Download OpenVM files (No checksum verification, but skips if file exists)
for url in "${OPENVM_URLS[@]}"; do
dest_subdir="$BASE_DOWNLOAD_DIR/$(basename "$(dirname "$url")")"
mkdir -p "$dest_subdir"
filepath="$dest_subdir/$(basename "$url")"
+
+ if [[ -f "$filepath" ]]; then
+ echo "File already exists, skipping: $filepath"
+ continue
+ fi
+
echo "Downloading $filepath..."
- curl -o "$filepath" -L "$url"
+ if ! curl -o "$filepath" -L "$url"; then
+ echo "Failed to download: $url" >&2
+ exit 1
+ fi
doneCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In permissionless-batches/conf/proving-service/local-prover/prover_run.sh around
lines 36 to 43, the download loop currently overwrites files without checking
for errors. Modify the script to first check if the target file already exists
and skip downloading if so. Add error handling after the curl command to detect
download failures and handle them appropriately, such as logging an error
message and optionally exiting or continuing based on the failure.
|
|
||
| # Download OpenVM files (No checksum verification, but skips if file exists) | ||
| for url in "${OPENVM_URLS[@]}"; do | ||
| dest_subdir="$OPENVM_DIR/$(basename $(dirname "$url"))" |
There was a problem hiding this comment.
Fix unquoted command substitution to prevent word splitting.
The unquoted command substitution can cause issues if directory names contain spaces.
- dest_subdir="$OPENVM_DIR/$(basename $(dirname "$url"))"
+ dest_subdir="$OPENVM_DIR/$(basename "$(dirname "$url")")"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 103-103: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In permissionless-batches/conf/coordinator/coordinator_run.sh at line 103, the
command substitution used to set dest_subdir is unquoted, which can cause word
splitting if directory names contain spaces. To fix this, enclose the command
substitution in double quotes to preserve the output as a single string and
prevent word splitting.
| local dest_dir="$2" | ||
| local checksum_file="$3" | ||
|
|
||
| local filename=$(basename "$url") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate variable declaration and assignment to avoid masking return values.
Combining declaration and assignment can mask the return value of the command substitution.
- local filename=$(basename "$url")
+ local filename
+ filename=$(basename "$url")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local filename=$(basename "$url") | |
| local filename | |
| filename=$(basename "$url") |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 68-68: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In permissionless-batches/conf/coordinator/coordinator_run.sh at line 68,
separate the variable declaration and assignment for the filename variable.
First declare the variable with 'local filename' on one line, then assign the
value using 'filename=$(basename "$url")' on the next line to avoid masking the
return value of the command substitution.
| func (r *MinimalRecovery) fetchL2Blocks(fromBlock uint64, l2BlockHeightLimit uint64) (uint64, error) { | ||
| if l2BlockHeightLimit > 0 && fromBlock > l2BlockHeightLimit { | ||
| return 0, fmt.Errorf("fromBlock (latest finalized L2 block) is higher than specified L2BlockHeightLimit: %d > %d", fromBlock, l2BlockHeightLimit) | ||
| } | ||
|
|
||
| log.Info("Fetching L2 blocks with", "fromBlock", fromBlock, "l2BlockHeightLimit", l2BlockHeightLimit) | ||
|
|
||
| // Fetch and insert the missing blocks from the last block in the batch to the latest L2 block. | ||
| latestL2Block, err := r.l2Watcher.Client.BlockNumber(r.ctx) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to get latest L2 block number: %w", err) | ||
| } | ||
|
|
||
| log.Info("Latest L2 block number", "latest L2 block", latestL2Block) | ||
|
|
||
| if l2BlockHeightLimit > latestL2Block { | ||
| return 0, fmt.Errorf("l2BlockHeightLimit is higher than the latest L2 block number, not all blocks are available in L2geth: %d > %d", l2BlockHeightLimit, latestL2Block) | ||
| } | ||
|
|
||
| toBlock := latestL2Block | ||
| if l2BlockHeightLimit > 0 { | ||
| toBlock = l2BlockHeightLimit | ||
| } | ||
|
|
||
| err = r.l2Watcher.GetAndStoreBlocks(r.ctx, fromBlock, toBlock) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to get and store blocks: %w", err) | ||
| } | ||
|
|
||
| log.Info("Fetched L2 blocks from", "fromBlock", fromBlock, "toBlock", toBlock) | ||
|
|
||
| return toBlock, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate block range to prevent potential issues.
The function should validate that fromBlock is not greater than toBlock before proceeding with block fetching.
func (r *MinimalRecovery) fetchL2Blocks(fromBlock uint64, l2BlockHeightLimit uint64) (uint64, error) {
if l2BlockHeightLimit > 0 && fromBlock > l2BlockHeightLimit {
return 0, fmt.Errorf("fromBlock (latest finalized L2 block) is higher than specified L2BlockHeightLimit: %d > %d", fromBlock, l2BlockHeightLimit)
}
+ if fromBlock == 0 {
+ return 0, fmt.Errorf("fromBlock cannot be 0")
+ }
log.Info("Fetching L2 blocks with", "fromBlock", fromBlock, "l2BlockHeightLimit", l2BlockHeightLimit)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rollup/internal/controller/permissionless_batches/minimal_recovery.go around
lines 413 to 445, add a validation to ensure fromBlock is not greater than
toBlock before fetching blocks. After determining toBlock (either latestL2Block
or l2BlockHeightLimit), check if fromBlock > toBlock and if so, return an error
indicating the invalid block range. This prevents potential issues from an
invalid block range during fetching.
| revertBatch, ok := event.(*l1.RevertBatchEventV7) | ||
| if !ok { | ||
| log.Error(fmt.Sprintf("unexpected type of revert event: %T, expected RevertEventV7Type", event)) | ||
| return false | ||
| } |
There was a problem hiding this comment.
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. This could leave the recovery in an incomplete state without the caller knowing.
Consider returning an error to properly propagate this failure:
- if !ok {
- log.Error(fmt.Sprintf("unexpected type of revert event: %T, expected RevertEventV7Type", event))
- return false
- }
+ if !ok {
+ return fmt.Errorf("unexpected type of revert event: %T, expected RevertEventV7Type", event)
+ }Note: This would require modifying the callback signature in FetchRollupEventsInRangeWithCallback to support error returns.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rollup/internal/controller/relayer/full_recovery.go around lines 160 to 164,
the type assertion failure currently logs an error and returns false, silently
stopping event processing. Modify the callback function to return an error
instead of a boolean, update the call to FetchRollupEventsInRangeWithCallback to
handle error returns, and propagate the error when the type assertion fails to
ensure the caller is aware of the failure and can handle it appropriately.
| args, err := f.l1Reader.FetchCommitTxData(b.commit) | ||
| if err != nil { | ||
| 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) | ||
| } | ||
|
|
||
| // all batches we process here will be > CodecV7 since that is the minimum codec version for permissionless batches | ||
| if args.Version < 7 { | ||
| return fmt.Errorf("unsupported codec version: %v, batch index: %v, tx hash: %s", args.Version, firstBatch.commit.BatchIndex().Uint64(), firstBatch.commit.TxHash().Hex()) | ||
| } |
There was a problem hiding this comment.
Fix incorrect batch references in error messages.
The error messages reference firstBatch but should reference the current batch being 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())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args, err := f.l1Reader.FetchCommitTxData(b.commit) | |
| if err != nil { | |
| 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) | |
| } | |
| // all batches we process here will be > CodecV7 since that is the minimum codec version for permissionless batches | |
| if args.Version < 7 { | |
| return fmt.Errorf("unsupported codec version: %v, batch index: %v, tx hash: %s", args.Version, firstBatch.commit.BatchIndex().Uint64(), firstBatch.commit.TxHash().Hex()) | |
| } | |
| args, err := f.l1Reader.FetchCommitTxData(b.commit) | |
| if err != nil { | |
| 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) | |
| } | |
| // all batches we process here will be > CodecV7 since that is the minimum codec version for permissionless batches | |
| if args.Version < 7 { | |
| return fmt.Errorf("unsupported codec version: %v, batch index: %v, tx hash: %s", args.Version, b.commit.BatchIndex().Uint64(), b.commit.TxHash().Hex()) | |
| } |
🤖 Prompt for AI Agents
In rollup/internal/controller/relayer/full_recovery.go around lines 266 to 274,
the error messages incorrectly reference firstBatch instead of the current batch
variable b. Update the error messages to use b.commit.BatchIndex() and
b.commit.TxHash() to accurately reflect the batch being processed when reporting
errors.
| @@ -3,10 +3,11 @@ go 1.22 | |||
| toolchain go1.22.2 | |||
|
|
|||
| use ( | |||
| ./bridge-history-api | |||
| //./bridge-history-api | |||
There was a problem hiding this comment.
Will commenting this out affect bridge-history-api?
| ./common | ||
| ./coordinator | ||
| ./database | ||
| ./rollup | ||
| ./tests/integration-test | ||
| //../go-ethereum |
There was a problem hiding this comment.
| //../go-ethereum |
| @@ -1357,7 +1360,7 @@ github.com/scroll-tech/da-codec v0.1.1-0.20241014152913-2703f226fb0b h1:5H6V6yba | |||
| github.com/scroll-tech/da-codec v0.1.1-0.20241014152913-2703f226fb0b/go.mod h1:48uxaqVgpD8ulH8p+nrBtfeLHZ9tX82bVVdPNkW3rPE= | |||
| github.com/scroll-tech/da-codec v0.1.3-0.20250227072756-a1482833595f h1:YYbhuUwjowqI4oyXtECRofck7Fyj18e1tcRjuQlZpJE= | |||
| github.com/scroll-tech/da-codec v0.1.3-0.20250227072756-a1482833595f/go.mod h1:xECEHZLVzbdUn+tNbRJhRIjLGTOTmnFQuTgUTeVLX58= | |||
| github.com/scroll-tech/da-codec v0.1.3-0.20250519114140-bfa7133d4ad1/go.mod h1:yhTS9OVC0xQGhg7DN5iV5KZJvnSIlFWAxDdp+6jxQtY= | |||
| github.com/scroll-tech/da-codec v0.1.3-0.20250313120912-344f2d5e33e1/go.mod h1:yhTS9OVC0xQGhg7DN5iV5KZJvnSIlFWAxDdp+6jxQtY= | |||
There was a problem hiding this comment.
seems it's reverted to an older version.
There was a problem hiding this comment.
Seems there's no content in this file and can be removed.
| "mock_mode": false, | ||
| "low_version_circuit" : { | ||
| "fork_name": "darwinV2", | ||
| "params_path": "/verifier/params", | ||
| "assets_path": "/verifier/assets", | ||
| "min_prover_version": "v4.4.56" | ||
| }, |
There was a problem hiding this comment.
| "mock_mode": false, | |
| "low_version_circuit" : { | |
| "fork_name": "darwinV2", | |
| "params_path": "/verifier/params", | |
| "assets_path": "/verifier/assets", | |
| "min_prover_version": "v4.4.56" | |
| }, |
| --da.sync=true --da.recovery --da.recovery.produceblocks \ | ||
| --miner.gaslimit 1 --miner.gasprice 1 --miner.maxaccountsnum 100 --rpc.gascap 0 --gpo.ignoreprice 1 \ | ||
| --miner.etherbase '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' --mine \ | ||
| --ccc \ |
| ### 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?
|
|
||
| Local Proving: | ||
|
|
||
| 1. Hardware spec for local prover: CPU: 36+ core, 128G memory GPU: 24G memory (eg. Rtx 3090/3090Ti/4090/A10/L4) |
There was a problem hiding this comment.
| 1. Hardware spec for local prover: CPU: 36+ core, 128G memory GPU: 24G memory (eg. Rtx 3090/3090Ti/4090/A10/L4) | |
| 1. Hardware spec for local prover: CPU: 36+ core, 128G memory GPU: 24G memory (e.g. Rtx 3090/3090Ti/4090/A10/L4) |
| Local Proving: | ||
|
|
||
| 1. Hardware spec for local prover: CPU: 36+ core, 128G memory GPU: 24G memory (eg. Rtx 3090/3090Ti/4090/A10/L4) | ||
| 2. Make sure `verifier` `low_version_circuit` and `high_version_circuit` in `conf/coordinator/config.json` are correct for the latest fork: [TODO link list with versions](#batch-production-toolkit) |
There was a problem hiding this comment.
| 2. Make sure `verifier` `low_version_circuit` and `high_version_circuit` in `conf/coordinator/config.json` are correct for the latest fork: [TODO link list with versions](#batch-production-toolkit) | |
| 2. Make sure `verifier` and `high_version_circuit` in `conf/coordinator/config.json` are correct for the latest fork: [TODO link list with versions](#batch-production-toolkit) |
| ForceLatestFinalizedBatch bool `json:"force_latest_finalized_batch"` // whether to force usage of the latest finalized batch - mainly used for testing | ||
|
|
||
| L2BlockHeightLimit uint64 `json:"l2_block_height_limit"` | ||
| ForceL1MessageCount uint64 `json:"force_l1_message_count"` |
There was a problem hiding this comment.
Adding a comment for this config's usage might be helpful.
| - 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). | ||
| - access to an Ethereum L1 RPC node (beacon node and execution client) | ||
| - ability to run a prover or access to a proving service (e.g. Sindri) |
There was a problem hiding this comment.
| - ability to run a prover or access to a proving service (e.g. Sindri) | |
| - ability to run a prover |
| Cloud Proving(not supported yet): | ||
|
|
||
| 1. Make sure `verifier` `low_version_circuit` and `high_version_circuit` in `conf/coordinator/config.json` are correct for the latest fork: [TODO link list with versions](#batch-production-toolkit) | ||
| 2. Set the `SCROLL_ZKVM_VERSION` environment variable on `Makefile` to the correct version. [TODO link list with versions](#batch-production-toolkit) | ||
| 3. Fill in the required fields in `conf/proving-service/cloud-prover/config.json`. It is recommended to use Sindri. You'll need to obtain credits and an API key from their [website](https://sindri.app/). | ||
|
|
||
| Run with `make cloud_prover`. | ||
|
|
||
| This will prove chunks, the batch and bundle. | ||
| Run `make check_proving_status` | ||
| `Success! You're ready to submit permissionless batch and proof!` indicates that everything is working correctly and the batch is ready to be submit. | ||
|
|
There was a problem hiding this comment.
| Cloud Proving(not supported yet): | |
| 1. Make sure `verifier` `low_version_circuit` and `high_version_circuit` in `conf/coordinator/config.json` are correct for the latest fork: [TODO link list with versions](#batch-production-toolkit) | |
| 2. Set the `SCROLL_ZKVM_VERSION` environment variable on `Makefile` to the correct version. [TODO link list with versions](#batch-production-toolkit) | |
| 3. Fill in the required fields in `conf/proving-service/cloud-prover/config.json`. It is recommended to use Sindri. You'll need to obtain credits and an API key from their [website](https://sindri.app/). | |
| Run with `make cloud_prover`. | |
| This will prove chunks, the batch and bundle. | |
| Run `make check_proving_status` | |
| `Success! You're ready to submit permissionless batch and proof!` indicates that everything is working correctly and the batch is ready to be submit. |
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