db/state: give CommitmentDomain its own lock in TemporalMemBatch

[sudeepdino008]

TemporalMemBatch (the in-RAM latest-state layer behind sd.mem) guarded every domain under a single latestStateLock. The parallel commitment calculator writes CommitmentDomain branches during the fold — on its own goroutine — so those writes contend that one lock with the exec workers' state reads (getLatest on Accounts/Storage/Code).

This gives CommitmentDomain its own lock:

• state reads/writes keep latestStateLock,
• commitment reads/writes use commitmentLock,
• multi-domain ops (Flush/Unwind) take both, in a fixed order (state before commitment) so there is no deadlock.

The domains are already stored in separate maps (domains[kv.DomainLen]), so this only splits the lock — no data restructuring.

Numbers (matched-window)

Mutex "delay" profile over an identical 300-block window on each binary (live chaintip, mainnet, on top of #23134):

mutex contention / 300 blocks	#23134 base	+ this PR
total	3.71s	2.87s (−23%)
sd.mem getLatestMetered (state reads)	1.60s	1.35s (−16%)

A measured, low-risk reduction in sd.mem lock contention by separating the commitment writer off the state lock. Single matched sample, so treat the exact percentage as approximate.

Scope, stated honestly: this reduces lock contention, not read wall-time — the sd.mem state read is dominated by the domain btree traversal, not the lock, so end-to-end gas/s is unchanged (tip throughput is gated by a separate dispatch bottleneck). It's lock hygiene that also helps as core count / pipeline depth grows (the contention is larger in batch/initial-sync).

Tests

-race coverage added for concurrent commitment-write vs state-read plus the both-lock Unwind path (deadlock-freedom). The db/state and execution/stagedsync suites pass under -race.

[Copilot]

Pull request overview

This PR reduces contention in the in-RAM “latest state” layers used by parallel execution by (1) splitting TemporalMemBatch locking so CommitmentDomain no longer contends with hot state-domain reads/writes, and (2) switching BlockStateCache’s committed storage cache to a lock-free sync.Map keyed by (addr, slot).

Changes:

• db/state: introduce a dedicated commitmentLock for kv.CommitmentDomain, and make multi-domain operations (Flush/Unwind) take both locks in a fixed order.
• execution/state: change BlockStateCache.committedStorage to sync.Map with a composite key, and adjust read paths to avoid mu on committed fallback.
• Add concurrency/race-focused tests for the new locking behavior and committed-storage cache semantics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
db/state/temporal_mem_batch.go	Splits locking: CommitmentDomain uses commitmentLock; Flush/Unwind take both locks in order.
db/state/temporal_mem_batch_test.go	Adds -race-oriented concurrency test covering commitment writes vs state reads and both-lock Unwind.
execution/state/rw_v3.go	Converts committed storage cache to sync.Map keyed by (addr, key) and adjusts committed fallback ordering.
execution/state/block_cache_committed_storage_test.go	Adds benchmarks and tests for committed-storage semantics and concurrent access.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

db/state/temporal_mem_batch_test.go:66

• This workload also passes if every domain uses the former single mutex, so it does not guard the PR's core lock-separation behavior. Add a deterministic assertion that the commitment and state domains resolve to different lock instances; the concurrent loop can continue covering races and deadlocks.

	const nKeys = 512

[AskAlexSharov]

Review notes. Grouped by what I think blocks vs. what is cleanup.

The split may not buy what it claims

False sharing between the two locks. Measured on this branch: unsafe.Offsetof(latestStateLock)=20, unsafe.Offsetof(commitmentLock)=44, sizeof(sync.RWMutex)=24. TemporalMemBatch is ~512B, so it lands 64B-aligned. Bytes 20..63 are one cache line and hold the hot words of both mutexes: latestStateLock.w (20..27) and its readerCount (36..39), plus commitmentLock.w (44..51) and its readerCount (60..63). Every commitmentLock.Lock() on the calculator goroutine does an atomic RMW that invalidates the exact line each exec worker atomically increments in latestStateLock.RLock(). A delay mutex profile cannot see this — it measures off-CPU wait, not coherence stalls — which fits the "end-to-end gas/s is unchanged" result. Pad between the two fields, or give commitmentLock its own 64B-aligned slot.

putLatest still serializes every domain on sd.metrics. It takes lockFor(domain) and then unconditionally runs updateMetrics, which does sd.metrics.Lock() — one DomainMetrics per SharedDomains. So putLatest(CommitmentDomain) on the calculator and putLatest(AccountsDomain) on a worker still block each other; the writer-vs-writer contention moved, it did not go away. And sd.metrics used to be uncontended because one lock already serialized every caller. The split makes it a newly contended mutex, now reachable under two different outer locks.

lockBoth takes the hot lock first. latestStateLock.Lock() then commitmentLock.Lock(). Go's RWMutex blocks new readers as soon as a writer queues, so from the moment Flush grabs latestStateLock every worker's GetLatest(Accounts/Storage/Code) stalls — including for the whole time Flush waits behind in-flight commitment writes. The single lock had no such straddling window. Taking the shorter-held, less contended commitmentLock first (and documenting that as the order) shrinks the state-read stall to the commitment critical section.

Locking correctness

IteratePrefix / HasPrefix can invert the lock order. Both hold lockFor(domain) across a caller-supplied callback. IteratePrefix(kv.CommitmentDomain, ...) is reachable through the kv.TemporalMemBatch interface, SharedDomains.IteratePrefix, SharedDomains.HasPrefix and temporalGetter.HasPrefix — all take an arbitrary Domain. A callback doing GetLatest(AccountsDomain) or HasPrefixInRAM(StorageDomain) then takes latestStateLock while holding commitmentLock; a concurrent Flush/Unwind in lockBoth holds latestStateLock and waits on commitmentLock → ABBA deadlock, hanging the flusher and every worker. Nothing passes CommitmentDomain today, so it is latent — but the rule lives only in a comment. Worth noting DomainDelPrefix already had to collect-then-delete to avoid re-entering under this lock, so the pattern is known-fragile here.

Merge touches every domain map under neither lock. It does maps.Copy into sd.domains[d] for all d including CommitmentDomain, other.storage.Scan → sd.storage.Set, and writes unwindToTxNum / unwindChangeset / unwindChangesetRaw. A concurrent GetLatest/Unwind on the receiver gets concurrent map read and map write — a fatal throw, not a detectable race. This predates the PR, but the PR is where the contract becomes explicit with a named helper, so the new comment now asserts a completeness that does not hold.

flushLocked docstring is stale. It still says the callback path can "run it inside latestStateLock without re-acquiring". Both callers now use lockBoth. A third caller that follows the comment takes only latestStateLock and iterates sd.domains[kv.CommitmentDomain] while the calculator writes it. The Flush docstring two lines below was updated; this one was missed.

The test does not test the split

TestSplitLock_ConcurrentCommitmentWriteVsStateRead cannot fail for the bug class it targets. Writers and readers both go through lockFor, so they agree by construction — the detector can only fire if a path forgot the lock outright. It exercises putLatest, GetLatest, Unwind and nothing else. Not covered: Flush, FlushWithCommitmentCallback (the other lockBoth callers), GetAsOf, IteratePrefix, HasPrefixInRAM, and Merge — the one genuinely unlocked multi-domain path. Concretely: revert Flush back to sd.latestStateLock.Lock() and this test still goes green.

Two more gaps:

• The struct literal leaves inMemHistoryReads false, but NewTemporalMemBatch sets it true. Under false, putLatest takes the default: branch (old[0] = valWithStep; ... = old[:1]) and the slice never grows. The production path — append(old, valWithStep), where the concurrent reader in getLatest indexes dataWithTxNums[len(dataWithTxNums)-1] against a header that may have just been reallocated — never runs concurrently at all. GetAsOf is unreachable too (it early-returns an error when the flag is false). Set the field, or share a constructor.
• Unwind is always called with a nil changeset, so unwindChangeset / unwindToTxNum are never non-nil while a reader is in flight.

Design

if domain == kv.CommitmentDomain special-cases one domain inside shared infrastructure. locks [kv.DomainLen]cachePadded[sync.RWMutex] with &sd.locks[domain] would:

• be branchless on the hottest read in exec — lockFor currently adds a compare, a branch and a pointer materialisation to every GetLatest;
• drop the Accounts-vs-Storage-vs-Code writer contention this PR leaves on the table;
• turn the deadlock rule into "ascending domain index", enforceable by a lockAll loop, instead of a two-name convention that a future third lock silently breaks;
• fix the false sharing by construction.

Smaller things

• FlushWithCommitmentCallback has no callers anywhere in the tree. execctx.CommitmentFlushCallback appears only at its declaration (db/state/execctx/domain_shared.go:53) and in this one method, and the method is not on the kv.TemporalMemBatch interface — the commitment-cache flush actually goes through kv.WithFlushCallback(kv.CommitmentDomain, ...) in SharedDomains.Commit. Every future locking change has to reason about a path that cannot execute. Suggest deleting the method and the type.
• The lockBoth comment lists call sites — (Flush, Unwind) — and is already incomplete in the same commit: FlushWithCommitmentCallback is a third caller. Per the repo comment policy, call-site lists do not belong in source. Drop the parenthetical; "order is fixed, state before commitment" is the part worth keeping.
• The lockFor docstring restates its two-line body. The part that is not in the code — callers must never take commitment-then-state — is already stated on the field and on lockBoth.