Skip to content

Conversation

@tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Nov 21, 2025

Overview

mocha has bumped their max so we can go back to our previous size

upstream is now 8mb https://github.com/celestiaorg/celestia-app/blob/v6.2.5/pkg/appconsts/app_consts.go#L25

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request performs a routine maintenance task by adjusting a system constant. Specifically, it increases the default maximum size allowed for blobs, enabling the system to handle larger data units without requiring explicit configuration overrides. This change ensures greater flexibility and capacity for blob storage operations.

Highlights

  • Increased Default Blob Size: The DefaultMaxBlobSize constant has been increased from 1.5MB to 1.9MB.
  • Configuration Update: The change was applied in the block/internal/common/consts.go file, which defines common constants for the block internal package.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 21, 2025

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedNov 21, 2025, 10:29 AM

@claude
Copy link
Contributor

claude bot commented Nov 21, 2025

Claude finished @tac0turtle's task —— View job


Code Review Complete

Reviewing PR #2877: chore: bump blob size

Todo List:

  • Read all 4 changed files to understand the modifications
  • Search codebase for other occurrences of blob size constants
  • Check for test files that may need updating
  • Analyze code quality and correctness issues
  • Provide comprehensive review feedback

Summary

This PR updates the DefaultMaxBlobSize constant across 4 files from 1.5MB to 2MB. However, the PR description states it should be 1.9MB based on the upstream Celestia change. There's a mismatch between the stated intent (1.9MB) and the actual implementation (2MB).

Critical Issues

1. Value Mismatch: 2MB vs 1.9MB ⚠️

Severity: High

The PR description references celestia-app v6.2.5 which sets the limit to 8MB, and states "we can go back to our previous size." However, the code implements 2MB instead of 1.9MB.

Files affected:

  • block/internal/common/consts.go:3
  • da/cmd/local-da/local.go:21
  • da/jsonrpc/proxy_test.go:33
  • pkg/cmd/run_node.go:29

Recommendation: Clarify the intended value:

  • If 2MB is correct, update the PR description to explain why 2MB was chosen instead of 1.9MB
  • If 1.9MB is correct, update all constants to match the PR description

2. Inconsistent Previous Value in local-da 📝

Severity: Medium

In da/cmd/local-da/local.go:21, the previous value was 64 * 64 * 481 = 1970176 bytes (~1.88MB), which is different from the 1.5MB used in other files. This PR standardizes it to 2MB, but this inconsistency history should be noted.

Before:

const DefaultMaxBlobSize uint64 = 64 * 64 * 481 // 1970176

After:

const DefaultMaxBlobSize uint64 = 2 * 1024 * 1024 // 2MB

This change is positive as it brings consistency across the codebase.

3. Integer Arithmetic is Correct

Severity: N/A (Good Practice)

Unlike Gemini's review comment suggesting 19 * 1024 * 1024 / 10 for 1.9MB, this PR correctly uses integer literals 2 * 1024 * 1024 which:

  • Evaluates to exactly 2097152 at compile time
  • Avoids floating-point arithmetic entirely
  • Is clearer and safer than division-based calculations

Note: If reverting to 1.9MB, the expression would be: (19 * 1024 * 1024) / 10 = 1992294 which is fine, but the current 2MB approach is cleaner.

Code Quality Assessment

Architecture & Design ✅

  • The change maintains the existing architectural pattern
  • Constants are properly centralized in appropriate locations
  • The da_submitter.go:42 correctly references common.DefaultMaxBlobSize
  • All application entry points (apps/*/cmd/run.go) correctly reference rollcmd.DefaultMaxBlobSize

Testing Coverage ⚠️

The test file da/jsonrpc/proxy_test.go was updated, but:

  1. There's a separate testMaxBlobSize = 100 constant for unit tests (line 31) - this is correct as it's for controlled testing
  2. The DefaultMaxBlobSize = 2 * 1024 * 1024 on line 33 is used by integration tests
  3. No new tests were added to verify the 2MB limit works correctly with real data

Recommendation: Consider adding an integration test that:

  • Submits a blob slightly under 2MB (e.g., 2MB - 1KB) - should succeed
  • Submits a blob slightly over 2MB (e.g., 2MB + 1KB) - should fail with ErrBlobSizeOverLimit

Security Considerations ✅

  • The size validation logic in da/cmd/local-da/local.go:185-188 and 214-218 correctly enforces the limit
  • The da_submitter.go has proper retry logic and size-based batching (lines 368-379)
  • No security regressions introduced

Performance Implications ✅

  • Increasing from 1.5MB to 2MB allows 33% larger blobs
  • This aligns with the upstream Celestia capacity increase
  • The limitBatchBySize function (lines 499-522 in da_submitter.go) handles size constraints correctly
  • Positive impact: Fewer submissions needed for the same amount of data

Error Handling ✅

  • Size limit violations correctly return coreda.ErrBlobSizeOverLimit
  • Logging is appropriate with blob size and max size included in error messages
  • Retry logic correctly handles StatusTooBig with exponential backoff

Documentation 📝

  • All constant definitions include inline comments with the size (e.g., // 2MB)
  • Missing: No update to any documentation files (README, guides) about the new limit
  • Run: grep -r "1\.5" docs/ to check if documentation needs updating

Consistency Across Codebase ✅

I verified all usages of DefaultMaxBlobSize and MaxBlobSize:

  • block/internal/common/consts.go - updated
  • da/cmd/local-da/local.go - updated
  • da/jsonrpc/proxy_test.go - updated
  • pkg/cmd/run_node.go - updated
  • block/internal/submitting/da_submitter.go:42 - references common.DefaultMaxBlobSize (correct)
  • ✅ All app entry points (apps/testapp/cmd/run.go:54, apps/grpc/single/cmd/run.go:55, apps/evm/single/cmd/run.go:56) - reference rollcmd.DefaultMaxBlobSize (correct)

Addressing Reviewer Comments

@julienrbrt's Comment: "this value is at more places"

Status:Resolved

I verified all locations where blob size constants appear. The PR correctly updates all 4 instances of the DefaultMaxBlobSize constant definition. All other references correctly point to these constants via imports. No additional changes are needed.

@gemini-code-assist's Comment: Floating-point truncation

Status:Not Applicable

The current PR uses 2 * 1024 * 1024 which is pure integer arithmetic with no floating-point operations. This concern would only apply if implementing 1.9MB, but even then, 19 * 1024 * 1024 / 10 would be an integer expression in Go.

Recommendations

Must Fix

  1. Clarify the intended value: The PR description says 1.9MB but code implements 2MB. Please:
    • If 2MB is intentional: Update PR description to explain why 2MB instead of 1.9MB
    • If 1.9MB is correct: Update all 4 constants to (19 * 1024 * 1024) / 10 with comment // 1.9MB

Should Consider

  1. Add integration tests: Test behavior at the 2MB boundary (just under and just over)
  2. Update documentation: Search for references to "1.5" or "1.5MB" in docs and update if needed
  3. Update commit message: The commit message should explain the rationale (aligning with upstream Celestia's 8MB support)

Nice to Have

  1. Consider making it configurable: The value is currently a compile-time constant. For future flexibility, consider making it a runtime configuration option.

Final Assessment

Overall Quality: Good ✅

The code changes are clean, consistent, and well-implemented. The primary concern is the discrepancy between PR description (1.9MB) and implementation (2MB), which needs clarification. Once that's resolved, this is a solid change.

Approval Status: ⏸️ Waiting for clarification on the value mismatch before approving.


Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request increases the DefaultMaxBlobSize. I've identified two issues with the change. First, the comment for the constant is outdated. Second, the new value is a floating-point number that gets truncated when used, which can be confusing and potentially error-prone. I've provided a suggestion that corrects the comment and uses integer arithmetic to make the code clearer and safer.

package common

const DefaultMaxBlobSize = 1.5 * 1024 * 1024 // 1.5MB fallback blob size limit
const DefaultMaxBlobSize = 1.9 * 1024 * 1024 // 1.5MB fallback blob size limit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comment for DefaultMaxBlobSize is outdated and should be updated to 1.9MB to match the new value.

Additionally, the expression 1.9 * 1024 * 1024 results in a floating-point value (1992294.4), which is then truncated when assigned to integer types (like MaxBlobBytes where this constant is used). The previous value 1.5 * 1024 * 1024 evaluated to an integer. To avoid implicit truncation and make the intent clear, it's better to use integer arithmetic.

Suggested change
const DefaultMaxBlobSize = 1.9 * 1024 * 1024 // 1.5MB fallback blob size limit
const DefaultMaxBlobSize = 19 * 1024 * 1024 / 10 // 1.9MB fallback blob size limit

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this value is at more places

@tac0turtle tac0turtle enabled auto-merge November 21, 2025 10:30
@codecov
Copy link

codecov bot commented Nov 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.88%. Comparing base (c652f7a) to head (b2f991a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2877      +/-   ##
==========================================
- Coverage   64.97%   64.88%   -0.10%     
==========================================
  Files          81       81              
  Lines        7250     7250              
==========================================
- Hits         4711     4704       -7     
- Misses       1998     2005       +7     
  Partials      541      541              
Flag Coverage Δ
combined 64.88% <ø> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tac0turtle tac0turtle added this pull request to the merge queue Nov 21, 2025
Merged via the queue into main with commit c19136e Nov 21, 2025
26 checks passed
@tac0turtle tac0turtle deleted the marko/bump_blob_size branch November 21, 2025 10:43
alpe added a commit that referenced this pull request Nov 24, 2025
* main:
  chore: remove extra github action yml file (#2882)
  fix(execution/evm): verify payload status (#2863)
  feat: fetch included da height from store (#2880)
  chore: better output on errors (#2879)
  refactor!: create da client and split cache interface (#2878)
  chore!: rename `evm-single` and `grpc-single` (#2839)
  build(deps): Bump golang.org/x/crypto from 0.42.0 to 0.45.0 in /tools/da-debug in the go_modules group across 1 directory (#2876)
  chore: parallel cache de/serialization (#2868)
  chore: bump blob size (#2877)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants