feat(rollup-relayer): add a tool to analyze chunk/batch/bundle proposing #1645
Conversation
WalkthroughThis change introduces a new "proposer tool" for the rollup project, enabling replay and testing of historical block data with customizable configuration for protocol upgrades and proposer parameters. The update adds a new CLI tool with associated configuration and genesis files, Docker Compose setup, and Dockerfile for containerization. The core logic includes enhancements to chunk, batch, and bundle proposers to support replay mode, new database handling for test data, and additional logging and metrics. Documentation is updated to describe the tool’s usage and capabilities, and new ORM methods are provided for chunk management in test scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (proposer_tool)
participant Config Loader
participant DB (Primary)
participant DB (Replay)
participant L2 Client
participant Chunk Proposer
participant Batch Proposer
participant Bundle Proposer
User->>CLI (proposer_tool): Start with config/genesis files
CLI (proposer_tool)->>Config Loader: Load configuration
CLI (proposer_tool)->>DB (Primary): Connect/reset
CLI (proposer_tool)->>DB (Replay): Connect
CLI (proposer_tool)->>L2 Client: Connect
CLI (proposer_tool)->>Chunk Proposer: Initialize with config/DB handles
CLI (proposer_tool)->>Batch Proposer: Initialize with config/DB handles
CLI (proposer_tool)->>Bundle Proposer: Initialize with config/DB handles
CLI (proposer_tool)->>Chunk Proposer: Start propose loop (concurrent)
CLI (proposer_tool)->>Batch Proposer: Start propose loop (concurrent)
CLI (proposer_tool)->>Bundle Proposer: Start propose loop (concurrent)
User-->>CLI (proposer_tool): Send interrupt signal (optional)
CLI (proposer_tool)->>CLI (proposer_tool): Graceful shutdown
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:
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 (
|
colinlyguo
force-pushed
the
feat-add-chunk-batch-bundle-proposing-tool
branch
from
78b7021 to
4e5839f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1645 +/- ##
===========================================
- Coverage 41.22% 40.73% -0.50%
===========================================
Files 222 225 +3
Lines 18167 18416 +249
===========================================
+ Hits 7490 7502 +12
- Misses 9948 10178 +230
- Partials 729 736 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
database/cmd/app/client.go (1)
44-49: 🛠️ Refactor suggestionUnused variable in database reset function.
The function has been updated to use
migrate.ResetDB(db.DB)instead ofmigrate.Rollback(db.DB, &version), which is an improvement. However, there are two issues:
- The
versionvariable on line 44 is now declared but never used.- The log message on line 49 still references the
versionvariable which will always be 0.func resetDB(ctx *cli.Context) error { cfg, err := getConfig(ctx) if err != nil { return err } db, err := initDB(cfg) if err != nil { return err } - var version int64 err = migrate.ResetDB(db.DB) if err != nil { return err } - log.Info("successful to reset", "init version", version) + log.Info("database successfully reset") return nil }
🧹 Nitpick comments (6)
rollup/proposer-tool-db-config.json (1)
1-6: Configuration is correct for development; consider security for production.The database configuration is well-structured and suitable for local or containerized development. For production, avoid hardcoded credentials and consider using environment variables or secrets management.
rollup/README.md (1)
37-64: Well-documented installation and usage instructions for the new proposer tool!The instructions provide clear step-by-step guidance on setting up and running the proposer tool. This documentation is essential for users who want to analyze chunk/batch proposing.
Consider the following minor improvements:
- Add language specifiers to your code blocks (lines 46-49, 57-59, and 62-64)
- Fix the blockquote formatting by removing the blank line between them (lines 51-53)
-``` +```bash cd rollup DOCKER_BUILDKIT=1 docker-compose -f docker-compose-proposer-tool.yml up -dNote: The port 5432 of the database is mapped to the host machine. You can use
psqlor any db clients to connect to the database.
The DSN for the database is
postgres://postgres:postgres@db:5432/scroll?sslmode=disable.-
+bash
docker-compose -f docker-compose-proposer-tool.yml down -v-``` +```bash docker images | grep rollup | awk '{print $3}' | xargs docker rmi -f<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...son`](./proposer-tool-genesis.json). 2. Set proposer config in [`proposer-tool-conf... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) --- [uncategorized] ~51-~51: You might be missing the article “the” here. Context: ...yml up -d ``` > Note: The port 5432 of database is mapped to the host machine. You can ... (AI_EN_LECTOR_MISSING_DETERMINER_THE) </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 44-44: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 46-46: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 52-52: Blank line inside blockquote null (MD028, no-blanks-blockquote) --- 57-57: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 62-62: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>rollup/cmd/proposer_tool/app/app.go (4)</summary><blockquote> `48-54`: **Consider robust error-handling for `NewConfig`.** While the code logs a critical error when `NewConfig` fails, you might want to provide additional context or fallback behavior (e.g., encouraging the user to fix the config path or specifying default configurations) instead of unconditionally exiting the application. This ensures a better user experience and easier error recovery. --- `57-66`: **Improve separation of concerns between context cancellation and DB closure.** Here, `cancel()` is called both before and inside the `defer` block that closes the DB connections. This makes it slightly unclear whether the DB is intended to be closed solely on context cancellation or also on normal returns. Consider clarifying or decoupling these concerns for cleaner shutdown logic. --- `137-141`: **Handle genesis file errors more gracefully.** When the genesis file is missing or invalid, the code unconditionally logs a critical error and exits. Consider providing fallback logic or clearer user guidance to handle scenarios where the genesis file is not found or is malformed. --- `156-158`: **Assess concurrency load on database.** Running `TryProposeChunk`, `TryProposeBatch`, and `TryProposeBundle` every 100ms in separate goroutines can place a high load on the database if frequent proposals occur. Consider adding rate-limiting or dynamic scheduling logic if controlling DB load or resource usage is a concern. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a77f1413eee7d36cc3f4486e861e8011b72f2f1a and 4e5839ff538f97742d5e71eafa64d90406908243. </details> <details> <summary>📒 Files selected for processing (18)</summary> * `common/version/version.go` (1 hunks) * `database/cmd/app/client.go` (1 hunks) * `rollup/README.md` (1 hunks) * `rollup/cmd/proposer_tool/app/app.go` (1 hunks) * `rollup/cmd/proposer_tool/main.go` (1 hunks) * `rollup/cmd/rollup_relayer/app/app.go` (1 hunks) * `rollup/docker-compose-proposer-tool.yml` (1 hunks) * `rollup/internal/config/config.go` (1 hunks) * `rollup/internal/controller/watcher/batch_proposer.go` (2 hunks) * `rollup/internal/controller/watcher/batch_proposer_test.go` (8 hunks) * `rollup/internal/controller/watcher/bundle_proposer_test.go` (1 hunks) * `rollup/internal/controller/watcher/chunk_proposer.go` (6 hunks) * `rollup/internal/controller/watcher/chunk_proposer_test.go` (2 hunks) * `rollup/proposer-tool-config.json` (1 hunks) * `rollup/proposer-tool-db-config.json` (1 hunks) * `rollup/proposer-tool-genesis.json` (1 hunks) * `rollup/proposer_tool.Dockerfile` (1 hunks) * `rollup/tests/rollup_test.go` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (8)</summary> <details> <summary>rollup/internal/config/config.go (3)</summary><blockquote> <details> <summary>rollup/internal/config/l1.go (1)</summary> * `L1Config` (4-11) </details> <details> <summary>rollup/internal/config/l2.go (1)</summary> * `L2Config` (10-27) </details> <details> <summary>database/config.go (1)</summary> * `DBConfig` (12-19) </details> </blockquote></details> <details> <summary>rollup/cmd/proposer_tool/main.go (1)</summary><blockquote> <details> <summary>rollup/cmd/proposer_tool/app/app.go (1)</summary> * `Run` (174-179) </details> </blockquote></details> <details> <summary>rollup/tests/rollup_test.go (1)</summary><blockquote> <details> <summary>rollup/internal/controller/watcher/batch_proposer.go (1)</summary> * `NewBatchProposer` (62-153) </details> </blockquote></details> <details> <summary>rollup/internal/controller/watcher/bundle_proposer_test.go (2)</summary><blockquote> <details> <summary>rollup/internal/controller/watcher/batch_proposer.go (1)</summary> * `NewBatchProposer` (62-153) </details> <details> <summary>rollup/internal/config/l2.go (1)</summary> * `BatchProposerConfig` (44-52) </details> </blockquote></details> <details> <summary>database/cmd/app/client.go (1)</summary><blockquote> <details> <summary>bridge-history-api/internal/orm/migrate/migrate.go (1)</summary> * `ResetDB` (41-46) </details> </blockquote></details> <details> <summary>rollup/internal/controller/watcher/batch_proposer.go (2)</summary><blockquote> <details> <summary>rollup/internal/config/l2.go (1)</summary> * `BatchProposerConfig` (44-52) </details> <details> <summary>rollup/internal/orm/l2_block.go (1)</summary> * `NewL2Block` (43-45) </details> </blockquote></details> <details> <summary>rollup/internal/controller/watcher/chunk_proposer.go (2)</summary><blockquote> <details> <summary>rollup/internal/config/l2.go (1)</summary> * `ChunkProposerConfig` (30-41) </details> <details> <summary>rollup/internal/orm/l2_block.go (1)</summary> * `NewL2Block` (43-45) </details> </blockquote></details> <details> <summary>rollup/cmd/proposer_tool/app/app.go (12)</summary><blockquote> <details> <summary>common/version/version.go (1)</summary> * `Version` (31-31) </details> <details> <summary>common/utils/flags.go (5)</summary> * `CommonFlags` (9-20) * `RollupRelayerFlags` (22-24) * `ConfigFileFlag` (26-30) * `Genesis` (82-86) * `MinCodecVersionFlag` (88-92) </details> <details> <summary>common/utils/logger.go (1)</summary> * `LogSetup` (15-42) </details> <details> <summary>rollup/internal/config/config.go (2)</summary> * `NewConfig` (25-90) * `Config` (17-22) </details> <details> <summary>common/database/db.go (2)</summary> * `InitDB` (48-80) * `CloseDB` (83-92) </details> <details> <summary>rollup/internal/config/l2.go (4)</summary> * `L2Config` (10-27) * `ChunkProposerConfig` (30-41) * `BatchProposerConfig` (44-52) * `BundleProposerConfig` (55-58) </details> <details> <summary>common/types/db.go (1)</summary> * `ProvingTaskVerified` (152-152) </details> <details> <summary>common/utils/utils.go (2)</summary> * `ReadGenesis` (73-84) * `Loop` (48-59) </details> <details> <summary>rollup/internal/controller/watcher/chunk_proposer.go (1)</summary> * `NewChunkProposer` (68-178) </details> <details> <summary>rollup/internal/controller/watcher/batch_proposer.go (1)</summary> * `NewBatchProposer` (62-153) </details> <details> <summary>rollup/internal/controller/watcher/bundle_proposer.go (1)</summary> * `NewBundleProposer` (45-90) </details> <details> <summary>rollup/cmd/rollup_relayer/app/app.go (1)</summary> * `Run` (145-150) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>rollup/README.md</summary> [style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...son`](./proposer-tool-genesis.json). 2. Set proposer config in [`proposer-tool-conf... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) --- [uncategorized] ~51-~51: You might be missing the article “the” here. Context: ...yml up -d ``` > Note: The port 5432 of database is mapped to the host machine. You can ... (AI_EN_LECTOR_MISSING_DETERMINER_THE) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>rollup/README.md</summary> 44-44: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 46-46: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 52-52: Blank line inside blockquote null (MD028, no-blanks-blockquote) --- 57-57: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 62-62: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (34)</summary><blockquote> <details> <summary>common/version/version.go (1)</summary> `8-8`: **Version bump is appropriate.** The version tag update to "v4.5.5" is correct and aligns with the addition of a new feature. </details> <details> <summary>rollup/proposer-tool-genesis.json (1)</summary> `1-19`: **Genesis file is well-formed for tool/test usage.** The genesis configuration is standard and appropriate for initializing a devnet or tool-specific chain. </details> <details> <summary>rollup/internal/config/config.go (1)</summary> `21-21`: **Addition of `DBConfigForReplay` is clear and non-breaking.** The new field enables flexible configuration for replay scenarios and maintains backward compatibility. </details> <details> <summary>rollup/cmd/proposer_tool/main.go (1)</summary> `1-7`: **Entrypoint is correct and idiomatic.** The main function delegates to the application logic as expected for a CLI tool. </details> <details> <summary>rollup/internal/controller/watcher/chunk_proposer_test.go (2)</summary> `205-205`: **Constructor parameter update is consistent with implementation.** The parameters to `NewChunkProposer` have been updated to match the new constructor signature, passing the database handle twice (once for each separate database connection) and adding the `false` flag that's not used by the tool. This change aligns with the updated implementation that supports the new proposer tool. --- `256-256`: **Constructor parameter update is consistent with implementation.** Similar to the previous change, this test case has been correctly updated to match the new constructor signature for `NewChunkProposer`, maintaining consistency across the codebase. </details> <details> <summary>rollup/internal/controller/watcher/bundle_proposer_test.go (2)</summary> `106-106`: **Constructor parameter update is consistent with implementation.** The parameters to `NewChunkProposer` have been updated to match the new constructor signature, passing the database handle twice (once for each separate database connection) and adding the `false` flag that's not used by the tool. This change aligns with the updated implementation that supports the new proposer tool. --- `114-114`: **Constructor parameter update is consistent with implementation.** The `NewBatchProposer` constructor call has been correctly updated to include the additional database parameter, maintaining consistency with the signature changes in the implementation that support the new proposer tool. </details> <details> <summary>rollup/cmd/rollup_relayer/app/app.go (2)</summary> `105-105`: **Constructor parameter update is consistent with implementation.** The `NewChunkProposer` constructor call has been correctly updated to match the new signature, passing the database handle twice (for separate database connections) and adding the `false` flag with an explanatory comment. This maintains consistency with the updated implementation supporting the new proposer tool. --- `106-106`: **Constructor parameter update is consistent with implementation.** The `NewBatchProposer` constructor call has been correctly updated to include the additional database parameter, aligning with the signature changes in the implementation that support the new proposer tool. </details> <details> <summary>rollup/proposer_tool.Dockerfile (1)</summary> `1-30`: **Properly structured multi-stage Docker build for the proposer tool.** The Dockerfile follows best practices with a multi-stage build approach that: 1. Downloads dependencies in a base stage 2. Builds the binary with proper caching in a builder stage 3. Creates a minimal runtime image with only necessary utilities This approach ensures small image size and good rebuild performance through caching. </details> <details> <summary>rollup/tests/rollup_test.go (2)</summary> `98-98`: **Updated constructor call with proper database separation.** The signature update correctly aligns with the changes in `watcher.NewChunkProposer()` constructor to support separate database connections. The boolean flag is properly passed as `false` to indicate this is not being used by the proposer tool. --- `106-106`: **Updated constructor call with proper database separation.** The signature update correctly aligns with the changes in `watcher.NewBatchProposer()` to support separate database connections for standard operations and L2 block operations. </details> <details> <summary>rollup/internal/controller/watcher/batch_proposer.go (2)</summary> `62-62`: **Updated constructor to support separate database connections.** This change properly modifies the function signature to accept both a dedicated L2 block database connection (`l2BlockDB`) and a standard database connection (`db`). This separation allows the proposer tool to use different database instances for replay and analysis purposes. --- `76-76`: **Modified L2BlockOrm initialization to use the dedicated database connection.** The L2BlockOrm is now correctly initialized with the separate `l2BlockDB` parameter, maintaining consistency with the updated constructor signature. This change enables the proposer tool to use a dedicated database for L2 block data while keeping other operations on the standard database. </details> <details> <summary>rollup/internal/controller/watcher/batch_proposer_test.go (5)</summary> `127-127`: **Constructor signature updated to support proposer tool.** The `NewChunkProposer` now accepts two database arguments and an additional boolean flag parameter. The comment clarifies that this flag is specifically for the new proposer tool functionality, which is helpful context. --- `151-151`: **Constructor signature updated to support proposer tool.** The `NewBatchProposer` constructor now accepts an additional database parameter, consistent with the updated implementation that allows separate database handles for different operations. --- `217-217`: **Consistent updating of constructor parameters.** The constructor call uses the same pattern of providing separate database connections and the tool flag parameter as line 127, maintaining consistency throughout the tests. --- `235-235`: **Consistent updating of constructor parameters.** The batch proposer constructor is updated with an additional database parameter, maintaining consistency with the pattern established earlier in the test file. --- `304-304`: **All constructor calls consistently updated.** All occurrences of the `NewChunkProposer` and `NewBatchProposer` constructor calls have been properly updated to use the new signatures, ensuring that the tests remain compatible with the modified implementation. Also applies to: 326-326, 400-400, 417-417 </details> <details> <summary>rollup/internal/controller/watcher/chunk_proposer.go (5)</summary> `39-39`: **New tool-specific flag added to ChunkProposer.** A boolean flag `toolFlag` has been added to the `ChunkProposer` struct to modify behavior when the proposer is used by the analysis tool. --- `68-68`: **Constructor signature extended to support proposer tool.** The `NewChunkProposer` constructor now accepts: 1. A separate database handle `l2BlockDB` for L2 block operations 2. A boolean `toolFlag` parameter to control tool-specific behavior This allows the proposer to connect to different databases and modify its behavior when used by the analysis tool. --- `85-85`: **Using separate database handle and initializing tool flag.** The constructor now: 1. Creates the L2 block ORM using the separate `l2BlockDB` database handle 2. Initializes the `toolFlag` field based on the provided parameter This enables the proposer to use different database connections for different operations, which is crucial for the analysis tool functionality. Also applies to: 95-95 --- `246-251`: **Added conditional logic to skip DB updates when used by tool.** The update to L2 blocks with chunk hash information is now skipped when `toolFlag` is true. This is a crucial change that allows the proposer tool to analyze chunk/batch proposing without modifying the database state, making it safe for analysis purposes. --- `448-448`: **Bypassing parent block validation for tool operation.** The validation that verifies the existence and correctness of the parent block is now conditionally bypassed when `toolFlag` is true. This allows the tool to proceed with analysis even when the complete chain history might not be available in the database. </details> <details> <summary>rollup/docker-compose-proposer-tool.yml (3)</summary> `1-19`: **Well-structured database service configuration.** The PostgreSQL database service is properly configured with: - Environment variables for credentials - Persistent volume for data storage - Health check to ensure service readiness - Exposed port for external connections This provides a solid foundation for the proposer tool's database needs. --- `20-30`: **Database initialization service with proper dependencies.** The `db-client` service: - Builds from an appropriate Dockerfile - Depends on database health before starting - Runs a reset command with mounted configuration - Uses volume mounting to provide configuration This ensures the database is properly initialized before the main tool runs. --- `31-48`: **Main proposer tool service configuration.** The `proposer-tool` service: - Builds from a dedicated Dockerfile - Depends on successful database initialization - Configures the tool with appropriate command-line arguments - Mounts necessary configuration files - Sets restart policy for reliability This configuration enables the tool to run with proper dependencies and settings. </details> <details> <summary>rollup/proposer-tool-config.json (2)</summary> `1-27`: **Comprehensive proposer configuration.** The configuration provides detailed settings for three key components: 1. Chunk proposer - with limits on block numbers, transactions, gas usage, etc. 2. Batch proposer - with constraints on gas, calldata size, timeouts, etc. 3. Bundle proposer - with batch limits and timeout settings These settings are crucial for controlling how the analysis tool processes rollup data. --- `28-33`: **Primary database configuration.** The primary database configuration is properly set up with: - PostgreSQL driver - Connection string that matches the Docker Compose environment - Connection pool settings for optimal performance This enables the tool to connect to the primary database for normal operations. </details> <details> <summary>rollup/cmd/proposer_tool/app/app.go (4)</summary> `39-41`: **Enable code coverage for new CLI flags.** These newly appended flags from `utils.CommonFlags` and `utils.RollupRelayerFlags` are not covered by any tests, which can result in reduced code coverage. Consider adding unit tests or integration tests that verify flag parsing and usage to maintain strong coverage. Would you like a sample test, possibly using an in-memory CLI context, to ensure that the flags are parsed correctly? --- `95-97`: **Validate feasibility of timeouts with the computed time drift.** Adding the entire chain time drift to chunk, batch, and bundle timeouts may cause extremely large or unexpected delays in some environments. Validate that this approach aligns with the intended design and user expectations. Consider bounding the maximum permissible offset or logging a warning if the time drift exceeds a threshold. --- `99-116`: **Verify the correctness of the initial genesis chunk insertion logic.** You insert the genesis chunk and mark it as verified. However, if there are future changes (e.g., different genesis parameters or alternative block 0 definitions), this might cause inconsistencies or require reimplementation. Confirm that the logic is valid for all targeted deployment scenarios. --- `173-179`: **Add direct testing for the `Run` entry point.** Currently, the `Run()` function does not appear to be covered by any tests, according to your code coverage feedback. Consider adding an integration or CLI test to ensure that application startup, flag processing, and graceful shutdown routines function as expected. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
colinlyguo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
rollup/README.md (5)
46-49: Add language specifier to code block for syntax highlighting.For better readability and markdown lint compliance, specify the language (e.g.,
bash) for the fenced code block:-``` +```bash cd rollup DOCKER_BUILDKIT=1 docker-compose -f docker-compose-proposer-tool.yml up -d🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
46-46: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
52-53: Remove blank line inside blockquote for markdown compliance.There is a blank line inside the blockquote, which is flagged by markdown linters. Remove the blank line for consistency:
-> Note: The port 5432 of database is mapped to the host machine. You can use `psql` or any db clients to connect to the database. - > The DSN for the database is `postgres://postgres:postgres@db:5432/scroll?sslmode=disable`.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
52-52: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
57-59: Add language specifier to code block for syntax highlighting.Specify the language (e.g.,
bash) for the fenced code block:-``` +```bash docker-compose -f docker-compose-proposer-tool.yml down -v🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
57-57: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
62-64: Add language specifier to code block for syntax highlighting.Specify the language (e.g.,
bash) for the fenced code block:-``` +```bash docker images | grep rollup | awk '{print $3}' | xargs docker rmi -f🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
62-62: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
41-42: Optional: Vary sentence structure for style.Both sentences begin with "Set". For improved readability, consider rewording:
-1. Set genesis config to enable desired hardforks in [`proposer-tool-genesis.json`](./proposer-tool-genesis.json). -2. Set proposer config in [`proposer-tool-config.json`](./proposer-tool-config.json) for data analysis. +1. Configure genesis to enable desired hardforks in [`proposer-tool-genesis.json`](./proposer-tool-genesis.json). +2. Specify proposer parameters in [`proposer-tool-config.json`](./proposer-tool-config.json) for data analysis.🧰 Tools
🪛 LanguageTool
[style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rollup/README.md(1 hunks)rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/internal/controller/watcher/batch_proposer.go(5 hunks)rollup/internal/controller/watcher/chunk_proposer.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- rollup/cmd/proposer_tool/app/app.go
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/watcher/chunk_proposer.go
🧰 Additional context used
🪛 LanguageTool
rollup/README.md
[style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
rollup/README.md
46-46: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
52-52: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
57-57: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
62-62: 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
🔇 Additional comments (1)
rollup/README.md (1)
37-65: Excellent operational documentation for the proposer tool.The new section is clear, comprehensive, and provides all necessary steps for users to configure, run, and reset the proposer tool environment. The inclusion of Docker Compose instructions and database access details is especially helpful.
🧰 Tools
🪛 LanguageTool
[style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
46-46: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
52-52: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
57-57: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
There was a problem hiding this comment.
Overall looks good, I think there could be better separation of concerns in general though. They are somewhat uncovered through this PR but eg in chunk updating the DB and proposing a chunk should be decoupled imo. Same for in batch where we update the batch hash if run as the proposer tool.
Having said that, I think we can revisit this after EuclidV2 to restructure and simplify the relayer more.
Nice suggestion. Added a task to simplify rollup-relayer logic post |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rollup/README.md (3)
56-56: Specify code block languagesThe fenced code blocks for Docker commands and image cleanup are missing language identifiers, which hinders syntax highlighting. Please add
bash(orshell) after the opening backticks at lines 56, 67, and 72:-``` +```bashAlso applies to: 67-67, 72-72
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
56-56: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
60-63: Remove blank line inside blockquoteThere’s an extra blank line within the blockquote (line 62), causing an unintended paragraph break. You can either remove the blank line or combine the statements:
-> Note: The port 5432 of the database is mapped to the host machine. You can use `psql` or any db clients to connect to the database. -> -> The DSN for the database is `postgres://postgres:postgres@db:5432/scroll?sslmode=disable`. +> Note: The port 5432 of the database is mapped to the host machine. You can use `psql` or any client to connect; the DSN is `postgres://postgres:postgres@db:5432/scroll?sslmode=disable`.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
62-62: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
51-52: Avoid repetitive sentence startsBoth list items begin with “Set,” which feels repetitive. Consider varying the verbs for clarity and style, for example:
-1. Set genesis config to enable desired hardforks in [proposer-tool-genesis.json]. -2. Set proposer config in [proposer-tool-config.json] for data analysis. +1. Configure genesis settings to enable desired hardforks in [proposer-tool-genesis.json]. +2. Adjust proposer parameters in [proposer-tool-config.json] for data analysis.🧰 Tools
🪛 LanguageTool
[style] ~52-~52: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
rollup/README.md
[style] ~52-~52: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
rollup/README.md
56-56: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
62-62: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
67-67: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
72-72: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rollup/README.md (3)
36-46: Maintain concise bullet phrasing
To avoid repetitive sentence openings ("Set"), consider varying the verb in bullet 2. For example, replace “Set proposer config…” with “Configure proposer settings…”:-2. Set proposer config in [`proposer-tool-config.json`](./proposer-tool-config.json) for data analysis. +2. Configure proposer settings in [`proposer-tool-config.json`](./proposer-tool-config.json) for data analysis.
56-59: Specify fenced code block language
Several code blocks omit a language identifier, which impairs syntax highlighting. Addbash(orsh) after each opening fence:- ``` + ```bashApply to:
- Lines 56–59 (docker-compose up)
- Lines 67–69 (docker-compose down)
- Lines 72–74 (docker images removal)
Also applies to: 67-69, 72-74
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
56-56: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
61-63: Remove blank line inside blockquote
Markdown lint flags an empty blockquote line. Remove the stray>on line 62 so the DSN remains a contiguous quote:-> Note: The port 5432 of database is mapped… -> -> The DSN for the database is… +> Note: The port 5432 of database is mapped… +> The DSN for the database is…🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
62-62: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rollup/README.md(1 hunks)rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/docker-compose-proposer-tool.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/cmd/proposer_tool/app/app.go
🧰 Additional context used
🪛 LanguageTool
rollup/README.md
[style] ~52-~52: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
rollup/README.md
56-56: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
62-62: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
67-67: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
72-72: 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
🔇 Additional comments (1)
rollup/docker-compose-proposer-tool.yml (1)
1-40: Compose file sanity check
The Docker Compose configuration cleanly defines thedbandproposer-toolservices with appropriate health checks, volume mounts, build context, and restart policies. Paths to the Dockerfile and JSON config files align with the instructions in the README.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
rollup/internal/orm/chunk.go (2)
281-322: Fix error message to reference correct method name.The error messages reference "Chunk.InsertChunk" instead of "Chunk.InsertTestChunkForProposerTool", which could be confusing during debugging.
- return nil, fmt.Errorf("Chunk.InsertChunk error: %w", err) + return nil, fmt.Errorf("Chunk.InsertTestChunkForProposerTool error: %w", err)- return nil, fmt.Errorf("Chunk.InsertChunk error: %w, chunk hash: %v", err, newChunk.Hash) + return nil, fmt.Errorf("Chunk.InsertTestChunkForProposerTool error: %w, chunk hash: %v", err, newChunk.Hash)
294-308: Consider documenting the minimal field set in test chunks.The method sets fewer fields compared to the regular
InsertChunkmethod. Adding a comment explaining why certain fields (e.g., parent chunk references, compression flags) are not needed for the proposer tool would improve maintainability.rollup/README.md (3)
51-53: Improve list item wording variety.All three configuration steps begin with "Set", which could be improved stylistically.
-1. Set genesis config to enable desired hardforks in [`proposer-tool-genesis.json`](./proposer-tool-genesis.json). -2. Set proposer config in [`proposer-tool-config.json`](./proposer-tool-config.json) for data analysis. -3. Set `start-l2-block` in the launch command of proposer-tool in [`docker-compose-proposer-tool.yml`](./docker-compose-proposer-tool.yml) to the block number you want to start from. The default is `0`, which means starting from the genesis block. +1. Configure genesis settings to enable desired hardforks in [`proposer-tool-genesis.json`](./proposer-tool-genesis.json). +2. Adjust proposer parameters in [`proposer-tool-config.json`](./proposer-tool-config.json) for data analysis. +3. Specify `start-l2-block` in the launch command of proposer-tool in [`docker-compose-proposer-tool.yml`](./docker-compose-proposer-tool.yml) to the block number you want to start from. The default is `0`, which means starting from the genesis block.🧰 Tools
🪛 LanguageTool
[style] ~52-~52: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~53-~53: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tool-config.json) for data analysis. 3. Setstart-l2-blockin the launch command ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
59-62: Add language specifiers to code blocks.Adding language specifiers to fenced code blocks improves syntax highlighting and readability.
-``` +```bash cd rollup DOCKER_BUILDKIT=1 docker-compose -f docker-compose-proposer-tool.yml up -d```diff -``` +```bash docker-compose -f docker-compose-proposer-tool.yml down -v```diff -``` +```bash docker images | grep rollup | awk '{print $3}' | xargs docker rmi -fAlso applies to: 70-72, 75-77 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 59-59: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `64-67`: **Fix blockquote formatting.** Remove the blank line between blockquotes for consistent Markdown rendering. ```diff > Note: The port 5432 of database is mapped to the host machine. You can use `psql` or any db clients to connect to the database. -> The DSN for the database is `postgres://postgres:postgres@db:5432/scroll?sslmode=disable`. +> The DSN for the database is `postgres://postgres:postgres@db:5432/scroll?sslmode=disable`.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
65-65: Blank line inside blockquote
null(MD028, no-blanks-blockquote)
rollup/docker-compose-proposer-tool.yml (1)
20-37: Consider parameterizing the start-l2-block value.The start block number is hardcoded to 10000, which may require frequent Docker Compose file edits when testing different scenarios.
command: [ "--config", "/app/conf/proposer-tool-config.json", "--genesis", "/app/conf/proposer-tool-genesis.json", "--min-codec-version", "4", - "--start-l2-block", "10000", + "--start-l2-block", "${START_L2_BLOCK:-10000}", "--log.debug", "--verbosity", "3" ]This change would allow you to specify the start block through an environment variable while keeping 10000 as the default if not specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/utils/flags.go(2 hunks)rollup/README.md(1 hunks)rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/docker-compose-proposer-tool.yml(1 hunks)rollup/internal/orm/chunk.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/cmd/proposer_tool/app/app.go
🧰 Additional context used
🪛 LanguageTool
rollup/README.md
[style] ~52-~52: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...son](./proposer-tool-genesis.json). 2. Set proposer config in [proposer-tool-conf...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~53-~53: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tool-config.json) for data analysis. 3. Set start-l2-block in the launch command ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
rollup/README.md
59-59: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
65-65: Blank line inside blockquote
null
(MD028, no-blanks-blockquote)
70-70: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
75-75: 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
🔇 Additional comments (5)
common/utils/flags.go (2)
25-28: LGTM! Well-organized addition of proposer tool flags.The new
ProposerToolFlagsslice follows the established pattern of organizing flags by their usage context, similar toCommonFlagsandRollupRelayerFlags.
97-102: LGTM! Clear and appropriate flag definition.The
StartL2BlockFlagis well-defined with:
- Appropriate type (Uint64Flag) for a block number
- Clear naming and usage description
- Sensible default value
rollup/internal/orm/chunk.go (1)
183-199: LGTM! Well-implemented parent chunk retrieval method.This method efficiently retrieves the most recent chunk before a specified block number, with proper error handling and consistent style.
rollup/README.md (1)
37-46: LGTM! Clear and concise introduction to the proposer tool.The description effectively explains the purpose and capabilities of the tool, helping users understand its value for protocol upgrade testing.
rollup/docker-compose-proposer-tool.yml (1)
4-19: LGTM! Well-configured database service with appropriate health checking.The PostgreSQL configuration includes:
- Proper image versioning (postgres:14)
- Environment variables for authentication
- Volume mapping for data persistence
- Health check to ensure database readiness
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
rollup/internal/controller/watcher/proposer_tool.go (3)
9-15:⚠️ Potential issueDuplicate
logimport – build currently failsThe two consecutive imports bring
github.com/ethereum/go-ethereum/logandgithub.com/scroll-tech/go-ethereum/loginto the same default alias, triggering the Go compiler error reported in CI (log redeclared in this block).- "github.com/ethereum/go-ethereum/log" ... - "github.com/scroll-tech/go-ethereum/log" + // keep only ONE import; most code in this repo uses scroll‑tech’s fork + "github.com/scroll-tech/go-ethereum/log"🧰 Tools
🪛 golangci-lint (1.64.8)
14-14: log redeclared in this block
(typecheck)
9-9: other declaration of log
(typecheck)
🪛 GitHub Actions: Rollup
[error] 14-14: Go build error: 'log' redeclared in this block.
83-87: 🛠️ Refactor suggestionUse the tool’s context instead of
context.Background()Invoking
client.BlockByNumberwithcontext.Background()prevents graceful shutdown because the RPC call will outliveProposerTool.Stop(). Re‑usep.ctxso cancellation propagates:- block, err := client.BlockByNumber(context.Background(), big.NewInt(int64(blockNum))) + block, err := client.BlockByNumber(p.ctx, big.NewInt(int64(blockNum)))
103-104: 🛠️ Refactor suggestionCodec version still hard‑coded to
CodecV0
InsertTestChunkForProposerToolreceivesencoding.CodecV0, ignoring theminCodecVersionparameter passed toNewProposerTool. This can cause codec mismatches once the minimum version bumps.- _, err = orm.NewChunk(db).InsertTestChunkForProposerTool(ctx, chunk, encoding.CodecV0, startQueueIndex) + _, err = orm.NewChunk(db).InsertTestChunkForProposerTool(ctx, chunk, minCodecVersion, startQueueIndex)
🧹 Nitpick comments (1)
rollup/internal/controller/watcher/proposer_tool.go (1)
82-99: Block‑by‑block scan can be very slow for large heightsThe sequential
forloop fetches every block fromstartBlocktostartL2BlockHeightvia individual RPC calls. For tens of thousands of blocks this will take minutes and stress the node.Options:
- Use
eth_getLogs/eth_getBlockRangestyle batched queries if available.- Parallelise calls with a bounded worker pool.
- Cache previously scanned ranges to disk.
Not critical for a one‑off tool but worth tracking if analysts process long ranges regularly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/internal/controller/watcher/proposer_tool.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/controller/watcher/proposer_tool.go (8)
rollup/internal/controller/watcher/chunk_proposer.go (2)
ChunkProposer(22-65)NewChunkProposer(68-178)rollup/internal/controller/watcher/batch_proposer.go (2)
BatchProposer(24-62)NewBatchProposer(65-157)rollup/internal/controller/watcher/bundle_proposer.go (2)
BundleProposer(21-42)NewBundleProposer(45-90)rollup/internal/config/config.go (1)
ConfigForReplay(26-29)common/database/db.go (2)
InitDB(48-80)CloseDB(83-92)bridge-history-api/internal/orm/migrate/migrate.go (1)
ResetDB(41-46)rollup/internal/config/l2.go (4)
L2Config(10-27)BatchProposerConfig(44-52)ChunkProposerConfig(30-41)BundleProposerConfig(55-58)common/utils/utils.go (1)
Loop(48-59)
🪛 golangci-lint (1.64.8)
rollup/internal/controller/watcher/proposer_tool.go
14-14: log redeclared in this block
(typecheck)
9-9: other declaration of log
(typecheck)
🪛 GitHub Actions: Rollup
rollup/internal/controller/watcher/proposer_tool.go
[error] 14-14: Go build error: 'log' redeclared in this block.
colinlyguo
force-pushed
the
feat-add-chunk-batch-bundle-proposing-tool
branch
from
4201ae8 to
b80e59a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rollup/internal/controller/watcher/proposer_tool.go (1)
40-151: Well-structured constructor but inconsistent codec version usageThe constructor is well-structured and follows a logical flow for initialization. However, there's an inconsistency in the codec version usage:
- dbBatch, err = orm.NewBatch(db).InsertBatch(ctx, batch, encoding.CodecV0, rutils.BatchMetrics{}) + dbBatch, err = orm.NewBatch(db).InsertBatch(ctx, batch, minCodecVersion, rutils.BatchMetrics{})Using the same
minCodecVersionparameter for both chunk and batch insertion ensures consistency.
🧹 Nitpick comments (3)
rollup/internal/controller/watcher/proposer_tool.go (3)
82-98: Potential performance issue with sequential block retrievalThe current implementation sequentially retrieves blocks one by one, which could be inefficient for large block ranges. Consider batch fetching or using a more efficient block retrieval mechanism, especially when the difference between
startBlockandstartL2BlockHeightis large.You might consider implementing a batch retrieval approach that processes blocks in chunks to improve performance:
- for blockNum := startBlock; blockNum <= startL2BlockHeight; blockNum++ { - block, err := client.BlockByNumber(ctx, new(big.Int).SetUint64(blockNum)) - if err != nil { - return nil, fmt.Errorf("failed to get block %d: %w", blockNum, err) - } + const batchSize = 100 // Adjust based on expected performance characteristics + for startBatch := startBlock; startBatch <= startL2BlockHeight; startBatch += batchSize { + endBatch := startBatch + batchSize - 1 + if endBatch > startL2BlockHeight { + endBatch = startL2BlockHeight + } + + log.Info("Processing block batch", "start", startBatch, "end", endBatch) + + for blockNum := startBatch; blockNum <= endBatch; blockNum++ { + block, err := client.BlockByNumber(ctx, new(big.Int).SetUint64(blockNum)) + if err != nil { + return nil, fmt.Errorf("failed to get block %d: %w", blockNum, err) + }
153-157: Consider adding log statements for debugging loop executionThe
Startmethod launches background goroutines but lacks logging. Adding log statements would help with monitoring and debugging.func (p *ProposerTool) Start() { + log.Info("Starting ProposerTool loops") go utils.Loop(p.ctx, 100*time.Millisecond, p.chunkProposer.TryProposeChunk) go utils.Loop(p.ctx, 100*time.Millisecond, p.batchProposer.TryProposeBatch) go utils.Loop(p.ctx, 100*time.Millisecond, p.bundleProposer.TryProposeBundle) + log.Info("ProposerTool loops started") }
159-168: Add completion confirmation for clean shutdownThe
Stopmethod handles resource cleanup well, but adding a final log statement would help confirm successful shutdown.func (p *ProposerTool) Stop() { + log.Info("Stopping ProposerTool") p.cancel() if err := database.CloseDB(p.db); err != nil { log.Error("failed to close db connection", "error", err) } if err := database.CloseDB(p.dbForReplay); err != nil { log.Error("failed to close dbForReplay connection", "error", err) } p.client.Close() + log.Info("ProposerTool stopped successfully") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/internal/controller/watcher/proposer_tool.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/cmd/proposer_tool/app/app.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (1)
rollup/internal/controller/watcher/proposer_tool.go (1)
97-97: Consider including full block data for the chunkThe current implementation only includes the block header in the chunk, but not the transactions. Verify if this is intentional and sufficient for your analysis needs.
Is including only the block header (without transactions) in the chunk sufficient for your analysis? If you need transaction data as well, consider:
- chunk = &encoding.Chunk{Blocks: []*encoding.Block{{Header: block.Header()}}} + chunk = &encoding.Chunk{Blocks: []*encoding.Block{{Header: block.Header(), Transactions: block.Transactions()}}}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rollup/internal/controller/watcher/chunk_proposer.go (1)
183-186: Potential thread safety issue with SetReplayDBWhile the method has a comment about thread safety, the implementation doesn't have any synchronization mechanisms. Consider adding a mutex lock if there's any possibility this could be called concurrently with other operations.
// SetReplayDB sets the replay database for the ChunkProposer. // This is used for the proposer tool only, to change the l2_block data source. // This function is not thread-safe and should be called after initializing the ChunkProposer and before starting to propose chunks. func (p *ChunkProposer) SetReplayDB(replayDB *gorm.DB) { + // Ensure this is called before the proposer starts running p.l2BlockOrm = orm.NewL2Block(replayDB) p.replayMode = true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/internal/controller/watcher/chunk_proposer.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/controller/watcher/chunk_proposer.go (1)
rollup/internal/orm/l2_block.go (1)
NewL2Block(43-45)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (4)
rollup/internal/controller/watcher/chunk_proposer.go (4)
39-39: LGTM: Adding replayMode flag for proposer toolThe new boolean field
replayModeand its initialization in the constructor look good. This will properly support the replay functionality needed for the proposer tool.Also applies to: 95-95
254-260: LGTM: Conditional skip of L2 block updates during replayThe condition to skip updating chunk hash in L2 blocks during replay mode is appropriate. This prevents modifications to the L2 blocks table when replaying historical data.
451-456: LGTM: Early return in tryProposeEuclidTransitionChunk for replay modeThe early return when in replay mode correctly bypasses the parent block validation, which could fail when the genesis block is missing in the mainnet database.
183-186:✅ Verification successful
Consider updating the main db field for consistency
The
SetReplayDBmethod updatesl2BlockOrmbut doesn't update the maindbfield in the struct. This means transactions inupdateDBChunkInfostill use the main database connection.Verify if this is intentional - you may want to use the replay database for reading block data but write chunk data to the main database, or you might want to isolate all operations to the replay database.
🏁 Script executed:
#!/bin/bash # Check how the proposer tool uses the database connections grep -r "SetReplayDB" --include="*.go" rollup/ grep -A 10 "updateDBChunkInfo" rollup/internal/controller/watcher/proposer_tool.goLength of output: 765
🏁 Script executed:
#!/bin/bash # Find the chunkProposer’s updateDBChunkInfo method grep -R "func (p \\*ChunkProposer) updateDBChunkInfo" -n rollup/internal/controller/watcher/chunk_proposer.go # Inspect the ChunkProposer struct to see its DB-related fields grep -R "type ChunkProposer" -n rollup/internal/controller/watcher/chunk_proposer.go # Show the top of the file to confirm field definitions head -n 50 rollup/internal/controller/watcher/chunk_proposer.goLength of output: 1980
🏁 Script executed:
#!/bin/bash # Locate constructor NewChunkProposer grep -R "NewChunkProposer" -n rollup/internal/controller/watcher/chunk_proposer.go # Show constructor for context sed -n '50,150p' rollup/internal/controller/watcher/chunk_proposer.go # Show updateDBChunkInfo implementation sed -n '180,260p' rollup/internal/controller/watcher/chunk_proposer.goLength of output: 8498
SetReplayDB intentionally only switches the L2‐block data source
The method’s GoDoc clearly states it “changes the l2_block data source” for replay mode. By design, p.db and chunkOrm remain pointed at the original DB so that chunk inserts/transactions always go to the primary database—even in replayMode (where only the block‐read ORM is swapped). No changes required.
Purpose or design rationale of this PR
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
Documentation