leather-io / extension

Leather browser extension
https://leather.io
MIT License
294 stars 141 forks source link

Cannot unlock p2tr transaction with path script #4125

Closed zefiroconsultancy closed 7 months ago

zefiroconsultancy commented 1 year ago

After the last update of the wallet i cannot anymore unlock script taproot with script-path.

I think can be related to the update used to solve the issue #4007 . In this update is added the following in src/app/features/psbt-signer/hooks/use-psbt-signer.tsx:

          // If type taproot, and the tapInternalKey is missing, assume it should
            // be the account publicKey
            if (taprootSigner && witnessOutputScript?.type === 'tr' && !input.tapInternalKey) {
              input.tapInternalKey = Buffer.from(taprootSigner?.payment.tapInternalKey);
            }

In my case i'm using a script path and the tapInteralkey in the inputs can miss or be different and hiro doesn't allow me anymore to sign it. Ps. if i force the tap internal key obviously it return invalid schnoor signature when broadcast.

in this address "tb1p6smgge3dyj8e2cwllu8yw6v8xeuu5yrpmrmeh4mpu4edjhm0nygqvejqku" can be osserved that the last script path unlock on 2023-07-24 the script path unlock was working, the following one i got blocked.

6.1.0 (2023-07-26) this is the data of the update i'm referring to.

Any help on how to solve this?

kyranjamie commented 1 year ago

Not sure I fully understand, but if you set the tapInternalKey yourself, this code won't alter your transaction.

Ps. if i force the tap internal key obviously it return invalid schnoor signature when broadcast.

Not if you use the correct schnorr key. We return this in the payload for taproot addresses, or you can convert it yourself by dropping the first byte of the ecdsa public key

https://github.com/hirosystems/wallet/blob/d5ce28115ff78d8b5980efac57f0014c69da2eb1/src/shared/crypto/bitcoin/bitcoin.utils.ts#L54-L57

zefiroconsultancy commented 1 year ago

Hello, Thanks for the answer.

I'm trying to spend the UTXO with a tapLeaf (script path) so my input will be the following:

    const scriptTree: Taptree = [
        {
            output: lock_script
        },
        {
            output: lock_script
        }
    ]

    const lock_redeem = {
        output: lock_script,
        redeemVersion: 192,
    };

    const lock_p2tr = payments.p2tr({
        internalPubkey: toXOnly(Buffer.from(VOID_PUBLIC_KEY, 'hex')),
        redeem: lock_redeem,
        network: network,
        scriptTree: scriptTree,
    });

    const psbt = new Psbt({ network });

    let totalAmount = 0
    for (const utxo of utxos) {
        const tx = Transaction.fromHex(await getTxHex(utxo.txid));

        psbt.addInput({
            hash: utxo.txid,
            index: utxo.vout,
            witnessUtxo: {
                value: +tx.outs[utxo.vout].value,
                script: lock_p2tr.output!
            },
            tapLeafScript: [
                {
                    leafVersion: lock_redeem.redeemVersion,
                    script: lock_redeem.output,
                    controlBlock: lock_p2tr.witness![lock_p2tr.witness!.length - 1] // extract control block from witness data
                }
            ],
        })
        totalAmount += +tx.outs[utxo.vout].value;
    }

As you can see i'm trying to spend with the tapLeafScript, so i'll not have any tapInternalKey and if with the code i referred to in my previous message you assume that the tapInternalKey is my address this is not true because of the VOID_PUBLIC_KEY.

I'm trying to use the script path something that before i was able to do.

kyranjamie commented 1 year ago

Thanks for the info @zefiroconsultancy. @fbwoolf wonder if this is a use case we should've considered?

Unisat also has this implicit internalTapKey behaviour, which we added because many developers forgot, or didn't know, to add it. We didn't realise this would break existing txs.

In mean time, isn't it possible to remove the added key if it isn't needed, once it's returned by the wallet?

zefiroconsultancy commented 1 year ago

Thanks @kyranjamie for the answer!

Regarding unisat i can tell you that i asked them to do the same (plus the possibility to sign with pubkey), time ago, before starting to use hiro. (there is still a commit in pending for unisat so solve this problem)

i don't think i can remove the added key after because that happen during the signature, so once is signed i cannot amend the psbt. What about using some boolean value for people that want to use the script path?

kyranjamie commented 1 year ago

It might even be better of erring on the side of leaving this to the developer and documenting better that tapInternalKey is necessary in most, but not all, cases.

fbwoolf commented 1 year ago

It might even be better of erring on the side of leaving this to the developer and documenting better that tapInternalKey is necessary in most, but not all, cases.

Couldn't we just add a check for the tapLeafScript and not add it? Then wouldn't break what we did for others? Also, fwiw, the code pasted into original description here is a bit outdated: https://github.com/hirosystems/wallet/blob/fc33210a969b8fffb8b0bcc877fdbe06d70f390e/src/app/features/psbt-signer/hooks/use-psbt-signer.tsx#L42C9-L42C9

kyranjamie commented 1 year ago

Couldn't we just add a check for the tapLeafScript and not add it?

Yep, this makes sense. Are there other conditions in which we might also not want to not auto-add internalTapKey?

zefiroconsultancy commented 1 year ago

@kyranjamie form my side should only be this one. Idk if there is something else.

gregdhill commented 8 months ago

Hi @kyranjamie @fbwoolf I think I'm running into this issue now, I'm trying to sign the reveal transaction for an inscription (key-path spend) using leather wallet but the Schnorr signature is invalid. I can't see any other obvious reason why it would not work as the same code works with other Bitcoin web wallets. You can see our code here: https://github.com/bob-collective/bob-ui/blob/bfd64e2e685718c0431f60be0cfa29f1f208ffe5/packages/sats-wagmi/src/connectors/leather.ts#L106

kyranjamie commented 8 months ago

@gregdhill could it be that you're using the address.publicKey rather than address.tweakedPublicKey when creating the PSBT?

gregdhill commented 8 months ago

@kyranjamie we actually disable tweak signing for this construction, see https://github.com/unisat-wallet/extension/issues/86

gregdhill commented 8 months ago

Also in this case leather succeeds in signing but then signature verification fails after.