-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only accept bare multisig outputs after addmultisigaddress #12874
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
"Do[n't] treat bare multisig as ours unless added as script" I assume |
sipa
force-pushed
the
201803_nowalletbaremultisig
branch
from
April 3, 2018 23:24
7fc888b
to
044770e
Compare
@instagibbs Oops, fixed. |
As discussed in the April 12th 2018 IRC meeting, closing this in favor of #13002. |
laanwj
added a commit
that referenced
this pull request
Apr 26, 2018
…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
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
May 21, 2021
…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
TheArbitrator
pushed a commit
to TheArbitrator/dash
that referenced
this pull request
Jun 4, 2021
…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
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Jun 5, 2021
…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
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Jun 5, 2021
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 public keys in it are ours.These are:
Furthermore, they are problematic in that means 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:
This PR implements the second, though I'm open to discussing the first option.