Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip sstore gas metering read for system tx #7958

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Skip sstore gas metering read for system tx #7958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93bde24
Skip sstore read for system tx
benaadams Dec 22, 2024
ce5e8a5
Override spec differently
benaadams Dec 22, 2024
aaef539
Faster add for UInt256
benaadams Dec 22, 2024
736d9e6
Merge branch 'master' into skip-sstore-read-for-system-tx
benaadams Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Nethermind/Nethermind.Blockchain.Test/BeaconBlockRootHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void Test_BeaconRootsAccessList_canInsertBeaconRootIsTrue_AccountExists()

Assert.That(accessList, Is.Not.Null);
Assert.That(accessList.Count.AddressesCount, Is.EqualTo(1));
Assert.That(accessList.Count.StorageKeysCount, Is.EqualTo(1));
Assert.That(accessList.Count.StorageKeysCount, Is.EqualTo(2));
}

[Test]
Expand Down
12 changes: 10 additions & 2 deletions src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ public class BeaconBlockRootHandler(ITransactionProcessor processor, IWorldState

public (Address? toAddress, AccessList? accessList) BeaconRootsAccessList(Block block, IReleaseSpec spec, bool includeStorageCells = true)
{
const int HistoryBufferLength = 8191;

BlockHeader? header = block.Header;
bool canInsertBeaconRoot = spec.IsBeaconBlockRootAvailable
&& !header.IsGenesis
Expand All @@ -39,7 +41,14 @@ public class BeaconBlockRootHandler(ITransactionProcessor processor, IWorldState

if (includeStorageCells)
{
builder.AddStorage(block.Timestamp % 8191);
// https://eips.ethereum.org/EIPS/eip-4788
// Set the storage value at header.timestamp % HISTORY_BUFFER_LENGTH to be header.timestamp
ulong slotIndex = header.Timestamp % HistoryBufferLength;
UInt256 slot256 = slotIndex;
builder.AddStorage(in slot256);
// Set the storage value at header.timestamp % HISTORY_BUFFER_LENGTH + HISTORY_BUFFER_LENGTH to be calldata[0:32]
slot256 = slotIndex + HistoryBufferLength;
builder.AddStorage(in slot256);
Comment on lines +44 to +51
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is correct, but I think Geth adds only 1 of them to access list - can you check?
Having different access list could potentially break on edge case of out of gas - generally shouldn't happen with 30mln gas...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec doesn't mention Access Lists; no gas is charged 30M should be enough for x2 SSTOREs

Spec is actually more wild and says

Clients may decide to omit an explicit EVM call and directly set the storage values. Note: While this is a valid optimization for Ethereum mainnet, it could be problematic on non-mainnet situations in case a different contract is used.

Copy link
Member

@LukaszRozmej LukaszRozmej Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarekM25 WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm I think some chains use a different implementation of this contract, so not sure if we should add anything besides just calling sys call

}

return (eip4788ContractAddress, builder.Build());
Expand All @@ -64,7 +73,6 @@ public void StoreBeaconRoot(Block block, IReleaseSpec spec, ITxTracer tracer)
};

transaction.Hash = transaction.CalculateHash();

processor.Execute(transaction, header, tracer);
}
}
Expand Down
66 changes: 37 additions & 29 deletions src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using Nethermind.Consensus.Withdrawals;
using Nethermind.Core;
using Nethermind.Core.Crypto;
using Nethermind.Core.Extensions;
using Nethermind.Core.Specs;
using Nethermind.Core.Threading;
using Nethermind.Crypto;
Expand Down Expand Up @@ -107,44 +108,51 @@ the previous head state.*/
preWarmTask = null;
WaitForCacheClear();
Block suggestedBlock = suggestedBlocks[i];
if (blocksCount > 64 && i % 8 == 0)
{
if (_logger.IsInfo) _logger.Info($"Processing part of a long blocks branch {i}/{blocksCount}. Block: {suggestedBlock}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log was helpful in determining that something went seriously wrong in the past :D

}

if (notReadOnly)
{
BlockProcessing?.Invoke(this, new BlockEventArgs(suggestedBlock));
}

CancellationTokenSource? cancellationTokenSource = null;
Block processedBlock;
TxReceipt[] receipts;

bool skipPrewarming = preWarmer is null || suggestedBlock.Transactions.Length < 3;
if (!skipPrewarming)
try
{
using CancellationTokenSource cancellationTokenSource = new();
preWarmTask = preWarmer.PreWarmCaches(suggestedBlock, preBlockStateRoot, _specProvider.GetSpec(suggestedBlock.Header), cancellationTokenSource.Token, _beaconBlockRootHandler);
(processedBlock, receipts) = ProcessOne(suggestedBlock, options, blockTracer);
// Block is processed, we can cancel the prewarm task
cancellationTokenSource.Cancel();
}
else
{
CacheType result = preWarmer?.ClearCaches() ?? default;
if (result != default)
bool skipPrewarming = preWarmer is null || suggestedBlock.Transactions.Length < 3;
if (!skipPrewarming)
{
cancellationTokenSource = new();
preWarmTask = preWarmer.PreWarmCaches(suggestedBlock, preBlockStateRoot, _specProvider.GetSpec(suggestedBlock.Header), cancellationTokenSource.Token, _beaconBlockRootHandler);
}
else
{
if (_logger.IsWarn) _logger.Warn($"Low txs, caches {result} are not empty. Clearing them.");
// Even though we skip prewarming we still need to ensure the caches are cleared
CacheType result = preWarmer?.ClearCaches() ?? default;
if (result != default)
{
if (_logger.IsWarn) _logger.Warn($"Low txs, caches {result} are not empty. Clearing them.");
}
}
// Even though we skip prewarming we still need to ensure the caches are cleared

if (blocksCount > 64 && i % 8 == 0)
{
if (_logger.IsInfo) _logger.Info($"Processing part of a long blocks branch {i}/{blocksCount}. Block: {suggestedBlock}");
}

if (notReadOnly)
{
BlockProcessing?.Invoke(this, new BlockEventArgs(suggestedBlock));
}

(processedBlock, receipts) = ProcessOne(suggestedBlock, options, blockTracer);
}

processedBlocks[i] = processedBlock;
processedBlocks[i] = processedBlock;

// be cautious here as AuRa depends on processing
PreCommitBlock(newBranchStateRoot, suggestedBlock.Number);
QueueClearCaches(preWarmTask);
// be cautious here as AuRa depends on processing
PreCommitBlock(newBranchStateRoot, suggestedBlock.Number);
}
finally
{
// Block is processed, we can cancel the prewarm task
CancellationTokenExtensions.CancelDisposeAndClear(ref cancellationTokenSource);
QueueClearCaches(preWarmTask);
}

if (notReadOnly)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Nethermind/Nethermind.Core/Specs/IReleaseSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,5 +419,10 @@ public interface IReleaseSpec : IEip1559Spec, IReceiptSpec
bool IsAuthorizationListEnabled => IsEip7702Enabled;

public bool RequestsEnabled => ConsolidationRequestsEnabled || WithdrawalRequestsEnabled || DepositsEnabled;

/// <summary>
/// Skip read in SSTORE for calculating gas cost as not charged
/// </summary>
bool IsSystemTransaction { get; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks weird, this is added to ReleaseSpec and the field seems to be referenced to Transaction object

}
}
2 changes: 2 additions & 0 deletions src/Nethermind/Nethermind.Core/Specs/ReleaseSpecDecorator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,6 @@ public class ReleaseSpecDecorator(IReleaseSpec spec) : IReleaseSpec
public virtual Address? FeeCollector => spec.FeeCollector;
public virtual UInt256? Eip1559BaseFeeMinValue => spec.Eip1559BaseFeeMinValue;
public virtual bool ValidateReceipts => spec.ValidateReceipts;

public virtual bool IsSystemTransaction { get => spec.IsSystemTransaction; }
}
150 changes: 80 additions & 70 deletions src/Nethermind/Nethermind.Evm/VirtualMachine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2567,100 +2567,110 @@ private EvmExceptionType InstructionSStore<TTracingInstructions, TTracingRefunds
bytes = !newIsZero ? bytes.WithoutLeadingZeros() : BytesZero;

StorageCell storageCell = new(vmState.Env.ExecutingAccount, result);
bool newSameAsCurrent;
scoped ReadOnlySpan<byte> currentValue = default;
if (spec.IsSystemTransaction && typeof(TTracingStorage) != typeof(IsTracing))
{
// Skip reads in SSTORE for calculating gas cost for system transaction
// as we don't actually charge gas
newSameAsCurrent = false;
}
else
{
if (!ChargeStorageAccessGas(
ref gasAvailable,
vmState,
in storageCell,
StorageAccessType.SSTORE,
spec)) return EvmExceptionType.OutOfGas;

if (!ChargeStorageAccessGas(
ref gasAvailable,
vmState,
in storageCell,
StorageAccessType.SSTORE,
spec)) return EvmExceptionType.OutOfGas;

ReadOnlySpan<byte> currentValue = _state.Get(in storageCell);
// Console.WriteLine($"current: {currentValue.ToHexString()} newValue {newValue.ToHexString()}");
bool currentIsZero = currentValue.IsZero();
currentValue = _state.Get(in storageCell);
// Console.WriteLine($"current: {currentValue.ToHexString()} newValue {newValue.ToHexString()}");
bool currentIsZero = currentValue.IsZero();

bool newSameAsCurrent = (newIsZero && currentIsZero) || Bytes.AreEqual(currentValue, bytes);
long sClearRefunds = RefundOf.SClear(spec.IsEip3529Enabled);
newSameAsCurrent = (newIsZero && currentIsZero) || Bytes.AreEqual(currentValue, bytes);
long sClearRefunds = RefundOf.SClear(spec.IsEip3529Enabled);

if (!spec.UseNetGasMetering) // note that for this case we already deducted 5000
{
if (newIsZero)
if (!spec.UseNetGasMetering) // note that for this case we already deducted 5000
{
if (!newSameAsCurrent)
if (newIsZero)
{
vmState.Refund += sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(sClearRefunds);
if (!newSameAsCurrent)
{
vmState.Refund += sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(sClearRefunds);
}
}
else if (currentIsZero)
{
if (!UpdateGas(GasCostOf.SSet - GasCostOf.SReset, ref gasAvailable)) return EvmExceptionType.OutOfGas;
}
}
else if (currentIsZero)
{
if (!UpdateGas(GasCostOf.SSet - GasCostOf.SReset, ref gasAvailable)) return EvmExceptionType.OutOfGas;
}
}
else // net metered
{
if (newSameAsCurrent)
{
if (!UpdateGas(spec.GetNetMeteredSStoreCost(), ref gasAvailable)) return EvmExceptionType.OutOfGas;
}
else // net metered, C != N
else // net metered
{
Span<byte> originalValue = _state.GetOriginal(in storageCell);
bool originalIsZero = originalValue.IsZero();

bool currentSameAsOriginal = Bytes.AreEqual(originalValue, currentValue);
if (currentSameAsOriginal)
if (newSameAsCurrent)
{
if (currentIsZero)
{
if (!UpdateGas(GasCostOf.SSet, ref gasAvailable)) return EvmExceptionType.OutOfGas;
}
else // net metered, current == original != new, !currentIsZero
{
if (!UpdateGas(spec.GetSStoreResetCost(), ref gasAvailable)) return EvmExceptionType.OutOfGas;

if (newIsZero)
{
vmState.Refund += sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(sClearRefunds);
}
}
if (!UpdateGas(spec.GetNetMeteredSStoreCost(), ref gasAvailable)) return EvmExceptionType.OutOfGas;
}
else // net metered, new != current != original
else // net metered, C != N
{
long netMeteredStoreCost = spec.GetNetMeteredSStoreCost();
if (!UpdateGas(netMeteredStoreCost, ref gasAvailable)) return EvmExceptionType.OutOfGas;
Span<byte> originalValue = _state.GetOriginal(in storageCell);
bool originalIsZero = originalValue.IsZero();

if (!originalIsZero) // net metered, new != current != original != 0
bool currentSameAsOriginal = Bytes.AreEqual(originalValue, currentValue);
if (currentSameAsOriginal)
{
if (currentIsZero)
{
vmState.Refund -= sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(-sClearRefunds);
if (!UpdateGas(GasCostOf.SSet, ref gasAvailable)) return EvmExceptionType.OutOfGas;
}

if (newIsZero)
else // net metered, current == original != new, !currentIsZero
{
vmState.Refund += sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(sClearRefunds);
if (!UpdateGas(spec.GetSStoreResetCost(), ref gasAvailable)) return EvmExceptionType.OutOfGas;

if (newIsZero)
{
vmState.Refund += sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(sClearRefunds);
}
}
}

bool newSameAsOriginal = Bytes.AreEqual(originalValue, bytes);
if (newSameAsOriginal)
else // net metered, new != current != original
{
long refundFromReversal;
if (originalIsZero)
long netMeteredStoreCost = spec.GetNetMeteredSStoreCost();
if (!UpdateGas(netMeteredStoreCost, ref gasAvailable)) return EvmExceptionType.OutOfGas;

if (!originalIsZero) // net metered, new != current != original != 0
{
refundFromReversal = spec.GetSetReversalRefund();
if (currentIsZero)
{
vmState.Refund -= sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(-sClearRefunds);
}

if (newIsZero)
{
vmState.Refund += sClearRefunds;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(sClearRefunds);
}
}
else

bool newSameAsOriginal = Bytes.AreEqual(originalValue, bytes);
if (newSameAsOriginal)
{
refundFromReversal = spec.GetClearReversalRefund();
}
long refundFromReversal;
if (originalIsZero)
{
refundFromReversal = spec.GetSetReversalRefund();
}
else
{
refundFromReversal = spec.GetClearReversalRefund();
}

vmState.Refund += refundFromReversal;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(refundFromReversal);
vmState.Refund += refundFromReversal;
if (typeof(TTracingRefunds) == typeof(IsTracing)) _txTracer.ReportRefund(refundFromReversal);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Nethermind/Nethermind.Specs.Test/OverridableReleaseSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,5 +179,6 @@ public ulong Eip4844TransitionTimestamp
public IBaseFeeCalculator BaseFeeCalculator => _spec.BaseFeeCalculator;
public bool IsEip6110Enabled => _spec.IsEip6110Enabled;
public Address DepositContractAddress => _spec.DepositContractAddress;
public bool IsSystemTransaction => _spec.IsSystemTransaction;
}
}
2 changes: 2 additions & 0 deletions src/Nethermind/Nethermind.Specs/ReleaseSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,5 +133,7 @@ public Address Eip2935ContractAddress
get => IsEip2935Enabled ? _eip2935ContractAddress : null;
set => _eip2935ContractAddress = value;
}

public bool IsSystemTransaction => false;
}
}
1 change: 1 addition & 0 deletions src/Nethermind/Nethermind.Specs/SystemTransactionReleaseSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ namespace Nethermind.Specs
public class SystemTransactionReleaseSpec(IReleaseSpec spec, bool isAura, bool isGenesis) : ReleaseSpecDecorator(spec)
{
public override bool IsEip158Enabled => !isAura && isGenesis && base.IsEip158Enabled;
public override bool IsSystemTransaction => true;
}
}
Loading