rpc: addmultisigaddress should fallback to OutputType::LEGACY when uncompressed public keys are provided

[ps1dr3x]

This is related to issue #16011

Currently, in addmultisigaddress function's code the output type is always given by pwallet->m_default_address_type. (https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1018)

This PR adds a fallback to legacy when one or more provided public keys are uncompressed.

[ps1dr3x]

I just noticed that the function AddAndGetDestinationForScript, called by addmultisigaddress to calculate the destination, should already do this check (calling IsSolvable function) and falling back to legacy, but for some reason it doesn't (IsSolvable returns true). (https://github.com/bitcoin/bitcoin/blob/master/src/outputtype.cpp#L76)
I'm digging into it

[maflcko]

Be reminded that bugfixes must be accompanied by a test that fails before the change and passes after it

[ps1dr3x]

Thank you for the reminder @MarcoFalke , I'll add a test soon. I'm just trying to figure out if my current fix is enough or there are any other broken parts in the process, because there's a comment here saying that the use of uncompressed keys should be managed, but apparently it's not

[gmaxwell]

@MarcoFalke so, you would prefer we close this fix of a potentially money losing bug if no one felt like adding a test case for trivial functionality? I couldn't more strongly disagree.

[maflcko]

So you would prefer to not have a test, so that we can regress on this in the future and cause money loss for users?

[sipa]

Nothing prevents fixing the bug, and the separately adding a test. I certainly agree we should have tests for issues discovered in the past, but that doesn't mean there is a strong requirement to have one in the same PR, by the same author.

[gmaxwell]

@MarcoFalke Perhaps I misunderstood you. It sounded to me like you were expressing the view that a fix wouldn't be accepted if it was submitted without an automated test, if so I believe that isn't a position that we should take.

If instead you were just expressing doubt that there was actually a bug here to fix, then I agree. But coming with an automated test is not the bar for determining if a bug exists or not. I also think it's great to ask contributors if they'd be willing to automate their test, but we should probably lean towards "I'll help you figure out how to do it" especially for new contributors, rather than "you must be this tall to ride, good luck".

And our urgency should be modulated somewhat by how easy it would be to accidentally regress: If someone finds a spelling error in a help message we're not exactly going to insist on adding a regression test which checks that help output for the misspelled word. :) Some fixes have a low probability of an accidentally regression and a targeted test which only catches that one behaviour may well not be worth its maintenance burden.

Similarly, if a 'bug' were really inconsequential-- then I could support a position that including a test would be a minimum proof of work before we'd bother dealing with it, but this would be a serious bug if it were real.

Which I suspect it isn't:

$ ./bitcoin-cli addmultisigaddress 2 '["04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f","047211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073dee6c89064984f03385237d92167c13e236446b417ab79a0fcae412ae3316b77","0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee"]' "" bech32
{
"address": "3J1pwTY92a2vcdqVZF4J4Ut1tdbsst68rE",

It's a little surprising that its silently auto-returning legacy when you give it an explicit parameter, it might be better to return an error, but before actually arguing that I'd want to go look to see if that had already been discussed. Auto-returning legacy when it must when there is no argument is at most a doc bug.

[ps1dr3x]

This is what I'm trying to understand @gmaxwell , it doesn't happen always:

./bitcoin-cli addmultisigaddress 1 '["045897fee25bd7c5692510b2f50fcae9aa20fbc4d49d59814f4c7fdb5c4bc6eb1c0ce382458f9588e922e0d509ed8d34856787380075b00418b02e0bf7c652ef9d","02ac46c6d74d15e60f4f1035ff07ef740aca1d68d55ba0b8d336a73d7a35858831","0224a4dc5620714a9ecf67a09583d1e4c04f5bedb8ecea99028da05bb15a2a7e07"]' "" bech32
{
  "address": "bc1qarhggzk55q4ws9z0jrrsl5qumgrwnwry7lk7xl3s3sel35zskl6qdtm33s",
  "redeemScript": "5141045897fee25bd7c5692510b2f50fcae9aa20fbc4d49d59814f4c7fdb5c4bc6eb1c0ce382458f9588e922e0d509ed8d34856787380075b00418b02e0bf7c652ef9d2102ac46c6d74d15e60f4f1035ff07ef740aca1d68d55ba0b8d336a73d7a35858831210224a4dc5620714a9ecf67a09583d1e4c04f5bedb8ecea99028da05bb15a2a7e0753ae"
}

[gmaxwell]

Indeed, I just tested further. It appears to me that it only auto switches to legacy when more than the threshold of keys is uncompressed. 0_o This is absolutely a bug.

$ ./bitcoin-cli addmultisigaddress 2 '["047211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073dee6c89064984f03385237d92167c13e236446b417ab79a0fcae412ae3316b77","0279BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798","0279BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798"]' "" bech32
{
"address": "bc1q44mnt024cw8r9r3egnq5ndscdqlcyufyppdhdtccf0l7dt8w70jsz37nnz",
"redeemScript": "5241047211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073dee6c89064984f03385237d92167c13e236446b417ab79a0fcae412ae3316b77210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f8179853ae"
}

[ps1dr3x]

I noticed that, and also this comment only after opening the PR, so this commit now seems more like a workaround for a problem which I still don't understand.

I followed the code of the function AddAndGetDestinationForScript but I still don't get where/when this fallback happens, and why only few cases are affected.

[achow101]

I believe this affects createmultisigaddress as well. A more correct fix would be to change AddAndGetDestinationForScript and possibly ProduceSignature as well. I can work on a better fix for this when I have time next week.

[ps1dr3x]

Maybe is unrelated and I'm missing something stupid because I don't have any experience on the bitcoin test framework, but I'm having some problems even with the test.
I wrote this simple test in rpc_createmultisig.py (because seems there's only one file for both the calls):

    def do_multisig_fallback_to_legacy(self):
        self.log.info('Check that addmultisigaddress and createmultisig fallback to legacy when uncompressed public keys are provided')

        pubkey1 = "045897fee25bd7c5692510b2f50fcae9aa20fbc4d49d59814f4c7fdb5c4bc6eb1c0ce382458f9588e922e0d509ed8d34856787380075b00418b02e0bf7c652ef9d"
        pubkey2 = "02ac46c6d74d15e60f4f1035ff07ef740aca1d68d55ba0b8d336a73d7a35858831"
        pubkey3 = "0224a4dc5620714a9ecf67a09583d1e4c04f5bedb8ecea99028da05bb15a2a7e07"

        for self.output_type in ["legacy", "p2sh-segwit", "bech32"]:
            msigw = self.nodes[0].addmultisigaddress(1, [pubkey1, pubkey2, pubkey3], None, self.output_type)
            assert msigw["address"] == "3GiimyxF1R5VixfBFAbQZbuy9EesD2r6n1"

But the assert fails, both with my commit and without, and if I print the 3 addresses (msigw["address"]) I get other different invalid addresses (but the same redeemScript):

2019-05-13T19:17:07.346000Z TestFramework (INFO): Check that addmultisigaddress and createmultisig fallback to legacy when uncompressed public keys are provided
{'address': '2N8GvqitGcsaqvkHivJDHBYuEMas2zoTNQm', 'redeemScript': '5141045897fee25bd7c5692510b2f50fcae9aa20fbc4d49d59814f4c7fdb5c4bc6eb1c0ce382458f9588e922e0d509ed8d34856787380075b00418b02e0bf7c652ef9d2102ac46c6d74d15e60f4f1035ff07ef740aca1d68d55ba0b8d336a73d7a35858831210224a4dc5620714a9ecf67a09583d1e4c04f5bedb8ecea99028da05bb15a2a7e0753ae'}
{'address': '2Mx2YyMfY5vMa8MwXt7K8RSaV1ieGMycCpF', 'redeemScript': '5141045897fee25bd7c5692510b2f50fcae9aa20fbc4d49d59814f4c7fdb5c4bc6eb1c0ce382458f9588e922e0d509ed8d34856787380075b00418b02e0bf7c652ef9d2102ac46c6d74d15e60f4f1035ff07ef740aca1d68d55ba0b8d336a73d7a35858831210224a4dc5620714a9ecf67a09583d1e4c04f5bedb8ecea99028da05bb15a2a7e0753ae'}
{'address': 'bcrt1qarhggzk55q4ws9z0jrrsl5qumgrwnwry7lk7xl3s3sel35zskl6qh68c79', 'redeemScript': '5141045897fee25bd7c5692510b2f50fcae9aa20fbc4d49d59814f4c7fdb5c4bc6eb1c0ce382458f9588e922e0d509ed8d34856787380075b00418b02e0bf7c652ef9d2102ac46c6d74d15e60f4f1035ff07ef740aca1d68d55ba0b8d336a73d7a35858831210224a4dc5620714a9ecf67a09583d1e4c04f5bedb8ecea99028da05bb15a2a7e0753ae'}

Do you have any idea on what's wrong here? The same call with bitcoin-cli to the compiled and running node (same code) returns the expected addresses...

[sdaftuar]

@ps1dr3x I haven't dived into this but I noticed that your test uses a "3..." prefix for the address, but the address encoding for regtest/testnet (which is what we use in our test framework) is different than mainnet, so P2SH addresses will start with a "2".

[ps1dr3x]

Oh yeah you're right, I didn't thought about that! Thank you @sdaftuar 👍

[maflcko]

It is fine if you don't add a test case if it is too hard to get one. I thought this would be a single line added with an assert, but I guess it turned out more involved?

[ps1dr3x]

Yes, and at this point maybe that test will not even fit for the real causes @MarcoFalke . I asked mainly because I was experimenting but I'll not commit that, at least before the real causes are discovered :)

Anyway, meanwhile that @achow101 cannot work on this, I'll continue to try getting more information (or hopefully finding possible fixes) on this

[achow101]

This was simpler than expected. I've opened a PR: #16022

[meshcollider]

Closing in favor of #16026, thank you for your help @ps1dr3x 😄 Hope you continue contributing in the future!