perf(identity): skip IsMine owner lookup on a pure-auditor node

[EvanYan1024]

Refs #1619.

Background

On a pure auditor node, the audit path (tokens.Service.Parse) calls Authorization.IsMine on every transaction output. The result is never used there — keep/discard is decided by the auditor flag — but each call still performs a LocalMembership owner lookup that, on an auditor, always misses. Under load these misses serialise on the LocalMembership write lock and dominate the auditor's mutex profile (~82–88% of mutex wait time in #1619's measurements).

Change

This implements the fix discussed and accepted in #1619: skip IsMine for a pure auditor — a node that holds an auditor wallet and no owner wallets.

NewTMSAuthorization decides at construction (amIAnAuditor && len(OwnerWalletIDs) == 0) and, when true, wraps the authorization in a small decorator whose IsMine short-circuits to ("", nil, false); all other checks delegate to the wrapped authorization via embedding.

The short-circuit is not frozen at construction. The wallet service signals successful owner-identity registrations through an optional notifier (consumed via an interface assertion, so driver.WalletService itself stays untouched); the decorator holds an atomic.Bool checked with a single load in IsMine, and the first successful RegisterOwnerIdentity permanently downgrades it to plain delegation:

type pureAuditorAuthorization struct {
	Authorization
	hasOwnerWallets atomic.Bool
}

func (p *pureAuditorAuthorization) IsMine(ctx context.Context, tok *token2.Token) (string, []string, bool) {
	if p.hasOwnerWallets.Load() {
		return p.Authorization.IsMine(ctx, tok)
	}

	return "", nil, false
}

The hook is attached before the emptiness check, so a registration racing with construction cannot be missed; when the node turns out not to be a pure auditor, the constructor detaches the hook again. If the wallet service cannot signal registrations, the decorator is not applied and the node stays on the unoptimized-but-correct path; a failing owner-wallet probe is debug-logged and falls back the same way.

This keeps WalletBasedAuthorization.IsMine a plain per-token predicate and isolates the node-role decision to construction, alongside the existing AuthorizationMultiplexer composition. NewTMSAuthorization now returns the Authorization interface; both call sites already pass the result into NewAuthorizationMultiplexer(...Authorization), so nothing depends on the concrete type.

Gating on "auditor and no owner wallets" (rather than the auditor flag alone) keeps a combined auditor+owner node correct: there the node is not wrapped, so ownership is still resolved and Mine is set on records it genuinely owns. On a pure auditor IsMine is already always false, so this only removes the always-missing lookup.

Testing

go build ./token/... and go test -race ./token/core/common/ ./token/services/identity/wallet/ pass. Covered cases: a pure auditor skips the owner lookup (asserted via the mock's OwnerWallet call count); a runtime owner registration downgrades the short-circuit; a wallet service without the notifier keeps the owner lookup; an owner-wallet probe failure keeps the owner lookup; an auditor that also owns wallets still resolves ownership and leaves no hook registered; on the wallet-service side, callbacks fire on successful registrations only and stop after unregistering.

[AkramBitar]

@EvanYan1024

Thanks a lot for working on this PR.

This PR LGTM except one concern.

As you noted in "Note for reviewers". The question now, can we have a scenario where a pure auditor later registers an owner wallet via RegisterOwnerIdentity? Since the check is fixed at construction, wouldn't it keep skipping and mis-report the tokens it now owns?

@adecaro can you have a look on that concern?

Regards,
Akram

[EvanYan1024]

Thanks @AkramBitar — the concern is valid, and it's exactly the caveat flagged in the PR description. Two facts to frame it, then a proposal.

Why the decision was frozen at construction. Re-checking per call is not free: OwnerWalletIDs() goes Registry.WalletIDs → Storage.GetWalletIDs, i.e. a storage query. Putting that on the per-token IsMine path would reintroduce the very overhead this PR removes, so a lazy re-check inside IsMine is not an option.

Scope of the gap. IsMine runs once, at token-append time, and the result is persisted. A node registering its first owner wallet at runtime already has a blind spot without this PR: every token appended before the registration was evaluated with no wallets and stored Mine=false, and there is no re-scan on wallet registration. The frozen decorator widens that gap to post-registration tokens.

That said, I have an approach to handle the runtime-registration case — downgrade on registration:

• pureAuditorAuthorization holds an atomic.Bool; IsMine does one atomic load — once set, it permanently delegates to the wrapped Authorization.
• wallet.Service.RegisterOwnerIdentity invokes registered callbacks after a successful registration.
• NewTMSAuthorization wires the two via an optional interface assertion, so driver.WalletService itself stays untouched (no mock churn). The hook is attached before the emptiness check, so a registration racing with construction cannot be missed. Safety fallback: if the wallet service cannot signal registrations, the decorator is not applied and the node stays on the unoptimized-but-correct path.
• Coverage: owner wallets come either from configured identities (caught by the constructor's OwnerWalletIDs check) or from RegisterOwnerIdentity (caught by the callback). RegisterRecipientIdentity registers counterparties' identities, not local owner wallets, so it doesn't need the hook.

If this shape looks good to you and @adecaro, I'll push it to this PR.

[AkramBitar]

Hello @EvanYan1024

Thanks a lot for your response.
For me the approach taken in this PR is ok for me.

@adecaro, could you please share your thoughts?”

Regards,
Akram

[adecaro]

Hi @EvanYan1024 , the approach sounds good. Please, go ahead. Many thanks 🙏

[EvanYan1024]

Pushed the downgrade-on-registration change as ad2155c.

As proposed above: the decorator now holds an atomic.Bool checked with a single load in IsMine — the first successful RegisterOwnerIdentity permanently downgrades it to plain delegation. The wallet service signals registrations through an optional notifier interface (driver.WalletService untouched); the hook is attached before the emptiness check so a registration racing with construction cannot be missed, and without a notifier the decorator is simply not applied.

Covered by tests on both sides: the decorator downgrade + no-notifier fallback in authorization_test.go, and listener firing (success only, not on failure) in wallet/service_test.go. @AkramBitar @adecaro

[AkramBitar]

LGTM

[adecaro]

okay, sorry for the late reply. I think this mechanism is not robust under replication. Image the issuer has two replicas. RegisterOwnerIdentity will be called only on one replica. The other replica will receive a signal form the postgres that a new identity has been registered and a wallet can be created from it if needed. So on this replica, the authorization flag will not be switched. We need something more robust. What do you think @EvanYan1024 ?

[EvanYan1024]

Hi @adecaro, you are right — thanks for catching this. I dug deeper and I now think the right fix is structural, not another patch on the flag. Let me summarize the options I worked through:

1. This PR as it stands (flag flipped by RegisterOwnerIdentity). Broken under replication exactly as you describe: the replica that learns about the new identity through the postgres signal never calls RegisterOwnerIdentity, so its flag never flips — even with perfect signal delivery.

2. Sinking the signal into LocalMembership. I prototyped moving the registration event down to LocalMembership, so that the postgres-notification path (handleConfig) also fires the downgrade on replicas. This fixes the scenario above, but it still shares the same structural weakness: the cached boolean is only as correct as the delivery of the event that invalidates it. A replica that is offline or loses its listen connection when the notification fires keeps answering from a stale flag indefinitely. Any design of the shape "cached answer + event-driven invalidation" inherits this failure mode; adding re-probing bounds the staleness window but adds yet more machinery.

3. Root cause. The reason the short-circuit needs all this care is that stock correctness comes from the miss path itself: every IsMine miss falls into refreshAndGet, which reloads every stored identity configuration under the global write lock and re-checks. That full reload is simultaneously the self-heal that makes lost notifications harmless — and the performance problem behind #1619 (a pure auditor misses on every foreign identity, so every lookup pays a full store scan serialized on one mutex, and a pending writer also blocks all readers).

So the new approach: keep the miss, make it cheap. Instead of skipping the lookup for pure auditors, refreshAndGet point-queries the identity store for the missed label only (a new ConfigurationsByID(id, type) on the store, answered by the existing (id, type, url) primary key): a hit loads just that configuration; a miss returns without ever taking the write lock. Labels that are not valid UTF-8 (raw identity bytes) cannot match a stored configuration id and skip the store entirely. handleConfig also moves its store read outside the write lock.

This restores exactly the stock semantics — the shared store is consulted on every miss, so replicas need no flags, no events, and there is nothing to invalidate — while removing the full-scan-under-lock cost. A pure auditor gets the same benefit this PR aimed for (its misses no longer take the write lock at all), without any role-based special-casing, and every other node profits too.

I have this implemented and tested (including the replication-discovery paths). Since it makes the decorator and the notifier wiring here unnecessary, I'll close this PR and submit the new approach as a fresh PR referencing #1619 and this discussion. Thanks @adecaro @AkramBitar for pushing on the robustness — the result is a much better fix.