SLOAD Improvements

[svlachakis]

Changes

Per-contract storage memo

(PersistentStorageProvider).** Consecutive SLOADs overwhelmingly hit the same contract; a one-entry memo (_lastStorageAddress → _lastStorage) removes the _storages dictionary lookup from the per-SLOAD path. Invalidated wherever _storages is cleared, so the pooled per-contract state is never reachable after return.

Skip cell hashing on read-mostly executions

** The intra-block cache only holds journaled writes. On read-mostly executions (e.g. eth_call) it is empty, so a _intraBlockCache.Count != 0 guard skips hashing the 52-byte storage cell on every SLOAD before the lookup that would miss anyway.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

[claude]

Claude finished @svlachakis's task in 4m 34s —— View job

---

Review

• Gather context and read changed files
• Review PersistentStorageProvider.cs — per-contract memo correctness
• Review PartialStorageProviderBase.cs — intra-block cache guard correctness
• Check edge cases (reset, rollback, re-entry, multi-contract)
• Check adherence to repo rules and coding style
• Post findings

---

Summary

Both optimizations are logically correct — no consensus bugs, no data hazards. One misleading comment and a few low-severity observations follow.

---

Findings

Medium

TryGetCachedValue comment is factually wrong (PartialStorageProviderBase.cs:178–181)

The comment states _intraBlockCache "only holds journaled WRITES", but that is false. SetupRegistry is called from both PushUpdate (writes, ChangeType.Update) and PushToRegistryOnly (reads, ChangeType.JustCache). Every SLOAD that misses the cache goes to LoadFromTree → PushToRegistryOnly, which adds a JustCache entry. After the very first SLOAD, Count == 1.

Consequently the guard fires at most once per transaction (only before the very first storage access), not broadly across "read-mostly" executions as the PR description implies. The guard itself is semantically correct (empty cache ⇒ TryGetValue would miss anyway), but the stated reasoning is wrong and risks misleading future maintainers. See inline comment with a suggested fix.

---

Low

Memo invalidation order in Reset — safe but worth noting

_storages.ResetAndClear();   // PerContractState.Return() is called here → object is pooled
_lastStorageAddress = null;  // memo nulled after pool return
_lastStorage = null;

_lastStorage briefly references an object that has already been returned to the pool (after ResetAndClear but before the next two lines). Because this code is single-threaded, no other code can observe that window, so there is no actual hazard. The order is fine; just worth being aware of if threading assumptions ever change.

---

_lastStorageAddress == address equality — correct, confirmed

Address.operator== delegates to Equals, which compares the 20-byte content. The memo comparison is value-based, not reference-based. No issue.

---

No regression tests

The PR opts out of testing for changes on the consensus-critical SLOAD hot path. At minimum, a test exercising GetCurrentValue with an initially-empty cache, a partially-filled cache, and after Reset would give confidence these shortcuts don't regress. The CommitCore path resets _intraBlockCache between transactions, so the Count == 0 branch is reachable on every transaction start — a deterministic path worth covering.

---

WarmUp brace change is pure noise — no behaviour change, could be omitted from the diff.

---

Verdict

The per-contract memo (_lastStorageAddress/_lastStorage) is correctly implemented: Address.== is value-based, the memo is cleared in every codepath that clears _storages (Reset, ClearStorageMap), and re-entry/rollback safety is preserved by the _changes/_intraBlockCache journal. The Count != 0 guard is semantically sound. The medium finding is a misleading comment — please correct it before merge.

[svlachakis]

@claude I fixed the MEDIUM COMMENT problem..lol review again

[claude]

Claude finished @svlachakis's task in 1m 27s —— View job

---

Review

• Gather context
• Read updated PartialStorageProviderBase.cs
• Read updated PersistentStorageProvider.cs
• Re-review findings
• Post updated verdict

---

Summary

The medium finding from the previous review is fixed. Both optimizations are correct and clean.

---

Findings

Medium — ✅ Resolved

TryGetCachedValue comment (PartialStorageProviderBase.cs:178–179)

The comment was updated to:

// If the cache is completely empty (no writes or reads yet this transaction),
// skip hashing the 52-byte cell — TryGetValue would miss anyway.

This is accurate. The old comment falsely claimed the cache "only holds journaled WRITES"; the new one correctly says "no writes or reads yet this transaction", which matches the actual semantics (both JustCache reads and Update writes populate the cache).

---

Low — unchanged from prior review

• No regression tests — no test covers the Count == 0 early-exit, the memo hit path, or behaviour after Reset. Low risk given the simplicity, but worth a follow-up.
• WarmUp brace change — cosmetic noise, no behaviour change.

---

Verdict

All medium findings are resolved. The per-contract storage memo is correctly invalidated in both Reset (when resetBlockChanges is true) and ClearStorageMap. The Count != 0 guard is semantically correct and the comment now accurately describes its scope. This is ready to merge.

[claude]

Claude finished @svlachakis's task in 2m 30s —— View job

---

Review

• Read changed files and diff
• Read relevant rules (performance, coding-style)
• Review correctness, edge cases, performance
• Post findings

---

Summary

Both optimizations are correct. The previous Medium finding (incorrect comment) was resolved. No new issues found. Detailed walkthrough below.

---

Per-contract memo (PersistentStorageProvider.cs)

Correctness ✅

Address.operator== is value-based (20-byte content comparison), so the memo comparison _lastStorageAddress == address is semantically equivalent to a value lookup — it won't spuriously hit or miss due to object identity.

Invalidation coverage ✅

Every path that discards _storages entries now also nulls the memo:

Path	_storages cleared	Memo cleared
Reset(resetBlockChanges: true)	ResetAndClear()	✅
ClearStorageMap()	Clear()	✅

After CommitCore (intra-block transaction boundary), _storages is intentionally preserved (block-scoped state) and the memo correctly keeps pointing to the last-used PerContractState, which is still live in the dict. ✅

Null safety of _lastStorage! ✅

When _lastStorageAddress == address is true and address is non-null, _lastStorageAddress was set in a prior GetOrCreateStorage call that also set _lastStorage to the non-null dict value. The ! forgiving operator is justified.

Memo update order in Reset — safe

_storages.ResetAndClear();   // Return() called → object pooled
_lastStorageAddress = null;  // nulled after pool return
_lastStorage = null;

There is a three-instruction window where _lastStorage references a pooled object. PersistentStorageProvider is single-threaded (one instance per execution context), so this window cannot be observed. No hazard.

---

Count != 0 guard (PartialStorageProviderBase.cs)

Correctness ✅

Dictionary<K,V>.Count is an O(1) field read. Skipping TryGetValue when Count == 0 is semantically equivalent to running it — it would return false. No data hazard.

Actual scope of the optimization

The comment is now accurate. Because PushToRegistryOnly adds a JustCache entry on every LoadFromTree call, Count becomes 1 after the first storage access. The guard skips the 52-byte hash once per transaction (before the first access), which is meaningful given the per-transaction Reset in CommitCore → base.CommitCore → Reset().

---

Low findings

No regression tests — the Count == 0 early-exit and the memo hit/miss paths are untested. Given the simplicity and the determinism of the Count == 0 path (fires at the start of every transaction after the base Reset), a simple parametric test would close this gap. Not blocking, but worth a follow-up.

WarmUp brace change — adds curly braces around a single-statement if. Style-conforming but cosmetic noise in the diff.

---

Verdict

No Critical, High, or Medium findings. The memo is correctly invalidated in all _storages-clearing paths, the equality check is value-based, and the Count != 0 guard comment is accurate. Ready to merge.