IsUsedDestination should count any known single-key address

instagibbs commented at 4:01 pm on November 27, 2019: member

This plugs the privacy leak detailed at #17605, at least for the single-key case.

fanquake added the label Wallet on Nov 27, 2019

fanquake added the label Needs Conceptual Review on Nov 27, 2019

instagibbs commented at 4:11 pm on November 27, 2019: member

cc @kallewoof since you designed the first iteration

fanquake added the label Needs backport (0.19) on Dec 1, 2019

fanquake added this to the "Blockers" column in a project

fanquake moved this from the "Blockers" to the "Chasing Concept ACK" column in a project

achow101 commented at 9:58 pm on December 6, 2019: member

Concept ACK

Tests please :)

kallewoof commented at 11:13 pm on December 6, 2019: member

Concept ACK

jonatack commented at 9:45 pm on December 8, 2019: member

Concept ACK on patching this in the interim before descriptor wallets arrive. Edit: Code review, built, ran tests, tested manually.

instagibbs force-pushed on Dec 9, 2019

instagibbs commented at 3:38 pm on December 9, 2019: member

Pushed test, updated logic to fix test failure.

Removed a function that resulted in a used footgun.

instagibbs force-pushed on Dec 10, 2019

fanquake removed the label Needs Conceptual Review on Dec 12, 2019

in src/wallet/wallet.cpp:724 in ef4a1a4941 outdated

725-    return IsMine(dst) && GetDestData(dst, "used", nullptr);
726-}
727-
728 bool CWallet::IsUsedDestination(const uint256& hash, unsigned int n) const
729 {
730+    AssertLockHeld(cs_wallet);

promag commented at 0:42 am on December 17, 2019:

nit, also add the EXCLUSIVE_LOCKS_REQUIRED annotation.

instagibbs commented at 2:21 pm on December 17, 2019:

done

achow101 commented at 10:05 pm on January 3, 2020:

The annotation is supposed to go in the header file.

instagibbs commented at 10:11 pm on January 3, 2020:

is it just a no-op here? I am not a clang wizard.

instagibbs commented at 10:20 pm on January 3, 2020:

Attributes must be attached to named declarations oooohkay fixed

achow101 commented at 10:22 pm on January 3, 2020:

From clang docs:

Attributes are part of the formal interface of a function, and should always go in the header, where they are visible to anything that includes the header. Attributes in the .cpp file are not visible outside of the immediate translation unit, which leads to false negatives and false positives.

in src/wallet/wallet.cpp:728 in ef4a1a4941 outdated

730+    AssertLockHeld(cs_wallet);
731     CTxDestination dst;
732     const CWalletTx* srctx = GetWalletTx(hash);
733-    return srctx && ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst) && IsUsedDestination(dst);
734+    if (srctx) {
735+        assert(srctx->tx->vout.size() >= n);

promag commented at 0:46 am on December 17, 2019:

Should be >?

instagibbs commented at 1:59 pm on December 17, 2019:

d’oh, I always have to sit there and reason with myself on this :)

in src/wallet/wallet.cpp:733 in ef4a1a4941 outdated

735+        assert(srctx->tx->vout.size() >= n);
736+        LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
737+        assert(spk_man != nullptr);
738+        for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) {
739+            WitnessV0KeyHash wpkh_dest(keyid);
740+            if (IsMine(wpkh_dest) && GetDestData(wpkh_dest, "used", nullptr)) {

promag commented at 0:49 am on December 17, 2019:

Haven’t profiled but looks like GetDestData && IsMine should be faster?

instagibbs commented at 2:20 pm on December 17, 2019:

I just got rid of the IsMine check. If a record for used exists in the wallet we should trust it.

instagibbs force-pushed on Dec 17, 2019

kallewoof approved

kallewoof commented at 3:51 am on December 18, 2019: member

utACK

fjahr commented at 1:11 pm on December 24, 2019: member

ACK 9f7eea56f2b0fbc128b83fccab344190b1e6ce54

Reviewed code and tested locally.

DrahtBot commented at 5:52 am on December 29, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17889 (wallet: Improve CWallet:MarkDestinationsDirty by promag)
#17843 (wallet: Reset reused transactions cache by fjahr)
#17838 (test: test the >10 UTXO case for output groups by kallewoof)
#17824 (wallet: Improve coin selection for destination groups >10 by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

IsUsedDestination should count any known single-key address 09502452bb

in test/functional/wallet_avoidreuse.py:201 in 9f7eea56f2 outdated

197@@ -193,7 +198,7 @@ def test_fund_send_fund_send(self):
198         '''
199         self.log.info("Test fund send fund send")
200 
201-        fundaddr = self.nodes[1].getnewaddress()
202+        fundaddr = self.nodes[1].getnewaddress(label="", address_type="legacy")

achow101 commented at 10:16 pm on January 3, 2020:

nit: label isn’t needed

kallewoof commented at 1:20 am on January 4, 2020:

It would be, once address type default is changed, no?

instagibbs force-pushed on Jan 3, 2020

achow101 commented at 10:23 pm on January 3, 2020: member

ACK 9f7eea56f2b0fbc128b83fccab344190b1e6ce54

luke-jr referenced this in commit f32661fad5 on Jan 5, 2020

in src/wallet/rpcwallet.cpp:2930 in 09502452bb

2926@@ -2927,7 +2927,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
2927         CTxDestination address;
2928         const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey;
2929         bool fValidAddress = ExtractDestination(scriptPubKey, address);
2930-        bool reused = avoid_reuse && pwallet->IsUsedDestination(address);
2931+        bool reused = avoid_reuse && pwallet->IsUsedDestination(out.tx->GetHash(), out.i);

promag commented at 11:26 pm on January 5, 2020:

nit, you could move this line after L2932 - no point in calling IsUsedDestination if this address doesn’t matter.

promag commented at 11:45 pm on January 5, 2020: member

Concept ACK.

fanquake requested review from meshcollider on Jan 6, 2020

meshcollider commented at 9:29 pm on January 7, 2020: contributor

Code Review ACK 09502452bbbe21bb974f1de8cf53196373921ab9

meshcollider referenced this in commit bcb4cdcca3 on Jan 7, 2020

meshcollider merged this on Jan 7, 2020

meshcollider closed this on Jan 7, 2020

fanquake removed this from the "Chasing Concept ACK" column in a project

sidhujag referenced this in commit 69d7ae3cc0 on Jan 8, 2020

promag referenced this in commit d4171662ea on Jan 14, 2020

fanquake removed the label Needs backport (0.19) on Jan 14, 2020

fanquake commented at 9:40 am on January 14, 2020: member

Being backported in 17792.

promag referenced this in commit a5489c9892 on Jan 14, 2020

laanwj referenced this in commit f018d0c9cd on Jan 16, 2020

laanwj referenced this in commit 98159132c3 on Jan 20, 2020

MarkLTZ referenced this in commit 2e6abb1507 on Mar 13, 2020

MarkLTZ referenced this in commit ece88bae51 on Mar 13, 2020

jasonbcox referenced this in commit 0af2f31d85 on Sep 22, 2020

jasonbcox referenced this in commit affe4ede9e on Oct 7, 2020

sidhujag referenced this in commit 7bb63d3ef8 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

IsUsedDestination should count any known single-key address #17621

Conflicts