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

fix: remove Hash + PartialEq on ConfirmedOrder and dependent structs #267

Merged
merged 1 commit into from
Jan 26, 2022
Merged

fix: remove Hash + PartialEq on ConfirmedOrder and dependent structs #267

merged 1 commit into from
Jan 26, 2022

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Jan 25, 2022

What:

Remove impls of Hash, PartialEq, Eq on CertifiedOrder and dependents

Why

It's a quick and easy way to check that certificates can't be used in a way that assumes the details of their signature set are meaningful. Certificate equivocation (change in signature set without loss of overall validity) is allowed.

We might indeed need to compare CertifiedOrders in the future, in which case a comment at the point where a static_assertions will blow up offers guidance on what to do.

Why not do more?

The reason this doesn't already implement the signature-agnostic way of comparing Certificates is to avoid misuse of that Eq implementation in a context where we do not have distinct ValidCertifiedOrder and UncheckedCertifiedOrder types.
Imagine you have a list of CertifiedOrders L and an Eq function that's signature agnostic. You also have a deduplicate function on vecs that uses Eq:

  • if all certificates are valid, should you call dedup? Probably yes,
  • if all certificates have yet to be checked, should you call dedup? Probably not.

But there's a lot of duplication of utility functions!

Yes. Those are used in a test context where doing the dangerous thing (testing certificates byte for byte) makes sense, either because one of:

  • we check correct ser/de,
  • we check an authority has not led to an update of our stores as a client, or of its own stores as an authority, under what should be an idempotent op

This could be implemented with less code duplication using a test feature (to make sure the utility functions can be exported across crates but only in a test context), but would lead to fighting Rust's feature unification. The ad-hoc utility functions are actually less trouble.

Contributes to #266

Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!!

It's a quick and easy way to check that certificates can't be used in a way that assumes their signature set is meaningful.

We might indeed need to compare CertifiedOrders in the future, in which case a note at the point where a `static_assertions` will blow up will offer a guidansce on what to do.

The reason this doesn't already implement the signature-agnostic way of comparing Certificates is to avoid misuse of that `Eq` implementation in a context where we do not have distinct ValidCertifiedOrder and UncheckedCertifiedOrder types.
Imagine you have a list of CertifiedOrders L and an Eq function that's signature agnostic. You also have a deduplicate function on vecs:
- if all certificates are valid, should you call dedup? Probably yes,
- if all certificates have yet to be checked, should you call dedup? Probably not.

Contributes to #266
@huitseeker huitseeker merged commit 5931d49 into MystenLabs:main Jan 26, 2022
@huitseeker huitseeker deleted the idempotent_certificates branch February 2, 2022 13:05
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