Do not treat bare multisig outputs as IsMine unless watched #13002

sipa · 2018-04-17T01:13:11Z

Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + OP_CHECKMULTISIG operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet.

This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them).

Furthermore, they are problematic in that producing a list of all scriptPubKeys we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it.

I think there are two options:

Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched)
Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable

This PR implements the first option. The second option was explored in #12874.

promag · 2018-04-17T19:22:01Z

doc/release-notes.md

+  such outputs is defined, and wallet software can't easily send to it. These
+  outputs will no longer show up in `listtransactions`, `listunspent`, or
+  contribute to your balance, unless they are explicitly watched (using
+  `importaddress with hex script argument). `signrawtransaction*` also still


missing backtick after importaddress.

promag

Some nits…

promag · 2018-04-18T14:22:23Z

src/script/ismine.cpp

-}
-
-isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, SigVersion sigversion)
+static isminetype IsMineInner(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion)


const CKeyStore& keystore

promag · 2018-04-18T14:23:20Z

src/script/ismine.cpp

@@ -150,3 +142,26 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
    }
    return ISMINE_NO;
 }
+
+isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid)


const CKeyStore& keystore

Fixed in the wrong commit.

promag · 2018-04-18T14:23:28Z

src/script/ismine.cpp

+    return IsMine(keystore, scriptPubKey, isInvalid);
+}
+
+isminetype IsMine(const CKeyStore &keystore, const CTxDestination& dest, bool& isInvalid)


const CKeyStore& keystore

sipa · 2018-04-18T22:09:38Z

Addressed @promag's nits, and added a few more code cleanups.

promag

utACK 7e827e4.

Change looks good. Commit 000c41a is nice because it also prevents repetitive hash computation. One nit fix is in the wrong commit though.

promag · 2018-04-18T22:25:42Z

src/wallet/rpcdump.cpp

                throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet");
            }

-            CTxDestination redeem_dest = CScriptID(redeemScript);
+            CTxDestination redeem_dest(redeem_id);
            CScript redeemDestination = GetScriptForDestination(redeem_dest);


Nit, drop "used only once" redeem_dest:

CScript redeemDestination = GetScriptForDestination(redeem_id);

promag · 2018-04-18T22:36:19Z

src/script/ismine.cpp

@@ -150,3 +142,26 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
    }
    return ISMINE_NO;
 }
+
+isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid)


Fixed in the wrong commit.

promag · 2018-04-18T23:29:57Z

doc/release-notes.md

+  outputs is defined, and wallet software can't easily send to it. These outputs
+  will no longer show up in `listtransactions`, `listunspent`, or contribute to
+  your balance, unless they are explicitly watched (using `importaddress` with
+  hex script argument). `signrawtransaction*` also still works for them.


`importaddress` or `importmulti`?

sipa · 2018-04-19T01:10:02Z

Addressed @promag's nits.

promag · 2018-04-19T01:25:44Z

@sipa thanks.

Code wise ACK 2687bd0.

jimpo

Looks good.

jimpo · 2018-04-20T02:24:15Z

src/script/ismine.cpp

@@ -70,7 +58,7 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
            // This also applies to the P2WSH case.
            break;
        }
-        isminetype ret = ::IsMine(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, SigVersion::WITNESS_V0);
+        isminetype ret = ::IsMineInner(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, SigVersion::WITNESS_V0);


commit: Do not expose SigVersion argument to IsMine

nit: Drop the ::. It's weird to have it on this call but not the other two.

jimpo · 2018-04-20T02:26:19Z

src/script/ismine.cpp

@@ -13,6 +13,12 @@

 typedef std::vector<unsigned char> valtype;

+enum class IsMineSigVersion


commit: Switch to a private version of SigVersion inside IsMine

Please let a comment explaining what this is and how it differs from SigVersion. Even though it's in the commit comment, it's good to have it in the code.

Done (in the next commit, which actually introduces the difference).

jimpo · 2018-04-20T02:30:55Z

src/script/ismine.cpp

@@ -49,7 +50,7 @@ static isminetype IsMineInner(const CKeyStore& keystore, const CScript& scriptPu
        break;
    case TX_PUBKEY:
        keyID = CPubKey(vSolutions[0]).GetID();
-        if (sigversion != IsMineSigVersion::BASE && vSolutions[0].size() != 33) {
+        if (sigversion != IsMineSigVersion::TOP && sigversion != IsMineSigVersion::P2SH && vSolutions[0].size() != 33) {


commit: Track difference between scriptPubKey and P2SH execution in IsMine

I think a function bool SigVersionPermitsUncompressed(IsMineSigVersion) would improve clarity.

Yes, that's much more readable. Done.

Only IsMine's internal code needs this, as part of a recursion into P2SH and P2WSH scripts. The exposed functions always operate on actual scriptPubKeys and not on redeemScripts or witness scripts.

This will allow us to have the consensus code and IsMine code diverge.

Inside IsMine we care about the distinction between scriptPubKey execution and P2SH redeemScript execution. The consensus code does not care about this distinction, and thus SigVersion does not have a field for P2SH. As the IsMine code will care, it uses a separate enum with more fields.

Inside P2SH scripts we already know that the P2SH script version of witness keys/scripts are acceptable, so there is no need to test for it again.

Such outputs can still be watched, and signed for, but they aren't treated as valid payments. That means they won't cause transactions to appear in listtransactions, their outputs to be shown under listunspent, or affect balances.

jimpo · 2018-04-24T18:27:03Z

utACK 7d0f80b

promag · 2018-04-24T21:11:33Z

utACK 7d0f80b.

laanwj · 2018-04-26T18:10:00Z

utACK 3deed79806bbf31e003444798d9e70efffe7b8f7

…ched 7d0f80b Use anonymous namespace instead of static functions (Pieter Wuille) b61fb71 Mention removal of bare multisig IsMine in release notes (Pieter Wuille) 9c2a8b8 Do not treat bare multisig as IsMine (Pieter Wuille) 08f3228 Optimization: only test for witness scripts at top level (Pieter Wuille) 3619735 Track difference between scriptPubKey and P2SH execution in IsMine (Pieter Wuille) ac6ec62 Switch to a private version of SigVersion inside IsMine (Pieter Wuille) 19fc973 Do not expose SigVersion argument to IsMine (Pieter Wuille) fb1dfbb Remove unused IsMine overload (Pieter Wuille) 952d821 Make CScript -> CScriptID conversion explicit (Pieter Wuille) Pull request description: Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + `OP_CHECKMULTISIG` operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet. This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them). Furthermore, they are problematic in that producing a list of all `scriptPubKeys` we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it. I think there are two options: * Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched) * Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable This PR implements the first option. The second option was explored in #12874. Tree-SHA512: 917ed45b3cac864cee53e27f9a3e900390c576277fbd6751b1250becea04d692b3b426fa09065a3399931013bd579c4f3dbeeb29d51d19ed0c64da75d430ad9a

Bitcoin wallet PRs 3 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7687 - bitcoin/bitcoin#11116 - Excludes SegWit tests - Excludes `isInvalid` code (which is part of the SegWit support structure) - bitcoin/bitcoin#13002 - Excludes changes to `importwallet` (missing bitcoin/bitcoin#11667) - Excludes changes to `ProcessImport` (missing bitcoin/bitcoin#7551) - Third commit was rewritten to add an internal SigVersion argument (which we didn't have because it was added to support SegWit logic).

…ess watched 7d0f80b Use anonymous namespace instead of static functions (Pieter Wuille) b61fb71 Mention removal of bare multisig IsMine in release notes (Pieter Wuille) 9c2a8b8 Do not treat bare multisig as IsMine (Pieter Wuille) 08f3228 Optimization: only test for witness scripts at top level (Pieter Wuille) 3619735 Track difference between scriptPubKey and P2SH execution in IsMine (Pieter Wuille) ac6ec62 Switch to a private version of SigVersion inside IsMine (Pieter Wuille) 19fc973 Do not expose SigVersion argument to IsMine (Pieter Wuille) fb1dfbb Remove unused IsMine overload (Pieter Wuille) 952d821 Make CScript -> CScriptID conversion explicit (Pieter Wuille) Pull request description: Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + `OP_CHECKMULTISIG` operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet. This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them). Furthermore, they are problematic in that producing a list of all `scriptPubKeys` we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it. I think there are two options: * Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched) * Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable This PR implements the first option. The second option was explored in bitcoin#12874. Tree-SHA512: 917ed45b3cac864cee53e27f9a3e900390c576277fbd6751b1250becea04d692b3b426fa09065a3399931013bd579c4f3dbeeb29d51d19ed0c64da75d430ad9a

sipa mentioned this pull request Apr 17, 2018

Only accept bare multisig outputs after addmultisigaddress #12874

Closed

fanquake added the Wallet label Apr 17, 2018

promag reviewed Apr 17, 2018

View reviewed changes

promag reviewed Apr 18, 2018

View reviewed changes

sipa force-pushed the 201804_cleanismine branch from afda52f to bc83bb8 Compare April 18, 2018 21:58

sipa force-pushed the 201804_cleanismine branch from bc83bb8 to 7e827e4 Compare April 18, 2018 22:43

promag reviewed Apr 18, 2018

View reviewed changes

sipa added 2 commits April 18, 2018 17:08

Make CScript -> CScriptID conversion explicit

952d821

Remove unused IsMine overload

fb1dfbb

sipa force-pushed the 201804_cleanismine branch from 7e827e4 to 2687bd0 Compare April 19, 2018 00:33

jimpo reviewed Apr 20, 2018

View reviewed changes

sipa added 7 commits April 19, 2018 20:52

Do not expose SigVersion argument to IsMine

19fc973

Only IsMine's internal code needs this, as part of a recursion into P2SH and P2WSH scripts. The exposed functions always operate on actual scriptPubKeys and not on redeemScripts or witness scripts.

Switch to a private version of SigVersion inside IsMine

ac6ec62

This will allow us to have the consensus code and IsMine code diverge.

Optimization: only test for witness scripts at top level

08f3228

Inside P2SH scripts we already know that the P2SH script version of witness keys/scripts are acceptable, so there is no need to test for it again.

Do not treat bare multisig as IsMine

9c2a8b8

Such outputs can still be watched, and signed for, but they aren't treated as valid payments. That means they won't cause transactions to appear in listtransactions, their outputs to be shown under listunspent, or affect balances.

Mention removal of bare multisig IsMine in release notes

b61fb71

Use anonymous namespace instead of static functions

7d0f80b

sipa force-pushed the 201804_cleanismine branch from 2687bd0 to 7d0f80b Compare April 20, 2018 04:10

laanwj merged commit 7d0f80b into bitcoin:master Apr 26, 2018

Bushstar mentioned this pull request Apr 27, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#311

Merged

sipa added the Needs release note label Aug 14, 2018

fanquake removed the Needs release note label Mar 20, 2019

instagibbs mentioned this pull request Nov 27, 2019

avoid_reuse=true can be bypassed by using pubkey in alternate addresses #17605

Closed

str4d mentioned this pull request Dec 19, 2019

Bitcoin wallet PRs 3 zcash/zcash#4271

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		@@ -13,6 +13,12 @@

		typedef std::vector<unsigned char> valtype;

		enum class IsMineSigVersion

Do not treat bare multisig outputs as IsMine unless watched #13002

Do not treat bare multisig outputs as IsMine unless watched #13002

Uh oh!

Conversation

sipa commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 18, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 19, 2018

Uh oh!

promag commented Apr 19, 2018

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo commented Apr 24, 2018

Uh oh!

promag commented Apr 24, 2018

Uh oh!

laanwj commented Apr 26, 2018

Uh oh!

Uh oh!