Skip to content
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

Replace ThirtyTwoByteHash trait impl for more generic From #211

Closed

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Apr 24, 2020

This patch replaces the ThirtyTwoByteHash trait with a more generic From impl that allows us to construct a message from any 32-byte array.

There is a companion PR for bitcoin_hashes that provides said impls which plays nicely together with this: rust-bitcoin/bitcoin_hashes#81

Judging from the comment on the original ThirtyTwoByteHash trait, the idea of this trait was to make good use of the type system to prevent conversions that make semantically no sense.
Hence, in an ideal world, the bytes going into a message should always come from a hash function.

I would argue that this is unnecessarily restrictive and the more general From impl is more useful and allows for type interactions that are more pleasant to use (see the updated examples).

Also, in practice, we already allow users to construct a message from arbitrary bytes using the from_slice function. The downside of this function is that imposes a runtime error that in many cases "can never happen", yet we have to deal with in in the type system.

@apoelstra
Copy link
Member

I'm starting to come around to this view. Especially in light of #207 where we observe that upstream allows 0 messages, maybe we should just remove this safety feature in favor of ergonomics.

@real-or-random
Copy link
Collaborator

I think these statements are true:

  • If upstream allows 0, we should allow 0, too.
  • If we allow the group order of the scalar group, we should allow 0.
  • If we have from_slice we should offer a similar thing for arrays.

The unusual thing here is that we use the type system to enforce a property that is not an ordinary invariant. A message can be any fixed (i.e., not random) value, including 0. It's not wrong or illegal for a message to be 0x00..00deadbeef or 0x00...00, it's just unlikely. In this respect, there's nothing special about 0.

This is why I wouldn't be happy with @stevenroose's suggestion to have a Message::zero constructor. This requires special casing on the caller side. The caller typically has an arbitrary byte array and not a zero constant. (Also, even if we had this, we shouldn't make it unsafe because zero would not violate any safeness guarantee of our library. You don't get UB with the 0 message.)

I would argue that this is unnecessarily restrictive and the more general From impl is more useful and allows for type interactions that are more pleasant to use (see the updated examples).

Yes, but these are separate concerns. It's restrictive but I don't agree that potential insecure usages should be pleasant to use.

What about having a "loud" API? We could call this insecure_from_array (or similar), also have insecure_from_slice and force the user to spell out the function call explicitly? This is like overflowing_add. The user can do all of this but they'll be reminded to read the docs.

@thomaseizinger
Copy link
Contributor Author

Closing because the ThirtyTwoByteHash works well enough if dependencies are kept up to date!

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.

None yet

3 participants