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

instagibbs · 2020-01-14T18:30:29Z

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.

maflcko · 2020-01-14T18:40:11Z

Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.

All wallet tests should probably be run on all possible address types. See also #17199 (comment)

instagibbs · 2020-01-14T18:48:04Z

It should ideally not be possible to footgun like this either by what I did. There's no situation I can think of that you'd want that conversion.

instagibbs · 2020-01-14T20:07:35Z

pushed a very non-comprehensive commit that would have stopped me in my tracks if I thought "nesting" worked with ScriptHash destination.

I can keep poking at something more comprehensive, but I felt this was unobtrusive enough to be a non-controversial start.

src/script/standard.h

…esting computation

Github-Pull: bitcoin#17924 Rebased-From: 4b8f1e9

meshcollider

Code review ACK 6dd59d2

I like the fact this reduces the chance of a similar future mistake. It certainly isn't obvious at a glance.

promag · 2020-01-15T09:39:34Z

src/script/standard.h

 struct ScriptHash : public uint160
 {
    ScriptHash() : uint160() {}
+    // These don't do what you'd expect.
+    // Use ScriptHash(GetScriptForDestination(...)) instead.
+    explicit ScriptHash(const WitnessV0KeyHash& hash) = delete;


Why explicit here?

I was going back and forth on how to fix this and left that there :) I think this is a no-op

Github-Pull: bitcoin#17924 Rebased-From: 4b8f1e9

…esting computation Github-Pull: bitcoin#17924 Rebased-From: 6dd59d2

achow101 · 2020-01-15T18:59:31Z

ACK 6dd59d2

maflcko · 2020-01-15T19:41:07Z

ACK 6dd59d2

maflcko · 2020-01-15T19:47:18Z

I guess this is less likely to happen, but should CKeyID also be deleted?

instagibbs · 2020-01-15T19:52:20Z

I think that's way less likely, and we might be able to do something smarter to be more generally robust as a followup?

luke-jr · 2020-01-15T23:04:42Z

Maybe we should just make the inheritance private?

Empact · 2020-01-16T00:55:03Z

I took a swing at it here: #17938

instagibbs · 2020-01-16T15:23:19Z

@Empact looks much more comprehensive. If people agree we can merge this, then rebase and work on reviewing something like that?

laanwj · 2020-01-16T18:19:41Z

Maybe we should just make the inheritance private?

In that case, why not go all the way, and use composition instead of inheritance.

But let's do this fix for 0.19 then maybe do something more comprehensive for 0.20.

…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

fanquake · 2020-01-17T02:32:10Z

Being backported in 17792.

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] wallet: Reset reused transactions cache bitcoin#18083 - 0.19: Backports bitcoin#17792 - psbt: handle unspendable psbts bitcoin#17524 - qt: Fix comparison function signature bitcoin#17634 - psbt: check that various indexes and amounts are within bounds bitcoin#17156 - [0.19] psbt: check that various indexes and amounts are within bounds bitcoin#18079 - [0.19] Final backports for 0.19.1 bitcoin#17988 - Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash bitcoin#17924 - qt: Fix deprecated QCharRef usage bitcoin#18101 - gui: Throttle GUI update pace when -reindex bitcoin#18121 - gui: Fix race in WalletModel::pollBalanceChanged bitcoin#18123 - gui: Fix unintialized WalletView::progressDialog bitcoin#18062 - Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open bitcoin#18007 - bug-fix macos: give free bytes to F_PREALLOCATE bitcoin#17887 - test: add missing #include to fix compiler errors bitcoin#17980 - zmq: Fix due to invalid argument and multiple notifiers bitcoin#17445

4d73691 Disallow automatic conversion between hash types (Ben Woosley) fa9ef2c Remove an apparently unnecessary conversion (Ben Woosley) 966a22d Explicitly support conversion between equivalent hash types (Ben Woosley) f32c1e0 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley) 2c54217 Use explicit conversion from PKHash -> CKeyID (Ben Woosley) a9e451f Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley) 3fcc468 Prefer explicit CScriptID construction (Ben Woosley) 0a5ea32 Prefer explicit uint160 conversion (Ben Woosley) Pull request description: This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type. Inspired by and built on #17924. Commits are small and focused to ease review. Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion". ACKs for top commit: achow101: ACK 4d73691 meshcollider: re-utACK 4d73691 Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54

…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

IsUsedDestination shouldn't use key id as script id for ScriptHash

4b8f1e9

instagibbs mentioned this pull request Jan 14, 2020

0.19: Backports #17792

Merged

instagibbs changed the title ~~IsUsedDestination shouldn't use key id as script id for ScriptHash~~ Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash Jan 14, 2020

DrahtBot added the Wallet label Jan 14, 2020

maflcko reviewed Jan 14, 2020

View reviewed changes

src/script/standard.h Outdated Show resolved Hide resolved

luke-jr reviewed Jan 14, 2020

View reviewed changes

src/script/standard.h Outdated Show resolved Hide resolved

Don't allow implementers to think ScriptHash(Witness*()) results in n…

6dd59d2

…esting computation

instagibbs force-pushed the reuse_regression branch from 6fe6e0c to 6dd59d2 Compare January 14, 2020 22:11

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

IsUsedDestination shouldn't use key id as script id for ScriptHash

54db35e

Github-Pull: bitcoin#17924 Rebased-From: 4b8f1e9

fanquake requested review from meshcollider and achow101 January 14, 2020 23:40

fanquake added the Needs backport (0.19) label Jan 15, 2020

meshcollider reviewed Jan 15, 2020

View reviewed changes

promag reviewed Jan 15, 2020

View reviewed changes

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

IsUsedDestination shouldn't use key id as script id for ScriptHash

e2c45d8

Github-Pull: bitcoin#17924 Rebased-From: 4b8f1e9

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

Don't allow implementers to think ScriptHash(Witness*()) results in n…

eac4907

…esting computation Github-Pull: bitcoin#17924 Rebased-From: 6dd59d2

Empact mentioned this pull request Jan 16, 2020

Disallow automatic conversion between disparate hash types #17938

Merged

laanwj merged commit 6dd59d2 into bitcoin:master Jan 16, 2020

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

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

maflcko mentioned this pull request Nov 26, 2020

Replace boost::variant with std::variant #20480

Merged

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

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

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

Uh oh!

Conversation

instagibbs commented Jan 14, 2020

Uh oh!

maflcko commented Jan 14, 2020

Uh oh!

instagibbs commented Jan 14, 2020

Uh oh!

instagibbs commented Jan 14, 2020

Uh oh!

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jan 15, 2020 • 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 Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 15, 2020

Uh oh!

maflcko commented Jan 15, 2020

Uh oh!

maflcko commented Jan 15, 2020

Uh oh!

instagibbs commented Jan 15, 2020

Uh oh!

luke-jr commented Jan 15, 2020

Uh oh!

Empact commented Jan 16, 2020

Uh oh!

instagibbs commented Jan 16, 2020

Uh oh!

laanwj commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jan 17, 2020

Uh oh!

Uh oh!

promag Jan 15, 2020 •

edited

Loading

laanwj commented Jan 16, 2020 •

edited

Loading