identity: areMe converts storage errors into false-negative ownership answers

[Effi-S]

Fixes #2066

Summary

Provider.areMe (backing both AreMe and IsMe) returns whatever partial result it has accumulated
when the storage lookup for signer existence errors, rather than propagating the error. This means a
transient storage failure makes IsMe report false ("not mine") for an identity that actually is
ours, which is on the token-ownership decision path.

Where

token/services/identity/provider.go:270-279:

// check Storage
found, err := p.storage.GetExistingSignerInfo(ctx, notFound...)
if err != nil {
	p.Logger.Errorf("failed checking if a signer exists [%s]", err)
	return result.ToSlice()
}
result.Add(found...)
return result.ToSlice()

On error, result contains only whatever was already resolved from the in-memory cache
(provider.go:257-264) before the storage call — everything not in cache is silently reported as
"not mine," logged only at Errorf with no error propagated to the caller.

Related: token/services/identity/wallet/service.go:138-149 Wallet() similarly discards two errors
from OwnerWallet/IssuerWallet and returns nil (a caller cannot distinguish "no wallet" from
"lookup failed"):

func (s *Service) Wallet(ctx context.Context, identity tdriver.Identity) tdriver.Wallet {
	w, _ := s.OwnerWallet(ctx, identity)
	if w != nil {
		return w
	}
	iw, _ := s.IssuerWallet(ctx, identity)
	if iw != nil {
		return iw
	}
	return nil
}

Impact

IsMe is used to decide ownership-related behavior (e.g. whether to react to a token as an owned
token). A transient storage error causing a false negative means an owned token can be treated as
not-owned for that call, with no visible error — the caller has no signal that the answer is
unreliable rather than authoritative.

Reproduction

Not yet committed. Mock Storage.GetExistingSignerInfo to return an error for a set of identities
that includes at least one genuinely-owned identity not already warm in the in-memory cache; assert
IsMe/AreMe either propagates the error or is documented as best-effort at the call sites that rely
on it for correctness-sensitive decisions.

Suggested fix

AreMe/IsMe's current signatures ([]string / bool, no error return) make it structurally unable
to distinguish "confirmed not mine" from "couldn't check." Changing the signature ripples through the
driver.IdentityProvider interface and every caller, so the minimal fix here is: keep the signatures,
but do not silently swallow storage errors into a negative result for the specific identities the
storage call was about — instead of returning early, consider retrying once, or making the
error-swallowing explicit and loud enough (metric increment, not just a log line) that an operator can
detect ownership answers were degraded. If callers need a hard guarantee, a follow-up to thread an
error return through AreMe is worth considering separately.

Severity

MEDIUM — requires a transient storage failure to trigger, and produces an unsignaled false negative
on an ownership decision.

[AkramBitar]

BEFORE (broken):
────────────────
IsMe(identity)
    │
    ▼
areMe()
    │
    ├──► check in-memory cache
    │         │
    │         ├── HIT  ──► add to result
    │         └── MISS ──► goes to storage
    │
    ├──► GetExistingSignerInfo()  ── FAILS (transient DB error)
    │         │
    │         ▼
    │    logger.Errorf(...)       ← error logged then DROPPED
    │         │
    │         ▼
    │    return result (cache-only)   ← partial, unreliable answer
    │
    ▼
IsMe returns false               ← "not mine" — WRONG, just uncached
    │
    ▼
caller treats it as authoritative
    │
    ▼
owned token treated as not-owned ← silent incorrect behaviour


AFTER (fixed):
──────────────
IsMe(identity)
    │
    ▼
areMe()
    │
    ├──► check in-memory cache
    │         │
    │         ├── HIT  ──► add to result
    │         └── MISS ──► goes to storage
    │
    ├──► GetExistingSignerInfo()  ── FAILS (transient DB error)
    │         │
    │         ▼
    │    return nil, err          ← error propagated up
    │
    ▼
IsMe returns (false, err)        ← caller knows answer is unreliable
    │
    ▼
caller propagates error          ← operation fails visibly
    │
    ▼
no silent incorrect ownership decision ✅

[AkramBitar]

Two more findings, on code that isn't in this diff but is directly in scope for the bug being fixed:

1. token/services/storage/db/kvs/identitydb.go:312 — the fix doesn't hold for the KVS-backed identity store, so #2066 is only closed for the SQL backend.

return s.kvs.GetExisting(ctx, keys...), nil can never report an error, and FSC's KVS.GetExisting (fabric-smart-client v0.18.0, platform/view/services/storage/kvs/kvs.go:109-111) does:

it, err := o.store.GetStateSetIterator(...)
if err != nil {
    return result
}

i.e. it swallows the store failure and returns the partial/empty list. So: node restarts (in-memory p.signers cache cold), backing store is briefly unavailable, Provider.areMe receives a short slice with err == nil and still answers "not mine" for an owned identity — exactly the false negative this PR is meant to eliminate.

The error-less signature is baked into the KVS interface at token/services/storage/db/kvs/kvs.go:17, so fixing it needs an interface change or a Get-based existence check.

2. token/services/ttx/collectendorsements.go:462 — the auditor-locality decision in a file this PR edits is still fail-open.

local := sigService.IsMe(context.Context(), c.tx.Opts.Auditor) uses FSC's sig.Service, whose AreMe logs and discards a FilterExistingSigners error (platform/view/services/sig/service.go:182-184).

Scenario: a self-auditing issuer restarts (cold signer cache) and the signer store errors → local=false → AuditingViewInitiator.Call takes startRemote → context.GetSession(a, <own identity>), which FSC explicitly does not support for self (see the comment on startLocal in token/services/ttx/auditor.go) → the auditing session fails or blocks until the one-minute ReceiveTypedWithTimeout.

Worth resolving locality through c.tx.TokenService().SigService().IsMe(...) (now error-returning) or FSC's SignerInfoExists.

#2172 (comment)

[AkramBitar]

The fix is correct — propagating the error rather than silently returning a false-negative is the right call. One blocking issue on the partial-slice contract, plus minor nits inline.

[AkramBitar]

@Effi-S

See my comments and could you please squash the commits to one?

Thanks a lot,
Akram

[AkramBitar]

Reviewed origin/main...fix-2066 at 03597e04 in a clean worktree. The direction and the interface shape are right — (bool, error) / ([]string, error) is exactly what lets a caller tell "confirmed not mine" from "couldn't check", and the doc comments you added on each interface are the kind of thing that keeps that distinction alive. Two things I think need another iteration before this lands, then some smaller notes.

1. CI is red from this PR's own test

TestProvider_AreMe_StorageErrorIsPropagated asserts AreMe returns the cache-only partial slice alongside the error, but areMe returns nil:

$ go test ./token/services/identity/
--- FAIL: TestProvider_AreMe_StorageErrorIsPropagated (0.00s)
    provider_test.go:131:
        	Error:      	Not equal:
        	            	expected: []string{"NnMBTnK2c4O+MCSFaUVVpXrTk6/euu1t7REKd1vQVW0="}
        	            	actual  : []string(nil)
FAIL	github.com/LFDT-Panurus/panurus/token/services/identity	0.085s

Worth deciding deliberately rather than just making the test pass: the docs on all three interfaces describe a partial slice, the code returns nil. Details inline on provider.go:152. (For the record, the contract is stated two ways — docs vs. implementation — not three; driver/wallet.go:44, provider.go:152 and token/sig.go:67 all agree with each other.)

2. The propagated error is swallowed one layer up, at the site where #2066 actually loses tokens

This is the one I'd most want your read on. #2066's concrete damage is an owned token being silently dropped, and that happens in token/core/common/authorization.go:61:

func (w *WalletBasedAuthorization) IsMine(ctx context.Context, tok *token2.Token) (string, []string, bool) {
	wallet, err := w.WalletService.OwnerWallet(ctx, tok.Owner)
	if err != nil {
		return "", nil, false
	}
	...
}

and then token/services/tokens/tokens.go:497:

ownerWalletID, ids, mine := auth.IsMine(ctx, &output.Token)
...
if !mine && !auditorFlag && !issuerFlag {
	logger.DebugfContext(ctx, "transaction [%s], discarding token, not mine, not an auditor, not an issuer", requestAnchor)
	continue
}

So the error this PR now correctly propagates out of areMe → IsMe → mapIdentityToID → Registry.Lookup → OwnerWallet arrives at IsMine, is turned straight back into mine = false, and the output is discarded and never recorded as owned — the exact symptom, unchanged. Same shape at four sibling sites:

• token/services/interop/htlc/script.go:144 and :151 — if wallet, err := s.WalletService.OwnerWallet(ctx, script.Sender); err == nil
• token/services/ttx/boolpolicy/auth.go:58
• token/services/ttx/multisig/auth.go:59
• token/services/identity/wallet/service.go:139 — w, _ := s.OwnerWallet(ctx, identity) (you already flag this one as "Related" in the PR description, so this may be half-acknowledged already)

The structural blocker is that driver.Authorization.IsMine (token/driver/wallet.go:332) has no error return, so there is nowhere for the distinction to go:

IsMine(ctx context.Context, tok *token.Token) (walletID string, additionalOwners []string, mine bool)

Extending Authorization is a bigger change than this PR, and splitting it out is completely reasonable — but if it isn't in scope here, it'd help to say so in the PR description and open the follow-up, because as it stands Fixes #2066 overstates what merges.

3. docs/upgradability.md needs an "SDK API Changes (Go)" entry

That section exists precisely for this (it was added for #2063's GetWalletID return-type change). This PR breaks these exported symbols:

• driver.IdentityProvider.IsMe / .AreMe (token/driver/wallet.go:44,48)
• token.SignatureService.IsMe / .AreMe (token/sig.go:70,77) — the public facade, so any application calling tms.SigService().IsMe(...) stops compiling
• role.LocalMembership.IsMe (token/services/identity/role/role.go:29)
• membership.IdentityProvider.IsMe (token/services/identity/membership/lm.go:103)
• ttx/dep.SignatureService.IsMe (token/services/ttx/dep/providers.go:62)

AGENTS.md's "Documentation Updates" rule makes this required before the task counts as done. (docs/services/identity.md:45 only carries a bare +IsMe() in a mermaid class diagram, so that one is not stale — no change needed there.)

4. None of the new error paths are exercised

$ grep -rn "IsMeReturns\|AreMeReturns" --include=*_test.go .
token/sig_test.go:128:	ip.IsMeReturns(true, nil)
token/sig_test.go:185:	ip.AreMeReturns(expectedHashes, nil)
token/services/identity/membership/lm_security_test.go:191:	ip.IsMeReturns(false, nil)
token/services/identity/membership/lm_test.go:37:	ip.IsMeReturns(true, nil)
token/services/ttx/endorse_test.go:73:	tokenIP.IsMeReturns(true, nil)
token/services/ttx/boolpolicy/spend_test.go:74:	ip.AreMeReturns(areMe, nil)
token/services/ttx/multisig/spend_test.go:74:	ip.AreMeReturns(areMe, nil)

Every stub passes nil. Uncovered: role/role.go:177, role/role.go:235, membership/lm.go:298, token/sig.go:70, token/sig.go:77, ttx/endorse.go:257, ttx/collectendorsements.go:694, ttx/boolpolicy/spend.go:153, ttx/multisig/spend.go:138. The two new registry_test.go cases stub MapToIdentityReturns(…, err) directly, so they cover Registry.Lookup's fallback but never role.go's propagation. A single error-returning stub per package would lock in the intent.

Smaller notes

Left inline: a nil-cache-probe bug at registry.go:210 (with a reproduction), a []byte-vs-string lookup asymmetry, the dropped storage cause in the string-label branch, and a context-cancellation conflation at provider.go:289.

Also minor: the comments at provider_test.go:118 and :129-130 describe the partial-slice behaviour that the implementation doesn't have, and assert.Empty(t, me) at :96 passes vacuously against nil — both will need a touch-up once #1 is settled either way.

Things I checked that are fine

• Build (go build ./... across all 9 modules), go vet, and golangci-lint run are all clean.
• Counterfeiter mocks are in sync — all four regenerated fakes still carry their var _ Interface = new(Fake) assertions and compile.
• The switch → if refactors in role.go preserve the original short-circuit ordering exactly (IsMe is still only reached when every earlier case is false).
• No goroutine-lifetime regressions: boolpolicy/spend.go returns before any go collectAnswers, and multisig/spend.go constructs the AnswersCollector first but it holds only a buffered channel — no goroutine, no timer — so nothing leaks on the new early return.
• collectendorsements.go:687 (FSC's AreMe, left swallowing) is benign as written: a false negative there just pushes the identity into remainingIds, which the token-level AreMe at :694 re-checks — so no change needed.
• The string-label fallback at registry.go:228 is fail-closed, not fail-open — I traced it end-to-end and when nothing resolves it still errors. My note there is only about the lost cause and the unchanged semantics.
• FYI, TestTranslatePath in token/services/identity/config fails in any worktree whose path doesn't contain "panurus". It reproduces on origin/main and is nothing to do with this PR.

Out of scope, for a possible follow-up issue rather than this PR: Registry.ContainsIdentity (role/registry.go:355) → storage/db/sql/common/wallet.go:143 still logs-and-returns result != "" on a query failure, and collectendorsements.go:462 uses FSC's sig.Service.IsMe, which still has no error return and decides self-vs-remote auditing. Both are the same bug class, both pre-existing and untouched here.

[Effi-S]

Reviewed origin/main...fix-2066 at 03597e04 in a clean worktree. The direction and the interface shape are right — (bool, error) / ([]string, error) is exactly what lets a caller tell "confirmed not mine" from "couldn't check", and the doc comments you added on each interface are the kind of thing that keeps that distinction alive. Two things I think need another iteration before this lands, then some smaller notes.

1. CI is red from this PR's own test

TestProvider_AreMe_StorageErrorIsPropagated asserts AreMe returns the cache-only partial slice alongside the error, but areMe returns nil:

$ go test ./token/services/identity/
--- FAIL: TestProvider_AreMe_StorageErrorIsPropagated (0.00s)
    provider_test.go:131:
        	Error:      	Not equal:
        	            	expected: []string{"NnMBTnK2c4O+MCSFaUVVpXrTk6/euu1t7REKd1vQVW0="}
        	            	actual  : []string(nil)
FAIL	github.com/LFDT-Panurus/panurus/token/services/identity	0.085s

Worth deciding deliberately rather than just making the test pass: the docs on all three interfaces describe a partial slice, the code returns nil. Details inline on provider.go:152. (For the record, the contract is stated two ways — docs vs. implementation — not three; driver/wallet.go:44, provider.go:152 and token/sig.go:67 all agree with each other.)

2. The propagated error is swallowed one layer up, at the site where #2066 actually loses tokens

This is the one I'd most want your read on. #2066's concrete damage is an owned token being silently dropped, and that happens in token/core/common/authorization.go:61:

func (w *WalletBasedAuthorization) IsMine(ctx context.Context, tok *token2.Token) (string, []string, bool) {
	wallet, err := w.WalletService.OwnerWallet(ctx, tok.Owner)
	if err != nil {
		return "", nil, false
	}
	...
}

and then token/services/tokens/tokens.go:497:

ownerWalletID, ids, mine := auth.IsMine(ctx, &output.Token)
...
if !mine && !auditorFlag && !issuerFlag {
	logger.DebugfContext(ctx, "transaction [%s], discarding token, not mine, not an auditor, not an issuer", requestAnchor)
	continue
}

So the error this PR now correctly propagates out of areMe → IsMe → mapIdentityToID → Registry.Lookup → OwnerWallet arrives at IsMine, is turned straight back into mine = false, and the output is discarded and never recorded as owned — the exact symptom, unchanged. Same shape at four sibling sites:

• token/services/interop/htlc/script.go:144 and :151 — if wallet, err := s.WalletService.OwnerWallet(ctx, script.Sender); err == nil
• token/services/ttx/boolpolicy/auth.go:58
• token/services/ttx/multisig/auth.go:59
• token/services/identity/wallet/service.go:139 — w, _ := s.OwnerWallet(ctx, identity) (you already flag this one as "Related" in the PR description, so this may be half-acknowledged already)

The structural blocker is that driver.Authorization.IsMine (token/driver/wallet.go:332) has no error return, so there is nowhere for the distinction to go:

IsMine(ctx context.Context, tok *token.Token) (walletID string, additionalOwners []string, mine bool)

Extending Authorization is a bigger change than this PR, and splitting it out is completely reasonable — but if it isn't in scope here, it'd help to say so in the PR description and open the follow-up, because as it stands Fixes #2066 overstates what merges.

3. docs/upgradability.md needs an "SDK API Changes (Go)" entry

That section exists precisely for this (it was added for #2063's GetWalletID return-type change). This PR breaks these exported symbols:

• driver.IdentityProvider.IsMe / .AreMe (token/driver/wallet.go:44,48)
• token.SignatureService.IsMe / .AreMe (token/sig.go:70,77) — the public facade, so any application calling tms.SigService().IsMe(...) stops compiling
• role.LocalMembership.IsMe (token/services/identity/role/role.go:29)
• membership.IdentityProvider.IsMe (token/services/identity/membership/lm.go:103)
• ttx/dep.SignatureService.IsMe (token/services/ttx/dep/providers.go:62)

AGENTS.md's "Documentation Updates" rule makes this required before the task counts as done. (docs/services/identity.md:45 only carries a bare +IsMe() in a mermaid class diagram, so that one is not stale — no change needed there.)

4. None of the new error paths are exercised

$ grep -rn "IsMeReturns\|AreMeReturns" --include=*_test.go .
token/sig_test.go:128:	ip.IsMeReturns(true, nil)
token/sig_test.go:185:	ip.AreMeReturns(expectedHashes, nil)
token/services/identity/membership/lm_security_test.go:191:	ip.IsMeReturns(false, nil)
token/services/identity/membership/lm_test.go:37:	ip.IsMeReturns(true, nil)
token/services/ttx/endorse_test.go:73:	tokenIP.IsMeReturns(true, nil)
token/services/ttx/boolpolicy/spend_test.go:74:	ip.AreMeReturns(areMe, nil)
token/services/ttx/multisig/spend_test.go:74:	ip.AreMeReturns(areMe, nil)

Every stub passes nil. Uncovered: role/role.go:177, role/role.go:235, membership/lm.go:298, token/sig.go:70, token/sig.go:77, ttx/endorse.go:257, ttx/collectendorsements.go:694, ttx/boolpolicy/spend.go:153, ttx/multisig/spend.go:138. The two new registry_test.go cases stub MapToIdentityReturns(…, err) directly, so they cover Registry.Lookup's fallback but never role.go's propagation. A single error-returning stub per package would lock in the intent.

Smaller notes

Left inline: a nil-cache-probe bug at registry.go:210 (with a reproduction), a []byte-vs-string lookup asymmetry, the dropped storage cause in the string-label branch, and a context-cancellation conflation at provider.go:289.

Also minor: the comments at provider_test.go:118 and :129-130 describe the partial-slice behaviour that the implementation doesn't have, and assert.Empty(t, me) at :96 passes vacuously against nil — both will need a touch-up once #1 is settled either way.

Things I checked that are fine

• Build (go build ./... across all 9 modules), go vet, and golangci-lint run are all clean.
• Counterfeiter mocks are in sync — all four regenerated fakes still carry their var _ Interface = new(Fake) assertions and compile.
• The switch → if refactors in role.go preserve the original short-circuit ordering exactly (IsMe is still only reached when every earlier case is false).
• No goroutine-lifetime regressions: boolpolicy/spend.go returns before any go collectAnswers, and multisig/spend.go constructs the AnswersCollector first but it holds only a buffered channel — no goroutine, no timer — so nothing leaks on the new early return.
• collectendorsements.go:687 (FSC's AreMe, left swallowing) is benign as written: a false negative there just pushes the identity into remainingIds, which the token-level AreMe at :694 re-checks — so no change needed.
• The string-label fallback at registry.go:228 is fail-closed, not fail-open — I traced it end-to-end and when nothing resolves it still errors. My note there is only about the lost cause and the unchanged semantics.
• FYI, TestTranslatePath in token/services/identity/config fails in any worktree whose path doesn't contain "panurus". It reproduces on origin/main and is nothing to do with this PR.

Out of scope, for a possible follow-up issue rather than this PR: Registry.ContainsIdentity (role/registry.go:355) → storage/db/sql/common/wallet.go:143 still logs-and-returns result != "" on a query failure, and collectendorsements.go:462 uses FSC's sig.Service.IsMe, which still has no error return and decides self-vs-remote auditing. Both are the same bug class, both pre-existing and untouched here.

I will implement 1,3 and 4 here.
2 Will be moved to PR: #2314