Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed to sign PSBT transaction with Bare MultiSig UTXO #3897

Closed
btcopenstamp opened this issue Jun 20, 2023 · 45 comments · Fixed by #4060 or #4168
Closed

Failed to sign PSBT transaction with Bare MultiSig UTXO #3897

btcopenstamp opened this issue Jun 20, 2023 · 45 comments · Fixed by #4060 or #4168
Assignees
Labels
area:api area:psbts bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds effort:small Expected to take up to 1 day of integration work

Comments

@btcopenstamp
Copy link

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

@markmhendrickson markmhendrickson added bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds area:api area:psbts labels Jun 20, 2023
@github-project-automation github-project-automation bot moved this to Enhancements backlog in Hiro Wallet (DEPRECATED) Jun 20, 2023
@markmhendrickson markmhendrickson moved this from Enhancements backlog to Assigned in Hiro Wallet (DEPRECATED) Jun 20, 2023
@fbwoolf fbwoolf added the effort:small Expected to take up to 1 day of integration work label Jun 26, 2023
@fbwoolf
Copy link
Contributor

fbwoolf commented Jun 26, 2023

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 fbwoolf moved this from Assigned to WIP in Hiro Wallet (DEPRECATED) Jun 27, 2023
@fbwoolf
Copy link
Contributor

fbwoolf commented Jun 27, 2023

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.

@fbwoolf fbwoolf closed this as completed Jun 27, 2023
@fbwoolf fbwoolf moved this from WIP to Released in Hiro Wallet (DEPRECATED) Jun 27, 2023
@btcopenstamp
Copy link
Author

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

@fbwoolf
Copy link
Contributor

fbwoolf commented Jun 28, 2023

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
Copy link
Author

btcopenstamp commented Jul 10, 2023

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
Copy link
Contributor

fbwoolf commented Jul 10, 2023

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
Copy link
Author

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

@fbwoolf
Copy link
Contributor

fbwoolf commented Jul 12, 2023

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

@fbwoolf fbwoolf reopened this Jul 12, 2023
@fbwoolf fbwoolf moved this to Assigned in Hiro Wallet (DEPRECATED) Jul 12, 2023
@fbwoolf
Copy link
Contributor

fbwoolf commented Jul 14, 2023

@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
Copy link
Author

btcopenstamp commented Jul 18, 2023

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
Copy link
Author

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

@fbwoolf
Copy link
Contributor

fbwoolf commented Jul 24, 2023

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: #4026

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

@fbwoolf fbwoolf moved this from Assigned to WIP in Hiro Wallet (DEPRECATED) Jul 25, 2023
@paulmillr
Copy link

@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
Copy link
Author

btcopenstamp commented Aug 4, 2023

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

Bare Multisig Locking Script Format: OP_M <pub_key_1> <pub_key_2> ... <pub_key_N> OP_N OP_CHECKMULTISIG

@markmhendrickson
Copy link
Collaborator

@fbwoolf works for me to merge!

@paulmillr
Copy link

paulmillr commented Aug 4, 2023

@fbwoolf
Copy link
Contributor

fbwoolf commented Aug 4, 2023

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

@btcopenstamp
Copy link
Author

btcopenstamp commented Aug 5, 2023

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
Copy link

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

@paulmillr
Copy link

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
Copy link
Author

btcopenstamp commented Aug 5, 2023

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
Copy link
Contributor

fbwoolf commented Aug 5, 2023

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

@markmhendrickson
Copy link
Collaborator

@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
Copy link
Author

btcopenstamp commented Aug 7, 2023

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
Copy link
Collaborator

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): bitcoin/bitcoin#28217

@btcopenstamp
Copy link
Author

btcopenstamp commented Oct 24, 2023

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
Copy link
Collaborator

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
Copy link
Author

Hi @markmhendrickson

Please check the current error screenshot.
image

@fbwoolf
Copy link
Contributor

fbwoolf commented Oct 25, 2023

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

@btcopenstamp
Copy link
Author

btcopenstamp commented Oct 26, 2023

Understood. I assumed issue #4186 had resolved this. So it appears we're still hindered by paulmillr/scure-btc-signer#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
Copy link
Author

Please also check the latest comment at #4215

@btcopenstamp
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api area:psbts bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds effort:small Expected to take up to 1 day of integration work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants