Skip to content

Conversation

@webmaster128
Copy link
Member

of secp256k1_verify/secp256k1_recover_pubkey

let message_hash = Sha256::digest(message);
assert_eq!(hash.as_slice(), message_hash.as_slice());

// Since the recovery param is mossing in the test vectors, we try both 0 and 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Since the recovery param is mossing in the test vectors, we try both 0 and 1
// Since the recovery param is missing in the test vectors, we try both 0 and 1

I understand that in a real scenario you need to know the recovery param. Otherwise, you can get a recovered pubkey that is not the right one.

I also understand that this is not related to the high/low-S comments, but another issue / improvement.

Will read more about all this and comment back if needed.

Copy link
Contributor

@maurolacy maurolacy Feb 25, 2023

Choose a reason for hiding this comment

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

I understand that in a real scenario you need to know the recovery param. Otherwise, you can get a recovered pubkey that is not the right one.

Will read more about all this and comment back if needed.

Here are some references about the recovery param:

So, it seems that there's a small probability of getting two valid pub keys for different recovery params.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also understand that this is not related to the high/low-S comments, but another issue / improvement.

Exactly. The only reason they are mixed together here is because I pull in the COSMOS_SECP256K1_TESTS_JSON tests which are missing the recovery param.

@webmaster128
Copy link
Member Author

Thanks a lot for the thorough review, @maurolacy!

@webmaster128 webmaster128 merged commit cbf0321 into main Mar 8, 2023
@webmaster128 webmaster128 deleted the low-high-s-comments branch March 8, 2023 12:33
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.

2 participants