Skip to content

Conversation

@dennisreimann
Copy link

This was introduced in #696 and it seems like an arbitrary limit.

@joemphilips What is the reason for this?

@joemphilips
Copy link
Contributor

This limit is taken from original implementation in bitcoind
https://github.com/bitcoin/bitcoin/blob/71abee86dba8f311775104c518644c55c3e123cf/src/test/descriptor_tests.cpp#L440

Why you want to use bare multisig? IIUC it is deprecated and you should use wsh(multi()) or sh(multi()) or sh(wsh(multi()))

@dennisreimann
Copy link
Author

This isn't a bare multisig, I'm coming across it in a wsh(sortedmulti(2,[4 PUBKEYS])) scenario when importing a wallet file containing that output descriptor in BTCPay Server.

But now that you hinted at that, I think the error isn't in the implementation here, but in our DerivationSchemeParser, which uses OutputDescriptor.Parse recursively, @NicolasDorier.

@dennisreimann
Copy link
Author

@joemphilips I was able to fix this issue on our end — thanks for the pointer, you are correct!

@dennisreimann dennisreimann deleted the patch-1 branch December 5, 2022 17:04
NicolasDorier pushed a commit to btcpayserver/btcpayserver that referenced this pull request Dec 7, 2022
* Fix Output Descriptor parsing for WSH multisig case

Reuse existing function for extracting from a multisig descriptor, instead of recursively parsing the inner output descriptor. The latter would run into invalid cases, because it'd be interpreted as bare multisig, which supports only up to three public keys. 

For further details see MetacoSA/NBitcoin#1151.

* Add CanParseDerivationSchemes test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants