Clean up the MiniscriptKey and ToPublicKey traits
#620
Conversation
|
And this PR led me to ask: #619 |
|
Yeah, this does look like a bug, though in normal use of the interpreter the Bitcoin consensus rules will protect us. Good catch! I also believe that in the interpreter context compressedness doesn't matter (because we are letting the consensus rules have full reign, and trying not to impose any further restrictions). But I don't remember exactly where we landed with the interpreter context. |
|
By "interpreter context" do you mean |
|
I'm still really struggling to understand these hash types. Conceptually a script hashlock is satisfied by providing the hash pre-image, right? It doesn't have anything to do with a key? Why are they in the |
|
Perhaps you can link me (again) to some consumer code of |
|
This is all non-urgent, answer at your leisure please. |
Correct, nothing to do with a key.
Because otherwise we'd need 5 generics instead of 1, which is especially annoying because most users don't use generic hash preimages. I certainly agree that this is confusing, to be abusing the pubkey traits as "generic miniscript object" traits. But this reflects that public keys are used overwhelmingly more often than the other Miniscript types. I would love if we could document this more clearly.
I'm not aware of anybody using hash preimages at all. But Miniscript supports them, and this library therefore needs to support them, and it would be a frustrating and surprising limitation if you could genericize public keys without genericizing hashes. But for an example of how generic keys are useful, see this file in ICBOC which has a generic "cached public key" type which holds a descriptor, but when needed, queries the hardware wallet (a ~100ms operation) to obtain an actual key.
Yes, the library itself pretty-much has only the one key type |
|
Ok, cool. Thanks for the explanation man. |
tcharding
force-pushed
the
10-18-cleanup-traits
branch
from
063b255
to
cae8d57
Compare
The `interpreter::BitcoinKey` uses the default implementation of `MiniscriptKey`, which means `is_uncompressed` returns `false`. However if the full key is a `bitcoin::PublicKey` it may be compressed. Manually implement `MiniscriptKey::is_uncompressed` for `BitcoinKey` and return the compressedness of the inner full key.
We want to support private keys in descriptors, in preparation improve the rustdocs on the `MiniscriptKey` trait by doing: - Use "key" instead of "pukbey". - Fix the links - Improve spacing, use header body foramt
Clean up `MiniscriptKey` implementation for `bitcoin::PublicKey`. - Be uniform, put the trait method implementation below the associated types. - Trait methods have documentation on the trait, remove the unnecessary rustdoc on the implementation.
We can see that the hashes are specified as strings, no need to comment this.
The `MiniscriptKey` and `ToPublicKey` traits are more-or-less the first point of call for users of this library, lets clean them up.
The `is_x_only_key` trait method is used for more than one thing, the code comment is either stale, not exhaustive, or wrong. Let's just remove it.
Implementors of `MiniscriptKey` must reason about the three trait methods, this implies the trait methods are required, not provided. We are using the default impls to remove code duplication, this is an abuse of the default impls. It makes the docs read incorrectly, by implying that implementors _do not_ need to think reason about these trait methods. Remove default trait method implementations and manually implement the trait methods for all current implementors.
There is no obvious reason why the associated types for `MiniscriptKey` are below the trait methods. Move them to the top. Note, the diff shows moving functions not associated types - same thing. Refactor only, no logic changes.
tcharding
force-pushed
the
10-18-cleanup-traits
branch
from
cae8d57
to
133569a
Compare
52198dd Manually implement is_uncompressed for BitcoinKey (Tobin C. Harding) Pull request description: The `interpreter::BitcoinKey` uses the default implementation of `MiniscriptKey`, which means `is_uncompressed` returns `false`. However if the full key is a `bitcoin::PublicKey` it may be compressed. Manually implement `MiniscriptKey::is_uncompressed` for `BitcoinKey` and return the compressedness of the inner full key. Originally done, and discussed, in #620. ACKs for top commit: apoelstra: ACK 52198dd Tree-SHA512: 59f514d24120cfc452e60c361f851280b0515e4b63eb5d2a94d84e39821e6c80f3af07eee5fc5f80bd7198d2e09c6e4465f6f6ef3e8f38dba87ef77a2e1a3b9a
Draft so #666 can go in separately since it is a bug fix and not a clean up.
The rest is clean up, all docs and refactors, except that we remove default implementations from public traits so this is a breaking change - inside the library there are no logic changes.