chore: wallet client recovery privacy improvements #5697

dpc · 2024-07-19T07:13:43Z

See comments in the code.

dpc · 2024-07-19T07:14:08Z

ATM. This needs another round of self-review, but is probably in reviewable state.

dpc · 2024-07-19T07:14:36Z

fedimint-bitcoind/src/lib.rs

@@ -64,24 +61,16 @@ static BITCOIN_RPC_REGISTRY: Lazy<Mutex<BTreeMap<String, DynBitcoindRpcFactory>>
 pub fn create_bitcoind(config: &BitcoinRpcConfig, handle: TaskHandle) -> Result<DynBitcoindRpc> {
    let registry = BITCOIN_RPC_REGISTRY.lock().expect("lock poisoned");

-    let kind = env::var(FM_FORCE_BITCOIN_RPC_KIND_ENV)


Ooops. I didn"t want to make this change.

elsirion

The algorithm likely skips scripts that could have received money

elsirion · 2024-07-29T10:43:41Z

modules/fedimint-wallet-client/src/backup/recovery_history_tracker.rs

+    // To avoid updating `decoys` for the whole recovery, which might be a lot of extra updates
+    // most of which will be thrown away, ignore script pubkeys from before this `session_idx`
+    decoy_session_threshold: u64,


nit: is writing to a vector really that expensive? The complexity doesn"t feel worth it.

The cost of a cloning is more of a problem, potentially. Also, we really only want some recently used addresses as a decoy, as we are going to check only few (potentially) last used addresses. An attacker might see through things if we query some much older addresses.

Since I might need to query old addresses anyway, maybe I"ll just replace it with some probabilistic thing to potentially keep some older addresses around...

modules/fedimint-wallet-client/src/backup/recovery_history_tracker.rs

modules/fedimint-wallet-client/src/backup.rs

elsirion · 2024-08-20T13:40:04Z

modules/fedimint-wallet-client/src/backup.rs

+        if used_tweak_idxes.contains(&cur_tweak_idx) {
+            debug!(target: LOG_CLIENT_MODULE_WALLET,
+                tweak_idx=%cur_tweak_idx,
+                "Skipping checking history of an address, as it was previously used");
            last_used_idx = Some(cur_tweak_idx);
-            tweak_idxes_with_pegins.insert(cur_tweak_idx);
+        } else {
+            let history = check_addr_history(cur_tweak_idx).await?;
+
+            if !history.is_empty() {
+                last_used_idx = Some(cur_tweak_idx);
+                tweak_idxes_with_pegins.insert(cur_tweak_idx);
+            }


The amount of mutability here makes it quite hard to review, I was procrastinating on reviewing this PR for way too long due to it. Let"s discuss during review club, I think there is a beautiful, small algorithm hidden in here that wants to get implemented with some iterator chaining 😆

call: could loop over a range

@elsirion Hmm... now that I"m looking at it ... if I do while or for, I can"t use break value; which will introduce a different mutable value.

elsirion · 2024-08-20T16:34:25Z

modules/fedimint-wallet-client/src/backup.rs

+            = recover_scan_idxes_for_activity(
+                self.state.next_unused_idx_from_backup,
+                self.state.tracker.used_tweak_idxes(),


Eventually we should scan from index zero to not miss anything, but we need the already claimed list in the backup to do so privately.

main point addressed

dpc · 2024-08-21T19:15:47Z

@elsirion @bradleystachurski Just went ahead and implemented the better backup for wallet as we"ve discussed during the review.

elsirion

I think the final version we settled on is good, I"m trying to simplify that one fn nonetheless, see my take here: https://github.com/fedimint/fedimint/pull/5903/files/30ccddc406fe318d941d39350a7cd77d21414c8e..3bd09478781a718bf051c74d9725bd585d8fd4d3

elsirion · 2024-08-22T09:48:21Z

modules/fedimint-wallet-client/src/backup.rs

            last_used_idx = Some(cur_tweak_idx);
-            tweak_idxes_with_pegins.insert(cur_tweak_idx);
+        } else {
+            let history = check_addr_history(cur_tweak_idx).await?;


While rewriting this fn I noticed that we might be losing a lot of progress here depending on the retry behavior of check_addr_history. We are very dependent on it never failing here.

See comments in the code. Fix fedimint#5584

Inspired by fedimint@3bd0947

dpc · 2024-08-22T18:18:09Z

@elsirion I took most of #5903 and altered it to make it work. I will squash after you review approve.

dpc · 2024-08-23T16:40:46Z

Seems like a legitimate flake https://github.com/fedimint/fedimint/actions/runs/10528393578/job/29173781412 , will try to repro locally.

dpc · 2024-08-23T20:58:06Z

@elsirion CI flakiness exposed a place I"ve missed. See commit.

bradleystachurski · 2024-08-23T21:57:07Z

fedimint-cli/src/lib.rs

@@ -794,10 +793,7 @@ impl FedimintCli {
                // TODO: until we implement recovery for other modules we can't really wait


[nit] Can also remove this TODO

dpc requested a review from a team as a code owner July 19, 2024 07:13

dpc commented Jul 19, 2024

View reviewed changes

dpc force-pushed the 24-07-15-wallet-module-recovery-privacy branch 4 times, most recently from 58c3d8d to 87ba9eb Compare July 19, 2024 16:43

dpc requested a review from elsirion July 27, 2024 03:18

elsirion previously requested changes Jul 29, 2024

View reviewed changes

dpc force-pushed the 24-07-15-wallet-module-recovery-privacy branch 2 times, most recently from 6a7f406 to bd66153 Compare August 9, 2024 19:10

dpc requested review from elsirion and bradleystachurski August 9, 2024 19:37

elsirion reviewed Aug 20, 2024

View reviewed changes

dpc requested a review from elsirion August 21, 2024 19:15

elsirion previously approved these changes Aug 22, 2024

View reviewed changes

dpc and others added 6 commits August 22, 2024 11:16

chore: wallet client recovery privacy improvements

1960a76

See comments in the code. Fix fedimint#5584

fix(wallet-client): recovery potentially missing some pegins

febc095

chore(client-wallet): better wallet backup format

833ea00

chore: equivalent transformation, no branching

c593e6a

chore: simplify recover_scan_idxes_for_activity

999c8d2

chore: sanity_test_recover_inner details

674409a

Inspired by fedimint@3bd0947

dpc dismissed elsirion’s stale review via 674409a August 22, 2024 18:16

dpc force-pushed the 24-07-15-wallet-module-recovery-privacy branch from 30ccddc to 674409a Compare August 22, 2024 18:16

elsirion previously approved these changes Aug 23, 2024

View reviewed changes

elsirion added this pull request to the merge queue Aug 23, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 23, 2024

dpc added this pull request to the merge queue Aug 23, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 23, 2024

dpc added 2 commits August 23, 2024 13:35

chore(client): wait for all recoveries to finish

7dab283

fix: incorrect formatting

f5255bb

dpc dismissed elsirion’s stale review via f5255bb August 23, 2024 20:49

dpc requested a review from elsirion August 23, 2024 21:19

bradleystachurski approved these changes Aug 23, 2024

View reviewed changes

bradleystachurski added this pull request to the merge queue Aug 23, 2024

Merged via the queue into fedimint:master with commit 5a34061 Aug 23, 2024
23 checks passed

dpc mentioned this pull request Sep 17, 2024

chore: wallet client recovery privacy improvements (v0.4 backport) #6027

Merged

dpc deleted the 24-07-15-wallet-module-recovery-privacy branch November 12, 2024 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: wallet client recovery privacy improvements #5697

chore: wallet client recovery privacy improvements #5697

dpc commented Jul 19, 2024

dpc commented Jul 19, 2024

dpc Jul 19, 2024

elsirion left a comment

elsirion Jul 29, 2024

dpc Aug 5, 2024

dpc Aug 5, 2024

elsirion Aug 20, 2024

elsirion Aug 20, 2024

dpc Aug 21, 2024

elsirion Aug 20, 2024

dpc commented Aug 21, 2024

elsirion left a comment

elsirion Aug 22, 2024

dpc commented Aug 22, 2024

dpc commented Aug 23, 2024

dpc commented Aug 23, 2024 •

edited

Loading

bradleystachurski Aug 23, 2024

		@@ -794,10 +793,7 @@ impl FedimintCli {
		// TODO: until we implement recovery for other modules we can't really wait

chore: wallet client recovery privacy improvements #5697

chore: wallet client recovery privacy improvements #5697

Conversation

dpc commented Jul 19, 2024

dpc commented Jul 19, 2024

Choose a reason for hiding this comment

elsirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc commented Aug 21, 2024

elsirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc commented Aug 22, 2024

dpc commented Aug 23, 2024

dpc commented Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

dpc commented Aug 23, 2024 •

edited

Loading