Skip to content

Commit 7d7f936

Browse files
Marchhillclaude
andcommitted
fix(evm): address devnet-6 review findings in the state-gas policy
- Clamp RemoveStateGasRefundFromReservoir for a net-spill (negative) reservoir so revoking an advanced refund cannot reclaim spilled state gas or over-deduct recorded usage; regression test included. - Set the OutOfGas flag when ConsumeStateGas fails on the spill path. - Saturate the EIP-7825 cap subtraction in CreateAvailableFromIntrinsic. - Extract the shared destroy/recreate logic in TransactionProcessor into DestroyAccount, removing the duplicated EIP-8246 blocks. - Note the intentional EIP-2780 divergence on the SELFDESTRUCT new-account charge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 86e1d28 commit 7d7f936

4 files changed

Lines changed: 38 additions & 26 deletions

File tree

src/Nethermind/Nethermind.Evm.Test/Eip8037Tests.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,20 @@ public void ConsumeStateGas_oog_does_not_zero_reservoir()
215215

216216
Assert.That((consumed, gas.Value, gas.StateReservoir, gas.StateGasUsed, gas.StateGasSpill),
217217
Is.EqualTo((false, 10L, 50L, 0L, 0L)));
218+
Assert.That(EthereumGasPolicy.IsOutOfGas(in gas), Is.True);
219+
}
220+
221+
[Test]
222+
public void Removing_advanced_refund_from_net_spill_reservoir_deducts_usage_only()
223+
{
224+
// A net-spill (negative) reservoir, as left by RestoreChildStateGas after nested spills.
225+
EthereumGasPolicy gas = new() { Value = 400, StateReservoir = -300, StateGasUsed = 800 };
226+
227+
// Revoking an advanced refund must not reclaim spilled gas via the negative reservoir;
228+
// it comes out of the recorded usage instead.
229+
EthereumGasPolicy.RemoveStateGasRefundFromReservoir(ref gas, 200);
230+
231+
Assert.That((gas.StateReservoir, gas.StateGasUsed), Is.EqualTo((-300L, 600L)));
218232
}
219233

220234
[Test]

src/Nethermind/Nethermind.Evm/GasPolicy/EthereumGasPolicy.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ public static bool ConsumeStateGas(ref EthereumGasPolicy gas, long stateGasCost)
137137
ulong spillAmount = CalculateStateGasSpill(in gas, stateGasCost);
138138
if (!TryConsume(ref gas, spillAmount))
139139
{
140+
gas.OutOfGas = true;
140141
return false;
141142
}
142143

@@ -398,7 +399,9 @@ public static void AddStateGasRefundToReservoir(ref EthereumGasPolicy gas, long
398399
[MethodImpl(MethodImplOptions.AggressiveInlining)]
399400
public static void RemoveStateGasRefundFromReservoir(ref EthereumGasPolicy gas, long amount)
400401
{
401-
long fromReservoir = Math.Min(amount, gas.StateReservoir);
402+
// A net-spill reservoir is negative: the advanced refund was already consumed, so the
403+
// whole removal comes out of the recorded usage and the spill accounting stays intact.
404+
long fromReservoir = Math.Max(0, Math.Min(amount, gas.StateReservoir));
402405
gas.StateReservoir -= fromReservoir;
403406
amount -= fromReservoir;
404407

@@ -547,7 +550,7 @@ public static EthereumGasPolicy CreateAvailableFromIntrinsic(ulong gasLimit, in
547550
if (spec.IsEip8037Enabled)
548551
{
549552
// EIP-8037: cap gas_left at TX_MAX_GAS_LIMIT - intrinsic_regular, overflow goes to reservoir
550-
ulong maxGasLeft = Eip7825Constants.DefaultTxGasLimitCap - intrinsicGas.Value;
553+
ulong maxGasLeft = Eip7825Constants.DefaultTxGasLimitCap.SaturatingSub(intrinsicGas.Value);
551554
reservoir = executionGas.SaturatingSub(maxGasLeft);
552555
executionGas -= reservoir;
553556
}

src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.ControlFlow.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ public static EvmExceptionType InstructionSelfDestruct<TGasPolicy, TEip8037, TEi
239239
if (vm.TxTracer.IsTracingActions)
240240
vm.TxTracer.ReportSelfDestruct(executingAccount, result, inheritor);
241241

242-
// Charge gas if transferring to a dead or non-existent account.
242+
// Charge gas if transferring to a dead or non-existent account (intentionally not repriced by EIP-2780).
243243
bool inheritorAccountExists = state.AccountExists(inheritor);
244244
bool chargesNewAccount = spec.ClearEmptyAccountWhenTouched switch
245245
{

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

Lines changed: 18 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -380,22 +380,29 @@ static void FinalizeDestroyedAccount(IWorldState worldState, in TransactionSubst
380380
substate.Logs.Add(TransferLog.CreateBurn(toBeDestroyed, balance));
381381
}
382382

383-
if (commit) worldState.MarkStorageDestroyed(toBeDestroyed);
384-
else worldState.ClearStorage(toBeDestroyed);
385-
worldState.DeleteAccount(toBeDestroyed);
386-
387-
// EIP-8246: preserve any remaining balance as a fresh nonce-0,
388-
// code-less account; an empty account stays deleted via EIP-161.
389-
if (removeSelfdestructBurn && !balance.IsZero)
390-
{
391-
worldState.CreateAccount(toBeDestroyed, balance);
392-
}
383+
DestroyAccount(worldState, toBeDestroyed, in balance, commit, removeSelfdestructBurn);
393384
}
394385
}
395386

396387
return FinalizeTransaction(tx, spec, tracer, opts, restore, commit, deleteCallerAccount, in senderReservedGasPayment, env.ExecutingAccount, in substate, spentGas, statusCode);
397388
}
398389

390+
private static void DestroyAccount(IWorldState worldState, Address toBeDestroyed, in UInt256 balance, bool commit, bool removeSelfdestructBurn)
391+
{
392+
// Build-up rounds (!commit) span the whole block: later txs may redeploy this address,
393+
// so the order-preserving journaled clear is required; the O(1) mark needs a commit after.
394+
if (commit) worldState.MarkStorageDestroyed(toBeDestroyed);
395+
else worldState.ClearStorage(toBeDestroyed);
396+
worldState.DeleteAccount(toBeDestroyed);
397+
398+
// EIP-8246: preserve any remaining balance as a fresh nonce-0, code-less account;
399+
// an empty account stays deleted via EIP-161.
400+
if (removeSelfdestructBurn && !balance.IsZero)
401+
{
402+
worldState.CreateAccount(toBeDestroyed, balance);
403+
}
404+
}
405+
399406
[SkipLocalsInit]
400407
[MethodImpl(MethodImplOptions.NoInlining)]
401408
private TransactionResult ExecuteSimpleTransfer(
@@ -1386,19 +1393,7 @@ private int ExecuteEvmCall<TTracingInst>(
13861393
substate.Logs.Add(TransferLog.CreateSelfDestruct(toBeDestroyed, balance));
13871394
}
13881395

1389-
// Build-up rounds (!commit) span the whole block: later txs may
1390-
// redeploy this address, so the order-preserving journaled clear
1391-
// is required; the O(1) mark is only valid when a commit follows.
1392-
if (commit) WorldState.MarkStorageDestroyed(toBeDestroyed);
1393-
else WorldState.ClearStorage(toBeDestroyed);
1394-
WorldState.DeleteAccount(toBeDestroyed);
1395-
1396-
// EIP-8246: preserve any remaining balance as a fresh nonce-0,
1397-
// code-less account; an empty account stays deleted via EIP-161.
1398-
if (removeSelfdestructBurn && !balance.IsZero)
1399-
{
1400-
WorldState.CreateAccount(toBeDestroyed, balance);
1401-
}
1396+
DestroyAccount(WorldState, toBeDestroyed, in balance, commit, removeSelfdestructBurn);
14021397

14031398
if (tracingRefunds)
14041399
{

0 commit comments

Comments
 (0)