Skip sstore gas metering read for system tx #7958
Conversation
|
Chain size failure #7959 |
| // 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); |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Don't we use system transactions somewhere where correct gas metering is important? (like estimate gas or some other RPC)? |
| @@ -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}"); | |||
There was a problem hiding this comment.
This log was helpful in determining that something went seriously wrong in the past :D
| /// <summary> | ||
| /// Skip read in SSTORE for calculating gas cost as not charged | ||
| /// </summary> | ||
| bool IsSystemTransaction { get; } |
There was a problem hiding this comment.
Looks weird, this is added to ReleaseSpec and the field seems to be referenced to Transaction object
| // 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); |
There was a problem hiding this comment.
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
Changes
Before
After
Types of changes
What types of changes does your code introduce?
Testing
Requires testing