Repository: bitcoin/bitcoin (language: cpp)

Issue:
rpc: createmultisig adds incorrect warning for address type p2sh-segwit
The `createmultisig` RPC adds a spurious warning message when creating for address type p2sh-segwit.

```
createmultisig 2 '["0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"]' 'p2sh-segwit'
{
  "address": "3QfZQY7wQrBGEUB7E5vVLggeeAUVZ1Bbg9",
  "redeemScript": "52210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f8179852ae",
  "descriptor": "sh(wsh(multi(2,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)))#m8ww0c9m",
  "warnings": [
    "Unable to make chosen address type, please ensure no uncompressed public keys are present."
  ]
}
```

I think this traces back to https://github.com/bitcoin/bitcoin/pull/23113

In the code, the comparison at https://github.com/bitcoin/bitcoin/blob/225e5b57b2ee2bc1acd7f09c89ccccc15ef8c85f/src/rpc/output_script.cpp#L166 fails since `output_type` is `OutputType::P2SH_SEGWIT` and https://github.com/bitcoin/bitcoin/blob/225e5b57b2ee2bc1acd7f09c89ccccc15ef8c85f/src/outputtype.cpp#L112 returns `OutputType::LEGACY`

**Expected behavior**

No warning message. For example:

```
createmultisig 2 '["0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"]' 'p2sh-segwit'
{
  "address": "3QfZQY7wQrBGEUB7E5vVLggeeAUVZ1Bbg9",
  "redeemScript": "52210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f8179852ae",
  "descriptor": "sh(wsh(multi(2,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)))#m8ww0c9m"
}
```


Discussion / hints:
Hello, @mruddy! 

I've tested it with the same data you showed here and got same thing. I use a debugger to understand better what is happening and noticed:

1 - When defining `dest`, it uses `AddAndGetMultisigDestination`
https://github.com/bitcoin/bitcoin/blob/3aa851ad2ae836f2bb8071396efec8b67347adcd/src/rpc/output_script.cpp#L155

2 - In `AddAndGetMultisigDestination`, I just noticed it goes to `AddAndGetDestinationForScript`.

https://github.com/bitcoin/bitcoin/blob/3aa851ad2ae836f2bb8071396efec8b67347adcd/src/rpc/util.cpp#L263

3 - In `AddAndGetDestinationForScript`, it returns an `Active Type =  ScriptHash`
```cpp
    case OutputType::P2SH_SEGWIT:
    case OutputType::BECH32: {
        CTxDestination witdest = WitnessV0ScriptHash(script);
        CScript witprog = GetScriptForDestination(witdest);
        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
        // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
        keystore.AddCScript(witprog);
        if (type == OutputType::BECH32) {
            return witdest;
        } else {
            return ScriptHash(witprog);
        }
    }
```

4 - Since `ScriptHash` is different to `P2SH_SEGWIT` in that comparison, it returns that warning.

--------

In `test/functional/rpc_createmultisig.py`, I just noticed every time it is creating a multisig with p2sh-segwit as output type, it is returning the warning message.
Thanks for looking.

I do agree that the tests must be wrong in order to accept this obviously wrong warning message.

I'm a bit confused about what you are saying since I stepped it through in gdb and am sure the check that fails is at https://github.com/bitcoin/bitcoin/blob/225e5b57b2ee2bc1acd7f09c89ccccc15ef8c85f/src/rpc/output_script.cpp#L166
Sorry if I wasn't so clear, but in my point 4 I was saying:
```
OutputTypeFromDestination(dest) -> returning ScriptHash
output_type -> P2SH_SEGWIT 
```

So, `OutputTypeFromDestination(dest) != output_type` is true.

I agree with you about the issue.
```cpp
CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType type)
{
    // Add script to keystore
    keystore.AddCScript(script);
    // Note that scripts over 520 bytes are not yet supported.
    switch (type) {
    case OutputType::LEGACY:
        return ScriptHash(script);
    case OutputType::P2SH_SEGWIT:
    case OutputType::BECH32: {
        CTxDestination witdest = WitnessV0ScriptHash(script);
        CScript witprog = GetScriptForDestination(witdest);
        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
        // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
        keystore.AddCScript(witprog);
        if (type == OutputType::BECH32) {
            return witdest;
        } else {
            return ScriptHash(witprog);
        }
    }
    case OutputType::BECH32M: {} // This function should not be used for BECH32M, so let it assert
    } // no default case, so the compiler can warn about missing cases
    assert(false);
}
```

You can note here that both LEGACY and P2SH_SEGWIT returns ScriptHash.

Fix patch:
diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp
index 115a656e120d3..f4bb76f50fd51 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() && OutputTypeFromDestination(dest) != 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 d5444f5051977..f25ad595282b0 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() && OutputTypeFromDestination(dest) != 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;
 },


Test patch:
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 1695acaaa80b6..716ee8f7efe0d 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -91,15 +91,17 @@ def run_test(self):
                 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)
 
         self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors')
         with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f:
@@ -173,6 +175,7 @@ def do_multisig(self):
         desc = descsum_create(desc)
 
         msig = node2.createmultisig(self.nsigs, self.pub, self.output_type)
+        assert 'warnings' not in msig
         madd = msig["address"]
         mredeem = msig["redeemScript"]
         assert_equal(desc, msig['descriptor'])