feat: upstream changes for ArbOS 60 multi-gas constraints#10765
Open
AnkushinDaniil wants to merge 12 commits intomasterfrom
Open
feat: upstream changes for ArbOS 60 multi-gas constraints#10765AnkushinDaniil wants to merge 12 commits intomasterfrom
AnkushinDaniil wants to merge 12 commits intomasterfrom
Conversation
a90e97a to
41f5be0
Compare
Contributor
EVM Opcode Benchmark DiffAggregated runs: base=1, pr=1 No significant regressions or improvements detected. |
Member
|
Can't you achieve similar with inheritance, or probably better - decorator, that on reset would recreate internal BlockTree from scratch? |
4d96f5f to
2acad04
Compare
src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.ControlFlow.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+71
to
+89
| if (!spec.UseHotAndColdStorage) return true; | ||
| if (isTracingAccess) | ||
| { | ||
| if (isTracingAccess) | ||
| { | ||
| // Ensure that tracing simulates access-list behavior. | ||
| accessTracker.WarmUp(address); | ||
| } | ||
| // Ensure that tracing simulates access-list behavior. | ||
| accessTracker.WarmUp(address); | ||
| } | ||
|
|
||
| // If the account is cold (and not a precompile), charge the cold access cost. | ||
| if (!spec.IsPrecompile(address) && accessTracker.WarmUp(address)) | ||
| { | ||
| result = UpdateGas(ref gas, GasCostOf.ColdAccountAccess); | ||
| } | ||
| else if (chargeForWarm) | ||
| bool result; | ||
| // If the account is cold (and not a precompile), charge the cold access cost. | ||
| if (!spec.IsPrecompile(address) && accessTracker.WarmUp(address)) | ||
| result = UpdateGas(ref gas, GasCostOf.ColdAccountAccess); | ||
| else | ||
| { | ||
| result = kind switch | ||
| { | ||
| // Otherwise, if warm access should be charged, apply the warm read cost. | ||
| result = UpdateGas(ref gas, GasCostOf.WarmStateRead); | ||
| } | ||
| AccountAccessKind.SelfDestructBeneficiary => true, // no warm charge | ||
| AccountAccessKind.Default => UpdateGas(ref gas, GasCostOf.WarmStateRead), | ||
| _ => throw new ArgumentOutOfRangeException(nameof(kind), kind, null) | ||
| }; |
Member
There was a problem hiding this comment.
we could do something like:
if (!spec.UseHotAndColdStorage) return true;
if (isTracingAccess) accessTracker.WarmUp(address);
long gasCost = !spec.IsPrecompile(address) && accessTracker.WarmUp(address)
? GasCostOf.ColdAccountAccess
: kind == AccountAccessKind.SelfDestructBeneficiary
? GasCostOf.Free
: GasCostOf.WarmStateRead;
return UpdateGas(ref gas, gasCost);
but not sure if calling UpdateGas with Free would change logic or not - maybe?
Member
There was a problem hiding this comment.
we could do:
if (!spec.UseHotAndColdStorage) return true;
if (isTracingAccess) accessTracker.WarmUp(address);
long gasCost = !spec.IsPrecompile(address) && accessTracker.WarmUp(address)
? GasCostOf.ColdAccountAccess
: kind == AccountAccessKind.SelfDestructBeneficiary
? GasCostOf.Free
: GasCostOf.WarmStateRead;
return gasCost == GasCostOf.Free || UpdateGas(ref gas, gasCost);
Member
There was a problem hiding this comment.
we could also have switch expression:
long gasCost = (!spec.IsPrecompile(address) && accessTracker.WarmUp(address)) switch
{
true => GasCostOf.ColdAccountAccess,
false when kind == AccountAccessKind.SelfDestructBeneficiary => GasCostOf.Free,
false => GasCostOf.WarmStateRead
};
Add IResettableBlockTree interface following the IClearableCache pattern from PR #10577. BlockTree implements it explicitly, allowing derived classes to reset internal state (Head, BestSuggestedHeader, etc.) for testing. Usage: (blockTree as IResettableBlockTree)?.ResetInternalState() This enables ArbitrumBlockTree to properly reset state during comparison testing without recreating the DI container.
Add BestSuggestedBeaconHeader, BestSuggestedBeaconBody, FinalizedHash, and SafeHash resets to IResettableBlockTree.ResetInternalState(). Also add thread-safety documentation to the interface and implementation clarifying that callers must ensure no concurrent operations.
Use ConsumeNewAccountCreation instead of UpdateGas for SELFDESTRUCT new account creation cost. This properly tracks the 25000 gas as StorageGrowth resource kind, matching Nitro's CreateBySelfdestructGas handling in operations_acl.go and gas_table.go.
…racking Add dedicated method for SELFDESTRUCT beneficiary account access gas charging. This enables Arbitrum to charge cold access as full StorageAccess (no Computation split) matching Nitro's makeSelfdestructGasFn behavior in operations_acl.go. - Add ConsumeSelfDestructBeneficiaryAccessGas to IGasPolicy interface - Add EthereumGasPolicy implementation (delegates to ConsumeAccountAccessGas) - Update EvmInstructions.ControlFlow.cs to use new method for SELFDESTRUCT
….Clear() public TrieStore.ClearForTesting() resets all internal state (dirty nodes, persisted hashes, commit queue, counters) for test isolation in comparison mode. CommitSetQueue.Clear() added to support this. CacheCodeInfoRepository.Clear() made public for external test access.
Add tuple type recognition in AbiType.GetTupleFromString so that ABI definitions with tuple parameters (e.g. MultiGas pricing constraints) can be parsed at runtime. Without this, ArbOwnerParser fails to register function selectors for tuple-based methods.
- TrieStore implements IClearableCache directly (eliminates external wrapper) - CacheCodeInfoRepository implements IClearableCache, revert Clear() to internal - Remove IResettableBlockTree interface (replaced by Arbitrum decorator)
Replace public static Clear() with explicit IClearableCache interface
implementation so cache clearing is only accessible through the DI-
discovered interface, not via cross-assembly visibility hacks.
Rejected: public static Clear() | exposes global cache destruction to all consumers
Rejected: InternalsVisibleTo("Nethermind.Arbitrum") | anti-pattern for production assemblies
Constraint: Benchmark assembly uses InternalsVisibleTo already, kept Clear() as internal static
Confidence: high
Scope-risk: narrow
…ssGas AccountAccessKind enum already encodes whether warm charge should be applied — SelfDestructBeneficiary skips warm charge, Default always charges. The chargeForWarm boolean was only ever false in the SELFDESTRUCT context, making it fully redundant with the kind discriminator. Constraint: chargeForWarm was only false at the SELFDESTRUCT call site Rejected: Keep both parameters for flexibility | no other caller needs chargeForWarm=false
Replace if/else with nested switch with a single switch expression on the cold/warm result, as suggested in code review.
cb8645c to
900b21a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
IResettableBlockTreeinterface for test state reset (followingIClearableCachepattern from feat: Add ClearCache() to store interfaces for testing support #10577)ConsumeSelfDestructBeneficiaryAccessGastoIGasPolicyfor multi-gas resource tracking — separate fromConsumeAccountAccessGasbecause the resource split differs: cold access charges fullColdAccountAccessto StorageAccess only (no Computation split), and warm access charges nothing (vs.WarmStateReadas Computation in regular access)ConsumeNewAccountCreationAbiType.TryParsefor dynamic decoding in precompilesTrieStore.ClearForTesting()and makeCacheCodeInfoRepository.Clear()publicIClearableCacheonCacheCodeInfoRepositoryvia nestedCacheClearServiceITestableTrieStoreinterface for test isolationTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Consumer PR: NethermindEth/nethermind-arbitrum#700 (ArbOS 60 multi-gas constraint pricing).
Pattern reference: #10577 (
IClearableCache).