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

Summary

Registry.WalletByID takes the registry's write lock and then calls WalletFactory.NewWallet while
still holding it — directly contradicting the method's own inline comment and the concurrency
invariant documented on the Registry type itself. NewWallet performs idemix pseudonym generation
and calls back into the registry via the IdentitySupport interface (BindIdentity), so any factory
implementation that calls back into the same registry self-deadlocks, since sync.RWMutex is not
reentrant.

Where

token/services/identity/role/registry.go:284-298:

// Register the newly created wallet but check if another goroutine already created it.
r.WalletMu.Lock()
defer r.WalletMu.Unlock()
if existing, ok := r.Wallets[wID]; ok {
	// Another goroutine created and registered the wallet in the meantime; prefer it.
	return existing, nil
}
// Create the wallet without holding the registry lock (avoid holding locks while calling external code).
r.Logger.DebugfContext(ctx, "create wallet [%s]", wID)
newWallet, err := r.WalletFactory.NewWallet(ctx, wID, role, r, idInfo)

The comment on line 291 says "without holding the registry lock," but the call on line 293 happens
while r.WalletMu is locked (:285, released only by the deferred Unlock at function return).
This also violates the type-level invariant documented at registry.go:27-33: "Methods in this file
follow the pattern of ... never holding locks while calling out to external services ... to avoid
blocking and potential deadlocks."

r is passed to NewWallet as the IdentitySupport argument. role/wallets.go:587 and :604 call
IdentitySupport.BindIdentity from within wallet methods — if a factory implementation calls
BindIdentity during construction (a plausible thing for a wallet to do while registering its
identity), it reaches back into Registry.BindIdentity... which does not itself take WalletMu, so
today it wouldn't deadlock via that specific path, but any factory that touches r.Wallets (e.g. via
RegisterWallet, which does take r.WalletMu.Lock() at :172-173) would deadlock immediately since
sync.RWMutex is non-reentrant.

Impact

• Every wallet creation for a given role serializes behind one lock, including the potentially
  expensive idemix pseudonym generation, DB reads, and DB writes performed inside NewWallet — this
  is a concurrency bottleneck, not just a correctness smell.
• A WalletFactory implementation that calls back into Registry.RegisterWallet (or otherwise
  re-enters WalletMu) during construction deadlocks the calling goroutine permanently.

Reproduction

Not yet committed. A regression test can assert the lock is not held during NewWallet by having a
fake WalletFactory.NewWallet implementation attempt r.RegisterWallet(ctx, id, w) from within its
own body (simulating a factory that registers itself) and confirming it does not hang; today this
would deadlock (test with a timeout/subprocess pattern, since a real deadlock has no natural
termination).

Suggested fix

Move wallet construction outside the critical section, using golang.org/x/sync/singleflight (already
vendored — go.mod:33 — and already the house pattern for this exact "coalesce concurrent creation
under one key without holding a lock across external calls" problem at
token/services/utils/cache/ristretto.go:13,32,88):

newWallet, err, _ := r.creationGroup.Do(wID, func() (interface{}, error) {
	r.WalletMu.RLock()
	if existing, ok := r.Wallets[wID]; ok {
		r.WalletMu.RUnlock()
		return existing, nil
	}
	r.WalletMu.RUnlock()
	return r.WalletFactory.NewWallet(ctx, wID, role, r, idInfo)
})
if err != nil {
	return nil, err
}
r.WalletMu.Lock()
r.Wallets[wID] = newWallet.(driver.Wallet)
r.WalletMu.Unlock()

Note: singleflight alone dedupes concurrent creation for the same key, but does not help when
storage genuinely reports "not found" due to a transient error (see #7) — that's a separate defect
in the input to this code path, and the two should be fixed together.

Severity

MEDIUM — real deadlock risk depends on factory implementation details, but the lock-scope violation
and its performance impact are present today regardless.