rpc: fix incorrect warning for address type p2sh-segwit in createmultisig #25220

pull brunoerg wants to merge 2 commits into bitcoin:master from brunoerg:2022-05-fix-incorrect-warning-createmultisig changing 3 files +11 −8
  1. brunoerg commented at 0:24 am on May 26, 2022: member

    Fixes #25127

    If there are any uncompressed keys when calling AddAndGetMultisigDestination, it will just default to a legacy address regardless of the chosen address_type. So, #23113 added a warnings field which will warn the user why their address format is different.

    However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (OutputTypeFromDestination), it returns ScriptHash for both legacy and P2SH_SEGWIT. So, since P2SH_SEGWIT is different from ScriptHash, it returns the warning: https://github.com/bitcoin/bitcoin/blob/192d639a6b1bd0feaa52e6ea4e63e33982704c32/src/rpc/output_script.cpp#L166-L169

    So, to avoid this mistake I changed OutputTypeFromDestination to descriptor->GetOutputType() to get the appropriate output type.

  2. DrahtBot added the label RPC/REST/ZMQ on May 26, 2022
  3. brunoerg force-pushed on May 26, 2022
  4. fanquake commented at 9:09 am on May 30, 2022: member
    @mruddy
  5. in src/rpc/output_script.cpp:169 in 794fb31a12 outdated 
    167             UniValue warnings(UniValue::VARR);
168             if (!request.params[2].isNull() && OutputTypeFromDestination(dest) != output_type) {
169-                // Only warns if the user has explicitly chosen an address type we cannot generate
170-                warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
171+                // Ensure it won't cause an incorrect warning for p2sh-segwit
172+                if (descriptor_str.find("wsh") == std::string::npos) {
    laanwj commented at 12:27 pm on May 31, 2022:
    I don’t particularly like using a find on the string to check the kind of descriptor here. It seems a bit fragile. Isn’t there a more formal, well-defined way?
    sipa commented at 3:27 pm on May 31, 2022:
    descriptor->GetOutputType() ?
    brunoerg commented at 4:13 pm on May 31, 2022:
    Just checked here and descriptor->GetOutputType() works well. I am changing the approach, thanks!
  6. brunoerg force-pushed on May 31, 2022
  7. brunoerg requested review from laanwj on May 31, 2022
  8. brunoerg requested review from sipa on May 31, 2022
  9. in src/rpc/output_script.cpp:166 in 31e57fdfb1 outdated 
    162@@ -163,7 +163,7 @@ static RPCHelpMan createmultisig()
163             result.pushKV("descriptor", descriptor->ToString());
164 
165             UniValue warnings(UniValue::VARR);
166-            if (!request.params[2].isNull() && OutputTypeFromDestination(dest) != output_type) {
167+            if (!request.params[2].isNull() && *descriptor->GetOutputType() != output_type) {
    mruddy commented at 1:22 pm on June 3, 2022:

    i prefer this: if (descriptor->GetOutputType() != output_type) {

    even if you keep the isNull check (i don’t see the need to), you don’t need to apply the * operator here, it’ll be applied within the !=

    brunoerg commented at 12:48 pm on June 6, 2022:
    thanks, done!
  10. mruddy commented at 1:24 pm on June 3, 2022: contributor

    I tested and left one review. Thanks for working on this. I’ll test again later. Here are two easy test cases:

    0createmultisig 2 '["0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"]' 'p2sh-segwit'

    0createmultisig 2 '["0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8"]' 'p2sh-segwit'

    EDIT: oops, just submitted the review that i had left pending

  11. furszy commented at 8:43 pm on June 3, 2022: member

    Code review ACK 31e57fdf

    Can apply the same changes to addmultisigaddress (wallet/rpc/addresses.cpp line 305).

  12. brunoerg commented at 1:53 pm on June 4, 2022: member

    Thanks, @furszy.

    Force-pushed addressing it.

  13. brunoerg force-pushed on Jun 4, 2022
  14. jonatack commented at 8:42 am on June 6, 2022: member

    ACK with some suggested improvements, and “addmultisigaddress” should be mentioned in the commit message(s).

     0diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp
 1index d86499a35f..f4bb76f50f 100644
 2--- a/src/rpc/output_script.cpp
 3+++ b/src/rpc/output_script.cpp
 4@@ -163,11 +163,11 @@ static RPCHelpMan createmultisig()
 5             result.pushKV("descriptor", descriptor->ToString());
 6 
 7             UniValue warnings(UniValue::VARR);
 8-            if (!request.params[2].isNull() && *descriptor->GetOutputType() != output_type) {
 9+            if (descriptor->GetOutputType() != output_type) {
10                 // Only warns if the user has explicitly chosen an address type we cannot generate
11                 warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
12             }
13-            if (warnings.size()) result.pushKV("warnings", warnings);
14+            if (!warnings.empty()) result.pushKV("warnings", warnings);
15 
16             return result;
17         },
18diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
19index 66aeb0c962..f25ad59528 100644
20--- a/src/wallet/rpc/addresses.cpp
21+++ b/src/wallet/rpc/addresses.cpp
22@@ -302,11 +302,11 @@ RPCHelpMan addmultisigaddress()
23     result.pushKV("descriptor", descriptor->ToString());
24 
25     UniValue warnings(UniValue::VARR);
26-    if (!request.params[3].isNull() && *descriptor->GetOutputType() != output_type) {
27+    if (descriptor->GetOutputType() != output_type) {
28         // Only warns if the user has explicitly chosen an address type we cannot generate
29         warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
30     }
31-    if (warnings.size()) result.pushKV("warnings", warnings);
32+    if (!warnings.empty()) result.pushKV("warnings", warnings);
33 
34     return result;
35 },
36diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
37index 77f072dce0..716ee8f7ef 100755
38--- a/test/functional/rpc_createmultisig.py
39+++ b/test/functional/rpc_createmultisig.py
40@@ -91,15 +91,17 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
41                 assert 'warnings' not in result
42 
43             # Generate addresses with the segwit types. These should all make legacy addresses
44+            err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."]
45+
46             for addr_type in ['bech32', 'p2sh-segwit']:
47-                result = self.nodes[0].createmultisig(2, keys, addr_type)
48+                result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type)
49                 assert_equal(legacy_addr, result['address'])
50-                assert_equal(result['warnings'], ["Unable to make chosen address type, please ensure no uncompressed public keys are present."])
51+                assert_equal(result['warnings'], err_msg)
52 
53                 if self.is_bdb_compiled():
54-                    result = wmulti0.addmultisigaddress(2, keys, '', addr_type)
55+                    result = wmulti0.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type)
56                     assert_equal(legacy_addr, result['address'])
57-                    assert_equal(result['warnings'], ["Unable to make chosen address type, please ensure no uncompressed public keys are present."])
58+                    assert_equal(result['warnings'], err_msg)
  15. rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress eaf6f630c0
  16. test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases 3a9b9bb38e
  17. brunoerg force-pushed on Jun 6, 2022
  18. brunoerg commented at 12:49 pm on June 6, 2022: member
    Thanks @jonatack and @mruddy for the reviews! Force-pushed addressing them.
  19. jonatack commented at 3:00 pm on June 6, 2022: member
    ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
  20. laanwj commented at 3:12 pm on June 6, 2022: member
    Code review ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
  21. laanwj merged this on Jun 6, 2022
  22. laanwj closed this on Jun 6, 2022
  23. MarcoFalke added this to the milestone 23.1 on Jun 6, 2022
  24. MarcoFalke added the label Needs backport (23.x) on Jun 6, 2022
  25. MarcoFalke commented at 3:17 pm on June 6, 2022: member
    tagged for backport
  26. sidhujag referenced this in commit 5528dc0a60 on Jun 6, 2022
  27. fanquake referenced this in commit 7658055c4e on Jun 9, 2022
  28. fanquake referenced this in commit 32fa522a80 on Jun 9, 2022
  29. fanquake commented at 11:26 am on June 9, 2022: member
    Backported in #25316.
  30. fanquake removed the label Needs backport (23.x) on Jun 9, 2022
  31. MarcoFalke referenced this in commit a33ec8a693 on Jul 8, 2022
  32. DrahtBot locked this on Jun 9, 2023


brunoerg fanquake laanwj sipa mruddy furszy jonatack MarcoFalke


laanwj sipa

Labels
RPC/REST/ZMQ

Milestone
23.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me