TransactionForRpc improvements

[svlachakis]

Fixes Closes Resolves #10547

Changes

Removes TransactionForRpc.EnsureDefaults entirely and folds defaulting + gas-cap enforcement into ToTransaction(gasCap). The "two methods, easy to forget one" pattern that produced the original bugs is structurally gone.

What changed

• TransactionForRpc.EnsureDefaults (abstract) and its overrides in LegacyTransactionForRpc and Optimism's DepositTransactionForRpc are deleted.
• ToTransaction(validateUserInput, gasCap, spec) is now the single conversion entry point. It always sets SenderAddress (defaults to Address.Zero) and GasLimit (defaults to gasCap, or long.MaxValue if unset; capped at gasCap when provided).
• The hardcoded Gas ?? 90_000 magic in LegacyTransactionForRpc.ToTransaction is removed.
• IDebugBridge.GetBundleTraces takes long? gasCap; the bridge converts internally. DebugRpcModule.PrepareTransactions is removed.

Bugs closed

• proof_call had no gas-cap enforcement — a caller sending gas: 0 got the full block gas limit, ignoring GasCap. Now properly capped via injected IJsonRpcConfig.
• SimulateTxExecutor.Prepare skipped defaulting — gas: null fell back to a hardcoded 90,000 instead of gasCap. Now consistent with all other paths.
• IDebugBridge.GetBundleTraces relied on the caller to pre-prepare transactions — now self-contained.

Behavior change worth flagging

gas: 0 is now treated as a literal request (intrinsic-gas error), not as a "missing" signal that defaults to gasCap. This matches Geth and is required by the multicall-insufficient-gas-38013 Hive spec test. gas: null is the only "missing" signal.

debug_traceCallMany and eth_simulateV1 need different default-gas policies for gas: null (gasCap vs. block gas limit). This is now expressed by a localized call.Gas ??= jsonRpcConfig.GasCap in DebugRpcModule.TraceCallManyWithOverrides, with a comment explaining why eth_simulateV1 deliberately omits it.

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

Hive tests Job: https://github.com/NethermindEth/nethermind/actions/runs/25330905724

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

[claude]

Claude finished @svlachakis's task in 5m 26s —— View job

---

PR Review

• Load repository rules and guidelines
• Read changed files and analyze PR
• Review correctness, edge cases, regressions
• Review security implications
• Review performance concerns
• Post inline comments for specific issues
• Post final review summary

---

Summary

The refactoring direction is sound. Folding EnsureDefaults + ToTransaction into a single ToTransaction(gasCap) call eliminates the "two methods, easy to forget one" hazard that produced the original bugs, and the described bug fixes (proof_call gas-cap enforcement, SimulateTxExecutor 90 000 default, debug bundle self-containment) are all real and correctly addressed. The test matrix for LegacyTransactionForRpc is well-structured.

---

Findings

Medium (1)

#	Location	Issue
1	DepositTransactionForRpc.cs:82	No regression tests for the new gas-cap logic in DepositTransactionForRpc.ToTransaction. The Optimism deposit path had its gas handling changed (old EnsureDefaults deleted, new capping logic introduced) but nothing tests it. AGENTS.md requires regression tests when fixing bugs. See inline comment for the suggested test matrix.

Low (3)

#	Location	Issue
2	DebugRpcModuleTests.TraceCallMany.cs:250	Comment says "via EnsureDefaults" — that method no longer exists. Should reference ToTransaction. (File not in this PR's diff, but was indirectly affected.)
3	DepositTransactionForRpc.cs:82–85	Gas-cap-zero semantics differ silently from LegacyTransactionForRpc: Gas=null, gasCap=0 yields GasLimit=0 here vs long.MaxValue in the legacy path. Gas is a required field for deposits so this edge case can't be hit via valid input — but the implementation pattern differs from the rest of the hierarchy without a comment explaining why.
4	DebugRpcModule.cs:511	call.Gas ??= jsonRpcConfig.GasCap is a no-op when GasCap is null. An explicit guard (if (jsonRpcConfig.GasCap is not null and not 0)) would make the intent visible. See inline comment.

---

Correctness notes

• proof_call semantic change: gas: 0 previously fell back to callHeader.GasLimit (the old if (GasLimit == 0) patch). It now returns an intrinsic-gas error. This matches Geth and is consistent with the PR description.
• HadGasLimitInRequest interaction: The pre-mutation call.Gas ??= jsonRpcConfig.GasCap in TraceCallManyWithOverrides correctly causes HadGasLimitInRequest = true for originally-gas-less transactions, opting them out of the simulate engine's BlockGasLeft fallback. Behaviour is intentional and clearly commented.
• DI for ProofRpcModule: The new IJsonRpcConfig constructor parameter is resolved through Autofac from the root lifetime scope via NethermindModule → RpcModules registration. No wiring issue.
• L2Api.cs and OptimismEthRpcModule: Both call ToTransaction() without gasCap, which is correct — the former processes actual chain transactions (gas is always present), the latter sends real transactions to the pool (gasCap enforcement is not appropriate).

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 6m 37s —— View job

---

PR Review

• Load repository rules and guidelines
• Read changed files and analyze PR
• Review correctness, edge cases, and regressions
• Review security implications
• Review performance concerns
• Post inline comments for specific issues
• Post final review summary

---

Summary

All findings from the previous review have been resolved. The PR is now clean.

---

Previous findings — status

#	Severity	Location	Status
1	Medium	DepositTransactionForRpc.cs – missing regression tests	✅ Fixed: 7 test cases added in DepositTransactionForRpcTests.cs, covering Gas×gasCap combinations including the ArgumentNullException path.
2	Low	DebugRpcModuleTests.TraceCallMany.cs:250 – stale "via EnsureDefaults" comment	✅ Fixed: updated to "debug_traceCallMany defaults missing gas to gasCap, not blockGasLimit".
3	Low	DepositTransactionForRpc.cs:82–85 – gasCap=0 asymmetry	✅ Acknowledged and tested. Old EnsureDefaults for deposits would cap Gas=5000 to 0 when gasCap=0 — now treated as "no cap" (matching legacy path). This is an intentional bug fix, tested by [TestCase(5_000L, 0L, 5_000L)].
4	Low	DebugRpcModule.cs:511 – call.Gas ??= jsonRpcConfig.GasCap no-op for null/zero cap	✅ Fixed: guard if (jsonRpcConfig.GasCap is not null and not 0) added.

---

New observations (this pass)

DI wiring for ProofRpcModule: The new IJsonRpcConfig constructor parameter resolves correctly via ConfigRegistrationSource, which dynamically satisfies any IConfig-derived type from IConfigProvider. Both the production stack (ProofModuleFactory.Create() → proofRpcScope.Resolve<IProofRpcModule>()) and the test stack (TestNethermindModule → PseudoNethermindModule → NethermindModule with ConfigRegistrationSource) correctly inject it. ✓

debug_executionWitnessCall (line 699–701): Calls ToTransaction(validateUserInput: false) without gasCap, but this is pre-existing behaviour — EnsureDefaults was never called in this path either. The tx.SenderAddress ??= Address.Zero that was removed is now handled inside ToTransaction. No regression. ✓

proof_call gas-limit change: With Gas = null and default config (GasCap = 100_000_000), GasLimit changes from the old implicit 90_000 (the ??-fallback in ToTransaction) to 100_000_000. Existing tests are unaffected because the EVM consumes only the gas needed; unused gas doesn't change proof inclusion or output. ✓

gas: 0 semantic change: All callers now treat Gas = 0 as a literal GasLimit = 0 (intrinsic-gas error), not as a "missing" signal. This matches Geth and is required by the multicall-insufficient-gas-38013 Hive test. Documented in PR description and backed by unit tests in EthRpcModuleTests. ✓

---

No outstanding findings. The refactoring direction is correct, the tests cover the critical cases, and the EnsureDefaults/ToTransaction hazard is structurally eliminated.

[claude]

Claude finished @svlachakis's task in 3m 18s —— View job

---

PR Review

• Load repository rules and guidelines
• Read changed files and analyze PR
• Review correctness, edge cases, and regressions
• Review security implications
• Review performance concerns
• Post inline comments for specific issues
• Post final review summary

---

Summary

Third review pass — no new findings. All code paths are correct and the prior findings remain resolved.

---

Review of all 18 changed files

TransactionForRpc.cs / LegacyTransactionForRpc.cs
EnsureDefaults deleted. New ToTransaction(gasCap) gas logic:

long effectiveCap = gasCap is null or 0 ? long.MaxValue : gasCap.Value;
tx.GasLimit = Gas is null ? effectiveCap : long.Min(Gas.Value, effectiveCap);

Verified against all 9 test cases in EthRpcModuleTests.ToTransaction_caps_and_defaults_gas. All correct.

DepositTransactionForRpc.cs

long effectiveCap = gasCap is null or 0 ? long.MaxValue : gasCap.Value;
long? gasOrDefault = Gas ?? (effectiveCap == long.MaxValue ? null : effectiveCap);
tx.GasLimit = long.Min(gasOrDefault ?? throw new ArgumentNullException(nameof(Gas)), effectiveCap);

Traced through all 7 DepositTransactionForRpcTests cases (including the null/0 gasCap throw path). All correct.

ProofRpcModule.cs
Old if (GasLimit == 0) GasLimit = callHeader.GasLimit patch correctly removed — it existed only to handle the gas:0 = missing semantics that the new code no longer uses. SenderAddress ??= Address.Zero correctly moved inside ToTransaction. IJsonRpcConfig DI resolves via ConfigRegistrationSource. ✓

EthRpcModule.TransactionExecutor.cs — EstimateGasTxExecutor
EstimateGasTxExecutor.Execute still pre-sets Gas = blockGasLimit when null, before calling base.Execute → ToTransaction(gasCap), which caps it at gasCap. Net result: GasLimit = min(blockGasLimit, gasCap) — same as before. ✓

SimulateTxExecutor.cs
hadGasLimitInRequest = asLegacy?.Gas is not null is evaluated on the (possibly pre-mutated) TransactionForRpc. For debug_traceCallMany, call.Gas ??= jsonRpcConfig.GasCap in TraceCallManyWithOverrides runs before Prepare, so hadGasLimitInRequest = true, opting out of the engine's BlockGasLeft fallback. Correct and explained by the comment. For eth_simulateV1, Gas is not pre-mutated, so the engine's fallback applies. ✓

DebugRpcModule.cs — TraceCallManyWithOverrides

if (jsonRpcConfig.GasCap is not null and not 0)
{
    foreach (...) call.Gas ??= jsonRpcConfig.GasCap;
}

Guard correctly handles null/zero-cap case. ✓

DebugBridge.cs
GetBundleTraces receives gasCap and passes it to ToTransaction(gasCap: gasCap). Previously relied on the caller calling EnsureDefaults — now self-contained. ✓

TraceRpcModule.cs
Both trace_call and trace_callMany drop EnsureDefaults and pass gasCap directly to ToTransaction. ✓

Test files

• EthRpcModuleTests.cs: old [Ignore]d disparity test replaced with a comprehensive 9-case parameterized matrix. ✓
• DepositTransactionForRpcTests.cs: 7 new test cases (5 gas/cap combos + 2 ArgumentNullException paths). ✓
• DebugRpcModuleTests.TraceCallMany.cs: stale "via EnsureDefaults" comment updated. ✓
• DebugModuleTests.cs: mock arg index corrected (ArgAt<CancellationToken>(3) after new gasCap parameter). ✓

---

Previous findings — final status

#	Severity	Location	Status
1	Medium	DepositTransactionForRpc – missing regression tests	✅ Resolved
2	Low	Stale "via EnsureDefaults" comment	✅ Resolved
3	Low	gasCap=0 asymmetry between deposit and legacy paths	✅ Resolved + tested
4	Low	call.Gas ??= jsonRpcConfig.GasCap no-op for null/zero cap	✅ Resolved with guard

No outstanding findings. The EnsureDefaults/ToTransaction hazard is structurally eliminated, all call sites are updated, and the test matrix covers the critical edge cases.