feat(permissionless batches): batch production toolkit and operator recovery #1555
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change set introduces a comprehensive "permissionless batches" feature for the Scroll rollup system, enabling anyone to submit batches with proofs in scenarios where the operator is unavailable or censoring. It adds new CLI applications, Dockerfiles, configuration files, and a detailed README for permissionless batch production and recovery. The implementation includes controllers for minimal and full recovery, submitter logic for on-chain batch submission, and enhancements to the ORM layer for permissionless operations. Several methods are made public for broader access, and supporting Docker and ignore files are updated or introduced to facilitate new workflows and deployment. Documentation and configuration files ensure reproducibility and clarity for users and operators. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (permissionless_batches)
participant DB
participant L2Watcher
participant Proposers
participant Submitter
participant L1
User->>CLI (permissionless_batches): Run with config and genesis
CLI (permissionless_batches)->>DB: Connect and initialize
CLI (permissionless_batches)->>L2Watcher: Connect to L2
CLI (permissionless_batches)->>Proposers: Initialize chunk, batch, bundle proposers
CLI (permissionless_batches)->>CLI (permissionless_batches): Check if recovery needed
alt Recovery needed
CLI (permissionless_batches)->>MinimalRecovery: Run recovery
MinimalRecovery->>DB: Reset and restore minimal state
MinimalRecovery->>L2Watcher: Fetch missing L2 blocks
MinimalRecovery->>Proposers: Propose chunks, batch, bundle
else No recovery needed
CLI (permissionless_batches)->>Submitter: Submit batch (with/without proof)
Submitter->>DB: Load latest bundle and batch
Submitter->>L1: Submit transaction with calldata and blob
Submitter->>DB: Update batch and bundle statuses
end
CLI (permissionless_batches)->>User: Output status and wait for confirmation/exit
Suggested reviewers
Poem
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:
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
…o feat/use-codec-v6
…ssionless-batches-recovery
| // reset and init DB | ||
| var v int64 | ||
| err = migrate.Rollback(sqlDB, &v) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to rollback db: %w", err) | ||
| } | ||
|
|
||
| err = migrate.Migrate(sqlDB) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to migrate db: %w", err) | ||
| } |
There was a problem hiding this comment.
Just implemented in another PR, this can be simplified by migrate.Reset(sqlDB).
|
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.
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
.dockerignoreand.gitignorefiles for improved build context management and source control hygiene.Refactor