revert: do not skip balance validation in eth_simulateV1#11368
Conversation
|
Claude finished @DarkLord017's task in 4m 46s —— View job PR Review
SummaryThis PR correctly reverts #11332, which improperly removed balance validation for FindingsMedium — No regression test for the correct behaviorFile: The removed test Per Suggested replacement test: [Test]
public async Task Test_eth_simulate_no_validation_still_returns_insufficient_balance()
{
TestRpcBlockchain chain = await EthRpcSimulateTestsBase.CreateChain();
SimulatePayload<TransactionForRpc> payload = new()
{
BlockStateCalls =
[
new() { Calls = [ new EIP1559TransactionForRpc { From = TestItem.AddressA, To = TestItem.AddressB, Value = 1_000_000.Ether() } ] }
],
Validation = false
};
SimulateTxExecutor<SimulateCallResult> executor = new(chain.Bridge, chain.BlockFinder, new JsonRpcConfig(), chain.SpecProvider, new SimulateBlockMutatorTracerFactory());
ResultWrapper<IReadOnlyList<SimulateBlockResult<SimulateCallResult>>> result = executor.Execute(payload, BlockParameter.Latest);
Assert.That((bool)result.Result, Is.True);
SimulateCallResult call = result.Data[0].Calls.First();
Assert.That(call.Error?.Code, Is.EqualTo(-38014)); // InsufficientFunds
}Low — PR type checkboxThe PR ticks Refactoring but this is a Bugfix — it corrects incorrect behavior introduced by #11332. Should also tick the Bugfix checkbox (or replace Refactoring with Bugfix). Low — Slightly imprecise
|
| Condition | Behavior |
|---|---|
validate:false, MaxFeePerGas = 0, balance < value |
InsufficientSenderBalance ✓ (matches geth) |
validate:false, MaxFeePerGas = 0, balance >= value |
proceeds, no gas charged from sender ✓ |
validate:false, MaxFeePerGas > 0 |
ShouldValidateGas returns true; gas fee checks also apply ✓ |
validate:true |
full Execute path, unchanged ✓ |
PayValue simplification is safe: BuyGas gate at line 611 ensures senderBalance ≥ value before PayValue is ever reached. The algebraic proof: BuyGas asserts senderBalance ≥ value + senderReservedGasPayment; after subtracting senderReservedGasPayment from state, the remaining balance satisfies remainingBalance ≥ value, so SubtractFromBalance in PayValue cannot underflow.
No orphaned Simulate method implementations remain in any ITransactionProcessor implementors (OptimismTransactionProcessor, TaikoTransactionProcessor, XdcTransactionProcessor, SystemTransactionProcessor — all checked).
|
@claude re-review |
DarkLord017
requested review from
LukaszRozmej,
benaadams and
svlachakis
as code owners
There was a problem hiding this comment.
@DarkLord017 please attach the link of a passing hive job
|
Claude finished @DarkLord017's task in 2m 15s —— View job PR Re-Review
SummaryAll findings from the previous review have been addressed. The revert is correct and complete. Previous Medium finding resolved: A regression test
This aligns with go-ethereum behavior (api_test.go L1568/L2463) and matches what the hive tests expect. FindingsLow — Pre-existing:
|
| Condition | Behavior |
|---|---|
validate:false, GasPrice=0, balance < value |
InsufficientSenderBalance ✓ |
validate:false, GasPrice=0, balance ≥ value |
proceeds, gas not charged from sender ✓ |
validate:false, MaxFeePerGas > 0 |
ShouldValidateGas returns true; fee checks apply ✓ |
validate:true |
full Execute path, unchanged ✓ |
PayValue simplification is safe: BuyGas gate at line 611 guarantees senderBalance ≥ value before PayValue is ever reached — no underflow possible.
Simulate method removal is clean: ITransactionProcessor, TransactionProcessorBase, and DisposableScopeOverridableEnvTests.TestTransactionProcessor are all consistent. No orphaned implementations.
Verdict: ready to merge from a code standpoint. The remaining ask from @svlachakis (link to a passing hive job) is a process/sign-off step, not a code concern.
|
Claude finished @DarkLord017's task in 4m 54s —— View job PR Re-Review
SummaryThe Medium finding from the previous review (missing regression test) has been addressed by commit Finding Resolution✅ Medium (Previous): Regression test — RESOLVEDCommit Assert.That(result.Result!.Error!.Contains("insufficient sender balance"), Is.True);
Assert.That(result.ErrorCode, Is.EqualTo(ErrorCodes.InsufficientFunds));The test exercises the full RPC stack via Informational nit (non-blocking): Pre-existing Low —
|
https://github.com/NethermindEth/nethermind/actions/runs/24989961006/job/73173043532 |
3 participants
Reverts #11332
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks