feat(walletapi): opt-in attribution modes (anonymous, self) + curated decoy rings#22
feat(walletapi): opt-in attribution modes (anonymous, self) + curated decoy rings#22DHEBP wants to merge 12 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).
Dirtybird99
left a comment
Dirtybird99
left a comment
There was a problem hiding this comment.
Reviewed the anonymous-attribution + curated-decoy changes across correctness, security, test, and product lenses. The additive ...WithOptions shim design and the backward-compatible rpc.Entry wire change are genuinely clean and worth preserving as-is. That said, I found one correctness issue that fails after signing, a security combination that re-creates a capability the design comment declares out of scope, and a safety-signal (SenderVerified) that doesn't reach any real consumer. Flagging as comments rather than a hard block, but I'd treat #1–#3 as must-fix before this is usable. Details inline; happy to discuss any of them.
| continue | ||
| } | ||
| seen[d] = true | ||
| alist = append(alist, d) |
There was a problem hiding this comment.
🔴 #1 correctness — fails after signing. Preferred decoys are deduped/validated by raw string (seen[d], append(alist, d)) and the assembly deduplicator (:402-404,:424) is string-keyed, but the actual ring member is the public key (:443). A decoy supplied in an alternate encoding (integrated / payment-id) of an account already in the ring — including the sender, or the recipient (never checked here; only d == self at :99) — slips past both string dedups and the :428 guard, placing the same pubkey twice. Consensus then rejects at transaction_verify.go:308 after the user has signed — the exact "passes wallet, rejects at consensus" failure the registration probe exists to prevent. The receiver path already canonicalizes via addr.BaseAddress() (:380,:394,:403); do the same here and seed seen with the sender and recipient base keys.
| // witness_index[2:] are the anonymity-set (decoy) slots: real, registered | ||
| // ring members that are neither the sender [0] nor the receiver [1]. | ||
| // Pointing attribution at one reduces the receiver to 1-of-N ring anonymity. | ||
| decoyPos := 2 + crand.Intn(len(witness_index)-2) |
There was a problem hiding this comment.
🔴 #2 security — framing primitive + opt-in tell. The doc says targeted attribution is "intentionally NOT defined… targeted impersonation, not privacy" — but anonymous + curation re-creates exactly that. Curate a single decoy at ring 3: decoyPos := 2 + crand.Intn(1) is forced to that one slot, so the sender deterministically points the attribution byte at a chosen real registered third party, and the receiver/exchange/UI renders "from X." Separately, because honest always points the byte at the receiver's own slot and anonymous points elsewhere, the receiver detects opt-in with one equality (sender_idx == j) — a tell that shrinks the very anonymity set it grows. Consider randomizing decoyPos independently of the curated set, and/or refusing sender-chosen attribution while curation is active.
| // the only other ring member is necessarily the sender — nothing is attested by | ||
| // the protocol). For ring size > 2 the sender chose the unauthenticated | ||
| // attribution byte and entry.Sender MUST NOT be trusted. | ||
| SenderVerified bool `json:"sender_verified"` |
There was a problem hiding this comment.
🔴 #3 security — SenderVerified is fail-open. entry.Sender is always populated, even when meaningless (at ring>2 honest mode it resolves to the receiver's own address; in anonymous mode to a sender-chosen decoy). Existing consumers have no reason to check the new bool: Entry.String() (:118) still prints a bare Sender: line and never renders SenderVerified/RingSize, and the Get_Transfers sender filter (:264) matches on Sender unconditionally. The field's whole purpose ("MUST NOT be trusted for ring>2") is defeated for anyone not parsing raw JSON. Leave Sender empty/sentinel unless verified, annotate it in String() (e.g. Sender: <addr> (UNVERIFIED — ring N)), and audit every reader.
| } | ||
|
|
||
| if sender_idx <= uint(tx.Payloads[t].Statement.RingSize) { | ||
| if sender_idx < uint(tx.Payloads[t].Statement.RingSize) { // off-by-one fix: valid indices are 0..RingSize-1 |
There was a problem hiding this comment.
🟠 #4 security — bounds vs Publickeylist length → MITM DoS. This tightens the guard to sender_idx < RingSize, but it indexes Publickeylist[sender_idx], and Publickeylist is rebuilt at :866-873 by decompressing each key and skipping on failure — so len(Publickeylist) can be < RingSize. A crafted byte in [len, RingSize) passes the guard and panics. It's recover()-guarded in SyncHistory (:543) and unreachable via consensus-valid txs, but a hostile/MITM daemon can serve a tx with an undecompressable ring key to repeatedly abort history sync (availability DoS). Bound against len(Publickeylist) (or reject when len != RingSize) here and at the CBOR_V2 arm (:1116).
| // TransferPayload0WithOptions is the additive variant carrying opt-in transfer | ||
| // privacy knobs (sender-attribution mode, decoy curation). A zero-value | ||
| // TransferOptions reproduces TransferPayload0 exactly. | ||
| func (w *Wallet_Memory) TransferPayload0WithOptions(transfers []rpc.Transfer, ringsize uint64, transfer_all bool, scdata rpc.Arguments, gasstorage uint64, dry_run bool, opts TransferOptions) (tx *transaction.Transaction, err error) { |
There was a problem hiding this comment.
🟠 #5 product — feature ships dark. TransferOptions / AttributionAnonymous / RingPreference are reachable only through this Go method — there's no JSON-RPC transfer param, no CLI flag, no config. The audience that drives the wallet via wallet-rpc / the CLI wallet cannot opt in, so the user-facing privacy benefit is currently unreachable. Either wire an optional options object into the RPC transfer params (+ a CLI flag), or scope the PR title/description as "Go-API primitive; RPC surface to follow" so reviewers aren't misled on deliverable scope.
| // GUARDRAIL: never witness_index[0] (the real sender) — that would reveal | ||
| // the true sender to the receiver on every transfer. | ||
| attrIndex := witness_index[1] | ||
| if opts.Attribution == AttributionAnonymous && len(witness_index) > 2 { |
There was a problem hiding this comment.
🟡 #8 API contract — silent no-op at ring 2. AttributionAnonymous silently falls through to honest witness_index[1] at ring 2 (the guard requires len(witness_index) > 2), with no error or signal — and the receiver decode then overrides to the counterparty and sets SenderVerified=true, so the caller broadcasts a verifiably attributed transfer while believing it's anonymized. Anonymity is impossible at ring 2, so reject the AttributionAnonymous + ring-2 combination with an explicit error rather than honoring neither.
| t.Fatalf("cannot read transaction_build.go: %v", err) | ||
| } | ||
|
|
||
| // The honest default assignment for the attribution byte index. |
There was a problem hiding this comment.
🟡 #9 test quality — brittle source-grep guard. This grep guard is brittle both ways. False negative: it matches the := literal at transaction_build.go:241, but the anonymous branch reassigns attrIndex two lines down (:247, with =) — a regression reassigning to witness_index[0] below the matched line ships green. False positive: a semantics-preserving refactor (attrIndex := honestAttrIndex(...)) fails CI, contradicting the docstring's claim that it "cannot be defeated by an unrelated refactor." A behavioral test that builds a tx in honest mode and asserts the decrypted leading byte equals the receiver slot pins the actual invariant.
| probable_members := w.Random_ring_members(transfers[t].SCID) | ||
| // curated preferred decoys (if any) go first; random members top up. With no | ||
| // RingPreference this returns exactly Random_ring_members(transfers[t].SCID). | ||
| probable_members, cerr := w.curatedRingCandidates(transfers[t].SCID, opts.Ring) |
There was a problem hiding this comment.
🟡 #10 efficiency. curatedRingCandidates runs at the top of for ringsize != 2 (re-iterable) and inside for t := range transfers, re-issuing one GetEncryptedBalanceAtTopoHeight per decoy each pass even though decoy validation is loop-invariant. The <=40 branch (:414) discards this call's result wholesale (:421) and couples the curation count to an unrelated daemon-pool heuristic; for a zero-SCID transfer each decoy is also probed again at :433. Hoist decoy validation above both loops and carry the curated prefix into the fallback instead of recomputing.
| addr.Mainnet = w.GetNetwork() | ||
| entry.Sender = addr.String() | ||
| } | ||
| entry.RingSize = uint64(tx.Payloads[t].Statement.RingSize) |
There was a problem hiding this comment.
🟡 #11 maintainability — copy-pasted decode arms. This RingSize/SenderVerified block (and the bounds guard + ring-2 override above it) is byte-for-byte identical to the CBOR_V2 arm at :1116-1123. Every decode-side fix above — the len-bound in #4, the Sender gating in #3 — must be applied to both arms or they silently diverge (the same class of bug the off-by-one fix just corrected). Factor an assignSender(entry, statement, senderIdx, network) helper and call it from both.
| } | ||
| if bal, _ := wsrc.Get_Balance(); bal == 0 { | ||
| t.Fatalf("sender has zero balance after funding; cannot send an Amount>=1 carrier") | ||
| } |
There was a problem hiding this comment.
🟢 #12 CI reliability. The new simulator tests are flaky under CI: wall-clock time.Sleep (:145,:373), fixed os.TempDir DB paths, and mutated package globals (globals.Arguments, config.Testnet/Mainnet.Genesis_*) make them unsafe to run concurrently / with -parallel. Use t.TempDir(), poll-until-synced with a deadline instead of fixed sleeps, and gate behind a build tag or a -short skip.
|
Follow-up to my review above — cross-checked against the PR description, the commit messages, and your in-code scope notes, to be fair about what's already flagged vs. what's new. Nothing retracted; just context. Already acknowledged in the PR (treat as "you know this," not a discovery):
Two findings that contradict an explicit guarantee in your commits (the ones I'd weigh most):
Consistent with your stated intent (not a contradiction):
The rest of the findings are unaffected by the description/commits. |
…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)
Dirtybird99
left a comment
Dirtybird99
left a comment
There was a problem hiding this comment.
Re-reviewed 3dfd70d against the earlier comments. This closes #1's integrated/payment-id and cross-network (dero↔deto) vectors (and mutation-tests them nicely), ports the #3/#9 scrub + the behavioral honest-byte guard, fails closed on ring-2 anon (#8), and the #4 bounds claim holds — Publickeylist[sender_idx] is safe because NewAddress panics on a bad ring key before the rebuild, so len == RingSize. Three follow-ups inline: one correctness gap that reopens #1 through the deroproof HRP axis the fix doesn't normalize (#13, I'd treat as must-fix), a duplication hazard (#14), and a test-path bug (#15). Minor: the new ring-2 guard reads ringsize < 3 while its message says “>= 4” — equivalent under the power-of-2 rule, but < 4 matches the message; and #4's safety is invariant-dependent, so a len(Publickeylist) != RingSize skip in the rebuild would harden it structurally rather than by assumption.
| // the seen-map and self/recipient checks compare pubkeys, not network-tagged strings. | ||
| canon := addr.BaseAddress() | ||
| canon.Mainnet = w.GetNetwork() | ||
| base := canon.String() // canonical identity for distinctness |
There was a problem hiding this comment.
🔴 #13 correctness — deroproof axis reopens #1, rejects after signing. The canonicalization closes the integrated/payment-id and cross-network (dero↔deto) vectors, but it pins only Mainnet. BaseAddress()→Clone() preserves Proof (rpc/address.go:97) and MarshalText forces hrp = "deroproof" whenever Proof is set (rpc/address.go:53-54), overriding the network. So a PreferredDecoy supplied as a deroproof… encoding of the recipient, the sender, or another decoy canonicalizes to a deroproof… string that misses the dero…/deto… seeds in seen{} (:99,:114), clears the registration probe at :128 (same registered pubkey), and lands the same pubkey in the ring twice — transaction_verify.go then rejects it after the user has signed, exactly the #1 failure, via the one HRP axis neither the fix nor the 4-vector test (curated_ring_altencoding_test.go) covers. Root cause: distinctness is keyed on the stringified address, so every HRP flag has to be hand-normalized one at a time. Key seen/self/recipientBase on the raw addr.PublicKey.EncodeCompressed() (the 33-byte pubkey consensus actually dedups on) and Mainnet, Proof, integrated, and any future axis collapse together — which also removes the caller-side recipient pin (:429) that otherwise must stay in lockstep with this block.
There was a problem hiding this comment.
Fixed in 2b30d561 — keyed dedup on the raw 33-byte pubkey (EncodeCompressed) instead of the stringified address, so Mainnet/Proof/integrated collapse to one identity; clear Proof on the emitted base so a deroproof decoy resolves to a registered base. New deroproof-decoy-alt test mutation-proves it (revert canon.Proof=false → RED, "axis is OPEN"). Also dropped the dead recipient Mainnet pin as you noted.
| // it re-derives the claimed sender via the public Publickeylist even after | ||
| // entry.Sender is blanked. So we blank entry.Sender AND zero payload[0] in the | ||
| // copy that feeds entry.Data. The verified case (ring 2, structural) is untouched. | ||
| exported_payload := tx.Payloads[t].RPCPayload |
There was a problem hiding this comment.
🟡 #14 maintainability — scrub + self-trust copy-pasted across both decode arms. This unverified-attribution scrub is byte-for-byte identical in the CBOR_V2 arm (:1151), and the self-authored RingSize/SenderVerified = true block is duplicated across the two self-send arms (:990, :1019). Both are correct today, but a future change to the scrub — e.g. zeroing another sender-derivable byte, or correcting the !entry.SenderVerified condition — applied to one arm but not the other silently re-opens the #3 sender-deanonymization leak for whichever encoding (CBOR vs CBOR_V2) is missed, and the source-grep guard can't force a new arm to call it. Factor one scrubExportedPayload(entry, decrypted) (and a sibling self-trust helper) invoked from every decode arm so a missing call is impossible.
There was a problem hiding this comment.
Fixed in 2b30d561 — factored scrubExportedPayload() + markSelfAuthored() as the single source of truth, called from every decode arm (zero inline copies remain). Byte-identical; the scrub behavioral test is unchanged and still green.
| t.Fatalf("Cannot create encrypted wallet, err %s", err) | ||
| } | ||
|
|
||
| wgenesis, err := Create_Encrypted_Wallet_From_Recovery_Words(wdst_temp_db, "QWER", "perfil lujo faja puma favor pedir detalle doble carbón neón paella cuarto ánimo cuento conga correr dental moneda león donar entero logro realidad acceso doble") |
There was a problem hiding this comment.
🟢 #15 test — wgenesis reuses wdst_temp_db. This passes wdst_temp_db (the receiver's DB path) instead of its own file, so the genesis and receiver wallets share one on-disk DB — clobber / “wallet already exists” risk, and the defer os.Remove(wdst_temp_db) only cleans the one path while the second open leaks. Give wgenesis its own temp path (e.g. wgenesis_temp_db).
There was a problem hiding this comment.
Fixed in 2b30d561 — wgenesis now uses its own temp DB path with its own cleanup.
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.
…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.
DHEBP
changed the title
…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.
|
Thanks for the thorough re-review — all 15 addressed. First round (#1–#12): closed in
Heads-up: |
Review — opt-in attribution modes (anonymous, self) + curated decoy ringsChecked out the exact head ( Findings (most severe first)1. Curated decoys can hang the wallet (deadlock, holds 2. 3. Lenient curation silently abandoned on a transient daemon error — 4. Unknown-RPCType outgoing entries never marked self-authored (plausible; not reachable today) — 5. Per-decoy registration probe runs up to 3× (efficiency) — 6. Anonymous→honest degrade guarded one layer above where it happens (altitude) — 7. Trust-polarity block duplicated per decode arm (simplification) — 8. Test-suite cleanup bundle (cleanup)
Refuted (checked, not real): the Verification: exact head checked out and built/tested locally (8/8 tests pass individually); findings surfaced via an 8-angle finder pass and individually verified against the checkout (27 candidates confirmed, 7 refuted). The engine's fail-closed guard holds — no false-anonymity ships in a built tx; findings 1–3 are a deadlock and two silent contract violations on the new curated path. |
… (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.
|
Thanks — #1–#3 fixed and adversarially hardened: #1 (curated-decoy spin): both halves of your "and/or", plus the holes a red-team pass found in them:
#2 (Strict void at ring 2): fail-closed guard beside the anon ring-2 guard — after effective-ringsize resolution (the wallet default can legally be 2), before any daemon call. Strict-only by design: lenient has no ring-2/ring-4 asymmetry (it never errors on bad decoys at any ring) and its documented silent-drop contract is load-bearing for the #23 CLI. All lenient drops now feed one four-reason funnel with a default-verbosity reduced-curation summary; a programmatic signal rides the "report effective curation" follow-up. #3 (silent curation abandonment): probe errors classified ( #5 (3× re-probe): largely subsumed — per-build verdict memoization (registration is monotonic; verdicts frozen at build start) + a 256-decoy cap: the registration probe now runs once per decoy per build. Proof: eleven pinning tests, each pre-fix red / post-fix green. Headline three: Deliberately-scoped residuals: wasm keeps the deadline-free websocket write path (browser websockets buffer in the JS runtime — rides the send-layer-timeout follow-up); the 5-block activity filter can't be exercised in the sim (disabled below topo 100 — covered by the arithmetic pin plus the on-chain stall-rescue test). Term nit: unbounded spin holding Heads-up: the new tests self-bound and share the fixed rpcport — same run-individually caveat as the existing sim tests. |
…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.
…line The last transfer-path call still on a bare Call. A half-open daemon parked the send goroutine for the kernel TCP retransmit timeout (~15+ min); the write-deadline can't bound it (the write already succeeded). No lock held, so a goroutine leak, not a wallet freeze. Now shares walletDaemonCallTimeout.
|
Pushed Believe that's the last open item on #22. #23 will need a rebase onto |
2 participants
Adds opt-in
TransferOptionsfor sender-attribution control through additive...WithOptionsvariants, so every existing caller keeps today's exact honest/random behavior. Three modes: anonymous (points the attribution byte at a decoy ring slot; needs ring >= 4, fails closed at ring 2), self (points the byte at the sender's own slot witness_index[0], the inverse of the receiver-pointed default; advanced-only, any ring size, unreachable from the default path), and curated decoy rings (user-supplied members, canonicalized and deduped). Behavioral + end-to-end tests prove the self byte points at the sender slot (mutation-proven) and that the ring>2 unverified-sender scrub still blanks it on decode. Builds on the receiver-honesty change in #21, which is what makes anonymous and self safe to expose.