removes MaxFeePerGas for eth_estimateGas

[DarkLord017]

Fixes Closes Resolves #11179 & #11188

Changes

• InsufficientBalance is now a public constant with value "insufficient funds for transfer" (previously a private constant "insufficient balance"), to ensure compatibility with Go-Ethereum error semantics.

• CapByAllowance now accepts feeCap as an explicit parameter instead of deriving it from tx.MaxFeePerGas; it uses tx.CalculateFeeCap() for accurate EIP-1559 fee handling.

• Added a pre-cap step before the initial probe:

  • Reads sender balance
  • Computes feeCap = tx.CalculateFeeCap()
  • Caps tx.GasLimit to (balance - value) / feeCap
    This prevents BuyGas-related failures (e.g., insufficient balance due to high gas limit × fee cap combinations) and therefore there are no changes needed in BuyGas as it will never reach that failure also transfer failures will be overriden by the newer error

• Updated error override condition to include || err == GasEstimator.InsufficientBalance, allowing "insufficient funds for transfer" (from GasEstimator) to override the initial probe’s "insufficient sender balance" error.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Requires E2E testing

Documentation

Requires documentation update

• Yes
• No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

• Yes
• No

If yes, fill in the details here. Remove if not applicable.

Remarks

[github-actions]

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: MULMOD

Regressions (1)

Opcode	Base Median (ns)	PR Median (ns)	Delta	Abs Δ (ns)	Base CV	PR CV	Threshold	Uncertainty	Effective
MULMOD	229.717	291.323	+26.82%	61.606	8.4%	10.5%	±5.0%	±11.6%	±20.9%

[DarkLord017]

@claude review , also am i in the right direction
I am still not sure about why i removed the Subtract Balance for estimate gas

[claude]

Claude finished @DarkLord017's task in 6m 22s —— View job

---

Code Review

• Gather context and read changed files
• Review GasEstimator.cs changes
• Review TransactionProcessor.cs changes
• Review test changes
• Answer question about balance subtraction removal
• Post final review

---

Direction Assessment

Yes, you're in the right direction. The root cause is correct: during eth_estimateGas, the binary search starts with rightBound = blockGasLimit (e.g. 4M gas). The EIP-1559 check balance < GasLimit × MaxFeePerGas then fails (e.g. 4M × 50 Gwei = 0.2 ETH > 0.1 ETH balance), even when the actual gas needed (21432 × 50 Gwei << 0.1 ETH) would comfortably fit. Skipping that check for estimation is the right fix. The CapByAllowance in GasEstimator.cs already correctly bounds the search space by balance, so we don't lose safety.

---

Issues Found

Critical: IsEstimate set unconditionally on all CallAndRestore calls

CallAndRestore is not exclusive to gas estimation. It's also used by:

• BlockchainBridge.cs:297 — eth_call and eth_createAccessList
• Contract.cs:181 — internal on-chain contract reads (validators, AuRa, etc.)
• SingleCallWitnessCollector.cs:40 — stateless witness collection

By setting IsEstimate inside CallAndRestore, you've changed the balance-check semantics for all of these callers, which is a broader change than intended. The correct approach is to expose a dedicated method (e.g., EstimateAndRestore) on ITransactionProcessor that internally uses CommitAndRestore | IsEstimate, and have only GasEstimator.TryExecute call it.

Fix this →

Moderate: Restore logic bug when IsEstimate skips SubtractFromBalance

The restore block (lines 269–287 in TransactionProcessor.cs) does:

if (!senderReservedGasPayment.IsZero)
{
    WorldState.AddToBalance(tx.SenderAddress!, senderReservedGasPayment, spec);
}
DecrementNonce(tx);
WorldState.Commit(spec, commitRoots: false);

This block exists to undo what BuyGas committed — specifically the balance subtraction and nonce increment. WorldState.Reset(resetBlockChanges: false) rolls back uncommitted execution changes only; the BuyGas commit is still in the state. The restore block then manually undoes it.

But when IsEstimate skips SubtractFromBalance, nothing was subtracted — yet the restore block still calls AddToBalance(senderReservedGasPayment). This leaves the sender with senderReservedGasPayment extra balance after every TryExecute call. Over multiple binary search iterations, this accumulates. Fix:

if (!senderReservedGasPayment.IsZero && !opts.HasFlag(ExecutionOptions.IsEstimate))
{
    WorldState.AddToBalance(tx.SenderAddress!, senderReservedGasPayment, spec);
}

Fix this →

Minor: Formatting inconsistencies

Two formatting issues in BuyGas:

// Outer if has brace on same line, inner uses new line — inconsistent
if (!opts.HasFlag(ExecutionOptions.IsEstimate)) {
if (overflows || senderReservedGasPayment > balanceLeft)
{

// Missing space before !
if (!opts.HasFlag(ExecutionOptions.IsEstimate) &&!senderReservedGasPayment.IsZero)

Should be:

if (!opts.HasFlag(ExecutionOptions.IsEstimate))
{
    if (overflows || senderReservedGasPayment > balanceLeft)
    {
        ...
    }
}

if (!opts.HasFlag(ExecutionOptions.IsEstimate) && !senderReservedGasPayment.IsZero)

Minor: Unnecessary comment in TryExecute

// Ensure IsEstimate is set for gas estimation
TransactionResult callResult = transactionProcessor.CallAndRestore(txClone, ...);

This comment is misleading — IsEstimate is set inside CallAndRestore, not here. Remove it.

---

Answering Your Question: Why Skip SubtractFromBalance?

The reason the check senderReservedGasPayment > balanceLeft needs to be skipped is clear: senderReservedGasPayment = GasLimit × effectiveGasPrice, and during binary search, GasLimit starts at blockGasLimit — much larger than actual gas needed.

The reason the subtraction was also skipped is less clear. In principle, you could keep the subtraction but cap the payment to balanceLeft to prevent underflow:

if (opts.HasFlag(ExecutionOptions.IsEstimate))
{
    // During estimation GasLimit is an upper bound; cap to avoid underflow.
    senderReservedGasPayment = UInt256.Min(senderReservedGasPayment, balanceLeft);
}
else if (overflows || senderReservedGasPayment > balanceLeft)
{
    TraceLogInvalidTx(tx, ...);
    return TransactionResult.InsufficientSenderBalance;
}
if (!senderReservedGasPayment.IsZero)
    WorldState.SubtractFromBalance(tx.SenderAddress, senderReservedGasPayment, spec);

This has the advantage of not needing to fix the restore logic, AND giving the EVM a more realistic sender balance during execution (contracts checking BALANCE opcode of msg.sender see a properly deducted balance). It's the approach Geth uses.

---

What's Good

• The CapByAllowance refactor to accept feeCap directly (with GasPrice fallback for legacy txs) is cleaner and correct.
• The new tests are valuable and well-structured; the regression scenario comments are especially helpful.
• GasEstimator.CheckFunds correctly handles the value-insufficient-balance case independently.

---

PR Description

The checkboxes in the PR template are not filled in (type of change, testing). Please complete these — they drive automatic labeling.

---

Overall: Right direction, needs the CallAndRestore scoping fix and the restore logic fix before merging.

[DarkLord017]

@claude review pls
capping wont cause in TryCallAndRestore right

[claude]

Claude finished @DarkLord017's task in 3m 29s —— View job

---

Review

• Read current state of changed files
• Answer capping question re: TryCallAndRestore
• Full review of updated code

---

Answering Your Question: Does capping cause issues in TryCallAndRestore?

No, capping is safe. Here's why it works:

The pre-cap in BlockchainBridge.EstimateGas ensures:

tx.GasLimit ≤ availableForGas / feeCap   (where feeCap = MaxFeePerGas for EIP-1559)

So inside BuyGas, when it runs the EIP-1559 check:

maxGasFee = tx.GasLimit × MaxFeePerGas ≤ availableForGas = balanceLeft  ✓

And the final balance check:

senderReservedGasPayment = tx.GasLimit × effectiveGasPrice ≤ tx.GasLimit × MaxFeePerGas ≤ balanceLeft  ✓

So SubtractFromBalance runs. The restore block then calls AddToBalance(senderReservedGasPayment) and symmetrically undoes it. The balance state is correctly restored after each binary search iteration. The previous review's "restore logic bug" concern no longer applies here because you didn't skip the subtraction — you pre-capped the input so it never overflows.

---

What's Fixed Since Last Review

• Critical fixed: EstimateAndRestore is now a separate method — CallAndRestore is no longer affected
• Restore bug N/A: Since BuyGas still subtracts, the restore's AddToBalance is still correct and symmetric
• Test coverage: New regression tests (Eth_estimateGas_succeeds_when_gas_price_set_but_balance_below_block_gas_limit_times_gas_price, Eth_estimateGas_returns_execution_reverted_when_gas_price_set_and_contract_reverts) directly target the original issue

---

Remaining Issues

Minor: gasPriceToReserve / gasLimitToReserve are dead code

In TransactionProcessor.cs (BuyGas):

UInt256 gasPriceToReserve = effectiveGasPrice;
UInt256 gasLimitToReserve = (UInt256)tx.GasLimit;

These are just copies of existing values — IsEstimate is never checked in BuyGas, so they're never given different values. It looks like an artifact of an earlier approach (capping inside BuyGas via IsEstimate). Since the fix went with pre-capping instead, these should be removed.

Fix this →

Minor: Misleading doc comment on EstimateAndRestore

ITransactionProcessor.cs line 24 says:

/// Execute transaction for gas estimation, rollback state. Skips balance/overflow checks that would fail when gas limit is an upper bound.

But it doesn't skip those checks — the pre-capping ensures they pass, but they still run. The comment should say something like "gas limit is pre-capped so balance checks pass" rather than "skips checks".

Minor: error assignment changed semantics

Old (BlockchainBridge.cs):

if (err is not null && (error is null || err.StartsWith(GasExceedsAllowanceMsgPrefix, ...)))
    error = err;

New:

error = err is not null ? err : error;

Previously only allowance errors from the binary search could override a non-null error from the probe; now any err does. This is probably intentional (binary search is more authoritative) but it's a behavior change that isn't mentioned in the PR description. Worth confirming.

Minor: Extra blank lines in BuyGas

Two consecutive blank lines appeared after the blob-fee block in BuyGas (the diff shows \n\n before the final balance check). Run dotnet format whitespace to clean up.

Note: IsEstimate flag is set but unused

The IsEstimate = 16 flag is correctly wired into EstimateAndRestore, but nothing in BuyGas (or anywhere else in the call path) checks it. It's not wrong — the fix is entirely through pre-capping — but it may confuse future readers into thinking BuyGas has special estimation behavior. Either use it or note in a comment that it's reserved for future use.

---

PR Description

The checkboxes (type of change, testing) are still unchecked. Please fill them in — they drive automatic labeling.

---

Overall: The approach is correct and safe. The capping correctly prevents BuyGas from seeing an oversized gas limit, the restore is symmetric, and EstimateAndRestore is properly scoped. The remaining items above are minor polish — clean up the dead variables and the doc comment before merging.

[DarkLord017]

@claude review

[claude]

Claude finished @DarkLord017's task in 17m 15s —— View job

---

Review

• Read changed files
• Review GasEstimator.cs
• Review BlockchainBridge.cs
• Review TransactionProcessor.cs
• Review test changes
• Check previous review items resolved
• Post final review

---

Direction

The approach is correct and sound. The previous two reviews identified and tracked issues through two rounds of iteration. This round checks whether all remaining items from Review 2 are resolved.

Yes — all items from Review 2 are resolved. The developer took a clean approach: revert the TransactionProcessor.cs changes entirely and instead pre-cap tx.GasLimit in BlockchainBridge.EstimateGas before passing to the binary search. This avoids the EstimateAndRestore/IsEstimate complexity entirely.

---

Previous Review Items Status

Item	Status
gasPriceToReserve/gasLimitToReserve dead code	✅ Resolved — TransactionProcessor.cs not changed
Misleading EstimateAndRestore doc comment	✅ Resolved — EstimateAndRestore doesn't exist in this approach
error assignment changed semantics	✅ Resolved — reverted to original `if (err is not null && (error is null
Extra blank lines in BuyGas	✅ Resolved — TransactionProcessor.cs not changed
Unused IsEstimate flag	✅ Resolved — flag not introduced

---

What's Good

BlockchainBridge.EstimateGas pre-cap (BlockchainBridge.cs:183-193): correctly mirrors Geth's hi = min(hi, (balance - value) / gasFeeCap). Applied before the initial probe, so BuyGas never sees a gas limit that makes gasLimit × feeCap exceed the sender's balance.

GasEstimator.CapByAllowance fix — The old code only applied the cap for EIP-1559 txs (tx.MaxFeePerGas). The fix correctly extends it to legacy txs via the feeCap parameter. This was also a bug: legacy txs with GasPrice > 0 could also hit the "insufficient balance" rejection.

GetError fix — gasTracer.Error ?? TransactionExecutionFails → gasTracer.Error ?? defaultError. This makes CheckFunds correctly return "insufficient sender balance" instead of the generic "Transaction execution fails" when the sender can't cover the value transfer. Test updates in GasEstimationTests.cs and TransactionProcessorTests.cs are correct and intentional.

TransactionProcessorTests.cs change — Adding .WithGasPrice(0) for the txValue == AccountBalance case correctly changes the expectation from an allowance error (old: gas cost of 1 wei × gasLimit > 0 remaining balance) to success (new: gas is free, so sending full balance as value is valid). The test is logically correct.

New regression tests in EthRpcModuleTests.EstimateGas.cs are well-structured and directly target the reported issue.

---

Remaining Minor Items

1. Duplicate feeCap computation

BlockchainBridge.cs:187 and GasEstimator.cs:81-83 both compute feeCap with slightly different style:

// BlockchainBridge.cs
UInt256 feeCap = tx.MaxFeePerGas > UInt256.Zero ? tx.MaxFeePerGas : tx.GasPrice;
if (feeCap > UInt256.Zero && ...) { ... }

// GasEstimator.cs
UInt256 feeCap = tx.MaxFeePerGas > UInt256.Zero ? tx.MaxFeePerGas
               : tx.GasPrice > UInt256.Zero ? tx.GasPrice
               : UInt256.Zero;

Both produce the same result (the feeCap > 0 guard in BlockchainBridge handles GasPrice = 0), but the duplication is mild. Consider extracting to Transaction.GetFeeCap() or a local extension, but this is a polish-level item. Fix this →

2. tx.GasLimit mutation in EstimateGas

BlockchainBridge.cs:192 mutates tx.GasLimit in-place. This is consistent with the existing mutations in CallAndRestore (e.g., transaction.SenderAddress ??= Address.Zero, transaction.Nonce = ...), so it's not a new pattern — just worth noting for clarity. No action required.

3. PR description checkboxes still empty

The type-of-change, testing, and documentation checkboxes in the PR template are still unchecked. Please fill them in — they drive automatic labeling.

---

Edge Case Verification

When allowance < intrinsicGas (e.g., sender can't afford even one transaction): tx.GasLimit gets capped to allowance. GasEstimator then uses header.GasLimit as rightBound (since tx.GasLimit < intrinsicGas), CapByAllowance re-caps it to allowance, leftBound > rightBound → CannotEstimateGasExceeded. Correct behavior.

When user explicitly sets a small gas value that passes the allowance: tx.GasLimit <= allowance → no cap applied, binary search uses user-specified gas limit. Correct.

Double-cap idempotency: BlockchainBridge pre-caps tx.GasLimit, GasEstimator.CapByAllowance re-caps rightBound with the same formula. Both produce the same allowance, so the second cap is a no-op. Clean.

---

Overall: Ready to merge after filling in PR description checkboxes. The implementation is correct and well-tested.

[DarkLord017]

@claude review

[claude]

Claude finished @DarkLord017's task in 3m 43s —— View job

---

Review

• Read changed files
• Review GasEstimator.cs
• Review BlockchainBridge.cs
• Review Transaction.cs and test files
• Check previous review items resolved
• Post final review

---

What Changed Since Review 3

Change	Assessment
InsufficientBalance constant: "insufficient balance" → "insufficient sender balance"	✅ Correct — now matches TransactionResult error string and is more descriptive
CapByAllowance takes feeCap directly (was Transaction tx)	✅ Cleaner — decouples from Transaction, reduces coupling
tx.CalculateFeeCap() added to Transaction.cs	✅ Good — unifies feeCap computation across BlockchainBridge and GasEstimator
GetError: StatusCode.Failure case uses defaultError parameter	✅ Correct — allows CheckFunds to pass InsufficientBalance without overriding the generic fallback
Test updates to match "insufficient sender balance"	✅ Correct
Removed EstimateGas_tx_returns_InsufficientMaxFeePerGasForSenderBalanceError test	✅ Correct — the pre-cap ensures this path can't be reached for EstimateGas; Call_tx_returns_InsufficientMaxFeePerGasForSenderBalanceError is preserved for the eth_call path

---

One Minor Observation

CalculateFeeCap() edge case for EIP-1559 with MaxFeePerGas = 0

// Transaction.cs:52
public UInt256 CalculateFeeCap() => MaxFeePerGas > UInt256.Zero ? MaxFeePerGas : GasPrice;

For EIP-1559 txs, GasPrice is the backing field _gasPrice which stores MaxPriorityFeePerGas. So if DecodedMaxFeePerGas = 0 and MaxPriorityFeePerGas = 5 Gwei, CalculateFeeCap() returns MaxPriorityFeePerGas = 5 Gwei instead of 0. This means the cap is slightly more conservative than needed. This is benign — a zero MaxFeePerGas EIP-1559 tx can't pay any gas (effectiveGasPrice ≥ baseFee > 0), so it would be rejected by BuyGas regardless. No action required, but worth knowing.

---

All Previous Review Items Still Resolved

Item from Review 2	Status
CallAndRestore scoping (critical)	✅ Resolved — EstimateAndRestore approach replaced with pre-capping
Restore logic bug	✅ N/A — no subtraction skipped, restore is symmetric
Dead vars gasPriceToReserve/gasLimitToReserve	✅ Resolved — TransactionProcessor.cs not touched
error assignment changed semantics	✅ Resolved — reverted to original if (err is not null && ...)
Formatting issues	✅ Resolved

---

PR Description

The checkboxes (type of change, testing) are still unchecked. Please fill them in — they drive automatic labeling. This is the only blocker.

---

Overall: The implementation is correct and complete. Ready to merge after filling in the PR description checkboxes.

[DarkLord017]

@claude review

[claude]

Claude finished @DarkLord017's task in 4m 31s —— View job

---

Review

• Read changed files
• Compare against previous review state
• Review GasEstimator.cs
• Review BlockchainBridge.cs
• Review Transaction.cs
• Review test changes
• Post final review

---

What Changed Since Review 4

Change	Assessment
InsufficientBalance promoted to public const with value "insufficient funds for transfer" (was private "insufficient sender balance")	✅ Correct — Geth parity: go-ethereum returns exactly this string for value-transfer failures
GetError StatusCode.Failure branch: gasTracer.Error ?? TransactionExecutionFails → gasTracer.Error ?? defaultError	✅ Correct — allows CheckFunds to pass InsufficientBalance as the fallback so the message propagates correctly when the tracer has no explicit error
BlockchainBridge override condition adds || err == GasEstimator.InsufficientBalance	✅ Correct — ensures "insufficient funds for transfer" from CheckFunds can override a non-null error from the initial probe
Test updates to match new string	✅ Correct
PR description checkboxes now filled in	✅ Resolved

---

Previous Review Items Status

Item from Reviews 1–4	Status
CallAndRestore scoping (Critical, R1)	✅ Resolved — pre-cap approach used instead
Restore logic bug (Moderate, R1)	✅ N/A — no subtraction skipped
Dead vars gasPriceToReserve/gasLimitToReserve (Minor, R2)	✅ Resolved — TransactionProcessor.cs not touched
error assignment semantics (Minor, R2)	✅ Resolved
EstimateAndRestore doc comment (Minor, R3)	✅ Resolved — approach doesn't use it
Duplicate feeCap computation (Minor, R3)	✅ Now unified via tx.CalculateFeeCap()
Unused IsEstimate flag (Minor, R3)	✅ Resolved — flag not introduced
CalculateFeeCap() EIP-1559 edge case (Note, R4)	✅ Acknowledged — benign in practice
PR description checkboxes (All reviews)	✅ Now filled in

---

Correctness of the GetError Fix

The previous state:

{ StatusCode: StatusCode.Failure } => gasTracer.Error ?? TransactionExecutionFails,

CheckFunds called GetError(gasTracer, InsufficientBalance) but the StatusCode.Failure branch ignored defaultError, so when gasTracer.Error == null it would return "Transaction execution fails" instead of "insufficient funds for transfer". The fix:

{ StatusCode: StatusCode.Failure } => gasTracer.Error ?? defaultError,

correctly propagates the caller-supplied defaultError. The test at GasEstimationTests.cs:49 and 79 now correctly expect "insufficient funds for transfer".

Correctness of the BlockchainBridge Condition

The added || err == GasEstimator.InsufficientBalance handles the case where:

1. Pre-cap wasn't applied (because tx.Value > senderBalance → SubtractUnderflow returns true)
2. The initial probe produced some error
3. CheckFunds correctly identifies the real issue as insufficient balance

In that flow, error from the probe would be non-null, but "insufficient funds for transfer" is more accurate. The condition correctly surfaces it. This is the path exercised by Eth_estimateGas_web3_should_return_insufficient_balance_error.

---

One Note (No Action Required)

GasEstimationTests.cs:446 still uses the literal "insufficient balance" (not the constant) as a manually-seeded tracer error, and line 451 asserts against it. This is intentional — the test seeds the tracer with an arbitrary error string and verifies it passes through. This path is independent of GasEstimator.InsufficientBalance and is correct as-is.

---

Overall: All previous issues resolved. The new changes are correct and complete Geth-compatibility for the "insufficient funds for transfer" error string. Ready to merge.