Add address type option to addmultisigaddress #12213

promag commented at 11:40 PM on January 17, 2018: member

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 renamed this:
~~2018 01 addmultisigaddress address type~~
Add address type option to addmultisigaddress
on Jan 17, 2018

promag commented at 12:00 AM on January 18, 2018: member

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

olledik commented at 3:34 AM on January 18, 2018: none

👍

laanwj added the label RPC/REST/ZMQ on Jan 18, 2018

laanwj added the label Wallet on Jan 18, 2018

laanwj added this to the milestone 0.16.0 on Jan 18, 2018

jonasschnelli approved

jonasschnelli commented at 7:39 PM on January 18, 2018: contributor

utACK d1d17607eba0cae096bfa0d2dbdc977a071385bd

MarcoFalke commented at 9:02 PM on January 18, 2018: member

Concept ACK. Should probably do the same for createmultisig?

promag commented at 9:50 PM on January 18, 2018: member

In this PR?

MarcoFalke commented at 10:10 PM on January 18, 2018: member

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 commented at 10:21 PM on January 18, 2018: member

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.

MarcoFalke commented at 10:28 PM on January 18, 2018: member

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 commented at 9:19 AM on January 19, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/d1d17607eba0cae096bfa0d2dbdc977a071385bd And createmultisig change suggested by Marco would be nice too IMO

in src/wallet/rpcwallet.cpp:1190 in d1d17607eb outdated

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

laanwj commented at 11:18 AM on January 19, 2018:

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

MarcoFalke commented at 7:38 PM on January 19, 2018:

Maybe even add a test for all new options

meshcollider commented at 1:09 PM on January 22, 2018:

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

ryanofsky commented at 7:13 PM on January 19, 2018: member

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 commented at 7:41 PM on January 19, 2018: member

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 commented at 7:56 PM on January 19, 2018: member

@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 force-pushed on Jan 21, 2018

promag commented at 10:19 PM on January 21, 2018: member

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

promag renamed this:
~~Add address type option to addmultisigaddress~~
Add address type to addmultisigaddress and createmultisig
on Jan 21, 2018

promag force-pushed on Jan 22, 2018

in src/rpc/misc.cpp:341 in 27b8162f25 outdated

 340 |              "     [\n"
 341 | -            "       \"key\"    (string) bitcoin address or hex-encoded public key\n"
 342 | +            "       \"key\"      (string) bitcoin address or hex-encoded public key\n"
 343 |              "       ,...\n"
 344 |              "     ]\n"
 345 | +            "4. \"address_type\" (string, optional) The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -addresstype.\n"

meshcollider commented at 11:13 AM on January 22, 2018:

Should be 3. not 4.

promag commented at 11:18 AM on January 22, 2018: member

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

meshcollider commented at 1:10 PM on January 22, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12213/commits/27b8162f257454b73e6e768c6f4e86ee5b7c64c0 modulo comments below and awaiting tests

promag force-pushed on Jan 22, 2018

jnewbery commented at 5:08 PM on January 22, 2018: member

needs rebase and comments addressed

fanquake deleted a comment on Jan 23, 2018

jnewbery commented at 9:07 PM on January 23, 2018: member

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 commented at 9:41 PM on January 23, 2018: member

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 commented at 10:49 PM on January 23, 2018: member

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 commented at 12:12 PM on January 24, 2018: member

Needs rebase.

meshcollider commented at 12:13 PM on January 24, 2018: contributor

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

promag force-pushed on Jan 24, 2018

[rpc] Add address type option to addmultisigaddress 886a92f25f

promag force-pushed on Jan 24, 2018

promag renamed this:
~~Add address type to addmultisigaddress and createmultisig~~
Add address type option to addmultisigaddress
on Jan 24, 2018

in test/functional/nulldummy.py:51 in bca85e6b7d outdated

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

ryanofsky commented at 4:05 PM on January 24, 2018:

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

promag commented at 4:11 PM on January 24, 2018:

Right, like above.

promag force-pushed on Jan 24, 2018

ryanofsky commented at 4:18 PM on January 24, 2018: member

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

[qa] Use address type in addmultisigaddress to avoid addwitnessaddress f523c6bec0

promag force-pushed on Jan 24, 2018

in test/functional/nulldummy.py:51 in f523c6bec0

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

jnewbery commented at 5:27 PM on January 24, 2018:

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

jnewbery commented at 5:32 PM on January 24, 2018: member

Tested ACK f523c6bec005b2c999651787d029dee0fe719eb1. One nit in the tests.

jonasschnelli commented at 7:01 PM on January 24, 2018: contributor

Re-utACK f523c6bec

jonasschnelli merged this on Jan 24, 2018

jonasschnelli closed this on Jan 24, 2018

jonasschnelli referenced this in commit eadb2dacc3 on Jan 24, 2018

MarcoFalke commented at 8:18 PM on January 24, 2018: member

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

promag commented at 9:00 PM on January 24, 2018: member

@MarcoFalke not yet.

promag deleted the branch on Jan 24, 2018

DrahtBot locked this on Sep 8, 2021