Bitcoin Core 0.20.1 seems to cause an issue with wally_psbt_sign

[Fonta1n3]

I have been using LibWally 0.7.7 (via libwally-swift) heavily in my apps for signing psbt's locally. However after updating Bitcoin Core to 0.20.1 I am having major issues which break the apps signing ability.

Please bitcoin/bitcoin#19650 for details. Just so the flow is clear:

• create psbt with bitcoin-cli walletcreatefundedpsbt
• utilize wally_psbt_sign to sign the psbt with child private key that matches the inputs pubkey, which does in fact sign and can be seen in the decoded psbt
• bitcoin-cli finalizepsbt does not recognize the psbt as complete and will not finalize it

If I bypass bitcoin-cli finalizepsbt and use LibWally to finalize it does return a hex encoded raw transaction but when attempting to broadcast I get:

error =     {
        code = "-26";
        message = "non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation)";
    }

To me it is clearly the recent change where both non_witness_utxo and witness_utxo are added to the psbt which may be confusing LibWally.

If you have some idea of how I may fix this issue please do let me know, for now I have to advise users so simply avoid updating Bitcoin Core as they will not be able to spend their btc.

[jgriffiths]

Workaround: running the following logic before signing will allow you to sign:

For each input, if utxo and witness utxo are both present, delete utxo

That undoes the core 20.1 change so the current signing logic will work; its not a proper fix but should get you going.

[Fonta1n3]

Workaround: running the following logic before signing will allow you to sign:

For each input, if utxo and witness utxo are both present, delete utxo

That undoes the core 20.1 change so the current signing logic will work; its not a proper fix but should get you going.

@jgriffiths Thanks for the tip, I can easily just fork the repo make my changes then and use my fork as a dependency while I wait. Cheers :)

[jgriffiths]

@Fonta1n3 I don't have much time to look at this, but if you have a testnet/regtest PSBT from core 20.1 and a private key that would sign it correctly if it was from an earlier version of core, please paste it here in base64, and when I get a chance I'll see if we can fix this simply.

The signing logic needs reworking and support for bip32 key signing but it might be a while before I can get to that due to other more commitments right now.

[Fonta1n3]

@jgriffiths I would really greatly appreciate it, because my app is so reliant on Bitcoin Core and Libwally and it is incorporated in node boxes like Nodl, myNode, BTCPayServer I can not simply stop people from updating... I can hack, but C code scares me.

cHNidP8BAJoCAAAAApK5TjHXfVVcz1p+fqGnU2+AtzZ1BVgN7RenG9UYiOCQAAAAAAD9////97Oqk3E4gOTA0WF9lUti+6FDGqao08nCXJduFzxy4s8BAAAAAP3///8CECcAAAAAAAAWABRy0wpXwwXefXespvbBjqImEuSEUREmAAAAAAAAFgAUSf2oVZuNzRod5Z9o3lIHt3FpttQAAAAAAAEAcgIAAAAB97Oqk3E4gOTA0WF9lUti+6FDGqao08nCXJduFzxy4s8AAAAAAP3///8CECcAAAAAAAAXqRRcl9Sf+c1s0P5r5CGoefIJLL2g+YcdwgAAAAAAABYAFJQPdrykhhpou8p1nGgQ8V+jy+UPAAAAAAEBIBAnAAAAAAAAF6kUXJfUn/nNbND+a+QhqHnyCSy9oPmHIgICLPmANjfAeTupxeEoFRBu+ucXx2RSqy4im42bqPP/bzxHMEQCIEaAyfUPgnxrep0B6HNYYvkLODrSJhSUCrXAnfeAvzZzAiB8Z+nfB7oAthnKcKJXLZTKuyqRMQzFV+v0E17l7JjcWQEBBBYAFMkSAYWcJSOES0+jQx5bqL5azd4gIgYCLPmANjfAeTupxeEoFRBu+ucXx2RSqy4im42bqPP/bzwYrkGyJ1QAAIABAACAAAAAgAAAAAAFAAAAAAEAiQIAAAABfRJscM0GWu793LYoAX15Mnj+dVr0G7yvRMBeWSmvPpQAAAAAFxYAFESkW2FnrJlkwmQZjTXL1IVM95lW/f///wK76QAAAAAAABYAFB33sq8WtoOlpvUpCvoWbxJJl5rhECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcAAAAAAQEgECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIciAgP9qPLNX3SCFD7TMg7aZVJ3d8ZiG/BiNqQYJjhY1jGh5kcwRAIgbkhlqqCB7RlqiE7TpsdJiJoc21X8WQP9yoAyg3fPRIICIFFuDZHcfihUCo13umjP0GJoltA6zjmGluBZDbjc1vGqAQEEFgAUiyJ5d3oAB4/xNccpnfb1nSe5J5kiBgP9qPLNX3SCFD7TMg7aZVJ3d8ZiG/BiNqQYJjhY1jGh5hiuQbInVAAAgAEAAIAAAACAAAAAAAMAAAAAIgIDBL/+Mrg+0Gju6WCWCAsHeaQa1HPmRRkMbSQvt+DiS/YYrkGyJ1QAAIABAACAAAAAgAAAAAAKAAAAACICAnPUOVkQtKAUlhfC6mc6lED0W6uaqfi1HPgDcTqYXUsKGK5BsidUAACAAQAAgAAAAIABAAAACAAAAAA=

The two signing keys:

cTatuMdjH4YA4F1pAm11QdbCt88T8t2TTMoAvVGzAxWAWmQZtkBZ
cR5yyo2g1SzzwCw2QAREzF7XhYuXZS9SzTTf8A9qerri9EXZcRYS

[jgriffiths]

@Fonta1n3 Thats helpful, thanks - can you also post the result of your signing this with core 20.1?

[Fonta1n3]

@jgriffiths The above actually is signed, but with Libwally of course. Or do you want me to import the WIF's into core then use walletprocesspsbt?

[Fonta1n3]

@jgriffiths I have been testing with removing the non_witness_utxo if both are present.

Strangely even when witness_utxo is present printing wally_psbt_input always shows witness_script: nil.

I've bypassed bitcoind and am trying to finalize with Libwally instead which does at least finalize the psbt but I always get an error when broadcasting (scriptsig field is always empty):

error =     {
        code = "-26";
        message = "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)";
    }

[jgriffiths]

Or do you want me to import the WIF's into core then use walletprocesspsbt?

I was hoping for this yes, it will let me check the core result vs wally (for various reasons I dont have v20.1 handy to test right now).

[Fonta1n3]

Or do you want me to import the WIF's into core then use walletprocesspsbt?

I was hoping for this yes, it will let me check the core result vs wally (for various reasons I dont have v20.1 handy to test right now).

@jgriffiths Ok, I have recreated the psbt after importing the two WIF's above using 0.20.1.

{"jsonrpc":"1.0","id":"curltest","method":"walletcreatefundedpsbt","params":[[], {"tb1qz6dpm3pfuqcgexyakf6e5xjrcyx2gdu874h5mn": 0.0001}, 0, {"includeWatching": true, "replaceable": true, "conf_target": 2}, true]}

result =     {
        changepos = 0;
        fee = "2.55e-06";
        psbt = "cHNidP8BAJoCAAAAAvezqpNxOIDkwNFhfZVLYvuhQxqmqNPJwlyXbhc8cuLPAQAAAAD9////krlOMdd9VVzPWn5+oadTb4C3NnUFWA3tF6cb1RiI4JAAAAAAAP3///8CESYAAAAAAAAWABQn/PFABd2EW5RsCUvJitAYNshf9BAnAAAAAAAAFgAUFpodxCngMIyYnbJ1mhpDwQykN4cAAAAAAAEAiQIAAAABfRJscM0GWu793LYoAX15Mnj+dVr0G7yvRMBeWSmvPpQAAAAAFxYAFESkW2FnrJlkwmQZjTXL1IVM95lW/f///wK76QAAAAAAABYAFB33sq8WtoOlpvUpCvoWbxJJl5rhECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcAAAAAAQEgECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcBBBYAFIsieXd6AAeP8TXHKZ329Z0nuSeZIgYD/ajyzV90ghQ+0zIO2mVSd3fGYhvwYjakGCY4WNYxoeYEiyJ5dwABAHICAAAAAfezqpNxOIDkwNFhfZVLYvuhQxqmqNPJwlyXbhc8cuLPAAAAAAD9////AhAnAAAAAAAAF6kUXJfUn/nNbND+a+QhqHnyCSy9oPmHHcIAAAAAAAAWABSUD3a8pIYaaLvKdZxoEPFfo8vlDwAAAAABASAQJwAAAAAAABepFFyX1J/5zWzQ/mvkIah58gksvaD5hwEEFgAUyRIBhZwlI4RLT6NDHluovlrN3iAiBgIs+YA2N8B5O6nF4SgVEG765xfHZFKrLiKbjZuo8/9vPATJEgGFACICAq8h+ABETC5Tczuts3xhCtXAzIEUHM5iMugvwFMrtCc4EBK06cYAAACAAQAAgMMAAIAAAA==";
    }

{"jsonrpc":"1.0","id":"curltest","method":"walletprocesspsbt","params":["cHNidP8BAJoCAAAAAvezqpNxOIDkwNFhfZVLYvuhQxqmqNPJwlyXbhc8cuLPAQAAAAD9////krlOMdd9VVzPWn5+oadTb4C3NnUFWA3tF6cb1RiI4JAAAAAAAP3///8CESYAAAAAAAAWABQn/PFABd2EW5RsCUvJitAYNshf9BAnAAAAAAAAFgAUFpodxCngMIyYnbJ1mhpDwQykN4cAAAAAAAEAiQIAAAABfRJscM0GWu793LYoAX15Mnj+dVr0G7yvRMBeWSmvPpQAAAAAFxYAFESkW2FnrJlkwmQZjTXL1IVM95lW/f///wK76QAAAAAAABYAFB33sq8WtoOlpvUpCvoWbxJJl5rhECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcAAAAAAQEgECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcBBBYAFIsieXd6AAeP8TXHKZ329Z0nuSeZIgYD/ajyzV90ghQ+0zIO2mVSd3fGYhvwYjakGCY4WNYxoeYEiyJ5dwABAHICAAAAAfezqpNxOIDkwNFhfZVLYvuhQxqmqNPJwlyXbhc8cuLPAAAAAAD9////AhAnAAAAAAAAF6kUXJfUn/nNbND+a+QhqHnyCSy9oPmHHcIAAAAAAAAWABSUD3a8pIYaaLvKdZxoEPFfo8vlDwAAAAABASAQJwAAAAAAABepFFyX1J/5zWzQ/mvkIah58gksvaD5hwEEFgAUyRIBhZwlI4RLT6NDHluovlrN3iAiBgIs+YA2N8B5O6nF4SgVEG765xfHZFKrLiKbjZuo8/9vPATJEgGFACICAq8h+ABETC5Tczuts3xhCtXAzIEUHM5iMugvwFMrtCc4EBK06cYAAACAAQAAgMMAAIAAAA==", true, "ALL", true]}

result =     {
        complete = 1;
        psbt = "cHNidP8BAJoCAAAAAvezqpNxOIDkwNFhfZVLYvuhQxqmqNPJwlyXbhc8cuLPAQAAAAD9////krlOMdd9VVzPWn5+oadTb4C3NnUFWA3tF6cb1RiI4JAAAAAAAP3///8CESYAAAAAAAAWABQn/PFABd2EW5RsCUvJitAYNshf9BAnAAAAAAAAFgAUFpodxCngMIyYnbJ1mhpDwQykN4cAAAAAAAEAiQIAAAABfRJscM0GWu793LYoAX15Mnj+dVr0G7yvRMBeWSmvPpQAAAAAFxYAFESkW2FnrJlkwmQZjTXL1IVM95lW/f///wK76QAAAAAAABYAFB33sq8WtoOlpvUpCvoWbxJJl5rhECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcAAAAAAQEgECcAAAAAAAAXqRTFhAlcZBMRkG4iAustDT6iSw6wkIcBBxcWABSLInl3egAHj/E1xymd9vWdJ7knmQEIawJHMEQCIAkPXe9sdpRjSDTjJ0gIrpwGGIWJby9xSd1rS9hPe1f0AiAJgqR7PL3G/MXyUu4KZdS1Z2O14fjxstF43k634u+4GAEhA/2o8s1fdIIUPtMyDtplUnd3xmIb8GI2pBgmOFjWMaHmAAEAcgIAAAAB97Oqk3E4gOTA0WF9lUti+6FDGqao08nCXJduFzxy4s8AAAAAAP3///8CECcAAAAAAAAXqRRcl9Sf+c1s0P5r5CGoefIJLL2g+YcdwgAAAAAAABYAFJQPdrykhhpou8p1nGgQ8V+jy+UPAAAAAAEBIBAnAAAAAAAAF6kUXJfUn/nNbND+a+QhqHnyCSy9oPmHAQcXFgAUyRIBhZwlI4RLT6NDHluovlrN3iABCGsCRzBEAiAOzRsNZ+2Et+VGCY/nXWO7WxGI3u39kpi025cUaJXQJgIgL6KtMqPfAwXGktQFWr9SNnOrHF2xjvKQI2VdeuQbxt0BIQIs+YA2N8B5O6nF4SgVEG765xfHZFKrLiKbjZuo8/9vPAAiAgKvIfgAREwuU3M7rbN8YQrVwMyBFBzOYjLoL8BTK7QnOBAStOnGAAAAgAEAAIDDAACAAAA=";
    }

If you need any other info please do let me know, and thanks very much for your time.

[jgriffiths]

Hi @Fonta1n3 please try #215 - this includes a fix and test case showing wally now produces the same result as core with the above inputs.

If you encounter any more signing issues and can provide the same test case info as your last commit that would really help when fixing.

If you can confirm that PR works for you, I'll clean it up and get it reviewed for master. Thanks!

[Fonta1n3]

@jgriffiths Thank you very much, I am going through it. A number of changes seem to break libwally-swift's PSBT.swift so its going to take me a little while..

[Fonta1n3]

@jgriffiths I was unfortunately unable to cope with all of the changes that are in your PR due to the fact I am (as I said above) working via the libwally-swift wrapper.. However!! I saw you commit message check for segwit utxos before non-segwit when signing so I implemented the same logic myself in my local version of libwally-core and can confirm it is working! Thank you for the great clue and for the help. I am sure your fix solves the issue, what a relief.

[Fonta1n3]

I am working off of 0.7.7 so its really only relevant to that codebase but here is the updated psbt.c wally_sign_psbt with the fix:

int wally_sign_psbt(
    struct wally_psbt *psbt,
    const unsigned char *key,
    size_t key_len)
{
    unsigned char pubkey[EC_PUBLIC_KEY_LEN], uncomp_pubkey[EC_PUBLIC_KEY_UNCOMPRESSED_LEN], sig[EC_SIGNATURE_LEN], der_sig[EC_SIGNATURE_DER_MAX_LEN + 1];
    size_t i, j, der_sig_len;
    int ret;

    if (!psbt || !psbt->tx || !key || key_len != EC_PRIVATE_KEY_LEN) {
        return WALLY_EINVAL;
    }

    // Get the pubkey
    if ((ret = wally_ec_public_key_from_private_key(key, key_len, pubkey, EC_PUBLIC_KEY_LEN)) != WALLY_OK) {
        return ret;
    }
    if ((ret = wally_ec_public_key_decompress(pubkey, EC_PUBLIC_KEY_LEN, uncomp_pubkey, EC_PUBLIC_KEY_UNCOMPRESSED_LEN)) != WALLY_OK) {
        return ret;
    }

    // Go through each of the inputs
    for (i = 0; i < psbt->num_inputs; ++i) {
        struct wally_psbt_input *input = &psbt->inputs[i];
        struct wally_tx_input *txin = &psbt->tx->inputs[i];
        unsigned char sighash[SHA256_LEN], *scriptcode, wpkh_sc[WALLY_SCRIPTPUBKEY_P2PKH_LEN];
        size_t scriptcode_len;
        bool match = false, comp = false;
        uint32_t sighash_type = WALLY_SIGHASH_ALL;

        if (!input->keypaths) {
            // Can't do anything without the keypaths
            continue;
        }

        // Go through each listed pubkey and see if it matches.
        for (j = 0; j < input->keypaths->num_items; ++j) {
            struct wally_keypath_item *item = &input->keypaths->items[j];
            if (item->pubkey[0] == 0x04 && memcmp((char *)item->pubkey, (char *)uncomp_pubkey, EC_PUBLIC_KEY_UNCOMPRESSED_LEN) == 0) {
                match = true;
                break;
            } else if (memcmp((char *)item->pubkey, (char *)pubkey, EC_PUBLIC_KEY_LEN) == 0) {
                match = true;
                comp = true;
                break;
            }
        }

        // Did not find pubkey, skip
        if (!match) {
            continue;
        }

        // Sighash type
        if (input->sighash_type > 0) {
            sighash_type = input->sighash_type;
        }

        // Get scriptcode and sighash
        if (input->redeem_script) {
            unsigned char sh[WALLY_SCRIPTPUBKEY_P2SH_LEN];
            size_t written;

            if ((ret = wally_scriptpubkey_p2sh_from_bytes(input->redeem_script, input->redeem_script_len, WALLY_SCRIPT_HASH160, sh, WALLY_SCRIPTPUBKEY_P2SH_LEN, &written)) != WALLY_OK) {
                return ret;
            }
            
            if (input->witness_utxo) {
                if (input->witness_utxo->script_len != WALLY_SCRIPTPUBKEY_P2SH_LEN ||
                    memcmp(sh, input->witness_utxo->script, WALLY_SCRIPTPUBKEY_P2SH_LEN) != 0) {
                    return WALLY_EINVAL;
                }
            } else if (input->non_witness_utxo) {
                if (input->non_witness_utxo->outputs[txin->index].script_len != WALLY_SCRIPTPUBKEY_P2SH_LEN ||
                    memcmp(sh, input->non_witness_utxo->outputs[txin->index].script, WALLY_SCRIPTPUBKEY_P2SH_LEN) != 0) {
                    return WALLY_EINVAL;
                }
            } else {
                continue;
            }
            scriptcode = input->redeem_script;
            scriptcode_len = input->redeem_script_len;
        } else {
            if (input->witness_utxo) {
                scriptcode = input->witness_utxo->script;
                scriptcode_len = input->witness_utxo->script_len;
            } else if (input->non_witness_utxo) {
                scriptcode = input->non_witness_utxo->outputs[txin->index].script;
                scriptcode_len = input->non_witness_utxo->outputs[txin->index].script_len;
            } else {
                continue;
            }
        }
        
        if (input->witness_utxo) {
            size_t type;
            if ((ret = wally_scriptpubkey_get_type(scriptcode, scriptcode_len, &type)) != WALLY_OK) {
                return ret;
            }
            if (type == WALLY_SCRIPT_TYPE_P2WPKH) {
                size_t written;
                if ((ret = wally_scriptpubkey_p2pkh_from_bytes(&scriptcode[2], HASH160_LEN, 0, wpkh_sc, WALLY_SCRIPTPUBKEY_P2PKH_LEN, &written)) != WALLY_OK) {
                    return ret;
                }
                scriptcode = wpkh_sc;
                scriptcode_len = WALLY_SCRIPTPUBKEY_P2PKH_LEN;
            } else if (type == WALLY_SCRIPT_TYPE_P2WSH && input->witness_script) {
                unsigned char wsh[WALLY_SCRIPTPUBKEY_P2WSH_LEN];
                size_t written;

                if ((ret = wally_witness_program_from_bytes(input->witness_script, input->witness_script_len, WALLY_SCRIPT_SHA256, wsh, WALLY_SCRIPTPUBKEY_P2WSH_LEN, &written)) != WALLY_OK) {
                    return ret;
                }
                if (scriptcode_len != WALLY_SCRIPTPUBKEY_P2WSH_LEN ||
                    memcmp((char *)wsh, (char *)scriptcode, WALLY_SCRIPTPUBKEY_P2WSH_LEN) != 0) {
                    return WALLY_EINVAL;
                }
                scriptcode = input->witness_script;
                scriptcode_len = input->witness_script_len;
            } else {
                // Not a recognized scriptPubKey type or not enough information
                continue;
            }

            if ((ret = wally_tx_get_btc_signature_hash(psbt->tx, i, scriptcode, scriptcode_len, input->witness_utxo->satoshi, sighash_type, WALLY_TX_FLAG_USE_WITNESS, sighash, SHA256_LEN)) != WALLY_OK) {
                return ret;
            }
            
        } else if (input->non_witness_utxo) {
            unsigned char txid[SHA256_LEN];

            if ((ret = get_txid(input->non_witness_utxo, txid, SHA256_LEN)) != WALLY_OK) {
                return ret;
            }
            if (memcmp((char *)txid, (char *)txin->txhash, SHA256_LEN) != 0) {
                return WALLY_EINVAL;
            }

            if ((ret = wally_tx_get_btc_signature_hash(psbt->tx, i, scriptcode, scriptcode_len, 0, sighash_type, 0, sighash, SHA256_LEN)) != WALLY_OK) {
                return ret;
            }
        }

        // Sign the sighash
        if ((ret = wally_ec_sig_from_bytes(key, key_len, sighash, SHA256_LEN, EC_FLAG_ECDSA | EC_FLAG_GRIND_R, sig, EC_SIGNATURE_LEN)) != WALLY_OK) {
            return ret;
        }
        if ((ret = wally_ec_sig_normalize(sig, EC_SIGNATURE_LEN, sig, EC_SIGNATURE_LEN)) != WALLY_OK) {
            return ret;
        }
        if ((ret = wally_ec_sig_to_der(sig, EC_SIGNATURE_LEN, der_sig, EC_SIGNATURE_DER_MAX_LEN, &der_sig_len)) != WALLY_OK) {
            return ret;
        }

        // Add the sighash type to the end of the sig
        der_sig[der_sig_len] = (unsigned char)sighash_type;
        der_sig_len++;

        // Copy the DER sig into the psbt
        if (!input->partial_sigs) {
            if ((ret = wally_partial_sigs_map_init_alloc(1, &input->partial_sigs)) != WALLY_OK) {
                return ret;
            }
        }
        if ((ret = wally_add_new_partial_sig(input->partial_sigs, comp ? pubkey : uncomp_pubkey, comp ? EC_PUBLIC_KEY_LEN : EC_PUBLIC_KEY_UNCOMPRESSED_LEN, der_sig, der_sig_len)) != WALLY_OK) {
            return ret;
        }
    }

    return WALLY_OK;
}

[jgriffiths]

@Fonta1n3 Great that you can confirm the fix. We will get this out in a v0.7.9 release at some point and libwally swift can do the upgrade for you (there are a lot of other fixes in the changes that you should really pick up).

I'm closing this now in expectation of #215 being merged shortly. Thanks for your help again.