feat: port 5 vulnerability detection heuristics from am-i.exposed - #19
Conversation
68987d2 to
8b3d50c
Compare
| fn find_spending_tx(&self, source_txid: &Txid, source_vout: u32) -> Option<Txid> { | ||
| for txid in &self.our_txids { | ||
| if let Some(tx) = self.fetch_tx(txid) { | ||
| let spends_outpoint = tx.vin.iter().any(|vin| { | ||
| vin.previous_txid == *source_txid && vin.previous_vout == source_vout | ||
| }); | ||
| if spends_outpoint { | ||
| return Some(*txid); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
find_spending_tx iterates all transactions and all their inputs on every call. Peel chain detection calls it up to 6 times per chain, and toxic change calls it once per small output. For a wallet with hundreds of transactions this adds up.
Build a reverse spending index once in from_wallet_history():
// HashMap<(Txid, u32), Txid> — maps (parent_txid, vout) → spending txid
spending_index: HashMap<(Txid, u32), Txid>,
The data is already available — from_wallet_history() already iterates every transaction to build input_cache and output_cache. Adding entries to this map in the same loop costs nothing extra, and makes every find_spending_tx call O(1) instead of O(n×m).
It could become something like this:
| fn find_spending_tx(&self, source_txid: &Txid, source_vout: u32) -> Option<Txid> { | |
| for txid in &self.our_txids { | |
| if let Some(tx) = self.fetch_tx(txid) { | |
| let spends_outpoint = tx.vin.iter().any(|vin| { | |
| vin.previous_txid == *source_txid && vin.previous_vout == source_vout | |
| }); | |
| if spends_outpoint { | |
| return Some(*txid); | |
| } | |
| } | |
| } | |
| None | |
| } | |
| fn find_spending_tx(&self, source_txid: &Txid, source_vout: u32) -> Option<Txid> { | |
| self.spending_index.get(&(*source_txid, source_vout)).copied() | |
| } |
|
|
||
| // ── 13. Dust Attack Detection ────────────────────────────────────────── | ||
| // | ||
| // Port of: am-i-exposed/src/lib/analysis/chain/backward.ts | ||
| // | ||
| // Detects when our wallet received a tiny UTXO from a probable dust | ||
| // attack transaction. A dust attack parent typically has ≥10 outputs, | ||
| // ≥5 of which are ≤ 546 sats, distributed to many distinct addresses. | ||
|
|
||
| fn detect_dust_attack(&self, findings: &mut Vec<Finding>) { | ||
| const MIN_OUTPUTS: usize = 10; | ||
| const DUST_THRESHOLD: u64 = 546; | ||
| const MIN_DUST_OUTPUTS: usize = 5; | ||
|
|
||
| // Check receiving transactions only (we didn't create them). | ||
| let txids: Vec<Txid> = self.our_txids.iter().copied().collect(); | ||
| for txid in txids { | ||
| let input_addrs = self.get_input_addresses(&txid); | ||
| let has_our_inputs = input_addrs.iter().any(|ia| self.is_ours(&ia.address)); | ||
| if has_our_inputs { | ||
| continue; // Skip our own sends | ||
| } | ||
|
|
||
| let outputs = self.get_output_addresses(&txid); | ||
| if outputs.len() < MIN_OUTPUTS { | ||
| continue; | ||
| } | ||
|
|
||
| let dust_outputs: Vec<_> = outputs | ||
| .iter() | ||
| .filter(|o| o.value.to_sat() <= DUST_THRESHOLD) | ||
| .collect(); | ||
| if dust_outputs.len() < MIN_DUST_OUTPUTS { | ||
| continue; | ||
| } | ||
|
|
||
| let unique_addrs: HashSet<String> = outputs | ||
| .iter() | ||
| .map(|o| o.address.assume_checked_ref().to_string()) | ||
| .collect(); | ||
| let diversity = unique_addrs.len() as f64 / outputs.len() as f64; | ||
| if diversity < 0.8 { | ||
| continue; | ||
| } | ||
|
|
||
| // Our wallet received from this dust attack tx | ||
| let our_outs: Vec<_> = outputs | ||
| .iter() | ||
| .filter(|o| self.is_ours(&o.address)) | ||
| .collect(); | ||
| if our_outs.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| findings.push(Finding { | ||
| vulnerability_type: VulnerabilityType::DustAttack, | ||
| severity: Severity::Critical, | ||
| description: format!( | ||
| "TX {} is a likely dust attack: {} outputs, {} of which are ≤{} sats, \ | ||
| targeting {} unique addresses", | ||
| txid, | ||
| outputs.len(), | ||
| dust_outputs.len(), | ||
| DUST_THRESHOLD, | ||
| unique_addrs.len() | ||
| ), | ||
| details: Some(json!({ | ||
| "txid": txid.to_string(), | ||
| "total_outputs": outputs.len(), | ||
| "dust_outputs": dust_outputs.len(), | ||
| "unique_addresses": unique_addrs.len(), | ||
| "diversity_ratio": (diversity * 100.0).round() as u32, | ||
| "our_received": our_outs.iter().map(|o| { | ||
| json!({ | ||
| "address": o.address.assume_checked_ref().to_string(), | ||
| "sats": o.value.to_sat() | ||
| }) | ||
| }).collect::<Vec<_>>(), | ||
| })), | ||
| correction: Some( | ||
| "Do NOT spend this dust UTXO — spending it reveals your other UTXOs \ | ||
| via common-input-ownership. Freeze it in your wallet immediately." | ||
| .into(), | ||
| ), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Detector 13 (Dust Attack) flags a subset of what 3 (Dust) already catches — every dust attack UTXO is also a dust UTXO. Having both fire for the same output is confusing for the user ("is this dust or a dust attack?") and means we maintain two separate detection paths for overlapping cases.
Could you make Dust Attack run inside the Dust detector instead? When the Dust detector finds a dust UTXO, check if the parent transaction matches the dust attack pattern (≥10 outputs, ≥5 dust, high address diversity). If it does, report it as DUST_ATTACK instead of DUST. One finding per output, and the user gets the most specific diagnosis.
| } else if ambiguity >= 0.6 { | ||
| warnings.push(Finding { | ||
| vulnerability_type: VulnerabilityType::DeterministicLink, | ||
| severity: Severity::Low, | ||
| description: format!( | ||
| "TX {} has good ambiguity ({:.0}%, {} valid interpretations)", | ||
| txid, | ||
| ambiguity * 100.0, | ||
| total_valid | ||
| ), | ||
| details: Some(json!({ | ||
| "txid": txid.to_string(), | ||
| "total_valid_interpretations": total_valid, | ||
| "ambiguity_pct": (ambiguity * 100.0).round() as u32, | ||
| })), | ||
| correction: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The "good ambiguity" warning fires when a transaction has ≥60% ambiguity — that's a positive signal, not a problem. Warnings should flag risks. Seeing "8 warnings" and discovering some are compliments is confusing.
Flip the logic: warn when ambiguity is low (e.g. <40%) but not fully deterministic. That's the risky middle ground — the link isn't proven, but an observer can make a strong guess. High ambiguity transactions don't need any output.
| const TOXIC_UPPER: u64 = 10_000; | ||
| const DUST_LOWER: u64 = 546; |
There was a problem hiding this comment.
These thresholds should live in DetectorThresholds in model/src/config.rs so we could keep all tuning values in one configurable place
| let child_txid = self.find_spending_tx(&trace_txid, trace_vout); | ||
| let child_txid = match child_txid { | ||
| Some(t) => t, | ||
| None => break, | ||
| }; |
There was a problem hiding this comment.
| let child_txid = self.find_spending_tx(&trace_txid, trace_vout); | |
| let child_txid = match child_txid { | |
| Some(t) => t, | |
| None => break, | |
| }; | |
| let Some(child_txid) = self.find_spending_tx(&trace_txid, trace_vout) else { | |
| break; | |
| }; |
| let child_txid = self.find_spending_tx(&txid, out.index); | ||
| let child_txid = match child_txid { | ||
| Some(t) => t, | ||
| None => continue, | ||
| }; |
There was a problem hiding this comment.
| let child_txid = self.find_spending_tx(&txid, out.index); | |
| let child_txid = match child_txid { | |
| Some(t) => t, | |
| None => continue, | |
| }; | |
| let Some(child_txid) = self.find_spending_tx(&txid, out.index) else { | |
| continue; | |
| }; |
Integrates CLI crate from PR stealth-bitcoin#18 with the new vulnerability detectors. Resolves model/src/config.rs by keeping the satsfy DetectorId / enabled_detectors infrastructure alongside the DetectorThresholds layout from main.
`find_spending_tx` walked `our_txids` on every call, making the peel-chain detector O(N*M) (up to 6 traversals per chain) and the toxic-change detector O(N) per small output. Build a `HashMap<(parent_txid, vout), child_txid>` once during `from_wallet_history()` — the inputs are already iterated to populate `input_cache`, so this is essentially free. Replace the two `find_spending_tx` call sites with `spending_index.get(...)` lookups and delete the helper. The index is restricted to entries whose child transaction touches the wallet, preserving the previous behaviour of the helper (which only scanned `our_txids`).
Detector 13 (Dust Attack) was a strict subset of Detector 3 (Dust): every dust-attack UTXO is, by construction, also a dust UTXO, so both detectors fired on the same outputs and emitted two findings. Fold the attack check into `detect_dust` via a `dust_attack_evidence` helper. When the dust UTXO's parent transaction matches the attack signature (≥10 outputs, ≥5 dust outputs, ≥80% address diversity, no inputs of ours), escalate the existing Dust finding to `Critical` and attach the attack evidence under `details.dust_attack` instead of emitting a separate finding. Unregister `detect_dust_attack` from `detect_all`. Update the integration test to assert the new shape. `DetectorId::DustAttack` and `VulnerabilityType::DustAttack` are left in place to avoid breaking downstream consumers of the model crate; they are simply no longer produced by the engine.
The deterministic-link detector emitted a warning at `ambiguity >= 0.6` (60% or higher), describing it as "good ambiguity". That is a positive privacy signal — warnings should flag *risks*, not commend good behaviour. Flip the predicate so we warn only when ambiguity is in the `(0%, 40%)` window: the transaction has no fully deterministic link (otherwise the finding-side branch fires) but the input→output mapping is still largely guessable. High-ambiguity transactions now emit nothing. Update the description and add a corrective recommendation.
The vulnerability table claimed "12 detectors" while listing 17 rows. After commit 2 of the PR stealth-bitcoin#19 review the dust-attack detector was folded into the Dust detector, leaving 16 detectors actually registered in `detect_all`: address_reuse, cioh, dust, dust_spending, change_detection, consolidation_origin, script_type_mixing, cluster_merge, lookback_depth, exchange_origin, tainted_utxos, behavioral_fingerprint, peel_chain, deterministic_links, unnecessary_input, toxic_change. Update the count to 16, drop the standalone `DUST_ATTACK` row, and note that the `DUST` detector now escalates to CRITICAL when the parent transaction matches a dust-attack pattern. Also rewrite the DETERMINISTIC_LINK warning row to reflect the flipped semantics.
The new vulnerability detectors hard-coded their tuning knobs as
`const` items inside the detector function bodies (peel-chain max
hops, ratio cut-off, critical-hop threshold; toxic-change bounds;
dust-attack diversity floor; low-ambiguity warning cut-off).
Hoist those into `DetectorThresholds` so an operator can tune the
engine through a single config object:
* `peel_chain_max_hops`, `peel_chain_min_hops`,
`peel_chain_critical_hops`, `peel_chain_ratio`
* `low_ambiguity_cutoff`
* `dust_attack_diversity`
* `toxic_change_lower` (the previously implicit 546-sat floor)
Thread `&DetectorThresholds` through `detect_peel_chain`,
`detect_deterministic_links` and `detect_toxic_change`, all of which
previously ignored the argument. `Eq` is dropped from `DetectorThresholds`
and `AnalysisConfig` because `f64` does not implement `Eq`; `PartialEq`
is retained.
The peel-chain trace loop now uses an `Option::copied` lookup through
`spending_index`. Replace the `match … { Some(t) => t, None => break }`
shape with `let … else { break }` — it expresses the early-exit intent
directly and drops two lines of plumbing.
Mirror the previous commit on the toxic-change call site: replace the
`match … { Some(t) => t, None => continue }` shape with
`let … else { continue }`. The Option lookup now lives on one line and
the early-exit reads directly.
LORDBABUINO
left a comment
There was a problem hiding this comment.
All review points addressed: reverse spending index in TxGraph, dust-attack folded into the Dust detector with severity escalation, low-ambiguity warning flip, detector magic numbers moved to DetectorThresholds, let-else cleanups, and README/rustdoc detector counts corrected to 16. Merged current main and verified rustfmt, clippy and the full test suite locally; CI green.
Depends on #16 (it will be in draft mode until its merged)
Ports 5 privacy heuristics inspired by am-i-exposed.
New privacy heuristics in this PR:
Reviewer Notes
Run integration tests with
cargo test.