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.

Sign up for GitHub

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

Jump to bottom

WIP Group verification #1032

Closed
wants to merge 5 commits into from
Closed

WIP Group verification #1032

wants to merge 5 commits into from

Conversation

peterdettman
Copy link
Contributor

Sets up pre- and post- method verification of _ge and _gej group elements. At the moment, this is concerned mainly with imposing a tighter limit (than the default) on the magnitudes of field elements x, y (,z).

Having guarantees about the magnitudes in input group elements can let us avoid some of the normalization calls needed at the start of several group addition methods, and perhaps e.g. use an alternative double algorithm. There may be a trade-off between the effort needed to get outputs to meet lower limits vs the benefits that provides to other methods.

@peterdettman
Copy link
Contributor Author

Removing _normalize_weak from several group add methods gives 2-3% speedup across major benchmarks (64 bit).

@peterdettman peterdettman mentioned this pull request Dec 6, 2021
Merged
@peterdettman
Copy link
Contributor Author

Originally conceived many years ago now: #159 .

@peterdettman peterdettman mentioned this pull request Dec 23, 2021
Merged
@peterdettman peterdettman mentioned this pull request Dec 24, 2021
Open
@peterdettman
Copy link
Contributor Author

Rebased and added some missing verify calls.

I've noted that there are several places where code directly manipulates the fields of group elements without calling a group method to do so. So the group structs are a bit too "open" at the moment. It should be possible to add suitable methods so that the group structs act more like abstract data types, and in particular so that we have a definite boundary at which to be able to place VERIFY calls in relation to group internals.

@peterdettman peterdettman mentioned this pull request Jan 11, 2022
Closed
@real-or-random
Copy link
Contributor

@peterdettman This has "WIP" in the title but it looks pretty mature already. Can you comment on the status?

@peterdettman
Copy link
Contributor Author

@real-or-random See my previous comment; basically there are still quite a few unguarded local operations on group structs (i.e. not abstracted as group methods). These are not too difficult to track down comprehensively, but it occurs to me that, even once committed, we might need to allow some time for the abstraction to sink in to developers' minds before trying to exploit it (as per the "Save _normalize_weak..." commit) - there might be some backsliding. We could discuss ways of enforcing the abstraction in the language (or tooling), but the field implementations are in the same boat and just rely on "it being understood".

@sipa
Copy link
Contributor

sipa commented May 10, 2023

A notion of group verification was introduced through #1299. The later commits here will need to be redone on top of that.

@sipa sipa mentioned this pull request Jun 14, 2023
Merged
theStack added a commit to theStack/secp256k1 that referenced this pull request Jun 15, 2023
@theStack @peterdettman
add missing group element invariant checks
78ef599 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
@theStack theStack mentioned this pull request Jun 15, 2023
Merged
theStack added a commit to theStack/secp256k1 that referenced this pull request Jun 27, 2023
@theStack @peterdettman
add missing group element invariant checks
a55902c 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
@real-or-random
Copy link
Contributor

Closing in favor of #1348

theStack added a commit to theStack/secp256k1 that referenced this pull request Jul 10, 2023
@theStack @peterdettman
add missing group element invariant checks
672b101 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
theStack added a commit to theStack/secp256k1 that referenced this pull request Jul 14, 2023
@theStack @peterdettman
add missing group element invariant checks
0ce5892 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
theStack added a commit to theStack/secp256k1 that referenced this pull request Jul 14, 2023
@theStack @peterdettman @real-or-random
add missing group element invariant checks
9b8aa98 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Also, for easier review, all functions handling group elements are
structured in the following wasy for easier review (idea suggested by
Tim Ruffing):

- on entry, verify all input ge, gej (and fe)
- empty line
- actual function body
- empty line
- on exit, verify all output ge, gej

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
theStack added a commit to theStack/secp256k1 that referenced this pull request Jul 15, 2023
@theStack @peterdettman @real-or-random
add missing group element invariant checks
72aa104 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Also, for easier review, all functions handling group elements are
structured in the following wasy for easier review (idea suggested by
Tim Ruffing):

- on entry, verify all input ge, gej (and fe)
- empty line
- actual function body
- empty line
- on exit, verify all output ge, gej

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
theStack added a commit to theStack/secp256k1 that referenced this pull request Jul 22, 2023
@theStack @peterdettman @real-or-random
add missing group element invariant checks
690b0fc 
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f202667
(PR bitcoin-core#1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba5 of WIP-PR bitcoin-core#1032 (which is
obviously not rebased on bitcoin-core#1299 yet).

Also, for easier review, all functions handling group elements are
structured in the following wasy for easier review (idea suggested by
Tim Ruffing):

- on entry, verify all input ge, gej (and fe)
- empty line
- actual function body
- empty line
- on exit, verify all output ge, gej

Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
real-or-random added a commit that referenced this pull request Aug 16, 2023
@real-or-random
Merge #1348: tighten group magnitude limits, save normalize_weak call…
eedd781 
…s in group add methods (revival of #1032)

b7c685e Save _normalize_weak calls in group add methods (Peter Dettman)
c83afa6 Tighten group magnitude limits (Peter Dettman)
173e8d0 Implement current magnitude assumptions (Peter Dettman)
49afd2f Take use of _fe_verify_magnitude in field_impl.h (Sebastian Falbesoner)
4e9661f Add _fe_verify_magnitude (no-op unless VERIFY is enabled) (Peter Dettman)
690b0fc add missing group element invariant checks (Sebastian Falbesoner)

Pull request description:

  This PR picks up #1032 by peterdettman. It's essentially a rebase on master; the original first commit (09dbba5) which introduced group verification methods has mostly been replaced by PR #1299 (commit f202667) and what remains now is only adding a few missing checks at some places. The remaining commits are unchanged, though some (easy-to-solve) conflicts appeared through cherry-picking. The last commit which actually removes the `normalize_weak` calls is obviously the critical one and needs the most attention for review.

ACKs for top commit:
  sipa:
    utACK b7c685e
  real-or-random:
    ACK b7c685e
  jonasnick:
    ACK b7c685e

Tree-SHA512: f15167eff7ef6ed971c726a4d738de9a15be95b0c947d7e38329e7b16656202b7113497d36625304e784866349f2293f6f1d8cb97df35393af9ea465a4156da3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
assurance performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@peterdettman @real-or-random @sipa