Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

Verify signature with compressed public key#5542

Merged
gumb0 merged 3 commits intomasterfrom
verify-compressed-key
Apr 8, 2019
Merged

Verify signature with compressed public key#5542
gumb0 merged 3 commits intomasterfrom
verify-compressed-key

Conversation

@gumb0
Copy link
Copy Markdown
Member

@gumb0 gumb0 commented Apr 2, 2019

We'll need to verify ENR signature having only compressed public key (33 bytes) and [r, s] part of the signature (64 bytes)

For reference go-ethereum code doing the same: https://github.com/ethereum/go-ethereum/blob/c4109d790ffd26d67feb7745d4af8a8bc5090bd9/crypto/secp256k1/ext.h#L46-L63

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 2, 2019

Codecov Report

Merging #5542 into master will increase coverage by 0.04%.
The diff coverage is 95.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5542      +/-   ##
==========================================
+ Coverage    61.9%   61.95%   +0.04%     
==========================================
  Files         344      344              
  Lines       28757    28776      +19     
  Branches     3267     3269       +2     
==========================================
+ Hits        17802    17827      +25     
+ Misses       9784     9781       -3     
+ Partials     1171     1168       -3

@gumb0 gumb0 requested review from chfast and halfalicious April 2, 2019 12:12
@gumb0 gumb0 mentioned this pull request Apr 4, 2019
bool verify(Public const& _k, Signature const& _s, h256 const& _hash);

// Verify signature with compressed public key
bool verify(PublicCompressed const& _key, h512 const& _signature, h256 const& _hash);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we name the args similar to the other verify function i.e. _k/_s?


BOOST_AUTO_TEST_CASE(verifyWithPublicCompressed)
{
PublicCompressed pub{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Presumably you encrypted the hash into the hex value you're using as the signature value outside of the test case...can we include the encryption as part of the test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's what signAndVerify test does


BOOST_AUTO_TEST_CASE(signAndVerify)
{
KeyPair kp = KeyPair::create();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Nit) can be auto

BOOST_AUTO_TEST_CASE(signAndVerify)
{
KeyPair kp = KeyPair::create();
auto msg = h256::random();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Nit) Can msg and sig be const?

auto msg = h256::random();
auto sig = sign(kp.secret(), msg);

auto pubCompressed = toPublicCompressed(kp.secret());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Nit) can pubCompressed be const?

Copy link
Copy Markdown
Contributor

@halfalicious halfalicious left a comment

Choose a reason for hiding this comment

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

Take my approval with a grain of salt since I'm not very familiar with encryption 😄 Nothing jumped out at me as obviously wrong though

@gumb0 gumb0 merged commit a611afe into master Apr 8, 2019
@gumb0 gumb0 deleted the verify-compressed-key branch April 8, 2019 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants