feat: add da blob client aws s3 #1209
Conversation
WalkthroughA new AWS S3 blob API endpoint configuration is introduced across the codebase. This includes a new command-line flag, updates to configuration structs, and logic in the syncing pipeline and rollup sync service to support fetching blobs from an AWS S3 source via a newly implemented client. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant SyncingPipeline
participant AwsS3Client
User->>CLI: Launches with --da.blob.awss3 flag
CLI->>Config: Parses flag and sets AwsS3BlobAPIEndpoint
Config->>SyncingPipeline: Passes config with AwsS3BlobAPIEndpoint
SyncingPipeline->>AwsS3Client: Initializes client with endpoint
SyncingPipeline->>AwsS3Client: Requests blob by versioned hash
AwsS3Client->>AwsS3Client: Fetches and verifies blob from S3
AwsS3Client-->>SyncingPipeline: Returns blob or error
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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 (
|
![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
🧹 Nitpick comments (4)
rollup/da_syncer/blob_client/aws_s3_client_test.go (1)
10-20: Consider improving test robustness and documentation.The test has several areas for improvement:
- Hardcoded endpoint: The test uses a real AWS S3 endpoint which may cause issues in CI/CD environments without internet access or AWS credentials.
- No error validation: The test only checks that no error occurred but doesn't validate the actual blob content.
- Missing documentation: The test function lacks a comment explaining what it's testing.
Consider these improvements:
+// TestGetBlobByVersionedHashAndBlockTime tests the AWS S3 client's ability to fetch blobs. +// Note: This test requires internet access and may fail in isolated environments. func TestGetBlobByVersionedHashAndBlockTime(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + apiEndpoint := "https://scroll-sepolia-blob-data.s3.us-west-2.amazonaws.com/" awsS3Client := NewAwsS3Client(apiEndpoint) ctx := context.Background() versionedHash := common.HexToHash("0x01e7f0962458d4a4ff61bad08437c3972d7cb443d7ccfdd41b32904e8a5fe24b") - _, err := awsS3Client.GetBlobByVersionedHashAndBlockTime(ctx, versionedHash, 0) + blob, err := awsS3Client.GetBlobByVersionedHashAndBlockTime(ctx, versionedHash, 0) if err != nil { t.Fatalf("unexpected error: %v", err) } + if blob == nil { + t.Fatal("expected non-nil blob") + } }rollup/da_syncer/blob_client/aws_s3_client.go (3)
33-33: Document unused parameter.The
blockTimeparameter is not used in the implementation but is part of the method signature. Consider documenting this or implementing time-based logic if needed.Add a comment explaining the unused parameter:
+// GetBlobByVersionedHashAndBlockTime retrieves a blob from AWS S3 by its versioned hash. +// The blockTime parameter is currently unused but maintained for interface compatibility. func (c *AwsS3Client) GetBlobByVersionedHashAndBlockTime(ctx context.Context, versionedHash common.Hash, blockTime uint64) (*kzg4844.Blob, error) {
36-39: Improve URL construction error handling.The current error message could be more descriptive about what failed during URL construction.
- return nil, fmt.Errorf("failed to join path, err: %w", err) + return nil, fmt.Errorf("failed to join API endpoint %q with versioned hash %q: %w", c.apiEndpoint, versionedHash.String(), err)
50-57: Add response size validation for security.Consider adding a maximum response size limit to prevent potential DoS attacks through oversized responses.
if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) + // Limit error response body size to prevent DoS + limitedReader := io.LimitReader(resp.Body, 1024) // 1KB limit for error responses + body, err := io.ReadAll(limitedReader) if err != nil { return nil, fmt.Errorf("aws s3 request failed with status: %s: could not read response body: %w", resp.Status, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(2 hunks)go.mod(2 hunks)rollup/da_syncer/blob_client/aws_s3_client.go(1 hunks)rollup/da_syncer/blob_client/aws_s3_client_test.go(1 hunks)rollup/da_syncer/syncing_pipeline.go(2 hunks)rollup/rollup_sync_service/rollup_sync_service.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
cmd/geth/usage.go (1)
243-243: LGTM! Consistent flag grouping.The AWS S3 blob API endpoint flag is correctly added to the "ROLLUP" flag group alongside other data availability endpoint flags, maintaining consistency with the existing CLI structure.
rollup/rollup_sync_service/rollup_sync_service.go (1)
117-119: LGTM! Follows established blob client pattern.The AWS S3 blob client integration follows the same conditional pattern as the existing blob clients (BeaconNode, BlobScan, BlockNative), ensuring consistency and maintaining the existing validation logic that requires at least one blob client to be configured.
cmd/geth/main.go (1)
185-185: LGTM! Proper CLI flag integration.The AWS S3 blob API endpoint flag is correctly added to the
nodeFlagsslice alongside other data availability flags, ensuring it's available through the command-line interface.cmd/utils/flags.go (2)
923-926: LGTM! Flag definition follows established patterns.The new AWS S3 blob API endpoint flag is properly defined with consistent naming convention and appropriate usage description.
1706-1708: LGTM! Flag handling logic is consistent with other blob clients.The implementation follows the same pattern as other blob API endpoint flags (BlobScan, BlockNative, BeaconNode) and correctly assigns the flag value to the configuration field.
go.mod (2)
3-5: Verify Go version compatibility across the project.The Go version was upgraded from 1.21 to 1.22 with a specific toolchain version. Ensure this doesn't break compatibility with existing code or CI/CD pipelines.
#!/bin/bash # Check if there are any Go version-specific features being used rg -n "go:build.*go1\." --type go
11-15: ```shell
#!/bin/bashCheck for Route53 client initialization patterns
rg -n 'route53.NewFromConfig' --type go -A3 -B3
Verify Go toolchain version requirement
rg -n '^go ' go.mod
</details> <details> <summary>rollup/da_syncer/syncing_pipeline.go (2)</summary> `26-26`: **LGTM! Config field addition is consistent.** The new `AwsS3BlobAPIEndpoint` field follows the same naming pattern and placement as other blob API endpoint fields in the Config struct. --- `78-80`: **LGTM! AWS S3 client integration follows established pattern.** The conditional client creation and addition to the blob client list is consistent with the existing pattern used for BeaconNode, BlobScan, and BlockNative clients. </details> <details> <summary>rollup/da_syncer/blob_client/aws_s3_client.go (2)</summary> `59-66`: **LGTM! Robust blob data reading with proper error handling.** The implementation correctly uses `io.ReadFull` to ensure complete blob data is read and handles both EOF conditions appropriately with informative error messages. --- `68-77`: **Excellent blob integrity verification.** The implementation includes proper sanity checking by converting the blob to a commitment, calculating the versioned hash, and verifying it matches the requested hash. This prevents data corruption issues. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rollup/da_syncer/blob_client/aws_s3_client.go (1)
21-31: LGTM! Past review issue resolved.The struct design is clean and the constructor correctly uses
AwsS3DefaultTimeout. The previous review comment about using the wrong timeout constant has been addressed.Consider adding basic validation for the
apiEndpointparameter to ensure it's not empty:func NewAwsS3Client(apiEndpoint string) *AwsS3Client { + if apiEndpoint == "" { + panic("apiEndpoint cannot be empty") + } return &AwsS3Client{ apiEndpoint: apiEndpoint, client: &http.Client{Timeout: AwsS3DefaultTimeout}, } }
🧹 Nitpick comments (1)
rollup/da_syncer/blob_client/aws_s3_client.go (1)
33-48: Well-implemented HTTP request handling.The URL construction using
url.JoinPathand HTTP request creation with context are properly implemented.Consider removing hardcoded endpoint comments.
The hardcoded AWS S3 endpoint URLs in comments (lines 34-35) may become outdated. Consider removing them or moving to documentation.
Note:
blockTimeparameter is unused.The
blockTimeparameter is not used in the method implementation. This may be intentional for interface compliance, but it could be confusing given the method name includes "AndBlockTime".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/da_syncer/blob_client/aws_s3_client.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
rollup/da_syncer/blob_client/aws_s3_client.go (3)
1-15: LGTM!The package declaration and imports are appropriate and comprehensive for the AWS S3 blob client functionality.
17-19: LGTM!The timeout constant is appropriately defined and the 15-second duration is reasonable for AWS S3 requests.
50-80: Excellent security implementation with proper blob validation.The response handling and blob validation logic is well-implemented:
- Comprehensive error handling with detailed status messages
- Proper use of
io.ReadFullto ensure complete blob data reading- Critical security feature: Blob validation by computing and verifying the versioned hash prevents data integrity issues and potential attacks
The blob validation (lines 68-77) is particularly important for ensuring the retrieved data matches the requested versioned hash.
1. Purpose or design rationale of this PR
Add a new blob data client that retrieves data from AWS S3.
2. 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:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Documentation