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

tendermint: use k256::ecdsa::VerifyingKey as Secp256k1 key #900

Merged

Conversation

tony-iqlusion
Copy link
Collaborator

See #873 for background.

This changes the type re-exported as tendermint::public_key::Secp256k1 from a k256::EncodedPoint to a k256::ecdsa::VerifyingKey.

The main distinction this provides is validating the public key (i.e. making sure it provides a valid solution to the secp256k1 curve equation), whereas EncodedPoint provides no validation of the public key.

If there were ever a Signature::Secp256k1 variant added, this would also make it easy to perform signature verification.

@tony-iqlusion
Copy link
Collaborator Author

Hmm, these build failures seem unrelated?

   Compiling flume v0.10.6
error[E0658]: or-patterns syntax is experimental
Error:   --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/flume-0.10.6/src/lib.rs:90:14
   |
90 |         let (Self::Full(msg) | Self::Disconnected(msg)) = self;
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information

error[E0658]: or-patterns syntax is experimental
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/flume-0.10.6/src/lib.rs:128:14
    |
128 |         let (Self::Timeout(msg) | Self::Disconnected(msg)) = self;
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information

@tarcieri tarcieri force-pushed the tony-iqlusion/use-k256-verifying-key-as-public-key branch from a3a2bed to 26d4a17 Compare June 10, 2021 16:38
@codecov-commenter
Copy link

Codecov Report

Merging #900 (26d4a17) into master (f0b307c) will increase coverage by 0.0%.
The diff coverage is 60.0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #900   +/-   ##
======================================
  Coverage    69.7%   69.8%           
======================================
  Files         200     200           
  Lines       16326   16326           
======================================
+ Hits        11391   11398    +7     
+ Misses       4935    4928    -7     
Impacted Files Coverage Δ
tendermint/src/validator.rs 70.0% <0.0%> (ø)
tendermint/src/public_key.rs 67.5% <58.3%> (+7.5%) ⬆️
tendermint/src/account.rs 90.4% <100.0%> (ø)
rpc/src/endpoint/blockchain.rs 40.0% <0.0%> (-60.0%) ⬇️
rpc/src/endpoint/validators.rs 20.0% <0.0%> (-30.0%) ⬇️
tendermint/src/abci/tag.rs 74.0% <0.0%> (-11.2%) ⬇️
tendermint/src/config.rs 60.8% <0.0%> (-0.9%) ⬇️
testgen/src/vote.rs 83.7% <0.0%> (-0.9%) ⬇️
tendermint/tests/config.rs 97.2% <0.0%> (-0.7%) ⬇️
testgen/src/header.rs 78.1% <0.0%> (-0.7%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b307c...26d4a17. Read the comment docs.

@tony-iqlusion
Copy link
Collaborator Author

Opened #901 to deal with the flume-related issue

See #873 for background.

This changes the type re-exported as `tendermint::public_key::Secp256k1`
from a `k256::EncodedPoint` to a `k256::ecdsa::VerifyingKey`.

The main distinction this provides is validating the public key (i.e.
making sure it provides a valid solution to the secp256k1 curve
equation), whereas `EncodedPoint` provides no validation of the public
key.

If there were ever a `Signature::Secp256k1` variant added, this would
also make it easy to perform signature verification.
@tarcieri tarcieri force-pushed the tony-iqlusion/use-k256-verifying-key-as-public-key branch from 26d4a17 to 501204c Compare June 10, 2021 17:29
@thanethomson
Copy link
Contributor

@tarcieri is it worth perhaps adding a .changelog entry for this? Seems like an enhancement we'd want people to know about?

@tony-iqlusion
Copy link
Collaborator Author

@thanethomson added in 768019d

Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Thanks @tony-iqlusion! 🙏

@thanethomson thanethomson merged commit 865fef4 into master Jun 11, 2021
@thanethomson thanethomson deleted the tony-iqlusion/use-k256-verifying-key-as-public-key branch June 11, 2021 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants