rpc: createmultisig adds incorrect warning for address type p2sh-segwit

mruddy commented at 12:33 pm on May 13, 2022: contributor

The createmultisig RPC adds a spurious warning message when creating for address type p2sh-segwit.

0createmultisig 2 '["0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"]' 'p2sh-segwit'
1{
2  "address": "3QfZQY7wQrBGEUB7E5vVLggeeAUVZ1Bbg9",
3  "redeemScript": "52210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f8179852ae",
4  "descriptor": "sh(wsh(multi(2,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)))#m8ww0c9m",
5  "warnings": [
6    "Unable to make chosen address type, please ensure no uncompressed public keys are present."
7  ]
8}

I think this traces back to #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:

0createmultisig 2 '["0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"]' 'p2sh-segwit'
1{
2  "address": "3QfZQY7wQrBGEUB7E5vVLggeeAUVZ1Bbg9",
3  "redeemScript": "52210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f8179852ae",
4  "descriptor": "sh(wsh(multi(2,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798,0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)))#m8ww0c9m"
5}

mruddy added the label Bug on May 13, 2022

brunoerg commented at 5:14 pm on May 20, 2022: member

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

 0    case OutputType::P2SH_SEGWIT:
 1    case OutputType::BECH32: {
 2        CTxDestination witdest = WitnessV0ScriptHash(script);
 3        CScript witprog = GetScriptForDestination(witdest);
 4        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
 5        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
 6        // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
 7        keystore.AddCScript(witprog);
 8        if (type == OutputType::BECH32) {
 9            return witdest;
10        } else {
11            return ScriptHash(witprog);
12        }
13    }

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.

mruddy commented at 12:37 pm on May 21, 2022: contributor

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

brunoerg commented at 2:35 pm on May 21, 2022: member

Sorry if I wasn’t so clear, but in my point 4 I was saying:

0OutputTypeFromDestination(dest) -> returning ScriptHash
1output_type -> P2SH_SEGWIT

So, OutputTypeFromDestination(dest) != output_type is true.

I agree with you about the issue.

brunoerg commented at 8:38 pm on May 21, 2022: member

 0CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType type)
 1{
 2    // Add script to keystore
 3    keystore.AddCScript(script);
 4    // Note that scripts over 520 bytes are not yet supported.
 5    switch (type) {
 6    case OutputType::LEGACY:
 7        return ScriptHash(script);
 8    case OutputType::P2SH_SEGWIT:
 9    case OutputType::BECH32: {
10        CTxDestination witdest = WitnessV0ScriptHash(script);
11        CScript witprog = GetScriptForDestination(witdest);
12        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
13        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
14        // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
15        keystore.AddCScript(witprog);
16        if (type == OutputType::BECH32) {
17            return witdest;
18        } else {
19            return ScriptHash(witprog);
20        }
21    }
22    case OutputType::BECH32M: {} // This function should not be used for BECH32M, so let it assert
23    } // no default case, so the compiler can warn about missing cases
24    assert(false);
25}

You can note here that both LEGACY and P2SH_SEGWIT returns ScriptHash.

laanwj closed this on Jun 6, 2022

sidhujag referenced this in commit 5528dc0a60 on Jun 6, 2022

DrahtBot locked this on Jun 6, 2023

rpc: createmultisig adds incorrect warning for address type p2sh-segwit #25127