Don't sign when segwit signatures are being used and the pubkey(s) are uncompressed #16022
Conversation
When signing, if the sigversion is WITNESS_V0, check that all public keys are compressed public keys before any kind of signature is made
| ret.push_back(valtype()); // workaround CHECKMULTISIG bug | ||
| for (size_t i = 1; i < vSolutions.size() - 1; ++i) { | ||
| CPubKey pubkey = CPubKey(vSolutions[i]); | ||
| // Signing with uncompressed keys is disabled in witness scripts |
There was a problem hiding this comment.
Is this part necessary? If we already have enough sigs it doesn't matter I think, and if not, the call to CheckSig below will catch the uncompressed case.
There was a problem hiding this comment.
Yes, it is necessary. If the number of compressed keys is >= the threshold, then enough signatures can be made to have the entire multisig still be valid if signed by those uncompressed keys.
In my testing, this is the thing that actually fixes the bug, the CreateSig check did not do so.
|
After some hours of debugging, although I think your changes might make sense anyway, I suspect they're not fixing the actual bug. I'll send you a PR as soon as possible with an addition that I think could be a better fix, so you can merge my modifications here if you think that makes sense. |
|
::Sigh:: I think we still want to be able to sign in these cases (otherwise we contribute to funds being stuck), just not createmultisig/addmultisig. |
|
I opened a PR on your fork/branch @achow101 Edit: |
|
@ps1dr3x NACK on your suggestion to change |
This is only a concern because this bug has existed since day one of segwit, yes? If this was correct when segwit was introduced, it would be fine, right? Other ways to solve this from within |
|
@gmaxwell It's not as simple. It seems that the actually implemented policy will actually reject spends where any uncompressed key is encountered, even if it's not one that contributes to the validity of OP_CHECKMULTISIG(VERIFY). If all signatures match up with the first k keys, and those are compressed, then the rest can be uncompressed. |
|
@achow101 Even if we never had the bug, someone else might have, so it would be somewhat preferable if it could still sign.... that said, yes, if it had been originally written this way, that would probably have been fine. |
|
@sipa Thanks for confirming. I think we should be willing to sign anything that is consensus valid but not create anything that uses uncompressed at all. This avoids creating new screwups but also gives people the most help possible at undoing a messup. |
…ys returns a legacy address a495034 Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow) Pull request description: `CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`. This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as #16022 does. Alternative to #16022 and #16012 Fixes #16011 ACKs for commit a49503: Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0
…ig always returns a legacy address a495034 Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow) Pull request description: `CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`. This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as bitcoin#16022 does. Alternative to bitcoin#16022 and bitcoin#16012 Fixes bitcoin#16011 ACKs for commit a49503: Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0
5 participants
This PR adds additional checks in
CreateSigandSignStepto prevent any signature from being created if an uncompressed pubkey is being used when the signature version is segwit. In particular,CreateSigwill have this check to prevent the dummy signer and whatever other signers we may use in the future, from having to include this check as well. Furthermore, the multisig signing logic will now fail if it finds an uncompressed public key in the list of pubkeys to be signed when the signature version is segwit.This faulty behavior is most evident when attempting to create a multisig address that contains mixed compressed and uncompressed public keys as indicated in #16011. As such, a test has been added which ensures that both
createmultisigandaddmultisigaddresscreate the legacy address for a multisig that has such mixed public keys.Alternative to #16012.
Fixes #16011