Remove MiniscriptKey associated hash types

[tcharding]

Done on top of #572 so just the last 3 patches.

This one seems too good to be true net 2000 lines of red in the diff! Raising as draft till I have another look at it tomorrow.

Remove the associated hash types from MiniscriptKey - epic!

Please note: this was a whole bunch of wild hacking based on a hunch, now I've got tests passing locally I'm not sure we need the first two patches and the new StringKey type. Gotta run, no time to investigate CI fails.

[apoelstra]

What replaces these associated types? It looks like it forces the hash types to always use the bitcoin_hashes hash types. How can a user handle generic keys that don't have fixed hashes of this form?

[tcharding]

Caveat: I'm only 1/4 of the way to understanding miniscript, if that and I don't fully grok what the String "key" is used for.

At first I called the new type Placeholder instead of StringKey until I realized we have a type named that already. As far as I can deduce a Policy<String> (eg, pk(A)) would only be useful either in tests or as a placeholder before A was filled in from somewhere (and this last use case I'm guessing). Am I missing something?

If that is the case my hunch was that String is really either a mock for testing or some sort of placeholder. Either way we can remove the associated types and drastically reduce/simplify the code (test can be mocked a better way, and placeholder can use Hash::ZERO or something).

[apoelstra]

How would you represent the placeholder for the hash of A if you have to stick an actual hash in there?

[tcharding]

In the FromStr impl of StringKey we could just store the A and use an obviously bogus const (eg, ZERO, EMPTY, PLACEHOLDER`).

[apoelstra]

Store it where?

Sorry to be so Socratic here -- my point is that we deliberately support having placeholders (or other custom types) for hashes just like we support placeholders for keys.

[tcharding]

Sorry to be so Socratic here

Not at all! There are likely better uses of your time but if its interesting then the reason for my "hunch" that we could remove these and simplify the code is that we have 4 key types (bitcoin/secp pubkey, xonly, and string), 3 real keys and a "fake" one with hash types for the 3 being real again and the String hash types "fake". Any time I see this sort of abstraction it smells to me like it can more cleanly be done another way. So I tried to work out what the fake one was used for and it seemed like a placeholder and for testing. For testing this is definitely a messy way to do it (the PR currently removes some hard code values that need to be put back somewhere). For placeholder functionality I'm pretty sure we can do something more clever also, but I didn't code it up. FWIW I watched these hash types get added and was suspicious of them since then, just never got around to looking into it.

[apoelstra]

we have 4 key types

These are public traits. All of my uses of rust-miniscript involve implementing these traits for custom key types. And String is no more "fake" than any other type. It is used when you have human-readable names for keys and hashes etc. It's not just for testing. "Placeholder" is a reasonable way to describe it, though it understates the value of this.

For placeholder functionality I'm pretty sure we can do something more clever also

Well, we used to have separate generics for all the hash types and the public keys, because conceptually these are all independent things. But having 5 generics on every type was a pretty awful burden on users of the codebase, so we moved them to associated types, which has been much nicer.

[tcharding]

These are public traits. All of my uses of rust-miniscript involve implementing these traits for custom key types.

ahhh, that changes things quite a lot. Thanks for being patient with me :)