Skip to content
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

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

Merged
merged 1 commit into from Jun 21, 2019

Conversation

@achow101
Copy link
Member

commented May 15, 2019

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

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

This sounds right to me.

@MarcoFalke MarcoFalke added this to the 0.18.1 milestone May 15, 2019

// Creates a multisig redeemscript from a given list of public keys and number required.
CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey>& pubkeys)
// Creates a multisig address from a given list of public keys, number required, and the address type
CTxDestination AddAndGetMultisigDestination(CKeyStore& keystore, CScript& script_out, const int required, const std::vector<CPubKey>& pubkeys, OutputType type)

This comment has been minimized.

Copy link
@Empact

Empact May 15, 2019

Member

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.

This comment has been minimized.

Copy link
@laanwj

laanwj May 29, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Jun 18, 2019

Author Member

Done

@laanwj

This comment has been minimized.

Copy link
Member

commented May 29, 2019

utACK 5fefdb3

@meshcollider
Copy link
Member

left a comment

utACK

Show resolved Hide resolved src/rpc/util.cpp Outdated

@achow101 achow101 force-pushed the achow101:dont-sw-ms-uncomp branch from 5fefdb3 to 8e45fd8 Jun 5, 2019

@MarcoFalke MarcoFalke closed this Jun 16, 2019

@MarcoFalke MarcoFalke reopened this Jun 16, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

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 achow101 force-pushed the achow101:dont-sw-ms-uncomp branch from 8e45fd8 to 4069662 Jun 18, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

re-utACK 4069662

@kallewoof
Copy link
Member

left a comment

ACK 4069662

Some nits, can be ignored (though would like comment fixed if possible).

@@ -150,8 +151,8 @@ CPubKey AddrToPubKey(CKeyStore* const keystore, const std::string& addr_in)
return vchPubKey;
}

// Creates a multisig redeemscript from a given list of public keys and number required.
CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey>& pubkeys)
// Creates a multisig address from a given list of public keys, number required, and the address type

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Member

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"?

This comment has been minimized.

Copy link
@achow101

achow101 Jun 19, 2019

Author Member

Done


import binascii
import decimal
import itertools

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 19, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Jun 19, 2019

Author Member

A bunch of other tests do system libs after our own 🤷‍♂

@achow101 achow101 force-pushed the achow101:dont-sw-ms-uncomp branch from 4069662 to 0c39c9d Jun 19, 2019

@kallewoof

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

re-ACK 0c39c9d

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@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 functio…
…n 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.

@achow101 achow101 force-pushed the achow101:dont-sw-ms-uncomp branch from 0c39c9d to a495034 Jun 20, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

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

Oops. Should be fixed now.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

re-utACK a495034

@meshcollider meshcollider merged commit a495034 into bitcoin:master Jun 21, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

meshcollider added a commit that referenced this pull request Jun 21, 2019

Merge #16026: Ensure that uncompressed public keys in a multisig alwa…
…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

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 21, 2019

Make and get the multisig redeemscript and destination in one functio…
…n 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.

Github-Pull: bitcoin#16026
Rebased-From: a495034

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 21, 2019

Make and get the multisig redeemscript and destination in one functio…
…n 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.

Github-Pull: bitcoin#16026
Rebased-From: a495034

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 21, 2019

Merge bitcoin#16026: Ensure that uncompressed public keys in a multis…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.