Skip to content

Conversation

@Thegaram
Copy link

@Thegaram Thegaram commented Oct 31, 2025

1. Purpose or design rationale of this PR

Implemented the new, simplified rollup fee formula, activated in the Galileo upgrade.

Spec: https://www.notion.so/scrollzkp/Galileo-Fee-Model-2a07792d22af8051a9e5e987c23c2c6a
Revm: scroll-tech/scroll-revm#65

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:

  • feat: A new feature

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

Summary by CodeRabbit

  • New Features

    • Added Galileo-era rollup fee calculation that uses transaction compression and a penalty factor to compute L1 data fees
    • Patch version incremented to reflect the new fee behavior
  • Tests

    • Expanded compression tests with numerous real-world payloads and exact ratio/size expectations
    • Added Galileo fee tests validating fees across multiple configurations

@Thegaram Thegaram requested a review from mmjahanara October 31, 2025 13:30
@coderabbitai
Copy link

coderabbitai bot commented Oct 31, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Bumps the patch version from 7 to 8 and adds Galileo-era L1 data fee logic: new compressed-size calculation via da-codec, a Galileo-specific encoded-fee function, branching in fee calculation, and expanded compression/fee tests.

Changes

Cohort / File(s) Summary
Version bump
params/version.go
Updated VersionPatch constant from 7 to 8, affecting derived version strings
Galileo fee logic
rollup/fees/rollup_fee.go
Added calculateTxCompressedSize(...) to compute compressed tx size (da-codec, cap at original size, 0 for empty); added calculateEncodedL1DataFeeGalileo(...) to compute Galileo-era L1 data fee; introduced Galileo branch in CalculateL1DataFee and propagated compression-related errors
Tests — compression & fees
rollup/fees/rollup_fee_test.go
Replaced/renamed compression tests: added TestL1DataFeeGalileo and expanded TestCompression with many payloads; compress helper now returns ratio and compressed size and tests assert exact expected values

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant CalculateL1DataFee
    participant calculateTxCompressedSize
    participant calculateEncodedL1DataFeeGalileo

    Caller->>CalculateL1DataFee: Request L1 data fee (tx, block, config)
    alt Galileo mode
        CalculateL1DataFee->>calculateTxCompressedSize: compress tx data (da-codec)
        calculateTxCompressedSize-->>CalculateL1DataFee: compressedSize / error
        CalculateL1DataFee->>calculateEncodedL1DataFeeGalileo: compute fee using compressedSize & scalars/penalty
        calculateEncodedL1DataFeeGalileo-->>CalculateL1DataFee: l1DataFee
    else Curie or Feynman
        CalculateL1DataFee-->>Caller: existing Curie/Feynman fee path
    end
    CalculateL1DataFee-->>Caller: return l1DataFee or error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing closer attention:
    • rollup/fees/rollup_fee.go: correctness of compression integration, edge cases (empty data, capping), precision/BigInt arithmetic, and error propagation.
    • rollup/fees/rollup_fee_test.go: validate expected compression ratios/sizes and fee values against spec; ensure tests aren't brittle.
    • params/version.go: trivial but verify release/versioning consistency.

Possibly related PRs

Suggested labels

bump-version

Suggested reviewers

  • jonastheis
  • colinlyguo
  • georgehao

Poem

🐰 I hopped through code from seven to eight,
I squeezed bytes with da-codec, small and neat,
Galileo's fees find their measured weight,
Ratios and scalars dance on tiny feet,
A rabbit cheers for tests that now complete.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: Galileo rollup fee' clearly describes the main feature being added - implementation of the Galileo rollup fee formula, which aligns with the primary changes across all modified files.
Description check ✅ Passed The PR description comprehensively covers all required sections: purpose/rationale explaining the Galileo upgrade fee formula with spec references, conventional commit type 'feat' confirmed, version bumped in params/version.go, and breaking change status clearly indicated.
✨ 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-galileo-rollup-fee

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbdf4b2 and 3d896c2.

📒 Files selected for processing (1)
  • rollup/fees/rollup_fee.go (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-11T15:54:05.820Z
Learnt from: Thegaram
Repo: scroll-tech/go-ethereum PR: 1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.

Applied to files:

  • rollup/fees/rollup_fee.go
📚 Learning: 2025-10-09T19:18:56.758Z
Learnt from: Thegaram
Repo: scroll-tech/go-ethereum PR: 1245
File: crypto/kzg4844/kzg4844_gokzg.go:118-150
Timestamp: 2025-10-09T19:18:56.758Z
Learning: The scroll-tech/go-ethereum repository uses Go 1.22+ and supports range-over-integer syntax such as `for range n` and `for i := range len(slice)`, which are valid Go 1.22 language features.

Applied to files:

  • rollup/fees/rollup_fee.go
🧬 Code graph analysis (1)
rollup/fees/rollup_fee.go (3)
params/config.go (1)
  • ChainConfig (641-686)
common/bytes.go (1)
  • Bytes2Hex (74-76)
rollup/rcfg/config.go (1)
  • Precision (54-54)
⏰ 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). (2)
  • GitHub Check: test
  • GitHub Check: Analyze (go)

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

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

@Thegaram Thegaram marked this pull request as ready for review November 4, 2025 19:15
@Thegaram Thegaram requested a review from Copilot November 4, 2025 19:15
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 150cba3 and cbdf4b2.

📒 Files selected for processing (3)
  • params/version.go (1 hunks)
  • rollup/fees/rollup_fee.go (4 hunks)
  • rollup/fees/rollup_fee_test.go (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-11T15:54:05.820Z
Learnt from: Thegaram
Repo: scroll-tech/go-ethereum PR: 1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.

Applied to files:

  • rollup/fees/rollup_fee.go
  • rollup/fees/rollup_fee_test.go
🧬 Code graph analysis (2)
rollup/fees/rollup_fee.go (3)
params/config.go (1)
  • ChainConfig (641-686)
common/bytes.go (1)
  • Bytes2Hex (74-76)
rollup/rcfg/config.go (1)
  • Precision (54-54)
rollup/fees/rollup_fee_test.go (3)
params/config.go (1)
  • TestChainConfig (505-545)
rollup/fees/rollup_fee.go (1)
  • U256MAX (20-20)
common/bytes.go (1)
  • Hex2Bytes (79-82)
⏰ 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: CodeQL analysis (go)
  • GitHub Check: test
  • GitHub Check: Analyze (go)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the Galileo upgrade for L1 data fee calculation, modifying the rollup fee structure from Feynman's compression ratio-based approach to a compressed size-based approach with a new penalty formula. It also bumps the version patch from 7 to 8.

Key changes:

  • Adds calculateTxCompressedSize function to compute compressed transaction size with an upper bound constraint
  • Implements calculateEncodedL1DataFeeGalileo with a new fee formula using compressed size and quadratic penalty term
  • Extends CalculateL1DataFee to handle the Galileo fork with appropriate logic branching

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
rollup/fees/rollup_fee.go Adds Galileo fee calculation functions (calculateTxCompressedSize and calculateEncodedL1DataFeeGalileo) and updates CalculateL1DataFee to handle the Galileo fork
rollup/fees/rollup_fee_test.go Adds comprehensive tests for Galileo fee calculation (TestL1DataFeeGalileo) and refactors compression tests to validate both ratio and compressed size
params/version.go Bumps patch version from 7 to 8

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

// Compress data using da-codec
compressed, err := encoding.CompressScrollBatchBytes(data, blockNumber, blockTime, config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a reminder: we need to watch out in case we change the zstd compression level here for batch submission

@Thegaram Thegaram merged commit 53408ef into develop Nov 5, 2025
14 checks passed
@Thegaram Thegaram deleted the feat-galileo-rollup-fee branch November 5, 2025 13:44
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.

4 participants