[DONOT MERGE]: Hotfix/submitter blob hoodi 1 #794
Conversation
WalkthroughUpdated go-ethereum subproject pointer and bumped go-ethereum dependency in tx-submitter/go.mod. Replaced direct use of eip4844.CalcBlobFee with an internal, fork-aware blob-fee path that computes a blobFee via getBlobFee(head) and scales it by 3; added helper functions (isTimestampForked, newUint64, fakeExponential) and removed the eip4844 import and an indirect memsize require. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as GetGasTipAndCap
participant Rollup as rollup.go
participant Head as ChainHead
Note over Rollup: New internal blob-fee path
Caller->>Rollup: request gas tip & cap
Rollup->>Head: fetch head
Rollup->>Rollup: isTimestampForked(head)
Rollup->>Rollup: getBlobFee(head)
Rollup->>Rollup: fakeExponential(...) -- Taylor approx
Rollup->>Rollup: blobFee * 3 -- congestion hedge
Rollup->>Caller: return gas tip, gas cap, blobFee
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go-ethereum(1 hunks)tx-submitter/go.mod(1 hunks)tx-submitter/services/rollup.go(2 hunks)
⏰ 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: test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
go-ethereum (2)
1-1: PR marked [DONOT MERGE]—resolve before requesting review.This PR has an explicit "[DONOT MERGE]" marker in the title, indicating it's not ready for merging. Please resolve any outstanding work and update the title before requesting formal review.
1-1: Verify go-ethereum commit and provide functional context.The file shown contains only a subproject pointer update, but the PR summary references significant functional changes to blob fee calculation in tx-submitter (custom exponential approximation, scaling factor, dependency removal). Please clarify:
- Is this PR expected to include changes to
tx-submitter/services/rollup.go,tx-submitter/go.mod, or related files? If so, please ensure all changed files are visible in the PR.- Verify that the new go-ethereum commit hash
151913b181b1dba79226e67fd1ccb595d8c60d92is valid and available.- Document the rationale for bumping go-ethereum and any compatibility concerns with the blob fee calculation refactoring mentioned in the summary.
tx-submitter/go.mod (1)
10-10: The review comment is based on incorrect assumptions about this commit.Commit 25b8e120126f ("Rm memsize import #217") removes the
github.com/fjl/memsizedependency—it has no connection to blob fee calculations or EIP-4844 changes.Additionally,
CalcBlobFeewas never used inrollup.go. The codebase implements blob fee calculation directly using a customfakeExponential()function in theGetGasTipAndCap()method, rather than importing a library function. There is no removal of external blob fee logic to verify.The go-ethereum version exists and is recent (Oct 30, 2025). The 404 on pkg.go.dev is expected for a very newly created pseudo-version that hasn't been indexed yet.
Likely an incorrect or invalid review comment.
tx-submitter/services/rollup.go (1)
1281-1286: Based on my web search, I've found important information that contradicts the review comment's premise. CalcBlobFee calculates the blobfee from the header's excess blob gas field and is a standard function in go-ethereum's eip4844 package. The function exists in current go-ethereum versions and is actively used in the codebase.Let me search for more specific information about the morph-l2 fork to understand why a custom implementation is being used:
Incorrect premise—the custom blob fee implementation is intentional, not a workaround for a removed function.
CalcBlobFee calculates the blobfee from the header's excess blob gas field and is a standard function in go-ethereum's eip4844 package. The original review comment assumes this function was removed from the morph-l2 go-ethereum fork, but this appears to be incorrect. The custom implementation using
fakeExponentialwith a 3x multiplier is intentional—likely to apply custom blob fee pricing logic for handling market congestion on the Morph chain, not a workaround for a missing function.The review comment's suggestion to "verify why eip4844.CalcBlobFee was removed" is based on a faulty assumption. If a custom blob fee calculation is needed for Morph's specific requirements, that's a valid design choice and should be documented, but it shouldn't be framed as restoring a removed standard library function.
Likely an incorrect or invalid review comment.
|
|
||
| // fakeExponential approximates factor * e ** (numerator / denominator) using | ||
| // Taylor expansion. | ||
| func fakeExponential(factor, numerator, denominator *big.Int) *big.Int { | ||
| var ( | ||
| output = new(big.Int) | ||
| accum = new(big.Int).Mul(factor, denominator) | ||
| ) | ||
| for i := 1; accum.Sign() > 0; i++ { | ||
| output.Add(output, accum) | ||
|
|
||
| accum.Mul(accum, numerator) | ||
| accum.Div(accum, denominator) | ||
| accum.Div(accum, big.NewInt(int64(i))) | ||
| } | ||
| return output.Div(output, denominator) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add iteration limit to prevent potential infinite loops.
The fakeExponential function implements a Taylor expansion that terminates when accum.Sign() > 0 becomes false. While this should converge for typical blob fee calculations, there's no explicit guard against infinite loops with unexpected inputs.
Apply this diff to add a safety limit:
func fakeExponential(factor, numerator, denominator *big.Int) *big.Int {
+ const maxIterations = 100 // Safety limit for Taylor expansion
var (
output = new(big.Int)
accum = new(big.Int).Mul(factor, denominator)
)
for i := 1; accum.Sign() > 0; i++ {
+ if i > maxIterations {
+ log.Warn("fakeExponential: max iterations reached",
+ "numerator", numerator.String(),
+ "denominator", denominator.String())
+ break
+ }
output.Add(output, accum)
accum.Mul(accum, numerator)
accum.Div(accum, denominator)
accum.Div(accum, big.NewInt(int64(i)))
}
return output.Div(output, denominator)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tx-submitter/services/rollup.go around lines 1906 to 1922, the
fakeExponential loop currently only exits when accum.Sign() <= 0 which can hang
for unexpected inputs; add a safety iteration limit (e.g. const maxIterations =
1000) and break the loop if i exceeds it, so the loop condition becomes both
accum.Sign() > 0 && i <= maxIterations; ensure any used loop-initialized
variables remain valid after the break and return the current computed output as
before.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tx-submitter/services/rollup.go (1)
1931-1947: Add iteration limit and input validation to prevent infinite loops and panics.The
fakeExponentialfunction has critical safety gaps:
- No iteration limit: The loop can run indefinitely with unexpected inputs (already flagged in past review comments)
- Missing input validation: No nil checks or validation of inputs could cause panics
- Zero denominator: Would cause division by zero panic on line 1946
Apply this diff to add comprehensive safety checks:
func fakeExponential(factor, numerator, denominator *big.Int) *big.Int { + // Input validation + if factor == nil || numerator == nil || denominator == nil { + log.Error("fakeExponential: nil parameter", + "factor_nil", factor == nil, + "numerator_nil", numerator == nil, + "denominator_nil", denominator == nil) + return big.NewInt(0) + } + if denominator.Sign() == 0 { + log.Error("fakeExponential: zero denominator") + return big.NewInt(0) + } + + const maxIterations = 100 // Safety limit for Taylor expansion var ( output = new(big.Int) accum = new(big.Int).Mul(factor, denominator) ) for i := 1; accum.Sign() > 0; i++ { + if i > maxIterations { + log.Warn("fakeExponential: max iterations reached", + "numerator", numerator.String(), + "denominator", denominator.String(), + "iterations", i) + break + } output.Add(output, accum) accum.Mul(accum, numerator) accum.Div(accum, denominator) accum.Div(accum, big.NewInt(int64(i))) } return output.Div(output, denominator) }Note: The missing iteration limit was previously flagged in past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tx-submitter/services/rollup.go(3 hunks)
⏰ 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: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
tx-submitter/services/rollup.go (2)
1296-1297: LGTM!Clean helper function following standard Go patterns for obtaining pointers to literals.
1312-1320: LGTM!Clean implementation of timestamp-based fork detection with proper nil handling and clear documentation.
| log.Info("market blob fee info", "excess blob gas", *head.ExcessBlobGas) | ||
| blobFee = r.getBlobFee(head) | ||
| // Set to 3x to handle blob market congestion | ||
| blobFee = new(big.Int).Mul(blobFee, big.NewInt(3)) | ||
| } |
There was a problem hiding this comment.
Consider adding nil and bounds validation for blob fee calculation.
The blob fee calculation lacks defensive checks:
getBlobFee(head)may return nil or zero in edge cases (though the current implementation doesn't)- No upper bound check on the final
blobFeevalue after the 3x multiplication to prevent excessive costs
Apply this diff to add validation:
if head.ExcessBlobGas != nil {
log.Info("market blob fee info", "excess blob gas", *head.ExcessBlobGas)
blobFee = r.getBlobFee(head)
+ if blobFee == nil || blobFee.Sign() <= 0 {
+ return nil, nil, nil, fmt.Errorf("invalid blob fee calculated")
+ }
// Set to 3x to handle blob market congestion
blobFee = new(big.Int).Mul(blobFee, big.NewInt(3))
+ // Consider adding max bounds check here if r.cfg.MaxBlobFee is added
}Note: The aggressive 3x multiplier and missing max bounds configuration have been previously flagged in past review comments.
| func (r *Rollup) getBlobFee(head *ethtypes.Header) *big.Int { | ||
| HoodiBPO1Time := newUint64(1762365720) | ||
| HoodiBPO2Time := newUint64(1762955544) | ||
| denominator := big.NewInt(5007716) | ||
| if isTimestampForked(HoodiBPO1Time, head.Time) { | ||
| denominator = big.NewInt(8346193) | ||
| } | ||
| if isTimestampForked(HoodiBPO2Time, head.Time) { | ||
| denominator = big.NewInt(11684671) | ||
| } | ||
| blobFee := fakeExponential(big.NewInt(params.BlobTxMinBlobGasprice), new(big.Int).SetUint64(uint64(*head.ExcessBlobGas)), denominator) | ||
| return blobFee | ||
| } |
There was a problem hiding this comment.
Replace hardcoded fork timestamps with documented constants.
The getBlobFee function contains hardcoded magic numbers that reduce maintainability and clarity:
- Fork timestamps (lines 1299-1300):
1762365720and1762955544are undocumented Unix timestamps with no context about which forks they represent - Denominator values (lines 1301, 1303, 1306): The values
5007716,8346193, and11684671lack explanation of their significance or how they were derived - Missing input validation: No check that
head.ExcessBlobGasis non-nil, though the caller currently handles this
Apply this diff to improve clarity and maintainability:
+// Fork activation timestamps for blob fee calculation adjustments
+const (
+ HoodiBPO1ForkTime = 1762365720 // TODO: Document which fork this represents
+ HoodiBPO2ForkTime = 1762955544 // TODO: Document which fork this represents
+)
+
+// Blob fee denominators for different fork eras
+const (
+ BlobFeeDenominatorBase = 5007716 // EIP-4844 BLOB_GASPRICE_UPDATE_FRACTION
+ BlobFeeDenominatorHoodi1 = 8346193 // Post-HoodiBPO1 adjustment
+ BlobFeeDenominatorHoodi2 = 11684671 // Post-HoodiBPO2 adjustment
+)
+
func (r *Rollup) getBlobFee(head *ethtypes.Header) *big.Int {
- HoodiBPO1Time := newUint64(1762365720)
- HoodiBPO2Time := newUint64(1762955544)
- denominator := big.NewInt(5007716)
+ if head.ExcessBlobGas == nil {
+ log.Warn("getBlobFee called with nil ExcessBlobGas")
+ return big.NewInt(params.BlobTxMinBlobGasprice)
+ }
+ HoodiBPO1Time := newUint64(HoodiBPO1ForkTime)
+ HoodiBPO2Time := newUint64(HoodiBPO2ForkTime)
+ denominator := big.NewInt(BlobFeeDenominatorBase)
if isTimestampForked(HoodiBPO1Time, head.Time) {
- denominator = big.NewInt(8346193)
+ denominator = big.NewInt(BlobFeeDenominatorHoodi1)
}
if isTimestampForked(HoodiBPO2Time, head.Time) {
- denominator = big.NewInt(11684671)
+ denominator = big.NewInt(BlobFeeDenominatorHoodi2)
}
blobFee := fakeExponential(big.NewInt(params.BlobTxMinBlobGasprice), new(big.Int).SetUint64(uint64(*head.ExcessBlobGas)), denominator)
return blobFee
}📝 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.
| func (r *Rollup) getBlobFee(head *ethtypes.Header) *big.Int { | |
| HoodiBPO1Time := newUint64(1762365720) | |
| HoodiBPO2Time := newUint64(1762955544) | |
| denominator := big.NewInt(5007716) | |
| if isTimestampForked(HoodiBPO1Time, head.Time) { | |
| denominator = big.NewInt(8346193) | |
| } | |
| if isTimestampForked(HoodiBPO2Time, head.Time) { | |
| denominator = big.NewInt(11684671) | |
| } | |
| blobFee := fakeExponential(big.NewInt(params.BlobTxMinBlobGasprice), new(big.Int).SetUint64(uint64(*head.ExcessBlobGas)), denominator) | |
| return blobFee | |
| } | |
| // Fork activation timestamps for blob fee calculation adjustments | |
| const ( | |
| HoodiBPO1ForkTime = 1762365720 // TODO: Document which fork this represents | |
| HoodiBPO2ForkTime = 1762955544 // TODO: Document which fork this represents | |
| ) | |
| // Blob fee denominators for different fork eras | |
| const ( | |
| BlobFeeDenominatorBase = 5007716 // EIP-4844 BLOB_GASPRICE_UPDATE_FRACTION | |
| BlobFeeDenominatorHoodi1 = 8346193 // Post-HoodiBPO1 adjustment | |
| BlobFeeDenominatorHoodi2 = 11684671 // Post-HoodiBPO2 adjustment | |
| ) | |
| func (r *Rollup) getBlobFee(head *ethtypes.Header) *big.Int { | |
| if head.ExcessBlobGas == nil { | |
| log.Warn("getBlobFee called with nil ExcessBlobGas") | |
| return big.NewInt(params.BlobTxMinBlobGasprice) | |
| } | |
| HoodiBPO1Time := newUint64(HoodiBPO1ForkTime) | |
| HoodiBPO2Time := newUint64(HoodiBPO2ForkTime) | |
| denominator := big.NewInt(BlobFeeDenominatorBase) | |
| if isTimestampForked(HoodiBPO1Time, head.Time) { | |
| denominator = big.NewInt(BlobFeeDenominatorHoodi1) | |
| } | |
| if isTimestampForked(HoodiBPO2Time, head.Time) { | |
| denominator = big.NewInt(BlobFeeDenominatorHoodi2) | |
| } | |
| blobFee := fakeExponential(big.NewInt(params.BlobTxMinBlobGasprice), new(big.Int).SetUint64(uint64(*head.ExcessBlobGas)), denominator) | |
| return blobFee | |
| } |
🤖 Prompt for AI Agents
In tx-submitter/services/rollup.go around lines 1298 to 1310, replace the magic
numbers: extract the two fork timestamps and the three denominator values into
named, documented package-level constants (e.g. BlobFeeFork1Timestamp,
BlobFeeFork2Timestamp, BlobFeeDenomPreFork, BlobFeeDenomFork1,
BlobFeeDenomFork2) with comments explaining which network upgrade/fork and how
the denominators were derived; update getBlobFee to use these constants instead
of literals and add a nil check for head.ExcessBlobGas (return an appropriate
default or error/zero big.Int when nil) before using SetUint64; keep the same
branching logic for isTimestampForked but reference the new constants so the
code is self-documenting and maintainable.
Summary by CodeRabbit
New Features
Chores