Do not treat bare multisig outputs as IsMine unless watched

sipa commented at 1:13 am on April 17, 2018: member

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.

fanquake added the label Wallet on Apr 17, 2018

in doc/release-notes.md:106 in afda52fe2a outdated

101+  for which you had all private keys in your wallets, there was generally no use
102+  for them compared to single-key schemes. Furthermore, no address format for
103+  such outputs is defined, and wallet software can't easily send to it. These
104+  outputs will no longer show up in `listtransactions`, `listunspent`, or
105+  contribute to your balance, unless they are explicitly watched (using
106+  `importaddress with hex script argument). `signrawtransaction*` also still

promag commented at 7:22 pm on April 17, 2018:

missing backtick after importaddress.

in src/script/ismine.cpp:32 in afda52fe2a outdated

46-    CScript script = GetScriptForDestination(dest);
47-    return IsMine(keystore, script, isInvalid, sigversion);
48-}
49-
50-isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, SigVersion sigversion)
51+static isminetype IsMineInner(const CKeyStore &keystore, const CScript& scriptPubKey, bool& isInvalid, IsMineSigVersion sigversion)

promag commented at 2:22 pm on April 18, 2018:

const CKeyStore& keystore

in src/script/ismine.cpp:146 in afda52fe2a outdated

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

promag commented at 2:23 pm on April 18, 2018:

const CKeyStore& keystore

promag commented at 10:36 pm on April 18, 2018:

Fixed in the wrong commit.

in src/script/ismine.cpp:157 in afda52fe2a outdated

152+{
153+    bool isInvalid = false;
154+    return IsMine(keystore, scriptPubKey, isInvalid);
155+}
156+
157+isminetype IsMine(const CKeyStore &keystore, const CTxDestination& dest, bool& isInvalid)

promag commented at 2:23 pm on April 18, 2018:

const CKeyStore& keystore

promag commented at 2:31 pm on April 18, 2018: member

Some nits…

sipa force-pushed on Apr 18, 2018

sipa commented at 10:09 pm on April 18, 2018: member

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

in src/wallet/rpcdump.cpp:910 in 000c41a3f0 outdated

907                 throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet");
908             }
909 
910-            CTxDestination redeem_dest = CScriptID(redeemScript);
911+            CTxDestination redeem_dest(redeem_id);
912             CScript redeemDestination = GetScriptForDestination(redeem_dest);

promag commented at 10:25 pm on April 18, 2018:

Nit, drop “used only once” redeem_dest:

0CScript redeemDestination = GetScriptForDestination(redeem_id);

sipa force-pushed on Apr 18, 2018

in doc/release-notes.md:106 in 7e827e4f90 outdated

101+  which you had all private keys in your wallet, there was generally no use for
102+  them compared to single-key schemes. Furthermore, no address format for such
103+  outputs is defined, and wallet software can't easily send to it. These outputs
104+  will no longer show up in `listtransactions`, `listunspent`, or contribute to
105+  your balance, unless they are explicitly watched (using `importaddress` with
106+  hex script argument). `signrawtransaction*` also still works for them.

promag commented at 11:29 pm on April 18, 2018:

`importaddress` or `importmulti`?

promag commented at 11:31 pm on April 18, 2018: member

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.

Make CScript -> CScriptID conversion explicit 952d8213a6

Remove unused IsMine overload fb1dfbbec0

sipa force-pushed on Apr 19, 2018

sipa commented at 1:10 am on April 19, 2018: member

Addressed @promag’s nits.

promag commented at 1:25 am on April 19, 2018: member

@sipa thanks.

Code wise ACK 2687bd0.

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

in src/script/ismine.cpp:61 in 30c7553c5d outdated

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

jimpo commented at 2:24 am on April 20, 2018:

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.

sipa commented at 4:10 am on April 20, 2018:

Fixed.

in src/script/ismine.cpp:24 in d9ca52e925 outdated

12@@ -13,6 +13,12 @@
13 
14 typedef std::vector<unsigned char> valtype;
15 
16+enum class IsMineSigVersion

jimpo commented at 2:26 am on April 20, 2018:

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.

sipa commented at 4:10 am on April 20, 2018:

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

in src/script/ismine.cpp:53 in da64e35f61 outdated

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

jimpo commented at 2:30 am on April 20, 2018:

commit: Track difference between scriptPubKey and P2SH execution in IsMine

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

sipa commented at 4:10 am on April 20, 2018:

Yes, that’s much more readable. Done.

jimpo commented at 2:47 am on April 20, 2018: contributor

Looks good.

Do not expose SigVersion argument to IsMine

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.

19fc973097

Switch to a private version of SigVersion inside IsMine

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

ac6ec62522

Track difference between scriptPubKey and P2SH execution in IsMine

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.

3619735b09

Optimization: only test for witness scripts at top level

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.

08f3228654

Do not treat bare multisig as IsMine

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.

9c2a8b8d34

Mention removal of bare multisig IsMine in release notes b61fb71136

Use anonymous namespace instead of static functions 7d0f80bbf4

sipa force-pushed on Apr 20, 2018

jimpo commented at 6:27 pm on April 24, 2018: contributor

utACK 7d0f80b

promag commented at 9:11 pm on April 24, 2018: member

utACK 7d0f80b.

laanwj commented at 6:10 pm on April 26, 2018: member

utACK 3deed79806bbf31e003444798d9e70efffe7b8f7

laanwj merged this on Apr 26, 2018

laanwj closed this on Apr 26, 2018

laanwj referenced this in commit 487dcbe80c on Apr 26, 2018

fanquake removed this from the "Blockers" column in a project

sipa added the label Needs release notes on Aug 14, 2018

fanquake removed the label Needs release note on Mar 20, 2019

zkbot referenced this in commit 900ed4555f on Dec 19, 2019

UdjinM6 referenced this in commit a1feaa997c on May 21, 2021

TheArbitrator referenced this in commit 55b74a62bb on Jun 4, 2021

UdjinM6 referenced this in commit 7e072720b4 on Jun 5, 2021

UdjinM6 referenced this in commit 370ec04a02 on Jun 5, 2021

DrahtBot locked this on Dec 16, 2021

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