Skip to content

fix(identity): do not hold the registry lock across WalletFactory.NewWallet - #2263

Draft
AkramBitar wants to merge 1 commit into
mainfrom
fix/2064-registry-lock-scope
Draft

fix(identity): do not hold the registry lock across WalletFactory.NewWallet#2263
AkramBitar wants to merge 1 commit into
mainfrom
fix/2064-registry-lock-scope

Conversation

@AkramBitar

Copy link
Copy Markdown
Contributor

Problem

Registry.WalletByID (token/services/identity/role/registry.go) built the wallet while holding the registry write lock, contradicting its own inline comment ("Create the wallet without holding the registry lock") and the invariant documented on the Registry type.

Flow before this PR:

  1. Caller asks for a wallet → cache miss.
  2. WalletByID takes r.WalletMu.Lock() (defer Unlock).
  3. Double-checks the Wallets map — still missing.
  4. Still holding the write lock, calls WalletFactory.NewWallet → idemix pseudonym generation, identity-store reads, identity-store writes.
  5. Stores the wallet in the map and returns, releasing the lock.

Two consequences:

  • Serialization: every wallet creation for a role queues behind step 4, and so does every other reader/writer of the map — the expensive crypto and DB work happens inside the critical section.
  • Deadlock: the factory is handed the registry itself as IdentitySupport, so it may legitimately call back into it (e.g. RegisterWallet, which takes WalletMu.Lock()). sync.RWMutex is not reentrant, so such a factory hangs forever at step 4.

Fix

Flow after this PR:

  1. Caller asks for a wallet → cache miss.
  2. Construction runs inside a singleflight.Group keyed by wallet id (golang.org/x/sync/singleflight, already a dependency and the existing house pattern in token/services/utils/cache/ristretto.go).
  3. Short RLock for the double-check, released immediately.
  4. WalletFactory.NewWallet is called with no lock held — a factory may safely re-enter the registry.
  5. WalletMu.Lock() only for the map write, then unlock.

singleflight keeps the "build once" guarantee that the lock used to provide: concurrent callers for the same wallet id share one NewWallet call and get the same instance, while distinct wallet ids are now built in parallel.

Tests

Both new tests were verified to fail against the unfixed registry.go:

  • TestWalletByID_FactoryReentersRegistry — a factory that calls RegisterWallet from inside NewWallet must return; on the old code it hangs (timeout-guarded, a deadlock has no natural termination).
  • TestWalletByID_DistinctWalletsAreCreatedConcurrently — two NewWallet calls for different wallet ids must be in flight simultaneously; on the old code they are serialized.
  • TestWalletByID_ConcurrentCreation additionally asserts exactly one factory call for concurrent same-id requests.

go test -race -count=5 ./token/services/identity/role/ green; make checks and make lint-auto-fix clean.

Docs

New "Wallet Registry concurrency" section in docs/services/identity.md stating the contract: WalletMu guards the map only, NewWallet is never called under a lock, same-id creations are coalesced rather than serialized.

Fixes #2064

…Wallet

Registry.WalletByID took WalletMu.Lock() and then called
WalletFactory.NewWallet while still holding it, contradicting both the
method's own inline comment and the concurrency invariant documented on
the Registry type. The factory receives the registry itself as
IdentitySupport, so a factory that calls back into the registry (e.g.
RegisterWallet) deadlocks permanently, since sync.RWMutex is not
reentrant. It also serialized every wallet creation for a role behind one
write lock, including idemix pseudonym generation and storage access.

Wallet construction now runs inside a singleflight group keyed by wallet
id, with no lock held: concurrent callers for the same wallet still share
a single NewWallet call, while distinct wallets are built in parallel.
WalletMu is only taken for the short double-check read and the map write.

Fixes #2064

Signed-off-by: AkramBitar <akram@il.ibm.com>
@AkramBitar
AkramBitar force-pushed the fix/2064-registry-lock-scope branch from b7f8be0 to 3469ce7 Compare August 18, 2026 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

identity/role: WalletByID holds the write lock across NewWallet, contradicting its own comment

1 participant