Return Taproot compiler errors

[febyeji]

Summary

Fix two cases where Taproot policy compilation could panic instead of returning an error. Resolves #995.

Follow-up

While working on this fix, I noticed that the current Huffman construction can produce a tree deeper than Taproot allows, even though arranging the same leaves differently could produce a valid tree. (regarding HuffmanTreeDepthExceeded)

Would it be worth developing a fallback that finds another valid tree when the current Huffman construction exceeds the depth limit of Taptree?

One option might be a length-limited Huffman construction, which minimizes weighted path length subject to a maximum depth. (The current construction optimizes weighted path length without maximum-depth constraint.)

I would appreciate feedback on whether this would make sense as a follow-up.

[apoelstra]

While working on this fix, I noticed that the current Huffman construction can produce a tree deeper than Taproot allows,

Can you describe an example of this? If your "right-skewed or" which has equal weight for every branch actually triggers this, that is a serious bug in our huffman encoding, since a 128-depth tree does not resemble the optimal huffman encoding at all.

In general, can you split your unit tests into separate commits from the fixes, so that it's easy to cherry-pick the tests onto the original branch (and various fix candidates)?

[apoelstra]

It shouldn't even be possible to express a policy where the huffman tree exceeds depth 128 because you'd need to write two weights that differed by a factor 2^128 and we refuse to parse numbers outside of [1, 2^32-1].

[febyeji]

Thanks. I was imprecise about the example I had in mind. I was thinking of a nested policy like or(1@pk(K0), 1@or(1@pk(K1), ...)), which produces geometrically decreasing Tapleaf probabilities without exceeding the parser’s integer limit.

use std::str::FromStr;
use miniscript::policy::Concrete;

fn main() {
    const N_KEYS: usize = 131;

    let mut source = format!("pk(K{})", N_KEYS - 1);
    for i in (0..N_KEYS - 1).rev() {
        source = format!("or(1@pk(K{i}),1@{source})");
    }
    
    let policy = Concrete::<String>::from_str(&source).unwrap();
    let error = policy.compile_tr(None).unwrap_err(); // reaches `expect` and panics in the original code
    
    println!("{error:?}");
}

I haven’t reviewed the Huffman implementation in detail, but I don’t suspect an issue with it. My point is that, for a nested policy like this, the resulting Huffman tree can exceed the TapTree depth limit even though the same set of Tapleaves could fit arranged in a more balanced shape.

For this PR, I’ll split the tests into a separate commit from the implementation.

[apoelstra]

This PR now has a commit with tests, none of which fail, followed by behavioral changes, none of which are tested. It also contains no example of a policy which can trigger the maximum-depth check and your vague description of one does not make sense to me.