feat: add blob storage service(WIP) #1672
Conversation
WalkthroughThis update introduces a new "blob uploader" component for uploading data blobs to AWS S3, including CLI, controller logic, metrics, configuration, Docker integration, and database schema migration. Supporting types, utility functions, and tests are added. The Makefile, GitHub Actions workflow, and configuration files are updated to build, test, and deploy the new component. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as blob-uploader CLI
participant Cfg as Config Loader
participant DB as Database
participant BU as BlobUploader
participant S3 as AWS S3
participant Metrics as Prometheus
CLI->>Cfg: Load config file
CLI->>DB: Connect to database
CLI->>BU: Initialize BlobUploader(ctx, DB, config, Metrics)
loop Every 2 seconds
BU->>DB: Get first unuploaded batch
alt Batch found
BU->>DB: Fetch chunks and L2 blocks
BU->>BU: Construct blob, calculate hash
BU->>S3: Upload blob data
alt Upload success
BU->>DB: Update blob_upload status: uploaded
BU->>Metrics: Increment success counter
else Upload failed
BU->>DB: Update blob_upload status: failed
BU->>Metrics: Increment failed counter
end
else No batch
BU-->>CLI: No action
end
end
CLI->>DB: Graceful shutdown
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd"" ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
yiweichi
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1672 +/- ##
===========================================
- Coverage 40.86% 40.26% -0.61%
===========================================
Files 225 232 +7
Lines 18075 18396 +321
===========================================
+ Hits 7387 7407 +20
- Misses 9970 10265 +295
- Partials 718 724 +6
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: 13
🧹 Nitpick comments (11)
common/utils/ethereum.go (1)
10-23: Well-implemented utility function following KZG4844 specification.The function correctly implements the two-step process of converting blob to commitment and then to versioned hash. Error handling is proper with meaningful error messages.
Consider enhancing the function documentation to be more descriptive:
-// CalculateVersionedBlobHash calculate the kzg4844 versioned blob hash from a blob +// CalculateVersionedBlobHash calculates the KZG4844 versioned blob hash from a blob. +// It follows the EIP-4844 specification by first computing the KZG commitment +// and then deriving the versioned hash using SHA-256.rollup/internal/controller/blob_uploader/blob_uploader_metrics.go (1)
24-24: Improve metric help text clarity.The help text contains grammatical errors and could be more descriptive.
- Help: "The total number of upload blob to S3 run total", + Help: "The total number of blobs uploaded to S3",common/utils/ethereum_test.go (1)
31-31: Remove debugging output.The
fmt.Printlnstatement should be removed as it's not needed in production test code and adds unnecessary noise to test output.- fmt.Println(blobData.BlobData)rollup/cmd/blob_uploader/app/app.go (1)
42-43: Incorrect app registration comment.The comment mentions "rollup-relayer-test" but this is a blob-uploader app, not a rollup-relayer.
- // Register `rollup-relayer-test` app for integration-test. + // Register `blob-uploader` app for integration-test.rollup/internal/controller/blob_uploader/s3_sender.go (1)
50-50: Consider making timeout configurable.The 30-second timeout is hard-coded, which might not be suitable for all deployment scenarios or blob sizes. Consider making this configurable through the config struct.
In the
config.AWSS3Configstruct, add:Timeout time.Duration `json:"timeout"`Then use it in the constructor:
+ timeout := cfg.Timeout + if timeout == 0 { + timeout = 30 * time.Second // default fallback + } return &S3Uploader{ client: s3.NewFromConfig(awsCfg), bucket: cfg.Bucket, region: cfg.Region, - timeout: 30 * time.Second, + timeout: timeout, }, nilrollup/internal/controller/blob_uploader/blob_uploader.go (1)
88-88: Add missing space in comment.Minor formatting issue.
- // calculate versioned blob hash + // calculate versioned blob hashdatabase/migrate/migrations/00027_ blob_upload.sql (5)
1-1: File name formatting
The migration file name includes an unintended space (00027_ blob_upload.sql), which could break goose’s version ordering. Rename it to remove the space (e.g.00027_blob_upload.sql).
4-9: Use timezone-aware timestamps
Switchupdated_at TIMESTAMPtoTIMESTAMPTZ(TIMESTAMP WITH TIME ZONE) to ensure consistent handling of time zones across environments. Align this with your existing conventions:- updated_at TIMESTAMP NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(),
11-13: Enforce referential integrity on batch deletions
Consider addingON DELETE CASCADE(or another behavior) to the foreign key so that removing a parentbatchautomatically cleans up itsblob_uploadrecords. Also, quote the reserved column name if needed:- FOREIGN KEY (batch_index) REFERENCES batch(index) + FOREIGN KEY (batch_index) REFERENCES batch("index") ON DELETE CASCADE
4-9: Add domain constraints for data integrity
To prevent invalid entries, addCHECKconstraints (or use native enum types) onstatusandplatform. For example:CREATE TABLE blob_upload ( batch_index BIGINT NOT NULL, platform TEXT NOT NULL CHECK (platform IN ('s3','arweave')), status SMALLINT NOT NULL CHECK (status IN (0,1,2,3)), updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), PRIMARY KEY (batch_index, platform), FOREIGN KEY (batch_index) REFERENCES batch("index") ON DELETE CASCADE );This enforces the same rules as your application code at the database level.
19-22: Harden the Down migration
Make the drop idempotent and cascade any dependent objects to avoid errors if the table doesn’t exist or has dependencies:- DROP TABLE blob_upload; + DROP TABLE IF EXISTS blob_upload CASCADE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
common/types/db.go(1 hunks)common/utils/ethereum.go(1 hunks)common/utils/ethereum_test.go(1 hunks)database/migrate/migrations/00027_ blob_upload.sql(1 hunks)database/migrate/migrations/00028_add_blob_upload_indexes.sql(1 hunks)rollup/cmd/blob_uploader/app/app.go(1 hunks)rollup/cmd/blob_uploader/main.go(1 hunks)rollup/go.mod(1 hunks)rollup/internal/config/l2.go(2 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader_metrics.go(1 hunks)rollup/internal/controller/blob_uploader/s3_sender.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)rollup/internal/orm/blob_upload.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
rollup/cmd/blob_uploader/main.go (1)
rollup/cmd/blob_uploader/app/app.go (1)
Run(149-154)
rollup/internal/orm/batch.go (2)
tests/integration-test/orm/batch.go (2)
Batch(17-60)Batch(68-70)common/types/db.go (2)
BlobStoragePlatform(358-358)BlobUploadStatusFailed(341-341)
common/utils/ethereum_test.go (1)
common/utils/ethereum.go (1)
CalculateVersionedBlobHash(11-23)
rollup/internal/controller/blob_uploader/s3_sender.go (1)
rollup/internal/config/l2.go (1)
AWSS3Config(58-63)
rollup/internal/controller/blob_uploader/blob_uploader.go (5)
rollup/internal/config/l2.go (2)
BlobUploaderConfig(53-55)AWSS3Config(58-63)rollup/internal/controller/blob_uploader/s3_sender.go (2)
S3Uploader(18-23)NewS3Uploader(25-52)rollup/internal/orm/batch.go (3)
Batch(22-72)Batch(80-82)NewBatch(75-77)common/types/db.go (1)
BlobStoragePlatformS3(364-364)common/utils/ethereum.go (1)
CalculateVersionedBlobHash(11-23)
rollup/internal/orm/blob_upload.go (2)
bridge-history-api/internal/orm/migrate/migrate.go (1)
Status(54-56)common/types/db.go (3)
BlobUploadStatus(331-331)BlobUploadStatusPending(337-337)BlobUploadStatusFailed(341-341)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (17)
rollup/go.mod (1)
25-42:✅ Verification successful
AWS SDK dependencies look appropriate for the blob uploader feature.
The added AWS SDK v2 dependencies provide comprehensive support for S3 operations and are correctly marked as indirect dependencies.
Let me verify that these dependency versions are current and secure:
🏁 Script executed:
#!/bin/bash # Description: Check for the latest AWS SDK versions and any security advisories echo "Checking latest AWS SDK Go v2 versions..." curl -s https://api.github.com/repos/aws/aws-sdk-go-v2/releases/latest | jq -r '.tag_name' echo "Checking for any known vulnerabilities in AWS SDK Go..." gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/aws/aws-sdk-go-v2") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 685
AWS SDK v2 dependencies are up-to-date and have no known vulnerabilities
- Latest AWS SDK Go v2 release tag is release-2025-05-30, so the current versions (e.g. core v1.36.3, S3 v1.80.0, STS v1.33.19) are recent and only one release behind.
- GitHub’s security advisories API returned no known vulnerabilities for
github.com/aws/aws-sdk-go-v2.- All AWS SDK modules remain correctly marked as
// indirectinrollup/go.mod(lines 25–42).No further changes required.
common/types/db.go (1)
330-355: Well-structured enum implementation following established patterns.The
BlobUploadStatusenum and itsString()method follow the consistent pattern used throughout the file with proper error handling for undefined values.database/migrate/migrations/00028_add_blob_upload_indexes.sql (1)
1-22: Well-structured database migration with appropriate indexes.The migration properly adds performance-optimizing indexes for the blob_upload table. The index choices align well with the expected query patterns:
statusindex for filtering by upload statusupdated_atindex for time-based queries and sorting- Composite
(batch_index, status)index for efficient batch status lookupsThe rollback section correctly drops all created indexes.
rollup/internal/config/l2.go (1)
27-28: Configuration structure looks good.The addition of
BlobUploaderConfigfollows the existing pattern and integrates well with the L2Config structure.common/utils/ethereum_test.go (1)
21-21:✅ Verification successful
Verify test data file path exists.
The test relies on a specific file path that may not exist. Ensure the test data file is properly included in the repository.
🏁 Script executed:
#!/bin/bash # Description: Check if the test data file exists and verify its structure # Expected: Find the blobdata.json file and confirm it has the expected structure # Find the test data file fd "blobdata.json" --type f # If found, examine its structure if [ -f "common/testdata/blobdata.json" ]; then echo "File contents:" cat common/testdata/blobdata.json | jq . else echo "Test data file not found. Checking alternative locations:" fd "testdata" --type d fiLength of output: 65719
Test data file path exists and is valid
Verified thatcommon/testdata/blobdata.jsonexists and contains valid JSON. The test’s relative path (../testdata/blobdata.json) fromcommon/utils/ethereum_test.goresolves correctly.rollup/cmd/blob_uploader/app/app.go (2)
82-97: Configuration validation logic is comprehensive.The sanity checks for batch submission and chunk proposal limits are thorough and provide clear error messages, which is excellent for operational debugging.
99-102:✅ Verification successful
Verify Layer2Relayer usage in blob uploader context.
The application creates a Layer2Relayer with
ServiceTypeL2RollupRelayer, but it's unclear if this component is actually needed for the blob uploader functionality or if it's leftover from copying from another service.
🏁 Script executed:
#!/bin/bash # Description: Check if Layer2Relayer is actually used in blob uploader context # Expected: Find usage of l2relayer variable or determine if it's needed # Search for l2relayer usage rg "l2relayer" -A 3 -B 3 # Check what ServiceTypeL2RollupRelayer does ast-grep --pattern "ServiceTypeL2RollupRelayer"Length of output: 4378
No changes needed—Layer2Relayer is actively used in blob uploader
The
l2relayerinstance is invoked in two background loops, so its instantiation withServiceTypeL2RollupRelayeris required:
- In rollup/cmd/blob_uploader/app/app.go you call:
go utils.Loop(subCtx, 2*time.Second, l2relayer.ProcessPendingBatches)go utils.Loop(subCtx, 15*time.Second, l2relayer.ProcessPendingBundles)These confirm that the relayer isn’t leftover code but a core part of blob-uploader functionality.
rollup/internal/controller/blob_uploader/s3_sender.go (2)
31-39: Good credential handling with conditional static credentials.The implementation properly handles both default AWS credential chain and static credentials, which provides flexibility for different deployment environments.
63-63: Appropriate content type for blob data.Using
application/octet-streamis correct for arbitrary binary blob data, ensuring proper handling by S3 and downstream consumers.rollup/internal/orm/blob_upload.go (6)
14-31: LGTM! Well-structured ORM model.The
BlobUploadstruct is properly designed with composite primary keys and appropriate field types. The TableName method correctly returns the table name.
34-47: LGTM! Proper implementation of insert method.The method correctly creates a new blob upload record with all required fields and proper error handling.
50-64: LGTM! Correct status update implementation.The method properly updates both status and timestamp using the composite primary key.
67-80: LGTM! Good error handling pattern.The method correctly handles the not-found case by returning nil without an error, which is a good practice for optional lookups.
83-108: LGTM! Consistent implementation for status-based queries.Both
GetPendingBlobUploadsandGetFailedBlobUploadscorrectly filter by status and order results by batch index for predictable processing order.
111-120: LGTM! Proper delete implementation.The method correctly deletes records using the composite primary key.
rollup/internal/controller/blob_uploader/blob_uploader.go (1)
107-152: LGTM! Well-implemented blob construction logic.The method properly validates codec version consistency between batches and chunks, and correctly constructs the blob from L2 blocks with comprehensive error handling.
database/migrate/migrations/00027_ blob_upload.sql (1)
1-17: Migration scaffolding is well-structured.
The gooseUp/Downmarkers and the table definition correctly establish your newblob_uploadschema for tracking upload statuses.
rollup/internal/controller/blob_uploader/blob_uploader_metrics.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
rollup/internal/controller/blob_uploader/blob_uploader.go (2)
58-58: Fix the typo in metrics initialization function name.The function name has incorrect casing and needs to be corrected.
Apply this fix:
- blobUploader.metrics = initblobUploaderMetrics(reg) + blobUploader.metrics = initBlobUploaderMetrics(reg)
80-89: 🛠️ Refactor suggestionConsider updating blob upload status on construction and hashing failures.
While S3 upload failures are properly handled with database status updates, construction and hashing failures (lines 82-84, 87-88, 94-95) only log errors without updating the blob upload status. This could result in these batches being retried indefinitely.
Consider adding status updates for these error cases:
case encoding.CodecV7: blob, err = b.constructBlobCodecV7(dbBatch) if err != nil { log.Error("failed to construct constructBlobCodecV7 payload for V7", "codecVersion", codecVersion, "batch index", dbBatch.Index, "err", err) + if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); updateErr != nil { + log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", updateErr) + } return } default: log.Error("unsupported codec version in UploadBlobToS3", "codecVersion", codecVersion, "batch index", dbBatch.Index) + if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); updateErr != nil { + log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", updateErr) + } return
🧹 Nitpick comments (4)
rollup/internal/orm/blob_upload.go (1)
97-140: Query methods are well-implemented with proper error handling.The query methods provide good functionality:
GetBlobUploadByBatchIndexAndPlatformproperly handles record not found case- Pending and failed queries use appropriate ordering by batch_index
- Consistent error handling patterns across all methods
- Proper use of GORM query building
One minor formatting issue:
func (o *BlobUpload) GetFailedBlobUploadsByPlatform(ctx context.Context, platform types.BlobStoragePlatform) ([]*BlobUpload, error) { - db := o.db.WithContext(ctx)database/migrate/migrations/00027_ blob_upload.sql (3)
18-18: Optional: Use PostgreSQLENUMforstatusType safety & clarity: Defining a native enum rather than a SMALLINT with a comment prevents invalid states and makes the schema self-documenting.
Suggested preamble:
CREATE TYPE blob_upload_status AS ENUM ('undefined', 'pending', 'uploaded', 'failed');Then in your table:
- status SMALLINT NOT NULL, + status blob_upload_status NOT NULL,
20-31: Optional: Consolidate partial indexes to reduce overheadMaintainability & performance: Six separate partial indexes may introduce write overhead. If your primary queries filter by
(batch_index, status, platform), the compound indexidx_blob_upload_batch_index_status_platformcan serve as a prefix index for many patterns. Review query profiles and consider dropping redundant indexes, for example:-- Drop single‐column or less‐selective indexes if not used: -DROP INDEX IF EXISTS idx_blob_upload_batch_index; -DROP INDEX IF EXISTS idx_blob_upload_status; -DROP INDEX IF EXISTS idx_blob_upload_status_platform;
36-36: Nitpick: MakeDROP TABLEidempotentResilience in rollbacks: Adding
IF EXISTSavoids errors when the table is absent.-DROP TABLE blob_upload; +DROP TABLE IF EXISTS blob_upload;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
database/migrate/migrations/00027_ blob_upload.sql(1 hunks)rollup/Makefile(1 hunks)rollup/cmd/blob_uploader/app/app.go(1 hunks)rollup/conf/config.json(1 hunks)rollup/internal/config/l2.go(2 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)rollup/internal/orm/blob_upload.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- rollup/conf/config.json
- rollup/Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- rollup/cmd/blob_uploader/app/app.go
- rollup/internal/config/l2.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (4)
rollup/internal/controller/blob_uploader/blob_uploader.go (2)
63-118: Database status updates have been properly implemented.The implementation now correctly handles database status updates for both success and failure cases, addressing the previous concerns:
- Lines 104-106: Updates status to failed when S3 upload fails
- Lines 111-114: Updates status to uploaded when S3 upload succeeds
- Proper error logging for both upload failures and database update failures
The method also includes metrics tracking and comprehensive logging.
120-165: Blob construction method is well-implemented.The
constructBlobCodecV7method properly:
- Validates codec version consistency between batch and chunks
- Retrieves all necessary data (chunks and L2 blocks) using ORM methods
- Constructs the encoding batch with proper field mapping
- Uses the appropriate codec for blob generation
- Provides comprehensive error handling with detailed context
The implementation follows good practices and handles edge cases appropriately.
rollup/internal/orm/blob_upload.go (2)
1-32: Well-structured ORM model following GORM best practices.The
BlobUploadstruct and constructor follow standard GORM patterns:
- Proper field tags for database mapping
- Composite primary key correctly defined
- Database instance excluded from serialization with
gorm:"-"- Clean constructor pattern
The use of enum types from
common/typesensures type safety across the codebase.
34-95: Excellent transaction and error handling patterns.The CRUD methods demonstrate good practices:
- Consistent transaction support through optional
dbTXparameter- Proper context propagation throughout
- Detailed error messages with relevant context (batch index, platform)
- Upsert method uses appropriate
ON CONFLICTclause for composite key- Time stamps are properly managed
The implementation handles both individual operations and batch scenarios effectively.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
rollup/internal/controller/blob_uploader/blob_uploader_metrics.go (1)
20-20: Fix function name casing.The function name
initblobUploaderMetricsstill has incorrect casing. It should follow Go naming conventions.Apply this fix:
-func initblobUploaderMetrics(reg prometheus.Registerer) *blobUploaderMetrics { +func initBlobUploaderMetrics(reg prometheus.Registerer) *blobUploaderMetrics {rollup/internal/controller/blob_uploader/blob_uploader.go (1)
58-58: Fix the typo in metrics initialization function name.The function name has incorrect casing, matching the issue in the metrics file.
Apply this fix:
- blobUploader.metrics = initblobUploaderMetrics(reg) + blobUploader.metrics = initBlobUploaderMetrics(reg)
🧹 Nitpick comments (1)
rollup/internal/controller/blob_uploader/blob_uploader_metrics.go (1)
25-25: Improve metric help text grammar and clarity.The help text for both metrics contains grammatical errors and unclear phrasing.
Apply these fixes for better clarity:
- Help: "The total number of upload blob to S3 run success total", + Help: "The total number of successful blob uploads to S3",- Help: "The total number of upload blob to S3 run failed total", + Help: "The total number of failed blob uploads to S3",Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader_metrics.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rollup/internal/controller/blob_uploader/blob_uploader.go (1)
58-58: Fix the typo in metrics initialization function name.The function name has incorrect casing, as previously identified in past reviews.
Apply this fix:
- blobUploader.metrics = initblobUploaderMetrics(reg) + blobUploader.metrics = initBlobUploaderMetrics(reg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/blob_uploader/arweave_sender.go(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rollup/internal/controller/blob_uploader/arweave_sender.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (2)
rollup/internal/controller/blob_uploader/blob_uploader.go (2)
63-123: LGTM! Comprehensive error handling and status tracking implemented.The upload method properly handles all error cases with appropriate database status updates and metrics tracking. The implementation addresses all the concerns from previous reviews regarding database status updates on both success and failure scenarios.
125-198: Well-structured blob construction with proper codec version handling.The method correctly handles different codec versions and includes appropriate error handling for database operations. The logic for constructing blobs for different codec versions is sound and the error messages are informative.
database/migrate/migrations/00027_alter_batch_add_primary_key_index.sql
Outdated
Show resolved
Hide resolved
rollup/internal/controller/blob_uploader/blob_uploader_metrics.go
Outdated
Show resolved
Hide resolved
Co-authored-by: colin <[email protected]>
Co-authored-by: colin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
common/utils/ethereum_test.go (1)
37-38: Add blob size validation before copying.The code copies bytes to a
kzg4844.Blobwithout checking if the byte slice length matches the expected blob size, which could lead to silent truncation or panic.// Convert []byte to kzg4844.Blob + if len(blobBytes) != len(blob) { + t.Fatalf("Invalid blob size: got %d bytes, expected %d", len(blobBytes), len(blob)) + } var blob kzg4844.Blob copy(blob[:], blobBytes)database/migrate/migrations/00027_ blob_upload.sql (1)
13-13:⚠️ Potential issueCritical syntax error: missing comma after
deleted_atcolumn
In PostgreSQL, each column definition in aCREATE TABLEmust be comma-terminated. Thedeleted_atline is missing a trailing comma before thePRIMARY KEYclause, causing the migration to fail.Apply this diff:
- deleted_at TIMESTAMP(0) DEFAULT NULL + deleted_at TIMESTAMP(0) DEFAULT NULL,
🧹 Nitpick comments (5)
rollup/go.mod (3)
29-42: Review indirect AWS SDK modules footprint.
A large set ofaws-sdk-go-v2internals and service clients are now indirect requirements. Consider trimming or vendoring only the needed modules to reduce dependency bloat.
74-74: Consider migrating offgolang/protobuf.
The legacygithub.com/golang/protobufmodule is included indirectly; if you’re usinggoogle.golang.org/protobuf, migrating fully and removing the old package could simplify maintenance.
149-149: Reevaluate use ofgolang.org/x/exp.
Experimental modules fromx/expcan be unstable; consider replacing with stable equivalents once available.database/migrate/migrations/00027_ blob_upload.sql (2)
8-9: Suggest adding a check constraint forstatus
Thestatuscolumn usesSMALLINTto encode states (undefined,pending,uploaded,failed). Enforcing valid values at the database level can prevent invalid inserts.Example:
ALTER TABLE blob_upload ADD CONSTRAINT chk_blob_upload_status CHECK (status IN (0, 1, 2, 3));
36-36: Optional: useDROP TABLE IF EXISTSin down migration
Making thedownstep idempotent avoids errors if the table is already gone.-- Instead of: DROP TABLE blob_upload; -- consider: DROP TABLE IF EXISTS blob_upload;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
common/utils/ethereum_test.go(1 hunks)database/migrate/migrate_test.go(1 hunks)database/migrate/migrations/00027_ blob_upload.sql(1 hunks)rollup/cmd/blob_uploader/app/app.go(1 hunks)rollup/go.mod(9 hunks)rollup/internal/controller/blob_uploader/arweave_sender.go(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)rollup/internal/orm/blob_upload.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- database/migrate/migrate_test.go
- rollup/cmd/blob_uploader/app/app.go
- rollup/internal/orm/blob_upload.go
- rollup/internal/orm/batch.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
common/utils/ethereum_test.go
6-6: "fmt" imported and not used
(typecheck)
rollup/internal/controller/blob_uploader/arweave_sender.go
4-4: "fmt" imported and not used
(typecheck)
5-5: "math/big" imported and not used
(typecheck)
6-6: "github.com/permadao/goar/schema" imported and not used
(typecheck)
7-7: "github.com/permadao/goar" imported and not used
(typecheck)
🪛 GitHub Actions: Common
common/utils/ethereum_test.go
[error] 6-6: "fmt" imported and not used
🪛 GitHub Actions: Rollup
rollup/internal/controller/blob_uploader/arweave_sender.go
[error] 4-4: "fmt" imported and not used
🔇 Additional comments (12)
rollup/go.mod (9)
3-3: Confirm Go version bump compatibility.
Bumped Go version to 1.22.4—ensure your CI pipelines, Docker images, and developer toolchains are updated accordingly.
7-10: Approved: AWS SDK v2 direct dependencies.
Addingaws-sdk-go-v2,config,credentials, andservice/s3modules aligns with the new blob uploader implementation.
45-47: Verify BTCD transitives.
Thebtcd,btcec/v2, andbtcutilmodules were pulled in transitively—confirm these are required for Ethereum blob hashing and don’t introduce unwanted conflicts.
56-56: Validate secp256k1 dependency.
The Secp256k1 v4 module was added indirectly. Ensure it’s necessary for your cryptographic routines and there isn’t a lighter-weight alternative.
59-62: Confirm Ethereum client libraries.
Multiple Ethereum-related modules (go-ethereum,ethrpc,goether,gojwk) are now indirect dependencies—verify their necessity in the blob uploader context.
108-108: Assessantspool library usage.
The concurrency pool modulegithub.com/panjf2000/ants/v2was introduced—confirm it’s actively used in the uploader to justify its inclusion.
110-110: Validate Arweave client dependency.
Thepermadao/goarmodule supports Arweave uploads; ensure it’s only pulled when enabling Arweave-backed storage.
135-137: Check Tidwall utilities.
gjson,match, andprettyare now indirect dependencies. Verify they’re consumed in parsing or formatting logic and aren’t leftover.
153-153: Confirm terminal module necessity.
Thegolang.org/x/termpackage is included; if your CLI doesn’t need advanced terminal controls, consider dropping it.rollup/internal/controller/blob_uploader/blob_uploader.go (3)
21-61: Well-structured uploader initialization with proper dependency injection.The constructor properly initializes all dependencies including conditional S3 uploader creation, ORM instances, and metrics registration. Error handling for S3 uploader creation is appropriate.
63-128: Comprehensive upload logic with proper error handling and status tracking.The upload method implements robust error handling with database status updates for both success and failure cases. The sequence of blob construction, hashing, upload, and status tracking is well-orchestrated with appropriate metrics collection.
130-203: Solid blob construction logic with codec version support.The blob construction method properly handles different codec versions (0-7) with appropriate data aggregation and validation. Error handling is comprehensive with clear error messages for various failure scenarios.
| if err != nil { | ||
| log.Error("failed to construct constructBlobCodec payload ", "codecVersion", codecVersion, "batch index", dbBatch.Index, "err", err) | ||
| b.metrics.rollupBlobUploaderUploadToS3FailedTotal.Inc() | ||
| if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); updateErr != nil { |
There was a problem hiding this comment.
So, the "successfully uploaded" batches are continuous (if a blob submission fails, the function returns and proceeds to the next loop)? i.e., there's no case such as (batch_i successful, batch_i+1 failed, batch_i+2 successful).
There was a problem hiding this comment.
Yeah, no such case. I just want to make it sample, like if we failed to upload a blob, we retry this one instead of proceeding the next blob, because there must be something wrong, we may need to fix this one.
And batch generation is slow, so we have time to upload them one by one. hdyt?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/docker.yml (2)
102-110: Introduceblob_uploaderjob with repeated steps—consider refactoring to a reusable workflowThe new job follows the same setup pattern as existing jobs (checkout, QEMU, buildx). To reduce duplication and ease maintenance, you could extract common steps into a reusable workflow or composite action.
🧰 Tools
🪛 actionlint (1.7.7)
108-108: the runner of "docker/setup-qemu-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
110-110: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
111-121: Check Docker Hub login action and consider OIDCThe
docker/login-action@v2step should be verified for compatibility with the latest version (actionlint flagged it). Additionally, consider replacing long-lived AWS credentials with GitHub OIDC for more secure, short-lived token-based access.🧰 Tools
🪛 actionlint (1.7.7)
112-112: the runner of "docker/login-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
rollup/internal/controller/blob_uploader/blob_uploader.go (1)
130-205: Consider breaking down the complex constructBlobCodec method.The
constructBlobCodecmethod is quite long and handles multiple responsibilities: chunk retrieval, validation, block fetching, and batch construction for different codec versions. Consider splitting it into smaller, focused methods for better maintainability.Potential structure:
validateChunkCodecVersions(dbBatch, dbChunks)fetchChunksWithBlocks(dbChunks, codecVersion)createEncodingBatch(dbBatch, chunks, codecVersion)createDABatch(encodingBatch, codecVersion)This would improve readability and make the code easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/docker.yml(1 hunks)common/utils/blob.go(1 hunks)common/utils/blob_test.go(1 hunks)database/migrate/migrations/00027_ blob_upload.sql(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- database/migrate/migrations/00027_ blob_upload.sql
🧰 Additional context used
🧬 Code Graph Analysis (1)
common/utils/blob_test.go (1)
common/utils/blob.go (1)
CalculateVersionedBlobHash(11-23)
🪛 actionlint (1.7.7)
.github/workflows/docker.yml
108-108: the runner of "docker/setup-qemu-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
110-110: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
112-112: the runner of "docker/login-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
131-131: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (6)
.github/workflows/docker.yml (3)
108-110: Verify GitHub Action versions for setup-qemu and setup-buildxactionlint flagged that
docker/setup-qemu-action@v2anddocker/setup-buildx-action@v2may be outdated. Please confirm that these are the intended versions and upgrade to the latest major release if needed.🧰 Tools
🪛 actionlint (1.7.7)
108-108: the runner of "docker/setup-qemu-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
110-110: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
122-130: Approve ECR login and repository bootstrapThe commands to login to ECR and conditionally create the repository are correct and idempotent. No changes needed here.
131-146: Validate build-push action version and Dockerfile pathactionlint flagged
docker/build-push-action@v3as potentially outdated; please confirm or bump to the latest. Also ensure that./build/dockerfiles/blob_uploader.Dockerfileexists and matches the expected multi-stage build for theblob_uploaderbinary.🧰 Tools
🪛 actionlint (1.7.7)
131-131: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
common/utils/blob.go (1)
10-23: LGTM! Solid implementation of versioned blob hash calculation.The function correctly implements the KZG4844 versioned blob hash calculation by first computing the KZG commitment and then applying the SHA-256 hasher with
CalcBlobHashV1. Error handling is appropriate with proper error wrapping.common/utils/blob_test.go (1)
17-51: Excellent test implementation with external test vectors.The test properly validates the
CalculateVersionedBlobHashfunction using external test data, with good error handling and clear failure messages. Using JSON test vectors ensures the implementation can be validated against known good values.rollup/internal/controller/blob_uploader/blob_uploader.go (1)
63-128: Excellent implementation with comprehensive error handling.The
UploadBlobToS3method properly handles the complete blob upload workflow with appropriate database status updates, metrics tracking, and error handling. The implementation addresses the concerns raised in previous reviews.
| case encoding.CodecV7: | ||
| var batchBlocks []*encoding.Block | ||
| for _, dbChunk := range dbChunks { | ||
| blocks, err := b.l2BlockOrm.GetL2BlocksInRange(b.ctx, dbChunk.StartBlockNumber, dbChunk.EndBlockNumber) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get blocks in range for batch %d: %w", dbBatch.Index, err) | ||
| } | ||
|
|
||
| batchBlocks = append(batchBlocks, blocks...) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate duplicate block fetching for CodecV7.
For CodecV7, blocks are already fetched in the main loop (lines 147-152) but are fetched again specifically for CodecV7. This is inefficient and creates unnecessary database queries.
Consider refactoring to collect blocks during the main chunk processing loop:
chunks := make([]*encoding.Chunk, len(dbChunks))
+var allBlocks []*encoding.Block // Collect blocks for CodecV7
for i, c := range dbChunks {
blocks, getErr := b.l2BlockOrm.GetL2BlocksInRange(b.ctx, c.StartBlockNumber, c.EndBlockNumber)
if getErr != nil {
return nil, fmt.Errorf("failed to get blocks in range for batch %d: %w", dbBatch.Index, getErr)
}
chunks[i] = &encoding.Chunk{Blocks: blocks}
+ if codecVersion == encoding.CodecV7 {
+ allBlocks = append(allBlocks, blocks...)
+ }
}
// ... later in CodecV7 case:
case encoding.CodecV7:
- var batchBlocks []*encoding.Block
- for _, dbChunk := range dbChunks {
- blocks, err := b.l2BlockOrm.GetL2BlocksInRange(b.ctx, dbChunk.StartBlockNumber, dbChunk.EndBlockNumber)
- if err != nil {
- return nil, fmt.Errorf("failed to get blocks in range for batch %d: %w", dbBatch.Index, err)
- }
-
- batchBlocks = append(batchBlocks, blocks...)
- }
encodingBatch = &encoding.Batch{
Index: dbBatch.Index,
ParentBatchHash: common.HexToHash(dbBatch.ParentBatchHash),
Chunks: chunks,
PrevL1MessageQueueHash: common.HexToHash(dbBatch.PrevL1MessageQueueHash),
PostL1MessageQueueHash: common.HexToHash(dbBatch.PostL1MessageQueueHash),
- Blocks: batchBlocks,
+ Blocks: allBlocks,
}🤖 Prompt for AI Agents
In rollup/internal/controller/blob_uploader/blob_uploader.go around lines 167 to
176, the code redundantly fetches blocks for CodecV7 even though they were
already fetched in the main loop at lines 147-152, causing unnecessary database
queries. Refactor by removing the duplicate block fetching inside the CodecV7
case and instead reuse the blocks collected during the main chunk processing
loop to improve efficiency.
| if err = b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); err != nil { | ||
| log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Update status to uploaded | ||
| if err = b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusUploaded); err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid reusing error variable to prevent confusion.
The variable err is being reused on lines 113 and 120, which could mask the original upload error and make debugging more difficult.
Apply this fix to use distinct variable names:
// Update status to failed
- if err = b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); err != nil {
+ if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); updateErr != nil {
- log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", err)
+ log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", updateErr)
}
return
}
// Update status to uploaded
-if err = b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusUploaded); err != nil {
+if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusUploaded); updateErr != nil {
- log.Error("failed to update blob upload status to uploaded", "batch index", dbBatch.Index, "err", err)
+ log.Error("failed to update blob upload status to uploaded", "batch index", dbBatch.Index, "err", updateErr)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err = b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); err != nil { | |
| log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", err) | |
| } | |
| return | |
| } | |
| // Update status to uploaded | |
| if err = b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusUploaded); err != nil { | |
| // Update status to failed | |
| if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusFailed); updateErr != nil { | |
| log.Error("failed to update blob upload status to failed", "batch index", dbBatch.Index, "err", updateErr) | |
| } | |
| return | |
| } | |
| // Update status to uploaded | |
| if updateErr := b.blobUploadOrm.InsertOrUpdateBlobUpload(b.ctx, dbBatch.Index, types.BlobStoragePlatformS3, types.BlobUploadStatusUploaded); updateErr != nil { | |
| log.Error("failed to update blob upload status to uploaded", "batch index", dbBatch.Index, "err", updateErr) | |
| } |
🤖 Prompt for AI Agents
In rollup/internal/controller/blob_uploader/blob_uploader.go around lines 113 to
120, the error variable `err` is reused for different operations, which can
overwrite the original error and hinder debugging. To fix this, declare a new
distinct error variable for the second InsertOrUpdateBlobUpload call to avoid
masking the initial error. This ensures each error is handled and logged
separately without confusion.
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
A blob uploader service to upload blob date to AWS S3, Google cloud, Arweave etc.
related docs here: https://www.notion.so/scrollzkp/Blob-Storage-Service-1f77792d22af80bb9d0ce188d0b2fab5
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
Database
blob_uploadtable to track the status of blob uploads.Bug Fixes
Documentation
Tests
Chores