IsUsedDestination should count any known single-key address #17621

instagibbs · 2019-11-27T16:01:24Z

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

instagibbs · 2019-11-27T16:11:26Z

cc @kallewoof since you designed the first iteration

achow101 · 2019-12-06T21:58:48Z

Concept ACK

Tests please :)

kallewoof · 2019-12-06T23:13:40Z

Concept ACK

jonatack

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

instagibbs · 2019-12-09T15:38:04Z

Pushed test, updated logic to fix test failure.

Removed a function that resulted in a used footgun.

src/wallet/wallet.cpp

kallewoof

utACK

fjahr · 2019-12-24T13:11:18Z

ACK 9f7eea5

Reviewed code and tested locally.

DrahtBot · 2019-12-29T05:52:01Z

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.

achow101

ACK 9f7eea5

achow101 · 2020-01-03T22:16:02Z

test/functional/wallet_avoidreuse.py

@@ -193,7 +198,7 @@ def test_fund_send_fund_send(self):
        '''
        self.log.info("Test fund send fund send")

-        fundaddr = self.nodes[1].getnewaddress()
+        fundaddr = self.nodes[1].getnewaddress(label="", address_type="legacy")


nit: label isn't needed

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

src/wallet/wallet.cpp

Github-Pull: bitcoin#17621 Rebased-From: 0950245

promag

Concept ACK.

promag · 2020-01-05T23:26:00Z

src/wallet/rpcwallet.cpp

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


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

meshcollider

Code Review ACK 0950245

…ress 0950245 IsUsedDestination should count any known single-key address (Gregory Sanders) Pull request description: This plugs the privacy leak detailed at #17605, at least for the single-key case. ACKs for top commit: meshcollider: Code Review ACK 0950245 Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c

…key address 0950245 IsUsedDestination should count any known single-key address (Gregory Sanders) Pull request description: This plugs the privacy leak detailed at bitcoin#17605, at least for the single-key case. ACKs for top commit: meshcollider: Code Review ACK 0950245 Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c

Github-Pull: bitcoin#17621 Rebased-From: 0950245

fanquake · 2020-01-14T09:40:38Z

Being backported in 17792.

Github-Pull: bitcoin#17621 Rebased-From: 0950245

…d for ScriptHash 6dd59d2 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders) 4b8f1e9 IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders) Pull request description: Regression introduced in #17621 which causes p2sh-segwit addresses to be erroneously missed. Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default. I'll devise a test case to catch this going forward. ACKs for top commit: achow101: ACK 6dd59d2 MarcoFalke: ACK 6dd59d2 meshcollider: Code review ACK 6dd59d2 Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950

cd67b1d Use correct C++11 header for std::swap() (Hennadii Stepanov) b8101fb Fix comparison function signature (Hennadii Stepanov) eac4907 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders) e2c45d8 IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders) a5489c9 IsUsedDestination should count any known single-key address (Gregory Sanders) 88729d8 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) eafcea7 gui: Fix duplicate wallet showing up (João Barbosa) 7e66d04 Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) 179d55f zmq: Fix due to invalid argument and multiple notifiers (João Barbosa) Pull request description: Backports - #16963 - #17445 - #17258 - #17621 - #17924 - #17634 ACKs for top commit: laanwj: ACK cd67b1d, checked that I got more or less the same result (including conflict resolution) backporting these commits Tree-SHA512: 645786267cfb10a01a56f7cfd91ddead5f1475df5714595ae480237e04d40c5cfb7460b40532279cacd83e4b775a4ace68a258ec2184b8ad0e997a690a9245e5

[0.19] Backports bitcoin#17858 Unbreak build with Boost 1.72.0 bitcoin#17654 cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice bitcoin#17687 rpc: require second argument only for scantxoutset start action bitcoin#17728 wallet: Fix origfee return for bumpfee with feerate arg bitcoin#17643 test: fix "bitcoind already running" warnings on macOS bitcoin#17488 net: Log to net category for exceptions in ProcessMessages bitcoin#17762 Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes bitcoin#17364 Appveyor improvement - text file for vcpkg package list bitcoin#17416 Update msvc build for Visual Studio 2019 v16.4 bitcoin#17736 scripts: fix symbol-check & security-check argument passing bitcoin#17857 qt: Periodic translations update for 0.19 branch IsUsedDestination should count any known single-key address bitcoin#17621 init: Stop indexes on shutdown after ChainStateFlushed callback. bitcoin#17897 qt: Translations update pre-rc1 wallet: Reset reused transactions cache bitcoin#17843 Squashed 'src/univalue/' changes from 7890db9..98261b1 0.19: Update univalue subtree bitcoin#18100 qt: Pre-rc2 translations update

[0.19] Backports bitcoin#17858 Unbreak build with Boost 1.72.0 bitcoin#17654 cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice bitcoin#17687 rpc: require second argument only for scantxoutset start action bitcoin#17728 wallet: Fix origfee return for bumpfee with feerate arg bitcoin#17643 test: fix "bitcoind already running" warnings on macOS bitcoin#17488 net: Log to net category for exceptions in ProcessMessages bitcoin#17762 Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes bitcoin#17364 Appveyor improvement - text file for vcpkg package list bitcoin#17416 Update msvc build for Visual Studio 2019 v16.4 bitcoin#17736 scripts: fix symbol-check & security-check argument passing bitcoin#17857 qt: Periodic translations update for 0.19 branch IsUsedDestination should count any known single-key address bitcoin#17621 init: Stop indexes on shutdown after ChainStateFlushed callback. bitcoin#17897 qt: Translations update pre-rc1 wallet: Reset reused transactions cache bitcoin#17843 Squashed 'src/univalue/' changes from 7890db9..98261b1 0.19: Update univalue subtree bitcoin#18100 qt: Pre-rc2 translations update [0.19] Further 0.19 backports bitcoin#18218 build: don't embed a build-id when building libdmg-hfsplus bitcoin#18004

Summary: This is a backport of Core [[bitcoin/bitcoin#17621 | PR17621]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D7528

…t id for ScriptHash Summary: 6dd59d2e491bc11ab26498668543e65440a3a931 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders) 4b8f1e989f3b969dc628b0801d5c31ebd373719c IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders) Pull request description: Regression introduced in bitcoin/bitcoin#17621 which causes p2sh-segwit addresses to be erroneously missed. Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default. I'll devise a test case to catch this going forward. --- Backport of Core [[bitcoin/bitcoin#17924 | PR17924]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7800

…key address 0950245 IsUsedDestination should count any known single-key address (Gregory Sanders) Pull request description: This plugs the privacy leak detailed at bitcoin#17605, at least for the single-key case. ACKs for top commit: meshcollider: Code Review ACK 0950245 Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c

fanquake added Wallet Needs Conceptual Review labels Nov 27, 2019

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

jonatack reviewed Dec 8, 2019

View reviewed changes

instagibbs force-pushed the actually_no_reuse branch from 90bb5da to 2ca5a16 Compare December 9, 2019 15:35

instagibbs force-pushed the actually_no_reuse branch from 2ca5a16 to ef4a1a4 Compare December 10, 2019 14:21

fanquake removed the Needs Conceptual Review label Dec 12, 2019

promag reviewed Dec 17, 2019

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

promag reviewed Dec 17, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

promag reviewed Dec 17, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the actually_no_reuse branch from ef4a1a4 to 9f7eea5 Compare December 17, 2019 14:20

kallewoof approved these changes Dec 18, 2019

View reviewed changes

promag mentioned this pull request Dec 23, 2019

0.19: Backports #17792

Merged

IsUsedDestination should count any known single-key address

0950245

instagibbs force-pushed the actually_no_reuse branch from 9f7eea5 to 0950245 Compare January 3, 2020 22:21

achow101 reviewed Jan 3, 2020

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020

IsUsedDestination should count any known single-key address

f32661f

Github-Pull: bitcoin#17621 Rebased-From: 0950245

promag reviewed Jan 5, 2020

View reviewed changes

fanquake requested a review from meshcollider January 6, 2020 00:02

meshcollider reviewed Jan 7, 2020

View reviewed changes

meshcollider merged commit 0950245 into bitcoin:master Jan 7, 2020

promag pushed a commit to promag/bitcoin that referenced this pull request Jan 14, 2020

IsUsedDestination should count any known single-key address

d417166

Github-Pull: bitcoin#17621 Rebased-From: 0950245

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

promag pushed a commit to promag/bitcoin that referenced this pull request Jan 14, 2020

IsUsedDestination should count any known single-key address

a5489c9

Github-Pull: bitcoin#17621 Rebased-From: 0950245

instagibbs mentioned this pull request Jan 14, 2020

Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash #17924

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

IsUsedDestination should count any known single-key address #17621

IsUsedDestination should count any known single-key address #17621

Uh oh!

Conversation

instagibbs commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Nov 27, 2019

Uh oh!

achow101 commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Dec 6, 2019

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Dec 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Dec 24, 2019

Uh oh!

DrahtBot commented Dec 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Jan 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jan 5, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jan 14, 2020

Uh oh!

Uh oh!

instagibbs commented Nov 27, 2019 •

edited

Loading

achow101 commented Dec 6, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

DrahtBot commented Dec 29, 2019 •

edited

Loading