Skip to content

WIP WIP WIP 5x64 field representation#967

Open
sipa wants to merge 6 commits intobitcoin-core:masterfrom
sipa:5x64
Open

WIP WIP WIP 5x64 field representation#967
sipa wants to merge 6 commits intobitcoin-core:masterfrom
sipa:5x64

Conversation

@sipa
Copy link
Copy Markdown
Contributor

@sipa sipa commented Jul 24, 2021 •
edited
Loading

This swaps out the 5x52 field with a 5x64 one, including both inline and external x86_64 asm code (by @kn-cs).

I'm just opening this to see if it doesn't break anything on the various platforms we have.

@sipa sipa force-pushed the 5x64 branch 3 times, most recently from bb2714b to e8412e4 Compare July 25, 2021 05:24
@sipa sipa force-pushed the 5x64 branch 3 times, most recently from b85d66c to 237baa9 Compare August 5, 2021 20:44
@sipa sipa force-pushed the 5x64 branch 2 times, most recently from 9759c2d to e609fb4 Compare August 23, 2021 21:24
@sipa
Copy link
Copy Markdown
Contributor Author

sipa commented Oct 17, 2021

So to give an idea of the status here:

  • The code works, is generally a speedup, and is unlikely to change much; if it does, it's probably restricted to just better asm code
  • There is an issue on macOS to figure out (linking fails; I wonder if the asm/linker detect platform-specific instructions, and need some kind of magic runtime detection annotation?)
  • As is, this PR removes the old 5x52 representation. That may be undesirable, as at least on some (uncommon) platforms, the new code is (slightly) slower. This may be even more the case with Avoid overly-wide multiplications in 5x52 field mul/sqr #810 merged now.
  • It's an open question how this is supposed to be reviewed or otherwise gained confidence in. I know there are formal methods of proving semantics of asm code, but I don't have experience with them. An alternative is adding lots of high-level description to the asm code to make it readable.
  • This PR leaves the decision on which repr/impl to use up to the ./configure step. That's useful in some settings, and enough to enable testing, but not what we want for normal production usage. I'm fine with that, but perhaps people want a clearer plan on how runtime autodetection etc would work before proceeding?

@real-or-random
Copy link
Copy Markdown
Contributor

real-or-random commented Oct 18, 2021 •
edited
Loading

There is an issue on macOS to figure out (linking fails; I wonder if the asm/linker detect platform-specific instructions, and need some kind of magic runtime detection annotation?)

According to the CI output, it's the assembler that fails because it does not like the .type directive. A shot in the dark is to simply remove them on MacOS (indicators: briansmith/ring#312 (comment) and https://code.woboq.org/llvm/compiler-rt/lib/builtins/assembly.h.html). I think if you rename .s to .S you can use the C preprocessor with things like #ifdef __APPLE__.

@elichai elichai mentioned this pull request Nov 3, 2021
Open
This was referenced Jan 1, 2022
Open
Closed
@sipa sipa mentioned this pull request Jan 29, 2022
Merged
sipa added a commit that referenced this pull request May 11, 2023
@sipa
Merge #1066: Abstract out and merge all the magnitude/normalized logic
c63ec88 
7fc642f Simplify secp256k1_fe_{impl_,}verify (Pieter Wuille)
4e176ad Abstract out verify logic for fe_is_square_var (Pieter Wuille)
4371f98 Abstract out verify logic for fe_add_int (Pieter Wuille)
89e324c Abstract out verify logic for fe_half (Pieter Wuille)
283cd80 Abstract out verify logic for fe_get_bounds (Pieter Wuille)
d5aa2f0 Abstract out verify logic for fe_inv{,_var} (Pieter Wuille)
3167646 Abstract out verify logic for fe_from_storage (Pieter Wuille)
76d31e5 Abstract out verify logic for fe_to_storage (Pieter Wuille)
1e6894b Abstract out verify logic for fe_cmov (Pieter Wuille)
be82bd8 Improve comments/checks for fe_sqrt (Pieter Wuille)
6ab3508 Abstract out verify logic for fe_sqr (Pieter Wuille)
4c25f6e Abstract out verify logic for fe_mul (Pieter Wuille)
e179e65 Abstract out verify logic for fe_add (Pieter Wuille)
7e7ad7f Abstract out verify logic for fe_mul_int (Pieter Wuille)
65d82a3 Abstract out verify logic for fe_negate (Pieter Wuille)
1446708 Abstract out verify logic for fe_get_b32 (Pieter Wuille)
f7a7666 Abstract out verify logic for fe_set_b32 (Pieter Wuille)
ce4d209 Abstract out verify logic for fe_cmp_var (Pieter Wuille)
7d7d43c Improve comments/check for fe_equal{,_var} (Pieter Wuille)
c5e788d Abstract out verify logic for fe_is_odd (Pieter Wuille)
d3f3fe8 Abstract out verify logic for fe_is_zero (Pieter Wuille)
c701d9a Abstract out verify logic for fe_clear (Pieter Wuille)
19a2bfe Abstract out verify logic for fe_set_int (Pieter Wuille)
864f9db Abstract out verify logic for fe_normalizes_to_zero{,_var} (Pieter Wuille)
6c31371 Abstract out verify logic for fe_normalize_var (Pieter Wuille)
e28b51f Abstract out verify logic for fe_normalize_weak (Pieter Wuille)
b6b6f9c Abstract out verify logic for fe_normalize (Pieter Wuille)
7fa5195 Bugfix: correct SECP256K1_FE_CONST mag/norm fields (Pieter Wuille)
b29566c Merge magnitude/normalized fields, move/improve comments (Pieter Wuille)

Pull request description:

  Right now, all the logic for propagating/computing the magnitude/normalized fields in `secp256k1_fe` (when `VERIFY` is defined) and the code for checking it, is duplicated across the two field implementations. I believe that is undesirable, as these properties should purely be a function of the performed fe_ functions, and not of the choice of field implementation. This becomes even uglier with #967, which would copy all that, and even needs an additional dimension that would then need to be added to the two other fields. It's also related to #1001, which I think will become easier if it doesn't need to be done/reasoned about separately for every field.

  This PR moves all logic around these fields (collectively called field verification) to implementations in field_impl.h, which dispatch to renamed functions in field_*_impl.h for the actual implementation.

  Fixes #1060.

ACKs for top commit:
  jonasnick:
    ACK 7fc642f
  real-or-random:
    ACK 7fc642f

Tree-SHA512: 0f94e13fedc47e47859261a182c4077308f8910495691f7e4d7877d9298385172c70e98b4a1e270b6bde4d0062b932607106306bdb35a519cdeab9695a5c71e4
@sipa sipa mentioned this pull request May 23, 2023
Open
5 tasks
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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sipa @real-or-random