Skip to content

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

Open
EvanYan1024 wants to merge 3 commits into
LFDT-Panurus:mainfrom
Built-by-Sign:perf/skip-ismine-pure-auditor
Open

perf(identity): skip IsMine owner lookup on a pure-auditor node#1827
EvanYan1024 wants to merge 3 commits into
LFDT-Panurus:mainfrom
Built-by-Sign:perf/skip-ismine-pure-auditor

Conversation

@EvanYan1024

@EvanYan1024 EvanYan1024 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

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.

The decision is made once in NewTMSAuthorization (amIAnAuditor && len(OwnerWalletIDs) == 0). When true, the authorization is wrapped in a small decorator whose IsMine returns ("", nil, false); all other checks delegate to the wrapped authorization via embedding:

type pureAuditorAuthorization struct {
	Authorization
}

func (pureAuditorAuthorization) IsMine(context.Context, *token2.Token) (string, []string, bool) {
	return "", nil, false
}

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.

Note for reviewers

The pure-auditor status is decided once at construction. A node that starts with no owner wallets and later registers one via RegisterOwnerIdentity would keep the short-circuit. That topology (an auditor dynamically gaining owner wallets at runtime) isn't one we hit, but if it should be supported, the decision would need to be re-evaluated on owner-wallet registration rather than frozen at construction — happy to follow up if you'd prefer that.

Testing

go build ./token/... and go test ./token/core/common/ pass, including two added cases: a pure auditor skips the owner lookup (asserted via the mock's OwnerWallet call count) and an auditor that also owns wallets still resolves ownership.

A node that holds an auditor wallet and no owner wallets can never satisfy
IsMine, yet the audit path calls it on every transaction output, each time
performing a LocalMembership owner lookup that always misses. Under load
these misses serialise on the LocalMembership write lock and dominate the
auditor's mutex profile.

Decide at construction whether the node is a pure auditor (auditor wallet
present, zero owner wallets) and, if so, wrap its authorization in a small
decorator whose IsMine returns false, skipping the owner lookup. This keeps
WalletBasedAuthorization.IsMine a plain per-token predicate and isolates the
node-role decision to the constructor. A node that also owns wallets is not
wrapped, so ownership is still resolved.

Refs LFDT-Panurus#1619

Signed-off-by: Evan <evanyan@sign.global>
@AkramBitar

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor Author

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.WalletIDsStorage.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.

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.

Auditor nodes call IsMine() on every output identity, causing LocalMembership lock contention under high load

3 participants