Add warnings to createmultisig and addmultisig if using uncompressed keys #23113

pull meshcollider wants to merge 3 commits into bitcoin:master from meshcollider:202109_createmultisig_warnings changing 4 files +43 −5

meshcollider commented at 2:17 am on September 28, 2021: contributor

Fixes #21368

Currently, if there are any uncompressed keys when calling AddAndGetMultisigDestination, it will just default to a legacy address regardless of the chosen address_type. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a warnings field which will warn the user why their address format is different.
meshcollider added the label Wallet on Sep 28, 2021
meshcollider added the label RPC/REST/ZMQ on Sep 28, 2021

in src/rpc/misc.cpp:106 in f95e5f755b outdated

102@@ -103,6 +103,7 @@ static RPCHelpMan createmultisig()
103                         {RPCResult::Type::STR, "address", "The value of the new multisig address."},
104                         {RPCResult::Type::STR_HEX, "redeemScript", "The string value of the hex-encoded redemption script."},
105                         {RPCResult::Type::STR, "descriptor", "The descriptor for this multisig"},
106+                        {RPCResult::Type::STR, "warnings", "Any warnings resulting from the creation of this multisig"},

laanwj commented at 1:39 pm on September 28, 2021:

I think it would make sense to make warnings an array of strings.

meshcollider commented at 8:53 pm on September 28, 2021:

Can do, although this is in-line with how other RPCs return warnings.

achow101 commented at 7:07 pm on September 29, 2021:

Warnings array is also preferable to me. There are a few other RPCs which return warnings array, and I think we should move to doing that more.

meshcollider commented at 11:47 pm on September 29, 2021:

Done.

laanwj commented at 1:39 pm on September 28, 2021: member

Concept ACK
ghost commented at 1:48 pm on September 28, 2021: none

Concept ACK
achow101 commented at 1:03 am on September 30, 2021: member

ACK 29a78ac431982aa34d909c526c3be6eeba97c34d

in src/rpc/misc.cpp:160 in 29a78ac431 outdated

154@@ -151,6 +155,13 @@ static RPCHelpMan createmultisig()
155     result.pushKV("redeemScript", HexStr(inner));
156     result.pushKV("descriptor", descriptor->ToString());
157 
158+    UniValue warnings(UniValue::VARR);
159+    std::optional<OutputType> created_output_type = OutputTypeFromDestination(dest);
160+    if (!created_output_type || created_output_type.value() != output_type) {

luke-jr commented at 6:00 pm on October 3, 2021:

Might check !request.params[2].isNull() here too, in case a future PR changes the default address type?

luke-jr referenced this in commit 8db937d00e on Oct 10, 2021
luke-jr referenced this in commit e866db948f on Oct 10, 2021
in test/functional/rpc_createmultisig.py:79 in 9c63a8f9f8 outdated
78@@ -79,8 +79,11 @@ def run_test(self): 79 assert_equal(legacy_addr, wmulti0.addmultisigaddress(2, keys, '', 'legacy')['address'])
instagibbs commented at 5:26 am on October 15, 2021:

also assert that there’s no error in the good case

in src/wallet/rpcwallet.cpp:1045 in 29a78ac431 outdated

1038@@ -1035,6 +1039,14 @@ static RPCHelpMan addmultisigaddress()
1039     result.pushKV("address", EncodeDestination(dest));
1040     result.pushKV("redeemScript", HexStr(inner));
1041     result.pushKV("descriptor", descriptor->ToString());
1042+
1043+    UniValue warnings(UniValue::VARR);
1044+    std::optional<OutputType> created_output_type = OutputTypeFromDestination(dest);
1045+    if (!created_output_type || created_output_type.value() != output_type) {

promag commented at 9:38 am on October 16, 2021:

How about

0if (OutputTypeFromDestination(dest) != output_type) {

promag commented at 9:38 am on October 16, 2021: member

Code review ACK 29a78ac431982aa34d909c526c3be6eeba97c34d.
practicalswift commented at 9:48 am on October 16, 2021: contributor

Concept ACK
meshcollider commented at 4:18 am on October 18, 2021: contributor

Feedback addressed in new commit, which ensures the warning message is only returned if an address type is explicitly chosen by the user. Also adds the test @instagibbs requested.
instagibbs approved
instagibbs commented at 12:48 pm on October 18, 2021: member

ACK 2d21025feefbbb33fcf7045b8bfc59b851e6a9db
instagibbs commented at 2:03 pm on October 18, 2021: member

@prayank23 hahaha one sec
promag commented at 8:34 pm on October 18, 2021: member

@meshcollider does it make sense to fail instead?
meshcollider commented at 9:38 pm on October 18, 2021: contributor

@promag I don’t think so personally, a warning is sufficient IMO.
DrahtBot commented at 3:30 pm on December 2, 2021: member

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

Conflicts

No conflicts as of last run.
Add warnings field to createmultisig to warn about uncompressed keys d1a9742623
Add warnings field to addmultisigaddress to warn about uncompressed keys e46fc935aa
Add createmultisig and addmultisigaddress warnings release note d5cab1a96d
meshcollider commented at 4:18 am on December 8, 2021: contributor

Rebased
achow101 commented at 5:00 pm on December 10, 2021: member

ACK d5cab1a96d26e66d342fb5ec35c809bb82869d00
MarcoFalke merged this on Dec 11, 2021
MarcoFalke closed this on Dec 11, 2021
meshcollider deleted the branch on Dec 11, 2021
sidhujag referenced this in commit 7b5b81499d on Dec 11, 2021
RandyMcMillan referenced this in commit db151038c1 on Dec 23, 2021
laanwj referenced this in commit 06ea2783a2 on Jun 6, 2022
sidhujag referenced this in commit 5528dc0a60 on Jun 6, 2022
DrahtBot locked this on Dec 11, 2022

Contributors
meshcollider laanwj achow101 ghost luke-jr instagibbs promag practicalswift DrahtBot

Labels
Wallet RPC/REST/ZMQ

Add warnings to createmultisig and addmultisig if using uncompressed keys #23113

Conflicts