Add address type option to addmultisigaddress #12213

promag · 2018-01-17T23:40:31Z

Adds the option address_type to addmultisigaddress and createmultisg RPC. This also allows to avoid addwitnessaddress to obtain an p2sh-segwit or bech32 multsig address.

Related to #12210 as this reduces addwitnessaddress usage.

promag · 2018-01-18T00:00:48Z

Another option to avoid addwitnessaddress is to have another node with addresstype=.... Not sure which is preferable.

olledik · 2018-01-18T03:34:58Z

👍

jonasschnelli

utACK d1d17607eba0cae096bfa0d2dbdc977a071385bd

maflcko · 2018-01-18T21:02:16Z

Concept ACK. Should probably do the same for createmultisig?

promag · 2018-01-18T21:50:26Z

In this PR?

maflcko · 2018-01-18T22:10:10Z

I mean they are related. To me it makes sense to update atomically, since I fail to see why you'd want the one but not the other.

promag · 2018-01-18T22:21:58Z

The test framework only calls addwitnessaddress on multisig addresses returned by addmultisigaddress, and this pull changes that.

But I guess it can be extended to createmultisig.

maflcko · 2018-01-18T22:28:32Z

When this is only about tests, we can untag from 0.16, imo. When this is about a needed feature for users, I don't see why createmultisig shouldn't support it.

meshcollider · 2018-01-19T09:19:10Z

utACK d1d1760
And createmultisig change suggested by Marco would be nice too IMO

laanwj · 2018-01-19T11:18:10Z

src/wallet/rpcwallet.cpp

@@ -1188,6 +1187,7 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
            "       ...,\n"
            "     ]\n"
            "3. \"account\"      (string, optional) DEPRECATED. An account to assign the addresses to.\n"
+            "4. \"address_type\" (string, optional) The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -addresstype.\n"


You need to update the RPC command table as well with this added argument, to make named arguments work.

Maybe even add a test for all new options

@promag you missed this update to the RPC command table for addmultisigaddress

ryanofsky

Conditional utACK d1d17607eba0cae096bfa0d2dbdc977a071385bd: this shouldn't be merged until the named argument fix Wladimir suggested is added.

Also agree createmultisig update would be nice here.

Sjors · 2018-01-19T19:41:33Z

If I set the default address type to bech32 and then create a 1 of 2 multisig address using a wallet address and the public key of some external bech32 address, and I set the type to legacy:

bitcoin-cli addmultisigaddress 1 '["bcrt1q0uh7xeh8jmnfzd0se9n39ut9n64sf7mt3ndhal","034c534e0cd400f69a948f7d6eecb891deca140c9e2035658033d5ebbaccd88058"]' "" legacy

The result of validateaddress is confusing:

{
  "isvalid": true,
  "address": "2NEso1BnKq8kPCyZqMaYzw8E6Vf5N4KCZk1",
  "scriptPubKey": "a914ed452d142f0601587ba077293a79caa35503797787",
  "ismine": false,
  "iswatchonly": false,
  "isscript": true,
  "iswitness": false,
  "script": "multisig",
  "hex": "512102bdb9ca3346c02befe78dbe4e47b6569df8cb312aebf165dc90439287910cc8d921034c534e0cd400f69a948f7d6eecb891deca140c9e2035658033d5ebbaccd8805852ae",
  "sigsrequired": 1,
  "pubkeys": [
    "02bdb9ca3346c02befe78dbe4e47b6569df8cb312aebf165dc90439287910cc8d9",
    "034c534e0cd400f69a948f7d6eecb891deca140c9e2035658033d5ebbaccd88058"
  ],
  "addresses": [
    "ms7TUkdntfebxKQw1oG685Kimdt34VVUaA",
    "muQMALGUmB3n4YLcAHUQrM9BHNF3VCEr5z"
  ],
  "account": ""
}

The only thing I recognize is the pub key of the external address. I haven't tried spending from it. The wallet doesn't know what address type was used by the external wallet, but shouldn't it at least know this about itself?

When setting the multisig address type to bech32 the result makes more sense, because it leaves out the address field:

bitcoin-cli addmultisigaddress 1 '["bcrt1q0uh7xeh8jmnfzd0se9n39ut9n64sf7mt3ndhal","034c534e0cd400f69a948f7d6eecb891deca140c9e2035658033d5ebbaccd88058"]' "" bech32

{
  "isvalid": true,
  "address": "bcrt1qdpzf4v83kq9gfamz53m4x6wx2rqw72pw87a5jf603g9ckp0j3jmqp0680q",
  "scriptPubKey": "002068449ab0f1b00a84f762a4775369c650c0ef282e3fbb49274f8a0b8b05f28cb6",
  "ismine": false,
  "iswatchonly": false,
  "isscript": true,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "68449ab0f1b00a84f762a4775369c650c0ef282e3fbb49274f8a0b8b05f28cb6",
  "script": "multisig",
  "hex": "512102bdb9ca3346c02befe78dbe4e47b6569df8cb312aebf165dc90439287910cc8d921034c534e0cd400f69a948f7d6eecb891deca140c9e2035658033d5ebbaccd8805852ae",
  "sigsrequired": 1,
  "pubkeys": [
    "02bdb9ca3346c02befe78dbe4e47b6569df8cb312aebf165dc90439287910cc8d9",
    "034c534e0cd400f69a948f7d6eecb891deca140c9e2035658033d5ebbaccd88058"
  ],
  "account": ""
}

Not sure if this is even related to this particular change.

sipa · 2018-01-19T19:56:09Z

@Sjors The 'addresses' field is very confusing, and I'd like to get rid of it, but it's there for backward compatibility. Historically there was no difference between addresses (shorthands for scriptPubKeys) and key identifiers, and it lives on in a number of calls. In this case, it's effectively the same as the 'pubkeys' field (which was added in #11403), but encoding the public keys as P2PKH addresses.

promag · 2018-01-21T22:19:26Z

Pushed createmultisig with address_type option. Please review. Meanwhile I'll add tests.

promag · 2018-01-22T11:18:48Z

@sipa currently when ENABLE_WALLET is false, createmultisig will always create legacy addresses. WDYT?

meshcollider

utACK 27b8162 modulo comments below and awaiting tests

meshcollider · 2018-01-22T11:13:25Z

src/rpc/misc.cpp

            "       ,...\n"
            "     ]\n"
+            "4. \"address_type\" (string, optional) The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -addresstype.\n"


Should be 3. not 4.

meshcollider · 2018-01-22T13:09:00Z

src/wallet/rpcwallet.cpp

@@ -1188,6 +1187,7 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
            "       ...,\n"
            "     ]\n"
            "3. \"account\"      (string, optional) DEPRECATED. An account to assign the addresses to.\n"
+            "4. \"address_type\" (string, optional) The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -addresstype.\n"


@promag you missed this update to the RPC command table for addmultisigaddress

jnewbery · 2018-01-22T17:08:11Z

needs rebase and comments addressed

jnewbery · 2018-01-23T21:07:51Z

utACK the first and third commits:

[rpc] Add address type option to addmultisigaddress
[qa] Use address type in addmultisigaddress to avoid addwitnessaddress

I'm NACKish on the second commit ([rpc] Add address type option to createmultisig) because it adds a wallet dependency to server which will be difficult to unpick, and will make substatial work for #11415.

promag · 2018-01-23T21:41:37Z

I can move [rpc] Add address type option to createmultisig to a new PR so it doesn't hold the rest. I don't feel both must be updated in one shot.

jnewbery · 2018-01-23T22:49:46Z

I can move [rpc] Add address type option to createmultisig to a new PR

I'd be in favour of that, but several other contributors asked for the opposite (for createmultisig to be added to this PR). I think wait to hear what they think.

laanwj · 2018-01-24T12:12:00Z

Needs rebase.

meshcollider · 2018-01-24T12:13:40Z

I'd be in favour of that, but several other contributors asked for the opposite (for createmultisig to be added to this PR). I think wait to hear what they think.

On second thought yeah, the wallet dependency complicates that part so separating it out for separate discussion would be good, makes this PR easier to merge

ryanofsky · 2018-01-24T16:05:31Z

test/functional/nulldummy.py

@@ -48,7 +48,7 @@ def run_test(self):
        self.address = self.nodes[0].getnewaddress()
        self.ms_address = self.nodes[0].addmultisigaddress(1,[self.address])['address']
        self.wit_address = self.nodes[0].addwitnessaddress(self.address)
-        self.wit_ms_address = self.nodes[0].addwitnessaddress(self.ms_address)
+        self.wit_ms_address = self.nodes[0].addmultisigaddress(1, [self.address], '', 'p2sh-segwit')


This line currently causes failure, but appending ['address'] fixes it. Similar changes are needed in segwit.py. This is due to #11415 merge.

Right, like above.

ryanofsky

Conditional utACK 5afbff9755768d191275736a19db078c139a5ec3 if segwit.py test is fixed. Only change since last review is rebase and fixing CRPCCommand args.

jnewbery

Tested ACK f523c6b. One nit in the tests.

jnewbery · 2018-01-24T17:27:55Z

test/functional/nulldummy.py

@@ -48,7 +48,7 @@ def run_test(self):
        self.address = self.nodes[0].getnewaddress()
        self.ms_address = self.nodes[0].addmultisigaddress(1,[self.address])['address']
        self.wit_address = self.nodes[0].addwitnessaddress(self.address)
-        self.wit_ms_address = self.nodes[0].addwitnessaddress(self.ms_address)
+        self.wit_ms_address = self.nodes[0].addmultisigaddress(1, [self.address], '', 'p2sh-segwit')['address']


nit: use named arguments (specifically, it's not clear what the third argument '' is for). Same for segwit.py

jonasschnelli · 2018-01-24T19:01:22Z

Re-utACK f523c6b

f523c6b [qa] Use address type in addmultisigaddress to avoid addwitnessaddress (João Barbosa) 886a92f [rpc] Add address type option to addmultisigaddress (João Barbosa) Pull request description: Adds the option `address_type` to `addmultisigaddress` and `createmultisg` RPC. This also allows to avoid `addwitnessaddress` to obtain an `p2sh-segwit` or `bech32` multsig address. Related to #12210 as this reduces `addwitnessaddress` usage. Tree-SHA512: 8f8f85dfcff66bb6c7e1e9865e37c285dead1d6dadb9672a89b92fa209d03cc35817ca1d656588c6c2146b728daaf7540b851929b640294653c62836cbefe7ee

maflcko · 2018-01-24T20:18:17Z

Is there a pull request for createmultisig, or am I blind?

promag · 2018-01-24T21:00:05Z

@MarcoFalke not yet.

promag changed the title ~~2018 01 addmultisigaddress address type~~ Add address type option to addmultisigaddress Jan 17, 2018

promag mentioned this pull request Jan 17, 2018

wallet: Deprecate addwitnessaddress #12210

Merged

laanwj added RPC/REST/ZMQ Wallet labels Jan 18, 2018

laanwj added this to the 0.16.0 milestone Jan 18, 2018

jonasschnelli approved these changes Jan 18, 2018

View reviewed changes

laanwj reviewed Jan 19, 2018

View reviewed changes

ryanofsky reviewed Jan 19, 2018

View reviewed changes

promag force-pushed the 2018-01-addmultisigaddress-address-type branch from d1d1760 to 5fee1f9 Compare January 21, 2018 22:18

promag changed the title ~~Add address type option to addmultisigaddress~~ Add address type to addmultisigaddress and createmultisig Jan 21, 2018

promag force-pushed the 2018-01-addmultisigaddress-address-type branch 5 times, most recently from f188594 to 27b8162 Compare January 22, 2018 10:43

meshcollider reviewed Jan 22, 2018

View reviewed changes

promag force-pushed the 2018-01-addmultisigaddress-address-type branch from 27b8162 to 363a438 Compare January 22, 2018 13:32

bitcoin deleted a comment Jan 23, 2018

ajtowns mentioned this pull request Jan 23, 2018

[tests] Rename functional tests #11774

Merged

jnewbery mentioned this pull request Jan 23, 2018

[RPC] Disallow using addresses in createmultisig #11415

Merged

promag force-pushed the 2018-01-addmultisigaddress-address-type branch 3 times, most recently from 6066545 to 30a4b23 Compare January 24, 2018 14:42

[rpc] Add address type option to addmultisigaddress

886a92f

promag force-pushed the 2018-01-addmultisigaddress-address-type branch from 30a4b23 to bca85e6 Compare January 24, 2018 14:45

promag changed the title ~~Add address type to addmultisigaddress and createmultisig~~ Add address type option to addmultisigaddress Jan 24, 2018

ryanofsky reviewed Jan 24, 2018

View reviewed changes

promag force-pushed the 2018-01-addmultisigaddress-address-type branch from bca85e6 to 5afbff9 Compare January 24, 2018 16:12

ryanofsky reviewed Jan 24, 2018

View reviewed changes

[qa] Use address type in addmultisigaddress to avoid addwitnessaddress

f523c6b

promag force-pushed the 2018-01-addmultisigaddress-address-type branch from 5afbff9 to f523c6b Compare January 24, 2018 16:24

jnewbery reviewed Jan 24, 2018

View reviewed changes

jonasschnelli merged commit f523c6b into bitcoin:master Jan 24, 2018

promag deleted the 2018-01-addmultisigaddress-address-type branch January 24, 2018 21:02

str4d mentioned this pull request Mar 6, 2018

Add Sapling keys and addresses to the wallet zcash/zcash#3059

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add address type option to addmultisigaddress #12213

Add address type option to addmultisigaddress #12213

Uh oh!

Conversation

promag commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 18, 2018

Uh oh!

olledik commented Jan 18, 2018

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 18, 2018

Uh oh!

promag commented Jan 18, 2018

Uh oh!

maflcko commented Jan 18, 2018

Uh oh!

promag commented Jan 18, 2018

Uh oh!

maflcko commented Jan 18, 2018

Uh oh!

meshcollider commented Jan 19, 2018

Uh oh!

laanwj Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

meshcollider Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 19, 2018

Uh oh!

sipa commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 21, 2018

Uh oh!

promag commented Jan 22, 2018

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

meshcollider Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jan 22, 2018

Uh oh!

jnewbery commented Jan 23, 2018

Uh oh!

promag commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jan 23, 2018

Uh oh!

laanwj commented Jan 24, 2018

Uh oh!

meshcollider commented Jan 24, 2018

Uh oh!

ryanofsky Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 17, 2018 •

edited

Loading

sipa commented Jan 19, 2018 •

edited

Loading

promag commented Jan 23, 2018 •

edited

Loading

ryanofsky Jan 24, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading