fix(auditor): attribute empty-EID inputs to their own token owner - #2199
fix(auditor): attribute empty-EID inputs to their own token owner#2199EvanYan1024 wants to merge 3 commits into
Conversation
5e052bf to
856fbd4
Compare
856fbd4 to
0148b19
Compare
0148b19 to
6f7cbab
Compare
|
Thanks a lot for submitting this PR. According to my understanding (please correct me if I am wrong) Problem The auditor's ledger recorded the wrong person as the spender. When a spend arrived without an owner attached, the code guessed by picking the first name on the receiving side — which is the recipient, not the payer. Fix Look up each spent coin and record its actual previous owner as the spender. If ownership can't be pinned to a single person (e.g. a joint wallet), leave it blank rather than guess. Scenario
I think the direction makes sense to me; attributing the spend to the actual token owner rather than the first output is clearly the right model. A few concerns before it lands:
Regards, |
6f7cbab to
c21333e
Compare
|
@AkramBitar Thanks — your summary is exactly right. (2), (3) and (5) are fixed in the latest push; on (1) you were right to ask and my first answer was wrong; (4) I would like your opinion on. 1. Multi-owner actions. Correcting an earlier version of this reply, in which I claimed this was unreachable: it is reachable.
One detail of the consequence is milder than feared: That leaves a real question: reject such an action earlier with a clear error, or teach the store to represent a multi-sender action. I would rather not settle it inside this PR — happy to open a separate issue unless you prefer it handled here. 2. Godoc vs. behaviour. Fixed — a leftover from an earlier revision that did fail closed. The Godoc and the doc page now both state that such an input is left unattributed. 3. Bounds/nil check. Fixed, mirroring 4. Two TMS sources. Narrowed by the push below: 5. The "returns immediately" note. Correct, and now moot: Also pushed. Since then: |
3a4fce6 to
62c3f2c
Compare
completeInputsWithEmptyEID booked every input with an unresolved enrollment ID under the first output's enrollment ID -- for a payment that is the recipient, so the payer's whole input was recorded as spent by the counterparty and both parties' eid-keyed audit balances corrupt silently. Resolve the enrollment ID (and revocation handle) from the input token's owner instead, preferring the audit info already carried by the record over the local identity store: WalletManager gains GetEIDAndRH taking explicit audit info, falling back to the stored one when empty. An owner that maps to no single enrollment ID -- a composite owner such as a multisig, or one whose audit info this auditor does not hold -- leaves its input unattributed. Amount aggregations skip an empty enrollment ID, so an unattributed input is counted for nobody, whereas a guessed one is charged to the wrong party. A token upgrade is the exception: across the built-in drivers its issue metadata describes no sender for the inputs, and the pre-upgrade owner predates the current driver, so it resolves to nothing here. The upgrade re-issues the spent tokens to the same party, so such an input takes the enrollment ID of the outputs issued by its own action, when every one of them resolves to the same party; the revocation handle comes from the same output. Leaving it unattributed would credit the upgraded amount without ever debiting it and double the owner's holding. Run the gap filling in Audit, before the enrollment IDs are collected, so the locks cover the enrollment ID each input is finally booked under, and return a cached record from Append as it stands. Re-filling there could attribute an input Audit deliberately left empty, storing it under an enrollment ID that was never locked; Append still attributes the record when called without a preceding Audit. Guard the vault result the way Request.AuditRecord does, with a length check and a nil check per token: the gap filling now runs on the audit approval path, where indexing past a short answer would panic. Fixes LFDT-Panurus#2198 Signed-off-by: Evan <evanyan@sign.global>
62c3f2c to
ecb71f9
Compare
…-attribution Signed-off-by: Evan <evanyan@sign.global>
…-attribution Signed-off-by: Evan <evanyan@sign.global>
Fixes #2198
What
WalletManager.GetEIDAndRH(ctx, identity, auditInfo), which prefers the audit info carried by the record and falls back to the locally stored one; when neither is available it returns empty values rather than an error.Audit, before the enrollment IDs are collected, so the locks cover the enrollment ID each input is finally booked under, and return a record cached byAuditfromAppendas it stands.Appendstill attributes the record when called without a precedingAudit.Request.AuditRecorddoes — a length check plus a per-token nil check — since this code now runs on the audit approval path.Why
In a payment the first output is the recipient, so the old
targetEID := record.Outputs.EnrollmentIDs()[0]recorded the payer's spend against the counterparty: the recipient is charged a spend it never made, and the payer's spend is never booked. Neither side errors or warns. TheTODOabove that line marked it as a placeholder.Leaving an unattributable input empty rather than guessing is the point of the change:
EnrollmentIDs()skips empty enrollment IDs, so such an input is counted for nobody, which is the honest outcome when the owner genuinely maps to no single enrollment ID. A multisig input is exactly that — several members, no single one to charge — so it is a normal state to represent, not a failure to raise.The upgrade case is the one where the same guess was right, and dropping it showed up as a red
TokensUpgradeintegration test (alice's holding 220 instead of 110).extractIssueInputsfills only the token id, and the pre-upgrade owner predates the current driver, so nothing in the record resolves it. But an upgrade re-issues the spent tokens to their owner under a fresh identity, so what the request issues to is the input's enrollment ID — leaving it empty credits the upgraded amount without ever debiting it. The fallback is confined to inputs the request describes no sender for, and scoped to the outputs of the input's own action: a second issue action neither suppresses the attribution nor lends its enrollment ID to it. Every issued output of that action must resolve to the same party — one resolving to none cannot be shown to belong to the others — and the handle is kept only while it stays paired with that enrollment ID.The lifecycle matters as much as the attribution.
Auditcollects the enrollment IDs it locks from the record andAppendwrites that record, so filling the gaps only inAppendwould leave the final record booked against a payer that was never locked — an exact spend has no change output, so the payer's enrollment ID appears nowhere in whatAuditsaw.The same reasoning applies in reverse to the cached record: re-running the gap filling in
Appendcould attribute an input thatAuditdeliberately left empty, storing it under an enrollment ID that was never locked.Appendtherefore returns the cached record as it stands. Together these keep the locked set and the stored record in agreement, and remove the second vault read for records carrying an unattributed input.Resolving from the input's own owner also drops the
record.Outputs.EnrollmentIDs()[0]index-out-of-range on a record with no outputs.Testing
token/services/auditor/auditor_internal_test.go: attribution from the token owner; record-carried audit info preferred over a local lookup (asserting no local lookup happens); a composite owner and a missing-audit-info owner both staying unattributed without an error, with the remaining fields still filled from the spent token; a genuine resolution error still propagating; a short vault answer and a nil vault token each erroring instead of panicking.TestRequestWrapper_AuditRecord_UpgradeInputAttributedToReceiverruns an upgrade-shaped request through the realRequest.AuditRecordpipeline rather than a hand-written record, so whatextractIssueInputsandextractIssueOutputsactually produce — the input arriving without an owner, the issued output carrying issuer, owner and enrollment ID under the same action index — is exercised instead of assumed. It also asserts the record's inputs and outputs sum equal, which is the holding that went to 220 inupdate-t1.TestRequestWrapper_AuditRecord_CachedKeepsUnattributedInput: a record cached byAuditwith an unattributed input is returned untouched even though the wallet service would now resolve it, with no further vault or identity lookup.TestService_Audit_AttributesAndLocksEmptyEIDInputpins the lifecycle over an exact spend whose sender does not resolve locally:Auditreturns the resolved payer enrollment ID and revocation handle, the acquired locks contain that payer and no empty ID, andAppendreuses the record without a second vault read or identity lookup. Moving the gap filling back after lock acquisition fails this test.gofmt,go build,go vet,go test -race, and golangci-lint v2.12.2 with the repo config.Docs
docs/services/auditor.mdgains an "Input Attribution" section: the per-input resolution order, the upgrade fallback, what happens to an owner with no single enrollment ID, and where in theAudit/Appendlifecycle it runs relative to the EID locking documented just below it.