Skip to content

add signatures and keys to signed header along with signature verification in validate method#788

Merged
nashqueue merged 8 commits intoevstack:mainfrom
gupadhyaya:add_sigs_keys_to_header
Mar 24, 2023
Merged

add signatures and keys to signed header along with signature verification in validate method#788
nashqueue merged 8 commits intoevstack:mainfrom
gupadhyaya:add_sigs_keys_to_header

Conversation

@gupadhyaya
Copy link
Copy Markdown
Contributor

@gupadhyaya gupadhyaya commented Mar 16, 2023

Fixes #782 #783 #784

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 16, 2023

Codecov Report

Patch coverage: 58.98% and project coverage change: -0.62 ⚠️

Comparison is base (f7d95c2) 55.27% compared to head (e602b73) 54.65%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #788      +/-   ##
==========================================
- Coverage   55.27%   54.65%   -0.62%     
==========================================
  Files          53       54       +1     
  Lines       10000    10225     +225     
==========================================
+ Hits         5527     5588      +61     
- Misses       3652     3809     +157     
- Partials      821      828       +7
Impacted Files Coverage Δ
node/light.go 0.00% <0.00%> (ø)
types/validation.go 30.90% <25.00%> (-20.26%) ⬇️
types/pb/rollkit/rollkit.pb.go 39.30% <38.53%> (-1.09%) ⬇️
types/signed_header.go 52.38% <52.38%> (ø)
state/executor.go 65.62% <86.84%> (+0.16%) ⬆️
types/serialization.go 76.65% <90.00%> (-0.53%) ⬇️
block/manager.go 64.82% <94.66%> (+0.42%) ⬆️
node/header_exchange.go 51.96% <95.00%> (+0.47%) ⬆️
config/config.go 90.32% <100.00%> (+0.66%) ⬆️
conv/abci/block.go 94.33% <100.00%> (ø)
... and 5 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gupadhyaya
Copy link
Copy Markdown
Contributor Author

gupadhyaya commented Mar 16, 2023

Please review this PR after #786 is merged, as this PR is built on top of it. Few clarifications:

  • signer keys are not added to Header, as they don't need to be signed. You can only verify the signatures using the right signer keys. note that, signatures are anyways not signed, so it does not make sense why the signer keys needs to be signed.
  • keeping the structs Commit and ValidatorSet, although they can be simplified to [][]byte, holding signatures and keys. the separate structs will allow for better future enhancements such as adding voting powers to validators, priorities, etc.

@gupadhyaya gupadhyaya force-pushed the add_sigs_keys_to_header branch from 0b93d9f to d580472 Compare March 16, 2023 16:45
gupadhyaya
gupadhyaya commented Mar 16, 2023
View reviewed changes
Comment thread block/manager.go
@gupadhyaya
Copy link
Copy Markdown
Contributor Author

gupadhyaya commented Mar 16, 2023

@nashqueue @tuxcanfly @S1nus @Manav-Aggarwal this PR is ready for your review.

nashqueue
nashqueue reviewed Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

Great work!

Comment thread block/manager.go
Comment thread proto/rollkit/rollkit.proto
Comment thread types/serialization_test.go Outdated
Comment thread types/validation.go
@gupadhyaya gupadhyaya force-pushed the add_sigs_keys_to_header branch 2 times, most recently from 704b2c3 to 8b23b7f Compare March 22, 2023 20:16
@gupadhyaya gupadhyaya requested a review from nashqueue March 22, 2023 20:16
@nashqueue nashqueue mentioned this pull request Mar 23, 2023
Closed
nashqueue
nashqueue previously approved these changes Mar 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

Good to go, created follow-up issues #797 #798 #796

tuxcanfly
tuxcanfly previously approved these changes Mar 23, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

OK.

@gupadhyaya gupadhyaya dismissed stale reviews from tuxcanfly and nashqueue via ab7ff8b March 23, 2023 16:01
@gupadhyaya gupadhyaya force-pushed the add_sigs_keys_to_header branch from 8b23b7f to ab7ff8b Compare March 23, 2023 16:01
@gupadhyaya gupadhyaya requested a review from tuxcanfly March 23, 2023 18:33
@gupadhyaya gupadhyaya requested a review from nashqueue March 23, 2023 18:33
tuxcanfly
tuxcanfly approved these changes Mar 23, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

OK.

nashqueue
nashqueue approved these changes Mar 24, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

LGTM

@nashqueue nashqueue merged commit 770a9bd into evstack:main Mar 24, 2023
nashqueue pushed a commit that referenced this pull request Apr 10, 2023
@gupadhyaya
add signatures and keys to signed header along with signature verific…
8d4023f 
…ation in validate method (#788)

Fixes #782 #783 #784

---------

Co-authored-by: Ganesha Upadhyaya <gupadhyaya@Ganeshas-MacBook-Pro-2.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tuxcanfly tuxcanfly tuxcanfly approved these changes

@tzdybal tzdybal Awaiting requested review from tzdybal

@Manav-Aggarwal Manav-Aggarwal Awaiting requested review from Manav-Aggarwal

@S1nus S1nus Awaiting requested review from S1nus

+1 more reviewer

@nashqueue nashqueue nashqueue approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gupadhyaya gupadhyaya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use SignedHeader.ValidateBasic in verifications to verify the signature of the Aggregator

4 participants

@gupadhyaya @codecov-commenter @tuxcanfly @nashqueue