refactor(service): centralize per-round DKG mutation locking - #95
Open
0xHansLee wants to merge 2 commits into
Open
refactor(service): centralize per-round DKG mutation locking#950xHansLee wants to merge 2 commits into
0xHansLee wants to merge 2 commits into
Conversation
Route every DKG-mutating RPC handler through a single withRoundMutation helper instead of scattered getDKGMutationMu + manual Lock/Unlock and ad-hoc closures. The helper defers Unlock so the lock cannot leak on an early return or panic. No behavior change.
…tation lock (#94) Resolving the generator outside getDKGMutationMu(round) let a timed-out handler and its retry share the same cached pointer; the winner could evict it on a persist failure, leaving the loser on a stale, evicted instance. Process and persist now run under the lock as one critical section. The generator is warmed OUTSIDE the lock so a cache-miss build -- for resharing an unbounded light-client query -- never runs while the lock is held; under the lock it is taken via a pure cache-get, re-warming outside the lock and retrying if the round was evicted concurrently (bounded). The loser re-resolves a fresh generator.
0xHansLee
requested review from
barry-peng-story,
jdubpark,
lucas2brh,
stevemilk,
wo-o and
yingyangxu2026
as code owners
July 24, 2026 05:56
|
The refactor correctly centralises per-round locking via Two narrower observations below; neither is a showstopper but both are worth a look before merge. Review iteration 1 · Commit 1bbf65b · 2026-07-24T05:57:34Z |
stevemilk
approved these changes
Jul 24, 2026
yingyangxu2026
approved these changes
Jul 27, 2026
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
3 participants
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.
Problem
getDKGMutationMu(round)(M) was taken at scattered sites with manualLock/Unlockand ad-hoc closures across the six DKG-mutating critical sections — easy to misuse or forget to release. Separately,ProcessDealsresolved the cached*dkg.DistKeyGeneratorbefore taking M, so a timed-out handler and its retry could capture the same pointer; when the winner evicted it on a persist failure (#87), the loser processed a stale, evicted instance.Fix
withRoundMutation(round, fn)and route all six mutating critical sections (ProcessDeals/ProcessResponses/ProcessJustification/FinalizeDKG/GenerateDealsand the resharing from-round share read) through it, so Unlock cannot leak on an early return or panic. M stays a leaf lock.ProcessDealswarms the generator OUTSIDE M (a cache-miss build does light-client IO), then under M takes it via a pure cache-get and processes; on an eviction between warm and lock it re-warms outside M and retries, bounded bymaxResolveDealsAttempts, so the loser resolves a fresh generator. ReturnsUnavailableif exhausted.Test coverage
go test -race ./service/...withRoundMutationapplyDealsUnderLockresolveAndApplyDeals,attemptDealswarmGeneratorissue: #94