Ensure that uncompressed public keys in a multisig always returns a legacy address #16026

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:dont-sw-ms-uncomp changing 5 files +53 −12

achow101 commented at 4:29 AM on May 15, 2019: member

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
fanquake added the label RPC/REST/ZMQ on May 15, 2019
gmaxwell commented at 4:42 AM on May 15, 2019: contributor

This sounds right to me.
MarcoFalke added the label Needs backport on May 15, 2019
MarcoFalke added this to the milestone 0.18.1 on May 15, 2019

in src/rpc/util.cpp:155 in 5fefdb3286 outdated

 150 | @@ -150,8 +151,8 @@ CPubKey AddrToPubKey(CKeyStore* const keystore, const std::string& addr_in)
 151 |      return vchPubKey;
 152 |  }
 153 |  
 154 | -// Creates a multisig redeemscript from a given list of public keys and number required.
 155 | -CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey>& pubkeys)
 156 | +// Creates a multisig address from a given list of public keys, number required, and the address type
 157 | +CTxDestination AddAndGetMultisigDestination(CKeyStore& keystore, CScript& script_out, const int required, const std::vector<CPubKey>& pubkeys, OutputType type)

Empact commented at 7:09 PM on May 15, 2019:

nit: as an out arg, it'd be somewhat more clear to place script_out towards the end. AddAndGetDestinationForScript's placement is based on the same not being an out arg.

laanwj commented at 11:52 AM on May 29, 2019:

yeah, that seems to en the convention, usually wish C++ had tuple unpacking and it was unnecessary to bother with 'output arguments'

achow101 commented at 3:38 PM on June 18, 2019:

Done

laanwj commented at 11:54 AM on May 29, 2019: member

utACK 5fefdb3286b617a74dd395381b1be0b29f21b0e6

in src/rpc/util.cpp:175 in 5fefdb3286 outdated

 174 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, (strprintf("redeemScript exceeds size limit: %d > %d", script_out.size(), MAX_SCRIPT_ELEMENT_SIZE)));
 175 |      }
 176 |  
 177 | -    return result;
 178 | +    // Check if any keys are uncompressed. If so, the type is legacy
 179 | +    for (CPubKey pk : pubkeys) {

meshcollider commented at 12:03 PM on June 5, 2019:

&

achow101 commented at 1:12 PM on June 5, 2019:

done

meshcollider commented at 12:03 PM on June 5, 2019: contributor

utACK
achow101 force-pushed on Jun 5, 2019
MarcoFalke closed this on Jun 16, 2019
MarcoFalke reopened this on Jun 16, 2019
DrahtBot commented at 5:07 AM on June 18, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16227 (Refactor CWallet's inheritance chain by achow101)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
achow101 force-pushed on Jun 18, 2019
meshcollider added this to the "Bugfixes" column in a project

meshcollider commented at 11:42 PM on June 18, 2019: contributor

re-utACK https://github.com/bitcoin/bitcoin/pull/16026/commits/40696620703e7339957177e95a9acb1d8be80e0e

in src/rpc/util.cpp:154 in 4069662070 outdated

 150 | @@ -150,8 +151,8 @@ CPubKey AddrToPubKey(CKeyStore* const keystore, const std::string& addr_in)
 151 |      return vchPubKey;
 152 |  }
 153 |  
 154 | -// Creates a multisig redeemscript from a given list of public keys and number required.
 155 | -CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey>& pubkeys)
 156 | +// Creates a multisig address from a given list of public keys, number required, and the address type

kallewoof commented at 1:39 PM on June 19, 2019:

This comment (number required) is confusing. Reading code I understand it's the threshold for sigs (k in k-of-n). Maybe "from a given list of public keys, number of required signatures, and the address type"?

achow101 commented at 10:05 PM on June 19, 2019:

Done

in test/functional/rpc_createmultisig.py:16 in 4069662070 outdated
```
  12 | -import decimal
  13 | +from test_framework.key import ECPubKey
  14 |  
  15 | +import binascii
  16 | +import decimal
  17 | +import itertools
```
kallewoof commented at 1:46 PM on June 19, 2019:

Unless I'm mistaken, convention is to import system libs first, then own stuff. decimal was imported here, though, so grain-of-salt.

achow101 commented at 10:05 PM on June 19, 2019:

A bunch of other tests do system libs after our own :man_shrugging:
kallewoof approved
kallewoof commented at 1:48 PM on June 19, 2019: member

ACK 40696620703e7339957177e95a9acb1d8be80e0e

Some nits, can be ignored (though would like comment fixed if possible).
achow101 force-pushed on Jun 19, 2019
kallewoof commented at 4:02 AM on June 20, 2019: member

re-ACK 0c39c9d
meshcollider commented at 8:43 AM on June 20, 2019: contributor

@achow101 I think your force push reversed the parameter reordering you did a couple of days ago

Make and get the multisig redeemscript and destination in one function instead of two

Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.

CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.

This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.

a49503402b

achow101 force-pushed on Jun 20, 2019
achow101 commented at 3:02 PM on June 20, 2019: member

@achow101 I think your force push reversed the parameter reordering you did a couple of days ago

Oops. Should be fixed now.
meshcollider commented at 7:43 AM on June 21, 2019: contributor

re-utACK https://github.com/bitcoin/bitcoin/pull/16026/commits/a49503402b6bc21e3878e151c07529941d36aed0
meshcollider merged this on Jun 21, 2019
meshcollider closed this on Jun 21, 2019
meshcollider referenced this in commit 303ec103ba on Jun 21, 2019
MarcoFalke referenced this in commit e472d545b1 on Jun 21, 2019
MarcoFalke removed the label Needs backport on Jun 21, 2019
MarcoFalke referenced this in commit e78007fc1a on Jun 21, 2019
sidhujag referenced this in commit f7efea7b67 on Jun 21, 2019
HashUnlimited referenced this in commit 6a8f0b36bd on Aug 23, 2019
Bushstar referenced this in commit c4b96875f2 on Aug 24, 2019
Bushstar referenced this in commit 93c3a2df3d on Aug 24, 2019
Bushstar referenced this in commit 92773cd27e on Aug 24, 2019
Bushstar referenced this in commit 27d3bcf55d on Aug 24, 2019
deadalnix referenced this in commit f14a55dc9e on Jun 12, 2020
jnewbery removed this from the "Bugfixes" column in a project

DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Milestone
0.18.1

Linked (view graph)

#16011 addmultisigaddress does not fallback to legacy when there are uncompressed public keys in the parameters #16012 rpc: addmultisigaddress should fallback to OutputType::LEGACY when uncompressed public keys are provided #16022 Don't sign when segwit signatures are being used and the pubkey(s) are uncompressed

Ensure that uncompressed public keys in a multisig always returns a legacy address #16026

Conflicts