Skip to content

POC: Add custom hash types to try and simplify all the associated hash type stuff #621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from

Conversation

tcharding
Copy link
Member

Hey @apoelstra this is another exploratory POC based on recent discussion about MiniscriptKey and ToPublicKey. It is also in a similar vein to #600.

Just the last patch, I think this works this time. Please take a look at your leisure.

From the last patch to save you pulling down the branch

    Add a custom Sha256 type
    
    It seems that we do a whole bunch of stuff with the hash types to keep
    the Rust compiler happy and be able the implement `MiniscriptKey` on
    `String`. I believe we are conflating two things:
    
    - Support for string "placeholders" in miniscript
    - The ability to mock in test code
    
    This all stems from the observation that `MiniscriptKey` does not
    _really_ describe a key and neither does `ToPublicKey` because of the
    hash types.
    
    To clarify that I understand how we got here; we have a generic key on
    most functions/types and also we need type information about hashes for
    terminals, so instead of carrying a bunch of generics we store it all in
    `MiniscriptKey`. Then we implement `MinisicriptKey` for `String` so we
    can use, for example, `pk(A)` but this breaks the hash type stuff so we
    add `ToPublicKey` for types that can be encoded as _real_ miniscript (I
    think).
    
    This patch is a POC that we can add a custom type, that way we can
    remove the associated hash types then we can do translation in the type
    because the type knows if it is a _real_ hash or a string placeholder
    thing.
    
    As this is a POC its only done for sha256.

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.
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.
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.
We can remove the whitespace between associated hash types in the
`MiniscriptKey` trait with on loss of clarity.
These trait method implementations are trivial, we can squash them down
with no loss of clarity (as we do elsewhere in the codebase).
It seems that we do a whole bunch of stuff with the hash types to keep
the Rust compiler happy and be able the implement `MiniscriptKey` on
`String`. I believe we are conflating two things:

- Support for string "placeholders" in miniscript
- The ability to mock in test code

This all stems from the observation that `MiniscriptKey` does not
_really_ describe a key and neither does `ToPublicKey` because of the
hash types.

To clarify that I understand how we got here; we have a generic key on
most functions/types and also we need type information about hashes for
terminals, so instead of carrying a bunch of generics we store it all in
`MiniscriptKey`. Then we implement `MinisicriptKey` for `String` so we
can use, for example, `pk(A)` but this breaks the hash type stuff so we
add `ToPublicKey` for types that can be encoded as _real_ miniscript (I
think).

This patch is a POC that we can add a custom type, that way we can
remove the associated hash types then we can do translation in the type
because the type knows if it is a _real_ hash or a string placeholder
thing.

As this is a POC its only done for sha256.
@tcharding tcharding marked this pull request as draft October 19, 2023 05:13
@apoelstra
Copy link
Member

NACK. This dramatically reduces functionality.

You can see in the ICBOC code that I linked that a "placeholder key" is not necessarily a string, nor is it necessarily human readable. In the case of ICBOC it is an indefinite descriptor and (optionally) a cached key that has been fetched from a HWW. In Liquid it is a bitcoin publickey and a flag indicating whether it should be tweaked when deriving peg-in addresses. In demo tools and tests it is often a literal string.

Hashes are no different from keys in this respect.

@tcharding
Copy link
Member Author

NACK. This dramatically reduces functionality.

Damn, I'll understand this crate one day, mark my word.

@tcharding tcharding closed this Dec 15, 2023
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