feat(wallet-cli): 3-way sender-attribution selector + curated decoys + unified build menu#23
Open
DHEBP wants to merge 15 commits into
Open
feat(wallet-cli): 3-way sender-attribution selector + curated decoys + unified build menu#23DHEBP wants to merge 15 commits into
DHEBP wants to merge 15 commits into
Conversation
Add SenderVerified and RingSize to rpc.Entry, populated in both receiver decode paths. SenderVerified is true only at ring size 2, where the receiver override pins attribution to the counterparty; for larger rings the sender chose the unauthenticated attribution byte, so entry.Sender must not be presented as trusted. RingSize is carried so consumers can render attribution confidence per entry.
Add an additive TransferPayload0WithOptions variant carrying a TransferOptions struct; the existing TransferPayload0 becomes a shim passing the zero value, so all callers keep today's behavior. The new AttributionAnonymous mode writes a decoy ring slot index (drawn from the anonymity set, never the sender or receiver slot) into the encrypted receiver payload, reducing the receiver to the ring's 1-of-N anonymity instead of being handed the sender's slot. Honest mode is unchanged and still writes the receiver slot. A source-level guard test locks honest attribution to witness_index[1] so a change to the sender slot fails loud.
Add a RingPreference (via TransferOptions.Ring) letting a caller supply preferred decoys for ring assembly; random members top up to ringsize. With no preference the candidate source is unchanged, so behavior is identical to today. Preferred decoys are validated parseable, non-self, distinct, and registered on the base balance tree before use. Registration is probed against the base tree (the tree consensus falls back to for ring membership) so a curated decoy cannot pass the wallet and then reject at consensus after signing. Strict mode hard-errors a bad decoy; otherwise it is skipped and random selection fills the slot.
Adds an on-chain gate for the curated-decoy ring-selection path: a transfer whose ring members are user-curated via RingPreference (not drawn from DERO.GetRandomAddress), carrying an action-less SCDATA body, is built at ring 8, accepted into the pool, mined, and read back from the persisted block store — proving the curated ring is consensus-valid, not merely wallet-valid, and that the registration-probe in curatedRingCandidates prevents the 'passes wallet, rejects at consensus' failure by construction. Asserts all ring-2 decoy slots are filled from the curated set (no random fallback) and the body reads back byte-equal. Falsifiable: disabling curation drops curated members from the ring and fails the gate. Single-node simulator scope: finalization = persisted into a mined block (0-conf; PoW/miniblock verify and the low-fee floor are sim-bypassed). Multi-node fork-choice and fee competition remain out of scope.
A 25-agent adversarial audit found the A2 proof SOUND (no false-green;
verified Verify_Transaction_NonCoinbase runs with skip_proof=false, so the
curated ring passes real bulletproof/ring verification, not just wallet
checks) but flagged honesty/falsifiability gaps. This closes them:
- Add Test_CuratedRing_NegativeControls_A2 making the registration-probe
and finalization branches PROVEN-RUN, not EXPECTED-BY-INSPECTION:
(a) Strict + unregistered decoy -> build hard-errors before signing;
(b) non-Strict + unregistered decoy -> dropped, random member substitutes,
only registered preferred decoys appear in the ring;
(c) never-mined carrier -> absent from Block_tx_store (finalization
assertion is non-vacuous).
- Bind curation to finalization: assert finalized_tx hash == submitted hash
(the ring is read off the built tx; the hash commits the serialized ring).
- Make the curation assertion positional/subset-equality (every non-sender/
non-recipient ring member is a preferred decoy) instead of a bare count.
- Correct the success log: drop the unproven fee-debit claim (sim bypasses
the fee floor), state proof verification is NOT bypassed, scope to base-SCID.
- Scope the header + engine comment: for a zero-SCID transfer the curation
registration-probe is redundant with the ring-assembly re-probe; it is the
sole wallet-side defense only on the non-zero-SCID path (not run here).
…anon Addresses the DEROFDN#22 review: - curatedRingCandidates canonicalizes the sender, recipient, and every preferred decoy to its base address before the distinctness/dedup checks, and the ring carries the base form. A ring member is identified by its pubkey, not its address string, so an integrated/payment-id encoding of an account already in the ring (sender, recipient, or another decoy) no longer slips the string-keyed checks and lands a duplicate pubkey that consensus rejects AFTER signing. Test_CuratedRing_AltEncoding_NoDuplicate_A2 covers recipient / sender / decoy-vs-decoy / cross-network, each mutation-proven. (review #1) - Anonymous attribution now fails closed at ring size < 4 (no decoy slot) instead of silently falling through to honest attribution. (review DEROFDN#8) - Port the unverified-attribution export scrub + self-send flag into the decode arms (this stack predates it); behavioral scrub test + an honest-byte guard that asserts honest mode writes the receiver slot (closes the grep guard's reassignment blind spot). (review DEROFDN#3, DEROFDN#9) The bounds guard is sufficient for the same reason as DEROFDN#21: bad ring keys panic at NewAddress before the Publickeylist rebuild, so len==RingSize. (review DEROFDN#4)
Third AttributionMode value pointing the receiver-readable attribution byte at the sender's own ring slot (witness_index[0]) — an explicit, advanced-only self-attribution path, the inverse of the receiver-pointed default. Works at any ring size (slot 0 always exists), so unlike anonymous it needs no ring<3 fail-closed guard; the default path can never reach it (named value only). Behavioral + end-to-end tests prove the byte points at the sender slot (mutation-proven) and that the ring>2 unverified-sender scrub still blanks it on decode.
…s (review DEROFDN#13/DEROFDN#14/DEROFDN#15) Addresses Dirtybird99's re-review of DEROFDN#22: - DEROFDN#13 (correctness): curated-decoy distinctness was string-keyed, so a deroproof (Proof-flag) encoding of an in-ring account rendered a distinct base string and slipped the dedup, landing a duplicate pubkey that consensus rejects after the user signs. Key the dedup on the raw 33-byte pubkey (hex(addr.PublicKey.EncodeCompressed())) — the identity consensus dedups on — so Mainnet, Proof, and integrated axes all collapse to one key; clear Proof on the emitted base so a deroproof-supplied decoy resolves to a registered base address. Removes the now-dead caller-side recipient Mainnet pin (was only needed to keep the string canon in lockstep). New deroproof-decoy-alt vector mutation-proves it (revert canon.Proof=false -> RED "axis is OPEN"). - DEROFDN#14 (maintainability): the unverified-attribution scrub and the self-authored trust block were copy-pasted across the CBOR/CBOR_V2 decode arms; a future one-arm edit could silently reopen the DEROFDN#3 sender-deanonymization leak. Factor scrubExportedPayload() + markSelfAuthored() as the single source of truth, called from every arm. Byte-identical behavior; scrub behavioral test unchanged + green. - DEROFDN#15 (test): wgenesis reused the receiver's temp DB; give it its own path + cleanup. Build + vet clean. Alt-encoding suite (6 vectors incl. 2 deroproof) and scrub behavioral test green (no -race per the pre-existing sim-daemon flake). Pre-existing flaky sim tests (Test_Creation_TX_morecheck, Test_Payload_TX) fail on the unmodified DEROFDN#22 head too — unrelated to this change.
|
Did a deep review of this PR — read through all 11 commits and the full diff. Overall this is a careful, well-tested change: the privacy-critical machinery holds up. The no-op default contract, the ring<3 anonymous fail-closed guard, the pubkey canonicalization against alt-encoding ring duplicates, and the ring>2 attribution scrub are all correct and backed by behavioral + simulator tests with real mutation teeth. I couldn't find a correctness or security bug in the send/sign/scrub path. A handful of smaller things worth considering: Correctness (display-only)
Cleanup / consistency
UX (judgment call)
I put together a patch covering the six clear-cut items (everything except the menu renumber — that one's a UX call better left to you). It builds and passes the targeted |
Review — CLI 3-way attribution selector + curated decoysChecked out the exact head ( Structural note first: the PR body says "the walletapi half of this diff IS #22", but by commit ancestry this branch contains #22's first 8 commits but not its current head Findings (most severe first)1. Prompt-mode sends silently bypass session privacy — 2. "Session" privacy lasts exactly one send — 3. The PR violates its own O3 "never persist anonymize-intent to disk" invariant — 4. Ring-size lifecycle is broken both directions — 5. Dead recovery instruction at the exact failure moment — 6. Deroproof decoy slips prompt-time dedup (interacts with the stale stack) — 7. Anonymous ring gate + ring-identity re-implemented in a drifting second dialect (cleanup) — 8. UX-consistency bundle (cleanup)
Verification: exact head checked out and built/tested locally; findings surfaced via an 8-angle finder pass and individually verified against both this branch and the current #22 head (27 candidates confirmed, 0 refuted). Nothing here is a false-anonymity in the built tx — the engine's fail-closed guard holds; these are contract/UX seams and one signing-time reject. |
… (review #1) The ring loop collected distinct members through a persistent deduplicator with no bound on passes: a candidate pool smaller than the ring spins forever holding transfer_mutex — the balance probe can't error on non-zero SCIDs (unregistered accounts get synthesized zero balances), success needs a full ring, and the candidate stream stops yielding. 41+ curated decoys made it deterministic: the combined count passed the <=40 scarcity check, so the base-tree rescue never fired. Seven changes: - Tail-only threshold: the <=40 check measures len(members)-curated (curatedRingCandidates reports its validated count per pass) — curated decoys no longer mask a scarce tree; upstream semantics restored. - Stall rescue: <=40 is a size heuristic blind to the 41..ringsize-1 dead band (upstream also hung there). After 2 consecutive barren passes (no new distinct candidate) assembly switches to base-tree candidates — sticky, base-fetch-only per pass. - Pass bounds: rescue armed, 32 further consecutive barren passes or 8*ringsize total -> explicit retryable error; an empty final fetch reports daemon failure, not pool exhaustion (Random_ring_members swallows RPC errors into empty lists — this also ends upstream's latent spin on a persistently failing daemon). - Wall-clock bound: exponential barren backoff (250ms doubling to 4s; realizable window ~112s > the daemon's 90s 5-block activity filter, so a transiently filtered pool recovers before exhaustion is declared) plus a 150s per-transfer stall budget (barren_passes resets on any progress, so a trickling daemon could otherwise re-arm ~32 windows ≈ 60 min at ring 128). Per-transfer scope matches the per-transfer barren state. - MaxTransfersPerBuild = 256, checked before any mutex-held RPC: every per-transfer cost above multiplies by len(transfers), and the consensus 300KB tx limit can't bound that (enforced after assembly has paid). Not a new restriction: the smallest payload serializes ~1.7KB wire, above the 300*1024/256 = 1200-byte floor — longer arrays could never have broadcast. Floor pinned on a real wire-form tx. - 45s per-RPC deadline (CallWithTimeout) on every mutex-held daemon call: random members, balance fetches, and NameToAddress (pre-assembly, reachable at ring 2 — deadline-free it kept the hang reproducible with every assembly bound bypassed). Without it a daemon that accepts the socket but never replies parks the goroutine forever with IsDaemonOnline() still true. RPC count is bounded across the whole body: <=2 fee fetches; per transfer 1 probe, <=20 resolver fetches, <=1 name resolution, then pass-capped assembly. - 45s per-write websocket deadline (rwc.NewWithWriteTimeout): jrpc2 holds one client mutex across socket writes AND timeout delivery, so one write blocked on a half-open daemon freezes every call and its deadline for the kernel TCP retransmit timeout (~15+ min) — and the wallet's own deadline-free 5s connectivity pinger makes that writer a certainty. The blocked write now errors at 45s, gorilla poisons the connection, Keep_Connectivity reconnects. Daemon/explorer rwc users keep the historical constructor. Worst-case mutex hold: (min(len(transfers),256)+1) x (150s + count-bounded RPCs x 3x45s) — an absolute constant. Residual: wasm keeps the deadline-free nhooyr channel (browser websockets buffer in the JS runtime); rides the send-layer timeout follow-up. Tests, each red pre-fix on its target defect: ring_pool_exhaustion_test (41 decoys + empty token tree at ring 64: hang -> seconds via rescue); ring_scarce_band_test (~92-leaf token tree ON CHAIN at ring 128: tail-only alone errors "have 90 of 128", the rescue fills it); ring2_payload_floor_test (pins the 1200-byte floor on wire form); transfer_array_cap_test (offline: cap+1 fails closed pre-network, cap passes); rpc_call_timeout_test (never-answering daemon: probe, members, name resolution all return at the deadline); rwc_write_timeout_test (peer that never reads: the blocked write errors at the deadline, not the TCP retransmit timeout); ring_backoff_test (arithmetic pin: backoff window > 5x BLOCK_TIME filter window — the filter is disabled below topoheight 100, so no simulator run can exercise it). The exhaustion-error branch is unreachable on a healthy chain (the 1326-account genesis premine floors the base pool; the rescue always reaches it) — covered by inspection. Test_Payload_TX / Test_Creation_TX_morecheck fail identically at base (pre-existing).
…ecoy-probe failures (review DEROFDN#2/DEROFDN#3) DEROFDN#2 — at ring 2 the assembly loop ("for ringsize != 2") never runs, so curatedRingCandidates — the only decoy validator — is never invoked: Strict garbage decoys built and could broadcast while the identical input at ring 4 hard-errors. Fixed with a fail-closed guard beside the anon ring-2 guard (after effective-ringsize resolution — the wallet default can legally be 2 — and before any daemon call, so it validates offline). Strict-only: lenient has no ring-2/ring-4 asymmetry, and its documented silent-drop contract is load-bearing for the DEROFDN#23 CLI (Strict:false, ambient session decoys, any ring size). Lenient ring-2 drops feed the funnel below. DEROFDN#3 — the registration probe treated EVERY failure as "decoy invalid": for a decoy address the daemon's "Account Unregistered" verdict and a transport failure return as the same opaque error (the unregistered special-cases require self or non-zero SCID), so a transient blip in lenient mode silently stripped all curation — the user signed a fully random ring believing it curated. Probe errors are now classified via isUnregisteredError (the repo's existing detection idiom): unregistered verdict -> unchanged (Strict errors, lenient skips); any other failure -> hard error in BOTH modes ("could not verify … — retry the send"). No in-probe retry (Keep_Connectivity heals on ~5s; retry belongs at the send layer). A daemon that rewords the verdict fails closed. Strict's transient-failure message changes from "not registered" to "could not verify" (nothing pinned the old text). O9, same surface — decoy slot capacity: a ring holds at most ringsize-2 curated decoys, but nothing checked the supplied count: valid decoys beyond capacity were probed, then silently never placed. Strict over-supply is now a hard error (pre-guarded on the supplied count before any RPC, re-checked in the validator); lenient surplus drops through the funnel with a "no decoy slot left" record. Lenient ring-2 gets the same upgrade, so the former log-only residual now gets the default-verbosity summary. The guard caught an in-repo instance: the A2 finalization fixture supplied 8 Strict decoys at ring 8; two validated and were never placed — it now supplies exactly ring-2. Two hardenings on the same surface: - Lenient drops are never invisible: all four drop reasons (unparseable / own address / duplicate / unregistered verdict) funnel through one recorder — a per-decoy V(1) log and a default-verbosity reduced-curation summary before signing. Recording only the daemon-verdict class would keep the bug alive for the wallet-side classes (an all-duplicate ambient list from the hardcoded-lenient DEROFDN#23 CLI signs a random ring with zero signal on a healthy chain). TRUST NOTE on RingPreference: a malicious daemon can still veto lenient curation by lying "unregistered" — visible, not preventable wallet-side (it already controls random member selection); Strict is the fail-closed mode. - Probe cost bounded and flat: registration verdicts are memoized per build (canonical-base-address key, shared by both call sites) — one probe per distinct decoy per build instead of O(decoys) per pass under transfer_mutex. Sound: registration is permanent; a stale unregistered verdict only defers the decoy to the next send. PreferredDecoys is capped at 256 (2x max ringsize) — hard error in both modes; list length is caller cost, not decoy quality. Tests, each red pre-fix: curated_ring2_guard_test (offline: Strict garbage at ring 2 passed every validation pre-fix); curated_probe_classification_test (sim; client websocket killed so IsDaemonOnline() stays true and the failure lands inside the probe's RPC — pre-fix lenient returned curated=0, err=nil); curated_slot_cap_test (offline, memo-seeded: lenient surplus records, Strict surplus errors at guard and validator); curated_drop_signal_test (offline: all three wallet-side drops recorded with reasons, no double-count across passes, Strict records nothing); rpc_call_timeout_test gains the classification leg (a hung daemon's probe deadline -> "could not verify", never a silent drop). Keep_Connectivity deliberately not started in the probe test (no quit path). RingPreference / curatedRingCandidates docs updated where the fixes falsified them. Test_Payload_TX / Test_Creation_TX_morecheck fail identically at base (pre-existing), as does attribution_self_behavior_test.go's gofmt drift.
…team hardened) CLI surface for the DEROFDN#22 anonymous-attribution + curated-decoy primitives, rebased onto the fixed engine (canonicalization + ring-2 fail-closed) and hardened by adversarial review (ledger: redteam-cli-anonymous-decoys.md): - Fail-closed anonymity at ring<4 with a truthful HONEST/ANON report before confirm and after dispatch (no false sense of anonymity). (O1/O2/O8/O9) - Attribution/decoy output console-only — no on-disk intent leak. (O3) - Recipient-aware, BaseAddress()-canonical decoy validation mirroring the engine fix. (O5) cmd/dero-wallet-cli only; engine + all four hardened DEROFDN#22 tests preserved (take-theirs on the 6 walletapi/simulator files per review O4). Full build + CLI tests + engine attribution + curated-ring sim suite green.
Reworks the opt-in privacy CLI surface per design review (quickbrownfox + Azylem):
- Unify ring size, extra sender privacy, and curated decoys into ONE submenu
("Transaction Build Options", menu 7) — a single consistent place to configure
how the next transfer's ring is built, instead of ringsize-by-command +
privacy-by-menu.
- Remove the `set ringsize` and `set anonymous` prompt commands so the submenu is
the single config path (no colliding third way to set the same state).
- Option 5 (Transfer) is now a live build readout: "(default, ringsize N)" on the
plain path, or "(ringsize N) (advanced settings enabled)" in red once extra
privacy / decoys are engaged — so the user sees the build before committing.
- "Set first, then fire": after a transfer dispatches, all per-tx build settings
(privacy, decoys, ring size) reset to defaults so an advanced configuration
cannot silently persist into the next, unrelated send.
- Suppress the attribution line on a plain default (honest) send — a normal
transfer no longer carries an unsolicited "your sender is visible" notice; the
truthful post-send "landed in ring" line is shown only on an advanced send, and
the redundant pre-send "requested" line is dropped.
- Renumber the post-open menu to a clean sequence (1-8, 10-16; 9/0 reserved for
navigation) so the new option does not leave a numbering gap.
cmd/dero-wallet-cli only; engine + all hardened DEROFDN#22 tests preserved. New unit tests
cover the silent send path, the post-send reset, and the option-5 state label
(mutation-checked). Build + go test ./cmd/dero-wallet-cli green.
Add an advanced-only AttributionSelf mode that points the receiver-readable attribution byte at the sender's own ring slot (witness_index[0]) — the attribution field's original purpose, offered explicitly rather than by default. The receiver-pointed default stays untouched and un-overridable; self-pointing is reachable ONLY via the named AttributionSelf value. Engine (walletapi/transaction_build.go): - AttributionMode gains AttributionSelf; one build branch writes witness_index[0]. Works at any ring size (slot 0 always exists), so it is intentionally not subject to the anonymous ring<4 fail-closed guard. - Guardrail comment reframed: honest/default can never self-point; the sender slot is reachable only via the explicit value. CLI (cmd/dero-wallet-cli): - session attribution flag becomes a 3-way mode; menu option 2 cycles Default -> Anonymous -> Self -> Default. Landing on Self triggers a mandatory, permanent-self-doxx warning; declining leaves the mode unchanged. - Self bypasses the anonymous ring-size downgrade and is reported verbatim. The result line is ring-2-aware: at ring 2 the sender is already structurally provable (decode overrides sender_idx; SenderVerified=true for honest too), so the written byte adds nothing over honest; at ring>2 it is a permanent on-chain record recoverable by a non-blanking/future reader. - --anonymous seeds Anonymous only; Self is never reachable from a flag. Tests: - Test_AttributionSelf_WritesSenderSlot: build-path proof the byte points at the sender slot (mutation-proven: flip to [1] -> RED). - Test_AttributionSelf_EndToEnd_ScrubHolds: build->broadcast->mined->decode proof the DEROFDN#21 scrub still blanks the byte for a current wallet at ring>2. - 3-way cycle, downgrade-bypass, result-line, and label-suffix unit tests. The DEROFDN#21 scrub blanks the byte for current recipient wallets at ring>2 but does not make self "safe" — the raw on-chain byte is permanent. That permanence is why the warning is mandatory.
DHEBP
force-pushed
the
feat/cli-anonymous-decoys-rebased
branch
from
2938de4 to
c301caf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CLI surface for the opt-in attribution modes in #22. Reworks the Transaction Build Options menu into a 3-way sender-attribution selector (option 2 cycles DEFAULT -> ANONYMOUS -> SELF -> DEFAULT), adds curated-decoy entry, and a unified build-options menu; the default send path stays byte-identical (no prompt, honest/random). SELF lands behind a mandatory permanent-self-doxx warning and bypasses the ring-size downgrade (slot 0 always exists); ANONYMOUS fails closed to honest below ring 4; the result line reports the effective truth at each ring tier. --anonymous seeds ANONYMOUS only; SELF is reachable only via the warned in-session choice.
Stacked on #22 (it calls the engine modes added there) — the walletapi half of this diff IS #22; review/test together, merge after #22. Branch builds and runs end-to-end as-is (go build ./...; go test ./cmd/dero-wallet-cli/ ./walletapi/ green).