Add API docs for secp256k1_instruction and secp256k1_recover #26065
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brson
force-pushed
the
docs-secp256k1
branch
9 times, most recently
from
014ce2f to
e7bd798
Compare
brson
force-pushed
the
docs-secp256k1
branch
2 times, most recently
from
59feee3 to
1d33e59
Compare
|
@seanyoung Can you take a look at these changes? |
Absolutely, I started looking this morning. I haven't read all the linked documentation yet. |
seanyoung
previously requested changes
Jul 7, 2022
brson
force-pushed
the
docs-secp256k1
branch
2 times, most recently
from
abc54ba to
51b03d8
Compare
|
This PR gets merge conflicts frequently. Could use another round of review to get this landed. |
CriesofCarrots
force-pushed
the
docs-secp256k1
branch
from
f175667 to
c793f54
Compare
|
LGTM |
mergify bot
pushed a commit
that referenced
this pull request
Aug 8, 2022
* Add API docs for secp256k1_instruction and secp256k1_recover * typo * Remove unused variable from secp256k1 program test * Bump solana_bpf_rust_secp256k1_recover ix count Co-authored-by: Tyera Eulberg <[email protected]> (cherry picked from commit ebe25fd) # Conflicts: # programs/bpf/Cargo.lock # programs/bpf/rust/secp256k1_recover/Cargo.toml # programs/bpf/tests/programs.rs
mergify bot
added a commit
that referenced
this pull request
Aug 9, 2022
…#26065) (#27007) * Add API docs for secp256k1_instruction and secp256k1_recover (#26065) * Add API docs for secp256k1_instruction and secp256k1_recover * typo * Remove unused variable from secp256k1 program test * Bump solana_bpf_rust_secp256k1_recover ix count Co-authored-by: Tyera Eulberg <[email protected]> (cherry picked from commit ebe25fd) # Conflicts: # programs/bpf/Cargo.lock # programs/bpf/rust/secp256k1_recover/Cargo.toml # programs/bpf/tests/programs.rs * Fix conflicts Co-authored-by: Brian Anderson <[email protected]> Co-authored-by: Tyera Eulberg <[email protected]>
xiangzhu70
pushed a commit
to xiangzhu70/solana
that referenced
this pull request
Aug 17, 2022
…labs#26065) * Add API docs for secp256k1_instruction and secp256k1_recover * typo * Remove unused variable from secp256k1 program test * Bump solana_bpf_rust_secp256k1_recover ix count Co-authored-by: Tyera Eulberg <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The secp256k1 facilities are underdocumented.
Summary of Changes
This adds fairly comprehensive API documentation to the secp256k1_instruction and secp256k1_recover modules, with examples containing copyable code snippets.
These APIs are difficult to use and warrant the addition of more helper functions, but I have not tried to address that here.
A few things to note:
There is one sentence here I am unsure about, regarding secp256k1_recover:
Recovery IDs are two bits. From reading the code of both Parity's libsecp256k1 and Bitcoin's secp256k1 I gather the second bit is set to "1" if the signature is "overflowing". I also see that Solana parses signatures with
Signature::parse_standardwhich rejects overflowing signatures. I also see that Ethereum always rejects signatures with the second bit set to "1". I do not know what it means to overflow here, nor if both these uses of "overflowing" are the same, and have been unable to find any documentation about this matter, and the math in these libraries is beyond my understanding. It would be desirable for a cryptographer to review this and explain.Secondly, I was surprised to discover that these APIs allow for signature malleability - Bitcoin's library disallows it by default, but Parity's does not. I have added a lot of text and links about this with code snippets explaining how to guard against it. I added test cases confirming malleability in both the secp256k1 program and secp256k1_recover.
This mostly addresses #23617, though it does not add any new APIs as proposed in that issue.
cc @tlambertz if you could have a look at this I would be grateful, re secp256k1 APIs