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

brunoerg · 2022-05-26T00:24:34Z

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:

bitcoin/src/rpc/output_script.cpp

Lines 166 to 169 in 192d639

    
           if (!request.params[2].isNull() && OutputTypeFromDestination(dest) != output_type) { 
        
               // Only warns if the user has explicitly chosen an address type we cannot generate 
        
               warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present."); 
        
           }

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

fanquake · 2022-05-30T09:09:01Z

@mruddy

src/rpc/output_script.cpp

mruddy · 2022-06-03T13:24:25Z

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

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

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

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

furszy

Code review ACK 31e57fd

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

brunoerg · 2022-06-04T13:53:46Z

Thanks, @furszy.

Force-pushed addressing it.

src/rpc/output_script.cpp

jonatack

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

diff

diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp
index d86499a35f..f4bb76f50f 100644
--- a/src/rpc/output_script.cpp
+++ b/src/rpc/output_script.cpp
@@ -163,11 +163,11 @@ static RPCHelpMan createmultisig()
             result.pushKV("descriptor", descriptor->ToString());
 
             UniValue warnings(UniValue::VARR);
-            if (!request.params[2].isNull() && *descriptor->GetOutputType() != output_type) {
+            if (descriptor->GetOutputType() != output_type) {
                 // Only warns if the user has explicitly chosen an address type we cannot generate
                 warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
             }
-            if (warnings.size()) result.pushKV("warnings", warnings);
+            if (!warnings.empty()) result.pushKV("warnings", warnings);
 
             return result;
         },
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 66aeb0c962..f25ad59528 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -302,11 +302,11 @@ RPCHelpMan addmultisigaddress()
     result.pushKV("descriptor", descriptor->ToString());
 
     UniValue warnings(UniValue::VARR);
-    if (!request.params[3].isNull() && *descriptor->GetOutputType() != output_type) {
+    if (descriptor->GetOutputType() != output_type) {
         // Only warns if the user has explicitly chosen an address type we cannot generate
         warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
     }
-    if (warnings.size()) result.pushKV("warnings", warnings);
+    if (!warnings.empty()) result.pushKV("warnings", warnings);
 
     return result;
 },
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 77f072dce0..716ee8f7ef 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -91,15 +91,17 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
                 assert 'warnings' not in result
 
             # Generate addresses with the segwit types. These should all make legacy addresses
+            err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."]
+
             for addr_type in ['bech32', 'p2sh-segwit']:
-                result = self.nodes[0].createmultisig(2, keys, addr_type)
+                result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type)
                 assert_equal(legacy_addr, result['address'])
-                assert_equal(result['warnings'], ["Unable to make chosen address type, please ensure no uncompressed public keys are present."])
+                assert_equal(result['warnings'], err_msg)
 
                 if self.is_bdb_compiled():
-                    result = wmulti0.addmultisigaddress(2, keys, '', addr_type)
+                    result = wmulti0.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type)
                     assert_equal(legacy_addr, result['address'])
-                    assert_equal(result['warnings'], ["Unable to make chosen address type, please ensure no uncompressed public keys are present."])
+                    assert_equal(result['warnings'], err_msg)

…multisig and addmultisigaddress

…any warning for expected cases

brunoerg · 2022-06-06T12:49:17Z

Thanks @jonatack and @mruddy for the reviews! Force-pushed addressing them.

jonatack · 2022-06-06T15:00:37Z

ACK 3a9b9bb

laanwj · 2022-06-06T15:12:32Z

Code review ACK 3a9b9bb

maflcko · 2022-06-06T15:17:30Z

tagged for backport

…-segwit in createmultisig 3a9b9bb test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg) eaf6f63 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg) Pull request description: Fixes bitcoin#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, bitcoin#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. ACKs for top commit: jonatack: ACK 3a9b9bb laanwj: Code review ACK 3a9b9bb Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369

…multisig and addmultisigaddress Github-Pull: bitcoin#25220 Rebased-From: eaf6f63

…any warning for expected cases Github-Pull: bitcoin#25220 Rebased-From: 3a9b9bb

fanquake · 2022-06-09T11:26:45Z

Backported in #25316.

4ebf6e3 p2p: always set nTime for self-advertisements (Martin Zumsande) 039ef21 tests: Use descriptor that requires both legacy and segwit (Andrew Chow) 5fd25eb tests: Calculate input weight more accurately (Andrew Chow) bd6d3ac windeploy: Renewed windows code signing certificate (Andrew Chow) 32fa522 test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg) 7658055 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg) Pull request description: Backports: - #24454 - #25201 - #25220 - #25314 ACKs for top commit: LarryRuane: re-utACK 4ebf6e3 achow101: ACK 4ebf6e3 Tree-SHA512: add3999d0330b3442f3894fce38ad9b5adc75da7d681c949e1d052bac5520c2c6fb06eba98bfbeb4aa9a560170451d24bf00d08dddd4a3d080030ecb8ad61882

DrahtBot added the RPC/REST/ZMQ label May 26, 2022

brunoerg force-pushed the 2022-05-fix-incorrect-warning-createmultisig branch from 704b064 to 794fb31 Compare May 26, 2022 12:17

laanwj reviewed May 31, 2022

View reviewed changes

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

brunoerg force-pushed the 2022-05-fix-incorrect-warning-createmultisig branch from 794fb31 to 31e57fd Compare May 31, 2022 16:18

brunoerg requested review from laanwj and sipa May 31, 2022 16:20

furszy reviewed Jun 3, 2022

View reviewed changes

brunoerg force-pushed the 2022-05-fix-incorrect-warning-createmultisig branch from 31e57fd to f105d14 Compare June 4, 2022 13:53

mruddy reviewed Jun 5, 2022

View reviewed changes

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

jonatack reviewed Jun 6, 2022

View reviewed changes

brunoerg added 2 commits June 6, 2022 09:46

rpc: fix inappropriate warning for address type p2sh-segwit in create…

eaf6f63

…multisig and addmultisigaddress

test: ensure createmultisig and addmultisigaddress are not returning …

3a9b9bb

…any warning for expected cases

brunoerg force-pushed the 2022-05-fix-incorrect-warning-createmultisig branch from f105d14 to 3a9b9bb Compare June 6, 2022 12:47

laanwj merged commit 06ea278 into bitcoin:master Jun 6, 2022

maflcko added this to the 23.1 milestone Jun 6, 2022

maflcko added the Needs backport (23.x) label Jun 6, 2022

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 9, 2022

rpc: fix inappropriate warning for address type p2sh-segwit in create…

7658055

…multisig and addmultisigaddress Github-Pull: bitcoin#25220 Rebased-From: eaf6f63

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 9, 2022

test: ensure createmultisig and addmultisigaddress are not returning …

32fa522

…any warning for expected cases Github-Pull: bitcoin#25220 Rebased-From: 3a9b9bb

fanquake mentioned this pull request Jun 9, 2022

23.x backports #25316

Merged

fanquake removed the Needs backport (23.x) label Jun 9, 2022

bitcoin locked and limited conversation to collaborators Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

brunoerg commented May 26, 2022 •

edited

Loading

Uh oh!

fanquake commented May 30, 2022

Uh oh!

Uh oh!

mruddy commented Jun 3, 2022 •

edited

Loading

Uh oh!

furszy left a comment

Uh oh!

brunoerg commented Jun 4, 2022

Uh oh!

Uh oh!

jonatack left a comment •

edited

Loading

Uh oh!

brunoerg commented Jun 6, 2022

Uh oh!

jonatack commented Jun 6, 2022

Uh oh!

laanwj commented Jun 6, 2022

Uh oh!

maflcko commented Jun 6, 2022

Uh oh!

fanquake commented Jun 9, 2022

Uh oh!

Uh oh!

	if (!request.params[2].isNull() && OutputTypeFromDestination(dest) != output_type) {
	// Only warns if the user has explicitly chosen an address type we cannot generate
	warnings.push_back("Unable to make chosen address type, please ensure no uncompressed public keys are present.");
	}

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

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

Uh oh!

Conversation

brunoerg commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented May 30, 2022

Uh oh!

Uh oh!

mruddy commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Jun 4, 2022

Uh oh!

Uh oh!

jonatack 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!

brunoerg commented Jun 6, 2022

Uh oh!

jonatack commented Jun 6, 2022

Uh oh!

laanwj commented Jun 6, 2022

Uh oh!

maflcko commented Jun 6, 2022

Uh oh!

fanquake commented Jun 9, 2022

Uh oh!

Uh oh!

brunoerg commented May 26, 2022 •

edited

Loading

mruddy commented Jun 3, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading