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

Use tighter loose bounds #799

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JasonGross
Copy link
Collaborator

We now compute the loose bounds to be as tight as possible given the
tight bounds: they are the tightest bounds that will let us add two
tightly-bounded field elements, and will also let us subtract two
tightly-bounded field elements (with balance), without needing to carry.

This is probably required for having 32-bit p448 work, but even with
this, p448 is still not working; see
#797. Seems worth merging
anyway.

@andres-erbsen @davidben please review either the changes in the bounds in the comments, or the changes in the definition of loose_upperbounds in src/UnsaturatedSolinasHeuristics.v, or both.

@jadephilipoom Please review my changes to your proof of loose_bounds_within_base_access_sizes in src/Bedrock/Tests/Proofs/X25519_64.v (the upper bounds are no longer repeat r n, so I had to use a lemma about relaxing them, and then relaxed them to the repetition of the max element).

Copy link
Collaborator

@davidben davidben left a comment

Choose a reason for hiding this comment

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

I transcribed the new curve25519 bounds into the asserts in our code and the tests still passed. I probably am not able to provide useful review beyond that. :-)

Copy link
Collaborator

@jadephilipoom jadephilipoom left a comment

Choose a reason for hiding this comment

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

The changes to Tests/ aren't too important because I've now completely deleted Tests/ in my branch (bedrock2_interface) and replaced it with a new thing. It might be a new form of headache now (but hopefully not, I think it's actually just left as computation of is_tighter_than_bool), but I'll consider that my problem.

JasonGross added a commit to JasonGross/fiat-crypto that referenced this pull request May 27, 2020
We now compute the loose bounds by recording the number of additions
and/or balanced subtractions we want to be able to do in a row before we
need to multiply and carry.  Note that balanced subtraction always takes
a bit more overhead than addition.

This is a more conservative variant of mit-plv#799 that doesn't actually change
the bounds, and makes the senuine change of mit-plv#799 easier to see, namely,
reducing `headspace_add_count` from `2` to `1`.
JasonGross added a commit to JasonGross/fiat-crypto that referenced this pull request May 27, 2020
We now compute the loose bounds by recording the number of additions
and/or balanced subtractions we want to be able to do in a row before we
need to multiply and carry.  Note that balanced subtraction always takes
a bit more overhead than addition.

This is a more conservative variant of mit-plv#799 that doesn't actually change
the bounds, and makes the genuine change of mit-plv#799 easier to see, namely,
reducing `headspace_add_count` from `2` to `1`.
@JasonGross JasonGross force-pushed the tighter-loose-bounds branch from e47f7bb to 71a03a6 Compare May 27, 2020 16:21
JasonGross added a commit to JasonGross/fiat-crypto that referenced this pull request May 28, 2020
We now compute the loose bounds by recording the number of additions
and/or balanced subtractions we want to be able to do in a row before we
need to multiply and carry.  Note that balanced subtraction always takes
a bit more overhead than addition.

This is a more conservative variant of mit-plv#799 that doesn't actually change
the bounds, and makes the genuine change of mit-plv#799 easier to see, namely,
reducing `headspace_add_count` from `2` to `1`.
JasonGross added a commit that referenced this pull request May 28, 2020
We now compute the loose bounds by recording the number of additions
and/or balanced subtractions we want to be able to do in a row before we
need to multiply and carry.  Note that balanced subtraction always takes
a bit more overhead than addition.

This is a more conservative variant of #799 that doesn't actually change
the bounds, and makes the genuine change of #799 easier to see, namely,
reducing `headspace_add_count` from `2` to `1`.
@JasonGross JasonGross force-pushed the tighter-loose-bounds branch from 71a03a6 to a0a02d2 Compare May 28, 2020 22:46
We now compute the loose bounds to be as tight as possible given the
tight bounds: they are the tightest bounds that will let us add two
tightly-bounded field elements, and will also let us subtract two
tightly-bounded field elements (with balance), without needing to carry.

This is probably required for having 32-bit p448 work, but even with
this, p448 is still not working; see
mit-plv#797.  Seems worth merging
anyway.
@JasonGross JasonGross force-pushed the tighter-loose-bounds branch from a0a02d2 to 25b497c Compare May 29, 2020 02:53
@hannesm
Copy link
Contributor

hannesm commented Feb 4, 2021

I came here while reading #797 which I'm interested in. I understand this does not bring fiat-crypto to p448-32, but already goes some way. I was wondering (since this has multiple positive reviews) what is missing to get this PR merged (apart from a rebase)?

@JasonGross
Copy link
Collaborator Author

My impression from #797 (comment) was that determining headroom is more complicated than just "how many additions / subtractions do you want to be able to do in a row without carrying?" and hence the one-line change here (changing headroom_add_count from 2 to 1) does not actually make progress conceptually towards handling p448-32. I did in fact merge the earlier underlying changes, i.e., factoring the computation in a way that lets us just declare how many times we want to be able to add in a row and bounds for that.

I think the thing standing in the way of merging a PR like this is having an understanding of what the right interface to present here is. Should we compute the loose bounds by asking people to specify how many additions they'd like to perform in a row (and encode it somehow in the types so that users can in fact perform that many additions in a row)? It's not clear to me what the landscape looks like around choosing bounds.

If it's useful to you, I'm happy to make headroom_add_count an argument to the binary, but again I think this is the wrong strategy in the absence of a vision about what should be tunable parameters of the implementation, and what should be computed automagically and how. Perhaps @bitwiseshiftleft or @kevaundray has thoughts on what a global plan for this should look like.

@hannesm
Copy link
Contributor

hannesm commented Feb 4, 2021

Thanks for your explanation @JasonGross. Indeed that sounds like this PR may not be relevant for the p448-32 goal. I'll read up on #797 to figure out the missing bits and pieces when I find enough time and headroom.

@JasonGross JasonGross marked this pull request as draft October 4, 2021 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants