Dynamic EIP-1559 params setup

[lucca30]

Description

This PR proposes a more flexible validation over EIP-1559 configurable TargetGas and BaseFeeDenominator, in the same way is already done by the gas limit. Which means that consensus will not block changes over this parameters set by the block producer. The goal is to allow more dynamic setups by the block producer whenever necessary.

The only proposed restriction is on the boundary range of 5% of parentBaseFee for increase or decrease. Which is already above the current limit of 1.7% on a scenario of 0% gas usage on parent block.

Changes

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)
• Changes only for a subset of nodes

[codecov]

Codecov Report

❌ Patch coverage is 97.67442% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.07%. Comparing base (8db7315) to head (ddb7557).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
params/protocol_params.go	84.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1989      +/-   ##
===========================================
+ Coverage    48.25%   49.07%   +0.82%     
===========================================
  Files          843      844       +1     
  Lines       144648   145299     +651     
===========================================
+ Hits         69793    71300    +1507     
+ Misses       70268    69284     -984     
- Partials      4587     4715     +128     

Files with missing lines	Coverage Δ
consensus/misc/eip1559/eip1559.go	94.17% <100.00%> (+15.79%)	⬆️
eth/peer.go	88.42% <100.00%> (+13.68%)	⬆️
internal/cli/server/config.go	61.30% <100.00%> (+0.80%)	⬆️
internal/cli/server/flags.go	100.00% <100.00%> (ø)
params/config.go	35.01% <100.00%> (+4.32%)	⬆️
params/protocol_params.go	84.21% <84.21%> (ø)

... and 39 files with indirect coverage changes

Files with missing lines	Coverage Δ
consensus/misc/eip1559/eip1559.go	94.17% <100.00%> (+15.79%)	⬆️
eth/peer.go	88.42% <100.00%> (+13.68%)	⬆️
internal/cli/server/config.go	61.30% <100.00%> (+0.80%)	⬆️
internal/cli/server/flags.go	100.00% <100.00%> (ø)
params/config.go	35.01% <100.00%> (+4.32%)	⬆️
params/protocol_params.go	84.21% <84.21%> (ø)

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[adamdossa]

@claude Please can you do a thorough review of this change, highlighting any potential safety issues, bugs or edge-cases.

[claude]

Claude finished @adamdossa's task in 1m 49s —— View job

---

Reviewing PR #1989: Dynamic EIP-1559 params setup

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and understand the changes
• Review for safety issues, bugs, and edge cases
• Provide comprehensive review feedback

---

Summary

This PR introduces dynamic configuration of EIP-1559 parameters (TargetGasPercentage and BaseFeeChangeDenominator) after the Dandeli hard fork, and removes base fee validation from consensus for post-Dandeli blocks. This is a significant change that shifts trust from protocol enforcement to block producer discretion.

---

Safety Concerns

1. Removal of Base Fee Validation is a Critical Consensus Change ⚠️

File: consensus/misc/eip1559/eip1559.go:47-54

// After Dandeli hard fork, base fee validation is removed to allow dynamic configuration.
if config.Bor != nil && config.Bor.IsDandeli(header.Number) {
    // Post-Dandeli: Skip base fee validation
    return nil
}

Concern: Post-Dandeli, validators can set any arbitrary base fee in their blocks, including:

• Zero base fee (effectively disabling EIP-1559 fee burning)
• Extremely high base fees (causing transaction failures)
• Values that don't follow the EIP-1559 adjustment formula

This is a fundamental change to how the network operates. While the PR description mentions this is intentional for "more dynamic setups," this opens the door to:

• Economic attacks where validators manipulate base fees
• Network instability if validators set conflicting base fees
• Loss of fee burning if validators consistently set low/zero base fees

Recommendation: Consider adding bounds checking even after Dandeli, or document the threat model and governance mechanisms that prevent misuse.

---

2. No Minimum Base Fee Enforcement ⚠️

File: consensus/misc/eip1559/eip1559.go:105-109 and test at line 643-653

The code accepts a base fee of zero:

t.Run("accepts zero base fee", func(t *testing.T) {
    header := &types.Header{
        BaseFee:  big.NewInt(0), // Zero is valid
    }
    err := VerifyEIP1559Header(testConfig, parent, header)
    require.NoError(t, err, "should accept header with zero base fee")
})

Concern: A zero base fee means:

• No tokens are burned (breaking EIP-1559's deflationary mechanism)
• Transactions can be included with zero priority fee
• Potential for spam attacks with very low cost

---

3. Node Heterogeneity Risk

File: params/config.go:887-888

// Runtime miner configuration (set via sealer/miner CLI flags, not from genesis JSON)
TargetGasPercentage      *uint64 `json:"-"` // Post-Dandeli: target gas as % of gas limit
BaseFeeChangeDenominator *uint64 `json:"-"` // Post-Dandeli: base fee change rate

These parameters are explicitly excluded from JSON serialization (json:"-"), meaning:

• Different validators can run with different parameters
• Non-mining nodes won't know what parameters validators are using
• This could lead to divergent base fee calculations across the network

Recommendation: Consider whether these parameters should be announced or recorded in block headers for transparency.

---

Potential Bugs

4. Division by Zero Protection - But Only Logged, Not Propagated

File: params/config.go:1004-1010

if val > 0 {
    return val
}
// Invalid value - log error and fall back to default
log.Error("Invalid BaseFeeChangeDenominator in BorConfig (must be > 0), falling back to default",
    "configured", val)

Observation: While division by zero is prevented by falling back to defaults, the error is only logged. This silent fallback could mask configuration mistakes.

Recommendation: Consider returning an error during startup validation (in buildEth) rather than silently falling back during block production.

---

5. Missing Validation at Startup

File: internal/cli/server/config.go:1179-1188

// Set runtime miner gas parameters in BorConfig (if not in developer mode and Bor chain)
if !c.Developer.Enabled && n.Genesis != nil && n.Genesis.Config != nil && n.Genesis.Config.Bor != nil {
    if c.Sealer.TargetGasPercentage > 0 {
        n.Genesis.Config.Bor.TargetGasPercentage = &c.Sealer.TargetGasPercentage
    }
    if c.Sealer.BaseFeeChangeDenominator > 0 {
        n.Genesis.Config.Bor.BaseFeeChangeDenominator = &c.Sealer.BaseFeeChangeDenominator
    }
}

Observation: The values are only validated when GetTargetGasPercentage or GetBaseFeeChangeDenominator are called at runtime, not at startup. A user could configure:

• --miner.targetGasPercentage=150 (invalid, >100%)
• --miner.baseFeeChangeDenominator=0 (invalid, would cause division by zero)

And only see an error log at block production time.

Recommendation: Add explicit validation during config building:

if c.Sealer.TargetGasPercentage > 100 {
    return nil, fmt.Errorf("miner.targetGasPercentage must be between 1-100, got %d", c.Sealer.TargetGasPercentage)
}

---

Edge Cases

6. Pre-Dandeli Blocks Not Affected

File: consensus/misc/eip1559/eip1559_test.go:789-831

Good test coverage for pre-Dandeli blocks still requiring correct base fee validation. The backward compatibility is properly maintained.

7. Boundary Block at Dandeli Activation

The code checks IsDandeli(header.Number) for the header being validated, which is correct. However, the tests could benefit from explicit testing of the exact block where Dandeli activates.

8. Integer Overflow in Gas Target Calculation

File: consensus/misc/eip1559/eip1559.go:119-120

targetPercentage := config.Bor.GetTargetGasPercentage(parent.Number)
return parent.GasLimit * targetPercentage / 100

Observation: For very large gas limits (approaching uint64 max), parent.GasLimit * targetPercentage could overflow before the division. While current gas limits are far from this, it's worth noting.

---

Code Quality

9. Good Validation with Fallbacks

The validation in GetTargetGasPercentage and GetBaseFeeChangeDenominator with appropriate fallbacks to hard-fork defaults is well implemented.

10. Comprehensive Test Coverage

The test file adds 443 lines of tests covering:

• Dynamic target gas percentage changes
• Dynamic base fee denominator changes
• Invalid parameter handling (zero, >100%)
• Pre/post Dandeli validation behavior
• Nil config handling

---

Documentation

The CLI documentation in docs/cli/server.md is updated appropriately with the new flags.

---

Summary of Recommendations

Priority	Issue	Recommendation
High	Base fee validation removed	Document threat model; consider minimum bounds
High	Zero base fee allowed	Consider enforcing minimum base fee
Medium	No startup validation	Add validation in buildEth for invalid values
Medium	Silent fallbacks	Consider failing loudly on misconfiguration
Low	Integer overflow potential	Add overflow check or document constraints

---

Overall, the code is well-structured with good test coverage, but the removal of base fee validation is a significant trust shift that should be carefully considered from a network security perspective.

[cffls]

Looks good overall. If possible, could you add an integration test (tests/bor/bor_test.go) that simulates the scenario where the BP changes these parameters at a certain block and the block still get accepted by a validator?

[sonarqubecloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.5% Duplication on New Code

See analysis details on SonarQube Cloud

[cffls]

@claude Can you review this PR, focusing on safety and performance? Provide a summary as a comment highlighting any concerns related to these changes.

[claude]

Claude finished @lucca30's task in 1m 29s —— View job

---

Reviewing PR #1989: Dynamic EIP-1559 params setup

• Analyze changed files focusing on safety and performance
• Review the EIP-1559 validation logic changes
• Examine boundary limit implementation
• Check configuration and startup validation
• Review test coverage for edge cases
• Provide summary with safety and performance concerns

---

Summary

This PR introduces dynamic configuration of EIP-1559 parameters (TargetGasPercentage and BaseFeeChangeDenominator) after the Dandeli hard fork, with a critical 5% boundary limit on base fee changes. This is a well-designed change that addresses the previous review concern about unrestricted base fee changes.

---

Safety Analysis

✅ Strengths

1. 5% Boundary Limit is Well-Implemented (consensus/misc/eip1559/eip1559.go:30-36, 72-97)

   • The MaxBaseFeeChangePercent = 5 constant limits base fee volatility
   • Both CalcBaseFee (production) and VerifyEIP1559Header (validation) enforce this limit
   • This is a significant safety improvement from the earlier version that skipped validation entirely

2. Dual Enforcement - The boundary is enforced in two places:

   • In CalcBaseFee (lines 119-155): Caps the calculated base fee change
   • In verifyBaseFeeWithinBoundaries (lines 75-97): Validates incoming blocks
   • This ensures blocks produced with aggressive parameters are still bounded

3. Startup Validation for TargetGasPercentage (internal/cli/server/config.go:1216-1218)

   if c.Sealer.TargetGasPercentage > 100 {
       return nil, fmt.Errorf("miner.targetGasPercentage must be between 1-100, got %d", c.Sealer.TargetGasPercentage)
   }

4. Comprehensive Test Coverage

   • eip1559_basefee_boundary_test.go: Tests boundary enforcement at various percentages
   • TestAggressiveParametersExceedBoundary: Verifies aggressive configs (denominator=4,8,16) are capped
   • TestDefaultParametersWithinBoundary: Documents default behavior (~1.7% max change)
   • Integration test in bor_config_change_test.go: End-to-end validation

---

Addressing @cffls's Question

Q: Is it possible for a BP to set values resulting in >5% base fee change?

A: No - Even with extreme parameter values, the 5% cap is enforced:

1. At Block Production (CalcBaseFee, lines 136-138 and 153-155):

   if applyBoundaryCap && num.Cmp(maxAllowedChange) > 0 {
       num.Set(maxAllowedChange)
   }

2. At Block Validation (verifyBaseFeeWithinBoundaries, lines 90-94):

   if actualChange.Cmp(maxAllowedChange) > 0 {
       return fmt.Errorf("baseFee change exceeds %d%% limit...", MaxBaseFeeChangePercent, ...)
   }

The test TestAggressiveParametersExceedBoundary explicitly verifies this with denominator=4 (extremely aggressive) and confirms the change is capped at 5%.

---

Potential Concerns

⚠️ Missing Startup Validation for BaseFeeChangeDenominator

File: internal/cli/server/config.go:1221-1223

if c.Sealer.BaseFeeChangeDenominator > 0 {
    n.Genesis.Config.Bor.BaseFeeChangeDenominator = &c.Sealer.BaseFeeChangeDenominator
}

Unlike TargetGasPercentage which validates > 100, BaseFeeChangeDenominator accepts any non-zero value. While runtime validation exists in params/protocol_params.go:254-260 with fallback behavior, there's no minimum denominator validation at startup.

Impact: Low - The boundary cap protects against aggressive values anyway, but it would be cleaner to warn users at startup if they configure something like --miner.base-fee-change-denominator=1.

Recommendation: Consider adding a sanity check:

if c.Sealer.BaseFeeChangeDenominator > 0 && c.Sealer.BaseFeeChangeDenominator < 8 {
    log.Warn("BaseFeeChangeDenominator is very aggressive, changes will be capped at 5%", 
        "configured", c.Sealer.BaseFeeChangeDenominator)
}

Add sanity check →

---

⚠️ Node Heterogeneity - Different BP Configurations

File: params/config.go:901-902

TargetGasPercentage      *uint64 `json:"-"` // Post-Dandeli: target gas as % of gas limit
BaseFeeChangeDenominator *uint64 `json:"-"` // Post-Dandeli: base fee change rate

These are runtime-only parameters (json:"-") not persisted in genesis or blocks. Different block producers can run with different values, which means:

• A BP with denominator=8 will calculate higher base fees than one with denominator=64
• The boundary cap ensures validators still accept these blocks

Impact: Low to Medium - This is by design for operational flexibility, but may cause some unpredictability in base fee behavior depending on which BP produced the block.

---

Performance Analysis

✅ No Performance Concerns

1. BigInt Operations: The boundary calculations use standard big.Int operations (Mul, Div, Sub, Cmp) which are already used extensively in the base fee calculation path. No additional allocations beyond what's already there.

2. Constant-time Checks: The boundary validation is O(1) - just a few big integer operations per block.

3. No Additional Database/Network Operations: The changes don't add any I/O or network calls.

---

Test Coverage Assessment

Area	Coverage	Notes
Boundary enforcement	✅ Excellent	Multiple test cases for increase/decrease
Aggressive parameters	✅ Excellent	Tests denominator=4,8,16
Pre-Dandeli compatibility	✅ Good	Verifies strict validation before fork
Startup validation	✅ Good	Tests invalid percentage values
Integration	✅ Good	End-to-end test with 2 validators

---

Conclusion

This PR is well-designed from a safety perspective. The 5% boundary cap is the critical safeguard that addresses the previous concern about unrestricted base fee changes. The implementation enforces this limit both at block production and validation, ensuring network-wide protection regardless of individual node configurations.

The minor suggestions above are improvements rather than blockers.

[cffls]

Looks like my concern has already been addressed. LGTM!