leather-io / extension

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

Failed to sign PSBT transaction with Bare MultiSig UTXO #3897

Closed btcopenstamp closed 10 months ago

btcopenstamp commented 1 year ago

Our platform is currently developing a SRC20 marketplace and plans to integrate with the Hiro wallet. During our testing, we found that Hiro cannot sign PSBT transactions that include Bare MultiSig outputs (similar to the 2nd and 3rd outputs in this transaction: https://mempool.space/tx/5b05ce32451f80ba0beb8887a0ba901c3f1c553d594e72fce7d4d50c5b557c48). The error we encountered is shown in the following screenshot.

Could you please assist us in understanding what may be causing this issue? By the way, we have tested with the UniSat wallet and it is able to sign these transactions correctly. However, our first choice is still Hiro, so we hope that you can assist us in resolving this issue. Thanks.

image

fbwoolf commented 1 year ago

This is likely an error in how the PSBT is built, not an issue with the wallet. Can you create a demo of your code so we can look at it?

fbwoolf commented 1 year ago

Closing this issue bc likely not a bug with the wallet, but user error in building the PSBT passed to us. Will look to providing better docs for apps to use this feature.

btcopenstamp commented 1 year ago

It's truly not a normal multisig, but a must format for src20 transactions. And we can sign the transaction through Unisat.

fbwoolf commented 1 year ago

It's truly not a normal multisig, but a must format for src20 transactions. And we can sign the transaction through Unisat.

Happy to help, but we need more information from you. We requested some demo code to understand what tx you are working with here. Can you provide? This is an error coming from the signing lib we use, which you can reference here: https://github.com/paulmillr/scure-btc-signer

btcopenstamp commented 1 year ago

It's not convenient to provide all the code. you can check the bare multi-sig UTXO format from https://mempool.space/tx/5b05ce32451f80ba0beb8887a0ba901c3f1c553d594e72fce7d4d50c5b557c48.

This is the latest error: image

We are trying to integrate Hiro to our platform(https://openstamp.io/), but we are stuck.

fbwoolf commented 1 year ago

This error means you are trying to sign the inputs with the wrong public key, are you passing the account index to us to sign with? Looks the the output script is the wrong pubkey? This error is coming from the lib we use to sign the inputs. Again, I posted the ref above. It is not our wallet, but how you are building the tx. I believe it is erroring here, where we just call a method from the lib to get the outscript:

const outScript = btc.OutScript.decode(script);

btcopenstamp commented 1 year ago

Check out 'SRC-20 BTC Transaction Specifications ' at https://github.com/hydren-crypto/stampchain/blob/main/docs/src20.md

fbwoolf commented 1 year ago

I am currently assigned/working on another issue with PSBTs, but I can reopen this and look at it after.

fbwoolf commented 1 year ago

@btcopenstamp can you provide a hex of the PSBT you are trying to sign, or a gist of the decoded PSBT? I am doing a big refactor of our PSBT implementation. I'd like to get a fix for this included in it if I can. It would help if I can quickly reproduce this on my end.

btcopenstamp commented 1 year ago

Hi. Sorry for the late response. You can try to create a TX with bare multi-sig output on your own for testing. First, you need to create a normal PSBT TX with inputs and outputs, then add the following 'bareMultiSigLockingScript' as a bare multi-sig output.

bitcoinjs-lib example:

let bareMultiSigLockingScript = "512102dc054e58b755f233295d2a8759a3e4cbf678619d8e75379e7989046dbce16be32102932b35a45d21395ac8bb54b8f9dae3fd2dbc309c24e550cf2211fe6aa897e5ca2102020202020202020202020202020202020202020202020202020202020202020253ae"

psbt.addOutput({ script: bareMultiSigLockingScript, value: 796 });

Here is an unsigned PSBT with bare multi-sig output:

70736274ff0100fd550102000000014d477394b42190a29f892a48b61eaac7b4b6b26721f89185aea105d60d322cf70000000000fdffffff04708e010000000000160014bf4ea576eca42fc46447afd294172352e79b88751c0300000000000069512102a92801734e48efdbcd3075c9d8a28393b2be3ea16019e0f2265c130e02cddef22103261b400c8a36ddb3972e0675e039e9f6b3d7212f45d019f08c5e8733226eff222102222222222222222222222222222222222222222222222222222222222222222253ae1c0300000000000069512102db89151a5a4ff5bacc8d63b961ec397c6a6619466c83cadc302c1445303ab7f32103cd77827da4cb2ac4278a612733e089da7c6897aaf5dc0aafdfb152ffff0a2daa2102222222222222222222222222222222222222222222222222222222222222222253ae2803000000000000160014b3322b56e4e79264a91ed9f85d608af0a70f78db000000000001011f2803000000000000160014b3322b56e4e79264a91ed9f85d608af0a70f78db010304810000000000000000

btcopenstamp commented 1 year ago

Hi @fbwoolf . How's the issue going? Is there an ETA to fix this?

fbwoolf commented 1 year ago

Hi @fbwoolf . How's the issue going? Is there an ETA to fix this?

I am currently working on a big PSBT UI refactor, here: https://github.com/hirosystems/wallet/pull/4026

After that is merged, it looks like I have two PSBT prioritized before this one.

fbwoolf commented 1 year ago

@btcopenstamp this is the error I see which comes from the btc-signer lib we use, not our wallet directly...

Screen Shot 2023-07-25 at 3 19 21 PM

Can be tracked to here in the code: https://github.com/paulmillr/scure-btc-signer/blob/4bf86688bbb64d923bd7a604d651ddd11b38b4e9/index.ts#L660

There is a comment on L658 that it will maybe work, do you want to put up an issue in that lib to address what you need? I'm not sure what we can do if that will catch script type ms?

fbwoolf commented 1 year ago

Actually, @btcopenstamp, if I initialize the tx with this opts, const tx = new btc.Transaction({ disableScriptCheck: true }); that check becomes disabled and it gets past that error.

EDIT: But, then I hit an error here: https://github.com/paulmillr/scure-btc-signer/blob/4bf86688bbb64d923bd7a604d651ddd11b38b4e9/index.ts#L1485

Will put up a PR fix with a build for you to test.

fbwoolf commented 1 year ago

@btcopenstamp can you try the build here to see if this works, and use disableScriptCheck like ^?https://github.com/hirosystems/wallet/actions/runs/5662009861

btcopenstamp commented 1 year ago

Hi @fbwoolf Thanks for information. After reviewed your suggestions, we have made some adjustments to the generation of bare multisig output, and now it can display properly. However, we have encountered two new problems. Thank you for your work on this.

Problem 1: https://github.com/hirosystems/wallet/issues/4070

Problem 2: We tried to sign a SIGHASH_ALL|SIGHASH_ANYONECANPAY PSBT transaction. After we click the sign button, hiro just returns the unsigned PSBT which we provided to sign, no PSBT with signature returned. I am not sure whether it is a bug or we are doing something wrong.

The following code shows how we call the signPsbt function:

const resp = await window.btc.request('signPsbt', {
          publicKey: tokenInfo.publicKey,
          hex: psbt.toHex(),
          // allowedSighash?: SignatureHash[];
          // signAtIndex?: number | number[];
          // network?: NetworkModes;        
          account: extractAccountNumber(tokenInfo.derivationPath)           
        });
fbwoolf commented 1 year ago

Problem 1: #4070

I think this is fixed bc the UI in the issue is outdated.

Problem 2: We tried to sign a SIGHASH_ALL|SIGHASH_ANYONECANPAY PSBT transaction. After we click the sign button, hiro just returns the unsigned PSBT which we provided to sign, no PSBT with signature returned. I am not sure whether it is a bug or we are doing something wrong.

I am hoping this PR will address your issue here with sighash type? https://github.com/hirosystems/wallet/pull/4061

btcopenstamp commented 1 year ago

I am unable to verify whether #4061 fixed this. I was trying to use the package built on 4061. But it failed to show the PSBT with following error, which works fine on the current chrome plugin version. image

fbwoolf commented 1 year ago

I am unable to verify whether #4061 fixed this. I was trying to use the package built on 4061. But it failed to show the PSBT with following error, which works fine on the current chrome plugin version.

It appears we need to merge some fixes together bc I did address this in the PR linked here. I'll rebase on https://github.com/hirosystems/wallet/pull/4061 now that it has been merged.

fbwoolf commented 1 year ago

@btcopenstamp I rebased on the sighash type fix here, can you test the build on this PR with the disableScriptCheck: true? https://github.com/hirosystems/wallet/pull/4060

fbwoolf commented 1 year ago

@btcopenstamp see issue in btc-signer lib about type ms ...being told you should change it to tr? https://github.com/paulmillr/scure-btc-signer/issues/55

paulmillr commented 1 year ago

see btc-signer readme for proper usage of p2tr_ms.

fbwoolf commented 1 year ago

@btcopenstamp are you able to refactor your implementation based on this ^ so we can test works?

btcopenstamp commented 1 year ago

@btcopenstamp I rebased on the sighash type fix here, can you test the build on this PR with the disableScriptCheck: true? #4060

Hi @fbwoolf. Sorry for the late response. Being busy these days.

It works fine to sign SIGHASH_ALL|SIGHASH_ANYONECANPAY PSBT transaction based on the build at #4060 .

And We are using bitcoinjs-lib to create PSBT trx, so there is no parameter 'disableScriptCheck'. But it seems all work fine for us right now.

TBH, I don't know much about p2tr_ms. If it is relate to taproot, it should be not related to the original issue. Because our transaction has nothing to do with taproot.

fbwoolf commented 1 year ago

@markmhx @kyranjamie what do you think? Should we merge the PR fix linked here?

paulmillr commented 1 year ago

@btcopenstamp if bitcoinjs-lib accepts your tx it doesn't mean it's valid. The checks are there for a reason.

If you are using basic multisig, it must be wrapped in P2SH / P2WSH / P2SH-P2WSH.

If you are using taproot multisig, it must be wrapped in P2TR.

btcopenstamp commented 1 year ago

Hi @paulmillr. Thanks for the reminder. We are using bare multisig which is not a basic multisig.

Bare Multisig Locking Script Format: OP_M ... OP_N OP_CHECKMULTISIG

markmhendrickson commented 1 year ago

@fbwoolf works for me to merge!

paulmillr commented 1 year ago

This is bare / basic multisig. And it must be wrapped into p2sh / p2wsh.

https://github.com/paulmillr/scure-btc-signer/blob/4bf86688bbb64d923bd7a604d651ddd11b38b4e9/index.ts#L1050

https://learnmeabitcoin.com/technical/p2ms

fbwoolf commented 1 year ago

@markmhx it doesn't seem like a good idea to merge it bc the implementation is outdated on the app side? ^

btcopenstamp commented 1 year ago

Please check out https://mempool.space/tx/757b329dcdd3fea73d7b82d4e3ae22bd8b7ac73e0a46f833ee67fa380ba31bdf

It used the pubkey of bare multi-sig output to store data, not normal(P2SH...) multisig to hold BTC.

paulmillr commented 1 year ago

It's doable but it should not be done. It's written here: https://learnmeabitcoin.com/technical/p2ms.

paulmillr commented 1 year ago

I'm not sure why I need to repeat it 5 times. You are doing something that has been discouraged since p2sh was released in 2011. This is bad behavior that should not be encouraged and we definitely should not add it as "default" behavior. If you need this, you should be fine with maintaining a fork.

btcopenstamp commented 1 year ago

Yeah. I got your point. But that's how counterparty and btc stamps protocol work, it's not what we want to do that way. So I think there's no need to discuss why we need to use that kind of multisig here.

Thank you for your thoughtful and detailed explanation.

fbwoolf commented 1 year ago

@kyranjamie wanted to make you aware of this ^ if you haven't been tracking the issue.

markmhendrickson commented 1 year ago

@fbwoolf I'm not following the details here, so it's up to you and @kyranjamie whether your PR seems appropriate to merge. It does sound like @paulmillr is cautioning against it. Let's perhaps discuss briefly as a team this coming week for shared context?

btcopenstamp commented 1 year ago

By the way, since src20 transfer function is related with bare multisig output, if Hiro would like to support Src20 transfer in the future, Hiro also needs to compose src20 transfer tx with bare multisig outputs.

markmhendrickson commented 1 year ago

There's a debate going on about Bare MultiSig on the protocol level here, due to a proposed change that would have nodes filter them out from the mempool by default (intended to prevent stamps, which are viewed by the change's proponents as network spam): https://github.com/bitcoin/bitcoin/pull/28217

btcopenstamp commented 11 months ago

Hi @fbwoolf @markmhendrickson ,

Sorry for the late feedback.

I am not sure whether Leather is gonna support signing bare multi-sig since the debate @markmhendrickson mentioned before.

I just tried the current Leather version, but it seems #4168 does not solve the problem. And like I said before, the test package on #4060 works fine for this issue, but #4060 still on open status.

markmhendrickson commented 11 months ago

Thanks for following up, @btcopenstamp.

Are you still getting the same error as originally reported in this issue when trying in the latest version? Or is there a different behavior?

btcopenstamp commented 11 months ago

Hi @markmhendrickson

Please check the current error screenshot. image

fbwoolf commented 11 months ago

Right, bc the fix was for address type being 'unknown`, makes sense it didn't fix for you.

btcopenstamp commented 11 months ago

Understood. I assumed issue #4186 had resolved this. So it appears we're still hindered by https://github.com/paulmillr/scure-btc-signer/issues/55. However, Paulmillr suggests avoiding the ms address type.

If Leather doesn't offer a workaround, it might be impossible to support SRC20 trading via Leather on OpenStamp. If that's the case, we could consider closing this issue.

btcopenstamp commented 11 months ago

Please also check the latest comment at https://github.com/leather-wallet/extension/issues/4215

btcopenstamp commented 10 months ago

@fbwoolf @markmhendrickson With SRC20 gaining popularity, many Leather users are moving to Unisat for its bare multi-sig output support. We suggest that the Leather team reconsider adding this feature.