Add a verify method which also takes in the uncompressed public key.#7
Add a verify method which also takes in the uncompressed public key.#7ValarDragon wants to merge 2 commits intotendermint:masterfrom
Conversation
|
|
||
| // https://tools.ietf.org/html/rfc8032#section-5.1.7 requires that s be in | ||
| // the range [0, order) in order to prevent signature malleability. | ||
| if !edwards25519.ScMinimal(&s) { |
There was a problem hiding this comment.
This also contains the diff for the upstream changes from x/crypto? I think we should merge this in a separate PR first and add the VerifyUncompressedKey here only (by rebasing / merging).
There was a problem hiding this comment.
You can keep the PR open. I'll merge #6 after making sure the diff exactly matches the changes on x/crypto. Then, you can rebase on the branch that contains the merge.
|
|
||
| // VerifyUncompressedKey returns true iff sig is a valid signature of message by publicKey. | ||
| // This takes in the uncompressed form of the public key (A) to avoid computing that internally. | ||
| func VerifyUncompressedKey(publicKey *[PublicKeySize]byte, A *edwards25519.ExtendedGroupElement, message []byte, sig *[SignatureSize]byte) bool { |
There was a problem hiding this comment.
If we introduce a new public method, we should have a test for that.
There was a problem hiding this comment.
How would such a test differ from the normal verify though? I would basically be replicating the code inside of Verify, and I'm not sure how useful that is as a test. (Uncompress the pubkeys using the same decompression method, and then run the same test vectors, which is exactly whats done by testing Verify)
There was a problem hiding this comment.
Tests also serve as a form of documentation here. One can look into the test and see the exact behaviour you just described above. Does that make sense to you?
That is pretty cool! 👍 We need to make sure that this 10% improvement, actually leads to a measurable improvement in a real deployment and hence is really worth exposing some edwards25519 compression interna (and later needing to change the keybase interface to provide the actual cached uncompressed public key. Would you like to do some perf measurements to compare how much of an improvement this would actually bring in tendermint/cosmos deployment? I would suggest not to merge before that. |
This is useful for the proof of stake use case, since we often validate multiple signatures from the same public key. Based off of the numbers in the official Ed25519 paper, the function with the uncompressed pubkey should save about 10% of the computation time.