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

Validate proof with ECVRF_decode_proof in vrfEd25519r2ishiguro.ProofToHash() #493

Merged

Conversation

Mdaiki0730
Copy link
Member

@Mdaiki0730 Mdaiki0730 commented Nov 10, 2022

Description

The external function ECVRF_proof2hash() only takes the slice from position 1 to 32 as the hash, but does not check the input vrf proof is valid or not. This implementation violates the ongoing standardization described in section 5.2 of the VRF documentation.
I think it's better to verify the vrf proof before calling r2ishiguro.ECVRF_proof2hash(proof) to keep up with the ongoing standardization.

PR Check List

  • Is the current test pattern sufficient?
  • Is the ProofToHash error pattern sufficient?

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #493 (d1f3bbf) into main (bf07442) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #493      +/-   ##
==========================================
+ Coverage   65.42%   65.47%   +0.05%     
==========================================
  Files         278      278              
  Lines       37923    37928       +5     
==========================================
+ Hits        24811    24835      +24     
+ Misses      11300    11283      -17     
+ Partials     1812     1810       -2     
Impacted Files Coverage Δ
crypto/vrf/vrf_r2ishiguro.go 100.00% <100.00%> (ø)
blockchain/v0/pool.go 77.77% <0.00%> (-1.30%) ⬇️
consensus/state.go 73.38% <0.00%> (-0.06%) ⬇️
light/client.go 61.61% <0.00%> (ø)
statesync/snapshots.go 93.71% <0.00%> (ø)
p2p/pex/addrbook.go 71.66% <0.00%> (+0.50%) ⬆️
consensus/reactor.go 75.63% <0.00%> (+0.61%) ⬆️
abci/client/socket_client.go 72.96% <0.00%> (+0.87%) ⬆️
mempool/reactor.go 79.67% <0.00%> (+1.09%) ⬆️
privval/signer_listener_endpoint.go 91.26% <0.00%> (+2.38%) ⬆️
... and 2 more

@Mdaiki0730 Mdaiki0730 self-assigned this Nov 10, 2022
@Mdaiki0730 Mdaiki0730 added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Nov 10, 2022
@Mdaiki0730 Mdaiki0730 marked this pull request as ready for review November 10, 2022 05:52
tnasu
tnasu previously approved these changes Nov 10, 2022
crypto/vrf/vrf_r2ishiguro_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@torao torao left a comment

Choose a reason for hiding this comment

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

LGTM

@Mdaiki0730 Mdaiki0730 merged commit 481dd31 into Finschia:main Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants