perf(identity): skip IsMine owner lookup on a pure-auditor node#1827
perf(identity): skip IsMine owner lookup on a pure-auditor node#1827EvanYan1024 wants to merge 3 commits into
Conversation
6e4c4bc to
a4cc30e
Compare
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>
f69f02f to
457ef6c
Compare
|
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, |
|
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: Scope of the gap. That said, I have an approach to handle the runtime-registration case — downgrade on registration:
If this shape looks good to you and @adecaro, I'll push it to this PR. |
Refs #1619.
Background
On a pure auditor node, the audit path (
tokens.Service.Parse) callsAuthorization.IsMineon every transaction output. The result is never used there — keep/discard is decided by the auditor flag — but each call still performs aLocalMembershipowner lookup that, on an auditor, always misses. Under load these misses serialise on theLocalMembershipwrite 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
IsMinefor 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 whoseIsMinereturns("", nil, false); all other checks delegate to the wrapped authorization via embedding:This keeps
WalletBasedAuthorization.IsMinea plain per-token predicate and isolates the node-role decision to construction, alongside the existingAuthorizationMultiplexercomposition.NewTMSAuthorizationnow returns theAuthorizationinterface; both call sites already pass the result intoNewAuthorizationMultiplexer(...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
Mineis set on records it genuinely owns. On a pure auditorIsMineis already alwaysfalse, 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
RegisterOwnerIdentitywould 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/...andgo test ./token/core/common/pass, including two added cases: a pure auditor skips the owner lookup (asserted via the mock'sOwnerWalletcall count) and an auditor that also owns wallets still resolves ownership.