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

Add validation of the ValidatorsHash, Round and Proof #510

Merged
merged 15 commits into from
Nov 30, 2022

Conversation

Mdaiki0730
Copy link
Member

@Mdaiki0730 Mdaiki0730 commented Nov 25, 2022

Description

Since the Header contains the ValidatorHash, Round, and Proof, the Header.ValidateBasic() should perform the stateless validation of the ValidatorsHash, Round and Proof as well.

We also added the below for libsodium support on M1 macs.

#cgo darwin,arm64 CFLAGS: -I./sodium/darwin_arm64/include/
#cgo darwin,arm64 LDFLAGS: -L./sodium/darwin_arm64/lib -lsodium

PR Check List

  • This implementation does not return an error if Proof is nil. Should we treat this as an error?
  • I changed the build tag of e2e test from libsodium to r2ishiguro.Is there any problem?

@Mdaiki0730 Mdaiki0730 added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Nov 25, 2022
@Mdaiki0730 Mdaiki0730 self-assigned this Nov 25, 2022
@Mdaiki0730 Mdaiki0730 force-pushed the fix/validatebasic-add-validation branch from 4e6f34d to 7c99da2 Compare November 25, 2022 10:09
@Mdaiki0730 Mdaiki0730 force-pushed the fix/validatebasic-add-validation branch from 7c99da2 to b322fdf Compare November 25, 2022 10:37
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #510 (14d66a5) into main (f92e500) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
+ Coverage   65.60%   65.68%   +0.07%     
==========================================
  Files         279      279              
  Lines       37988    38032      +44     
==========================================
+ Hits        24923    24981      +58     
+ Misses      11268    11255      -13     
+ Partials     1797     1796       -1     
Impacted Files Coverage Δ
crypto/vrf/vrf_r2ishiguro.go 100.00% <ø> (ø)
types/block.go 77.10% <100.00%> (+0.22%) ⬆️
types/validation.go 64.00% <100.00%> (+16.94%) ⬆️
crypto/sr25519/pubkey.go 35.89% <0.00%> (-7.70%) ⬇️
blockchain/v0/pool.go 78.81% <0.00%> (-1.04%) ⬇️
consensus/state.go 72.92% <0.00%> (-0.41%) ⬇️
mempool/clist_mempool.go 82.16% <0.00%> (-0.25%) ⬇️
mempool/mempool.go 100.00% <0.00%> (ø)
state/tx_filter.go 100.00% <0.00%> (ø)
rpc/core/mempool.go 0.00% <0.00%> (ø)
... and 10 more

@Mdaiki0730 Mdaiki0730 marked this pull request as ready for review November 25, 2022 11:02
crypto/vrf/vrf.go Outdated Show resolved Hide resolved
crypto/vrf/vrf_libsodium.go Outdated Show resolved Hide resolved
@Mdaiki0730 Mdaiki0730 force-pushed the fix/validatebasic-add-validation branch from 1f82b3a to e4cea2b Compare November 28, 2022 03:25
@Mdaiki0730 Mdaiki0730 force-pushed the fix/validatebasic-add-validation branch from e4cea2b to 7730830 Compare November 28, 2022 03:27
@Mdaiki0730 Mdaiki0730 requested a review from torao November 28, 2022 06:26
torao
torao previously approved these changes Nov 28, 2022
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

@tnasu
Copy link
Member

tnasu commented Nov 28, 2022

I changed the build tag of e2e test from libsodium to r2ishiguro.Is there any problem?

Why did you do that? I think it should do the e2e-test with libsodium.

crypto/vrf/README.md Show resolved Hide resolved
crypto/vrf/vrf_libsodium.go Show resolved Hide resolved
@Mdaiki0730 Mdaiki0730 requested review from ulbqb and torao November 29, 2022 07:35
ulbqb
ulbqb previously approved these changes Nov 29, 2022
Copy link
Member

@tnasu tnasu left a comment

Choose a reason for hiding this comment

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

Would you add the below in the description why do you modify it?

#cgo darwin,arm64 CFLAGS: -I./sodium/darwin_arm64/include/
#cgo darwin,arm64 LDFLAGS: -L./sodium/darwin_arm64/lib -lsodium

@Mdaiki0730
Copy link
Member Author

I got it. Thank you for your advice.

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.

4 participants