identity/role: GetWalletID swallows storage errors, so a transient DB blip creates a duplicate wallet

[Effi-S]

Fixes #2063

Summary

Registry.GetWalletID swallows every storage error and returns ("", nil), which its callers treat
identically to "no wallet is bound to this identity." A transient storage error (DB blip, timeout)
therefore looks exactly like "not registered yet," and the wallet-lookup fallback chain in Lookup
creates a brand-new wallet and a second identity binding for an identity that already has one.

Where

token/services/identity/role/registry.go:237-246:

func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) {
	wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID()))
	if err != nil {
		//nolint:nilerr
		return "", nil
	}
	...
	return wID, nil
}

All three call sites treat err == nil && len(wID) == 0 as "not found, try the next fallback":
registry.go:85-92, registry.go:114-117, registry.go:131-133.

Impact

Lookup (registry.go:72-166) is the fallback path used by WalletByID (registry.go:271-281)
whenever the in-memory cache misses. If the storage-level GetWalletID call fails transiently (not
"no row found," but an actual error — timeout, connection reset, etc.), the registry cannot
distinguish that from "this identity has never been registered." It falls through to
WalletFactory.NewWallet (registry.go:293), creating a second wallet and a second identity→wallet
binding for the same identity. This duplicates wallet state and, depending on the wallet
implementation, can duplicate key material bookkeeping.

Reproduction

Not yet committed. Mock idriver.WalletStoreService.GetWalletID to return a non-nil error (e.g. a
simulated transient DB error) on the first call for an identity that has a real binding, and a
successful lookup on a second, independent call. Assert Registry.GetWalletID propagates the error
rather than returning ("", nil), and that WalletByID does not create a duplicate wallet when the
underlying storage error is transient.

Suggested fix

Propagate the error and let callers decide:

func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) {
	wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID()))
	if err != nil {
		return "", errors.Wrapf(err, "failed to get wallet id for identity [%s]", identity)
	}
	...
	return wID, nil
}

This requires updating the three call sites in Lookup to distinguish "storage error" (abort/retry)
from "no row found" (continue to the next fallback) — today they're merged into one case. This
should land together with #8 (WalletByID write-lock scope), since both touch the same
creation path and fixing one without the other leaves the duplicate-wallet risk only partially
addressed.

Severity

MEDIUM — requires a transient storage failure to trigger, but results in persisted duplicate wallet
state, which is hard to reconcile after the fact.

[AkramBitar]

e

[AkramBitar]

Still need to review

[AkramBitar]

Thanks for taking this on — replacing the (string, error) ambiguity with an explicit WalletIDResolution is the right shape for #2063, and GetWalletID itself now does exactly the right thing.

Two things I think need another pass before this lands, one of which means the bug is still reachable:

1. The KVS store still can't distinguish "no binding" from "lookup failed" (walletdb.go:92), so for a KVS-backed wallet store the duplicate-wallet path from #2063 survives — the swallowed error just moved one layer down, where it is no longer visible.
2. Two of the three new fail-closed branches are on an optional probe (registry.go:166 and :190), which turns a transient storage blip into a failed transaction on a path that previously succeeded with no duplicate risk.

Details inline. The remaining comments are small: a missing Unbound(), an exported-API break worth calling out, and a test assertion that can't fail.

Two repo conventions to tick off as well (AGENTS.md): there is no plan.md for this change, and no docs/ update — the error semantics of Lookup/WalletByID change in a user-visible way (they now fail where they previously fell back), which the wallet/identity docs should reflect.

[AkramBitar]

kvs.Exists can't give us the contract this comment promises. It is a bare bool, implemented upstream as len(GetExisting(...)) > 0, and GetExisting swallows store failures:

// fabric-smart-client@v0.17.0/platform/view/services/storage/kvs/kvs.go:110-113
it, err := o.store.GetStateSetIterator(ctx, o.namespace, ids...)
if err != nil {
    return result   // <- error dropped, empty result
}

So on a genuine failure (connection reset, timeout, no such table) we get Exists → false → GetWalletID → ("", nil) → Registry.GetWalletID → WalletIDUnbound, which this PR documents as "a definitive, successful miss: it is safe to fall through … to create a wallet". That is the #2063 failure mode unchanged — and now harder to spot, because the error is dropped inside FSC instead of in our registry. It is a live path, not hypothetical: the integration identity SDK wires the KVS wallet store (integration/token/common/sdk/identity/provider.go:25).

The awkward part is that kvs.Get has no sentinel for absence either — it returns errors.Errorf("state [%s,%s] does not exist", ...) (kvs.go:172-174). Options as I see them, best first:

• Add a kvs.ErrNotFound (or an error-returning Exists/GetIfExists) upstream in FSC and use it here. Cleanest, and other FSC consumers have the same problem.
• Interim: call Get and classify only the "does not exist" error as unbound, propagating everything else. Works today, but matching on an upstream message is fragile — worth a TODO pointing at the FSC issue.
• If neither fits in this PR, then this store cannot satisfy the contract yet, and we should say so explicitly (in the WalletStoreService doc and in identity/role: GetWalletID swallows storage errors, so a transient DB blip creates a duplicate wallet #2063) rather than leave a comment claiming it does.

Either way, a KVS test that makes the underlying store fail and asserts the error propagates would lock this down — right now TestGetWalletIDNotFound only covers real absence.

[AkramBitar]

Failing closed is right at line 131, but I don't think it is here. At this point MapToIdentity has already succeeded: walletID and ident are authoritative, and wID is already in walletIdentifiers. The GetWalletID(passedIdentity) call here is only an optimization — "is this identity already bound to a wallet we can reuse?" — so a failed probe leaves nothing in doubt.

Concretely, after a restart: WalletByID(ctx, role, aliceIdentityBytes) misses the cache, MapToIdentity returns ("alice"), this probe hits a transient DB error → we now return an error and the caller's transaction fails. Before this change we fell through to Role.GetIdentityInfo("alice") and correctly returned/created wallet "alice" — with no duplicate risk, because the id came from the role mapping and WalletByID re-checks the cache under the write lock before creating anything.

Suggest logging at warn and continuing here (and in the same branch at :190), keeping the mapped candidate:

res := r.GetWalletID(ctx, passedIdentity)
if res.Failed() {
    // Optional probe: the mapped wallet id is already authoritative, so a failed
    // lookup only costs us a reuse opportunity, not correctness.
    r.Logger.Warnf("failed to check wallet binding for identity [%s], continuing with mapped wallet [%s]: %v", passedIdentity, wID, res.Err)
} else if res.Bound() {
    ...
}

Line 131 is the one branch where the probe's answer decides whether we create a wallet — that is where fail-closed belongs, and it is correct as written.

[AkramBitar]

Same as the comment on line 166: by here MapToIdentity has succeeded and ident is authoritative, so this probe is an optimization and a storage failure should be logged and skipped rather than aborting the lookup.

[AkramBitar]

Minor, and I think pre-existing: when res is WalletIDUnbound, res.WalletID is "", so this appends an empty id to walletIdentifiers (same at :206). The old code appended the same empty wID, but now that the status is explicit it is easy to guard — if res.Bound() { walletIdentifiers = append(...) }.

[AkramBitar]

The doc says WalletIDUnknown "guards against a WalletIDResolution that was constructed without going through GetWalletID", but nothing actually guards. With only Bound() and Failed() defined, a zero-value WalletIDResolution{} is indistinguishable from WalletIDUnbound at every call site — i.e. the zero value silently means "authoritative miss, safe to create a wallet", which is precisely the interpretation this type was introduced to make impossible. A mock, or a future constructor that forgets to set Status, gets the dangerous default.

Related: the field comment below tells callers to inspect via Bound/Unbound/Failed, but there is no Unbound() method. Adding it and branching exhaustively fixes both:

func (r WalletIDResolution) Unbound() bool { return r.Status == WalletIDUnbound }

…then at each call site treat "neither Bound() nor Unbound()" as a failure.

[AkramBitar]

Registry.GetWalletID changes from (string, error) to WalletIDResolution. Nothing in-tree breaks (the only callers are the three sites in Lookup; wallet.RoleRegistry does not include the method), but Registry is exported API of a released SDK, so downstream code compiled against the old signature will not build.

Either keep GetWalletID(ctx, identity) (string, error) — now returning the honest error instead of swallowing it — and expose the resolution under a new name (ResolveWalletID?), or make sure this lands in the release notes as a breaking change.

[AkramBitar]

require.Equal(t, 0, wf.NewWalletCallCount()) can't fail in these Lookup subtests (same at :195) — Lookup never touches WalletFactory; only WalletByID does (registry.go:371). These assertions would still pass with the old swallow-and-fall-through behaviour restored inside Lookup, so they don't guard the regression. Asserting on the returned error/values is the real check here; TestWalletByID_StorageErrorDoesNotCreateDuplicate is doing the actual work.

Worth adding one more WalletByID case where MapToIdentity succeeds and the probe fails — that is the common path, and the one affected by my comment on registry.go:166.