descriptor_parse: crash with tr descriptor

[brunoerg]

Descriptor: tr(02d1d11100A0000000000000000000003509693017680d02d00008c0d00008cc,pkh(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1))

Bitcoin Core successfully parses it, rust-miniscript returns the following error "pubkey string should be 66 or 130 digits long, got: 64".

[apoelstra]

With Taproot descriptors you need to set the pubkey type that you're parsing to XOnlyPublicKey if you want to have bare keys.

I'm kinda curious how Core represents the thing that you're parsing here. Does it always accept 32-byte keys in all descriptors? Or does its "descriptor public key" type support 32-byte keys but there's an extra check when parsing Taproot descriptors? Or what?

[apoelstra]

Broadly speaking, I'm open to us rethinking the relationship between x-only and old-style keys in rust-miniscript. Maybe all keys should be representable either in 33- or 32-byte form, with DescriptorPublicKey having an x-only variant which indicates that a key should be hex-serialized in 32-byte form in the string representation of descriptors?

[brunoerg]

With Taproot descriptors you need to set the pubkey type that you're parsing to XOnlyPublicKey if you want to have bare keys.

I think this is done. See: https://github.com/brunoerg/bitcoinfuzz/blob/v2/modules/rustminiscript/rust_miniscript_lib/src/lib.rs#L21

I'm kinda curious how Core represents the thing that you're parsing here. Does it always accept 32-byte keys in all descriptors? Or does its "descriptor public key" type support 32-byte keys but there's an extra check when parsing Taproot descriptors? Or what?

I quickly debugged it, and I noticed that "02d1d11100A0000000000000000000003509693017680d02d00008c0d00008cc" becomes "0202d1d11100A0000000000000000000003509693017680d02d00008c0d00008cc" which is valid.

When the data is 32-byte and under P2TR, it puts a 0x02 at the start.

} else if (data.size() == 32 && ctx == ParseScriptContext::P2TR) {
    unsigned char fullkey[33] = {0x02};
    std::copy(data.begin(), data.end(), fullkey + 1);
    pubkey.Set(std::begin(fullkey), std::end(fullkey));
    if (pubkey.IsFullyValid()) {
        ret.emplace_back(std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, true));
        return ret;
    }
}

[apoelstra]

I think this is done. See:

I think you're gonna fail to parse with DescriptorPublicKey because of the xonly key in the keyspend slot, and fail to parse with XOnlyPublicKey because of the bip32(?) key in the pkh() combinator.

When the data is 32-byte and under P2TR, it puts a 0x02 at the start.

Hmm. I'm skeptical that we want to reproduce this behavior. What do you think @sanket1729?

[brunoerg]

It seems Core converts x-only keys to compressed public keys for validation.

[sanket1729]

When the data is 32-byte and under P2TR, it puts a 0x02 at the start.

Hmm. I'm skeptical that we want to reproduce this behavior. What do you think @sanket1729?

I think we already do something similar. See the test here that checks that x-only keys are parsed correctly in tr context.

        Desc::from_str(&format!("tr({},pk({}))", x_only_key, x_only_key)).unwrap(); // works

@apoelstra the issue is about pkh() fragment takes in a wif private key. We don't support parsing private keys in from_str method. We have a separate Descriptor::parse_descriptor method for parsing private keys. I wonder if this can be tackled now that we static contexts in secp. Maybe we can somehow directly implement MiniscriptKey for bitcoin::PrivateKey or bip32::Xpriv

@brunoerg

https://github.com/brunoerg/bitcoinfuzz/blob/5d72efc41882e8d95f7b3989e83055539dbf2547/modules/rustminiscript/rust_miniscript_lib/src/lib.rs#L19C24-L35

The code can be simplified to use only Descriptor<DescriptorPublicKey> as it covers all the cases of PublicKey, XOnlyPublicKey, bip32 keys. DescriptorPublicKey should be compliant with all descriptor bips including multi-path descriptors. After using DescriptorPublicKey in fuzztest, we see that first part of x-only key is successfully parsed but the second pkh part fails.

If you want to also parse private keys, you can use the parse_descriptor API .

Summary, the core issue here is bitcoin core accepts private key descriptors, but our from_str does not. We can use parse_descriptor to get the same results.

[sanket1729]

One of the annoying thing we still have is, it hard to accurately report which Pk parsing failed that is no-std compatible. Maybe when using pub struct DescriptorKeyParseError(&'static str), we can attach first 8 chars of Pk string to provide user more context.

On master this fails with

Parse(FromStr(DescriptorKeyParseError("Key too short (<66 char), doesn't match any format")))

which is incorrect, but also does not provide any info about which key failed.

[sanket1729]

rust-bitcoin/rust-miniscript#786 . Fixing the error message, we can discuss identification of which key failed in a separate issue

[brunoerg]

@apoelstra the issue is about pkh() fragment takes in a wif private key. We don't support parsing private keys in from_str method. We have a separate Descriptor::parse_descriptor method for parsing private keys. I wonder if this can be tackled now that we static contexts in secp. Maybe we can somehow directly implement MiniscriptKey for bitcoin::PrivateKey or bip32::Xpriv

Cool, I'm gonna address it.