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

achow101 · 2019-05-15T04:29:54Z

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 · 2019-05-15T04:42:10Z

This sounds right to me.

Empact · 2019-05-15T19:09:58Z

src/rpc/util.cpp

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.

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

laanwj · 2019-05-29T11:54:51Z

utACK 5fefdb3286b617a74dd395381b1be0b29f21b0e6

meshcollider

utACK

src/rpc/util.cpp

DrahtBot · 2019-06-18T05:07:59Z

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.

meshcollider · 2019-06-18T23:42:04Z

re-utACK 4069662

kallewoof

ACK 40696620703e7339957177e95a9acb1d8be80e0e

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

kallewoof · 2019-06-19T13:39:35Z

src/rpc/util.cpp

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

kallewoof · 2019-06-19T13:46:28Z

test/functional/rpc_createmultisig.py

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

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

kallewoof · 2019-06-20T04:02:17Z

re-ACK 0c39c9d

meshcollider · 2019-06-20T08:43:44Z

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

…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 · 2019-06-20T15:02:17Z

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

Oops. Should be fixed now.

meshcollider · 2019-06-21T07:43:59Z

re-utACK a495034

…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

…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

…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

…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

…lways returns a legacy address Summary: a49503402b6bc21e3878e151c07529941d36aed0 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 --- Backport of Core [[bitcoin/bitcoin#16026 | PR16026]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6552

fanquake added the RPC/REST/ZMQ label May 15, 2019

maflcko added the Needs backport label May 15, 2019

maflcko added this to the 0.18.1 milestone May 15, 2019

Empact reviewed May 15, 2019

View reviewed changes

meshcollider reviewed Jun 5, 2019

View reviewed changes

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

achow101 force-pushed the dont-sw-ms-uncomp branch from 5fefdb3 to 8e45fd8 Compare June 5, 2019 13:12

maflcko closed this Jun 16, 2019

maflcko reopened this Jun 16, 2019

achow101 force-pushed the dont-sw-ms-uncomp branch from 8e45fd8 to 4069662 Compare June 18, 2019 15:38

meshcollider mentioned this pull request Jun 18, 2019

rpc: addmultisigaddress should fallback to OutputType::LEGACY when uncompressed public keys are provided #16012

Closed

kallewoof approved these changes Jun 19, 2019

View reviewed changes

achow101 force-pushed the dont-sw-ms-uncomp branch from 4069662 to 0c39c9d Compare June 19, 2019 22:05

achow101 force-pushed the dont-sw-ms-uncomp branch from 0c39c9d to a495034 Compare June 20, 2019 15:02

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

maflcko removed the Needs backport label Jun 21, 2019

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

achow101 commented May 15, 2019

Uh oh!

gmaxwell commented May 15, 2019

Uh oh!

Empact May 15, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj May 29, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 29, 2019

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider commented Jun 18, 2019

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

kallewoof Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jun 20, 2019

Uh oh!

meshcollider commented Jun 20, 2019

Uh oh!

achow101 commented Jun 20, 2019

Uh oh!

meshcollider commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Jun 18, 2019 •

edited

Loading