feat(blob-uploader): upload blob once proposed

[yiweichi]

Purpose or design rationale of this PR

This PR applies a feature to blob-uploader to upload blobs before it's committed. This is to alleviate the case nodes try to fetch the blob from s3 before its uploaded.

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:

• build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
• ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
• docs: Documentation-only changes
• feat: A new feature
• fix: A bug fix
• perf: A code change that improves performance
• refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
• style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
• test: Adding missing tests or correcting existing tests

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

• No, this PR doesn't involve a new deployment, git tag, docker image tag
• Yes

Breaking change label

Does this PR have the breaking-change label?

• No, this PR is not a breaking change
• Yes

Summary by CodeRabbit

• New Features

  • Blob upload functionality now supports uncommitted transactions, enabling more flexible batch processing operations.

• Chores

  • Version updated to v4.7.2.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Version is bumped from v4.7.1 to v4.7.2. The blob uploader logic is modified to allow uploading batches with empty CommitTxHash by commenting out the conditional that previously enforced committed-only uploads.

Changes

Cohort / File(s)	Summary
Version Bump common/version/version.go	Updated version tag from v4.7.1 to v4.7.2 in the Version string composition.
Blob Uploader Control Flow rollup/internal/controller/blob_uploader/blob_uploader.go	Disabled the early-return conditional that checked for non-empty CommitTxHash, allowing batches to be uploaded regardless of commit status. Previous logic preserved in comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Attention needed: The blob uploader change removes a validation check that prevented uploading uncommitted batches. Understanding the business logic rationale for this behavioral change and potential implications on downstream processing is important.

Possibly related PRs

• feat: add blob storage service #1672 — Introduced the original blob uploader implementation with CommitTxHash validation; this PR directly disables that check by commenting out the early-return conditional.

Suggested reviewers

• georgehao
• Thegaram
• colinlyguo

Poem

🐰 A version hopped from 4.7.1 to 4.7.2,
While blobs now fly uncommitted, wild and free,
No more CommitTxHash checks to delay,
The uploader bounces without guardrails today! 🚀

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The PR description covers the purpose/rationale (uploading blobs before commitment), correctly identifies the change type as 'feat', confirms version tag update, and explicitly states it's not a breaking change. All required template sections are addressed.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'feat(blob-uploader): upload blob once proposed' accurately describes the main change: enabling blob uploads before commit (from the disabled CommitTxHash check), which aligns with the PR's core objective.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat-blob-uploader-upload-blob-once-proposed

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

rollup/internal/controller/blob_uploader/blob_uploader.go (1)

200-252: Align function comment and behavior now that CommitTxHash check is disabled

The doc comment still states “The batch must have a commit_tx_hash (committed).” but the actual CommitTxHash guard is now commented out, so GetFirstUnuploadedBatchByPlatform will return batches even when CommitTxHash is empty.

This is a non-trivial behavioral change (uploading blobs for uncommitted batches) and the existing comment is now misleading.

Consider:

• Updating the function comment to explicitly state that uncommitted batches are now eligible for upload, and why (to avoid S3 race conditions).
• Optionally adjusting the inline comment to make it clear this is an intentional, permanent behavior change rather than a temporary hack.

This keeps future maintainers from assuming that a commit hash is still a precondition for uploads.

rollup/internal/controller/relayer/l2_relayer.go (1)

372-403: Disabling timeout/backlog/fee heuristics fundamentally changes when batches are submitted

By commenting out the TimeoutSec/BacklogMax/skipSubmitByFee logic:

• forceSubmit is never set to true, so the “too old” and “backlog too big” fast‑paths are effectively disabled.
• skipSubmitByFee is never called; blob‑fee target pricing and its metrics become dead code.
• Submission is now driven solely by len(batchesToSubmit) >= minBatches — regardless of batch age, backlog size, or current blob fee.

This is a significant behavior change compared to the function’s comment (“forceSubmit”, backlog, fee target), which is now stale. It can impact both:

• Cost behavior: batches will be sent even when blob fees are well above the previous target.
• Backlog behavior: timeouts and backlog thresholds no longer force inclusion; only minBatches limits apply.

If this simplification is intentional (e.g., for an experiment or a new strategy), I’d suggest:

• Updating the ProcessPendingBatches doc comment to describe the new, simpler policy.
• Adding a brief inline comment where the old logic is commented out, clarifying whether this is temporary and whether config knobs like TimeoutSec / BacklogMax are currently ignored.
• Optionally gating the old heuristics behind a feature flag instead of commenting them out, so you can re‑enable them without code changes.

If it’s not intended that fee/timeout/backlog controls are dropped, this needs to be revisited before release.

Also applies to: 471-479

🧹 Nitpick comments (2)

rollup/internal/controller/sender/sender.go (1)

31-32: Clean up new blob hash and raw transaction logging for long‑term use

The new logic to compute and log the versioned blob hash and raw transaction looks functionally fine, but the logging patterns are a bit ad‑hoc:

• Log messages use "------------------------------Morty ..." which reads like temporary debug text.
• For blob txs with multiple blobs, only the first blob’s versioned hash is logged; that may be confusing when correlating with multiple S3 objects.
• Logging full raw tx hex at Info level can be very noisy in production.

Suggested tweaks:

• Replace the “Morty” messages with stable, descriptive strings (e.g., "blob tx versioned hash" and "raw transaction for eth_sendRawTransaction"), and include context like len(blobs) or a slice of hashes if you need to correlate multiple blobs.
• Consider lowering the raw‑tx log to Debug or guarding it behind a config flag so production logs don’t get flooded.

No correctness issues, just polish for observability and log hygiene.

Also applies to: 329-336, 359-366

rollup/internal/controller/relayer/l2_relayer.go (1)

124-147: Document or reconsider removal of commit/finalize sender address validation

The pre‑flight parsing and validation of commit/finalize signer addresses (addrFromSignerConfig + “must be different” check) is now fully commented out, so:

• Misconfigured signer addresses (including commit and finalize being identical) will only be detected much later, if at all.
• The helper addrFromSignerConfig is effectively unused outside this commented block.

If the intent is to allow shared signers or to relax these checks, consider:

• Updating comments around this switch case to spell out the new expectations (e.g., “commit/finalize may share an address; we rely on contract‑level checks only”).
• Or, if safety is still desired, keeping a minimal validation (e.g., non‑empty addresses, optional inequality check behind a config flag).

This makes the configuration contract for relayer signers clearer and avoids surprising operator errors.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9dceae1 and e62942f.

📒 Files selected for processing (4)

• common/version/version.go (1 hunks)
• rollup/internal/controller/blob_uploader/blob_uploader.go (1 hunks)
• rollup/internal/controller/relayer/l2_relayer.go (2 hunks)
• rollup/internal/controller/sender/sender.go (3 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.

Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1714
File: rollup/internal/controller/relayer/l2_relayer.go:1548-1555
Timestamp: 2025-07-29T16:38:24.647Z
Learning: In rollup/internal/controller/relayer/l2_relayer.go, the validateBatchFields function should error out when GetBatchByHash fails to find a parent batch. This is intentional behavior - missing parent batches represent genuine error conditions that should halt batch submission processing. Genesis batch handling occurs separately from normal batch validation flow.

📚 Learning: 2025-10-18T06:49:24.796Z

Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.

Applied to files:

• rollup/internal/controller/blob_uploader/blob_uploader.go
• rollup/internal/controller/sender/sender.go
• rollup/internal/controller/relayer/l2_relayer.go

📚 Learning: 2025-07-29T16:38:24.647Z

Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1714
File: rollup/internal/controller/relayer/l2_relayer.go:1548-1555
Timestamp: 2025-07-29T16:38:24.647Z
Learning: In rollup/internal/controller/relayer/l2_relayer.go, the validateBatchFields function should error out when GetBatchByHash fails to find a parent batch. This is intentional behavior - missing parent batches represent genuine error conditions that should halt batch submission processing. Genesis batch handling occurs separately from normal batch validation flow.

Applied to files:

• rollup/internal/controller/blob_uploader/blob_uploader.go
• rollup/internal/controller/relayer/l2_relayer.go

📚 Learning: 2024-10-20T16:13:20.397Z

Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1530
File: rollup/internal/controller/watcher/batch_proposer.go:291-294
Timestamp: 2024-10-20T16:13:20.397Z
Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.

Applied to files:

• rollup/internal/controller/relayer/l2_relayer.go

🧬 Code graph analysis (1)

rollup/internal/controller/sender/sender.go (1)

common/utils/blob.go (1)

• CalculateVersionedBlobHash (11-23)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: check

🔇 Additional comments (1)

common/version/version.go (1)

8-8: Version bump looks consistent

Only the tag constant is updated to "v4.7.2"; the surrounding versioning logic is unchanged and remains correct.