Skip to content

Added signed header verification - #1026

Merged
Manav-Aggarwal merged 19 commits into
mainfrom
manav/add_header_verification
Jul 3, 2023
Merged

Added signed header verification#1026
Manav-Aggarwal merged 19 commits into
mainfrom
manav/add_header_verification

Conversation

@Manav-Aggarwal

@Manav-AggarwalManav-Aggarwal commented Jun 21, 2023

Copy link
Copy Markdown
Member

Overview

Resolves: #1027, Resolves: #1048

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@Manav-AggarwalManav-Aggarwal changed the title add header verificationAdded signed header verificationJun 21, 2023
@codecov

codecovBot commented Jun 21, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 72.82% and project coverage change: +1.38 🎉

Comparison is base (4c951e4) 54.48% compared to head (c7e9725) 55.86%.

Additional details and impacted files
@@ Coverage Diff @@## main #1026 +/- ##
==========================================
+ Coverage 54.48% 55.86% +1.38% 
==========================================
Files 63 63 Lines 6655 6759 +104 ==========================================
+ Hits 3626 3776 +150 + Misses 2638 2573 -65 - Partials 391 410 +19 
Impacted FilesCoverage Δ
types/signed_header.go44.06% <46.80%> (+44.06%)⬆️
types/block.go42.42% <68.29%> (+42.42%)⬆️
types/test_utils.go84.21% <84.21%> (ø)
da/test/da_test_helpers.go78.18% <100.00%> (-2.47%)⬇️
node/full_client.go46.51% <100.00%> (ø)
state/executor.go50.00% <100.00%> (-0.80%)⬇️
store/test_helpers.go100.00% <100.00%> (ø)
types/header.go32.00% <100.00%> (+32.00%)⬆️

... and 2 files with indirect coverage changes

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

@Manav-Aggarwal
Manav-Aggarwal marked this pull request as ready for review June 21, 2023 19:20
Comment threadtypes/signed_header.go Outdated
S1nus
S1nus previously approved these changes Jun 21, 2023
Comment threadtypes/signed_header.go Outdated
gupadhyaya
gupadhyaya previously approved these changes Jun 22, 2023
Comment threadtypes/signed_header.go Outdated
Comment threadtypes/signed_header.go Outdated
Comment threadtypes/signed_header.go Outdated
Comment threadtypes/signed_header.go Outdated
@nashqueue

Copy link
Copy Markdown
Contributor

Another thing that we should consider is that we have 2 options for verifying a header. Either if it is adjacent, we check it through the last hash etc. OR if it is in the future, we can assume it is correct because the Sequencer signed it and can be fraud proven later.

@Manav-Aggarwal

Copy link
Copy Markdown
MemberAuthor

Another thing that we should consider is that we have 2 options for verifying a header. Either if it is adjacent, we check it through the last hash etc. OR if it is in the future, we can assume it is correct because the Sequencer signed it and can be fraud proven later.

We should discuss this and track it in an issue.

nashqueue
nashqueue previously approved these changes Jun 23, 2023

@nashqueuenashqueue left a comment

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.

LGTM Last point in a non-blocking refactor and can be done in a follow up

@MSeveyMSevey left a comment

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.

some minor edits, but then will be good

Comment threadtypes/signed_header.go Outdated
Comment threadtypes/signed_header.go Outdated
Comment threadtypes/signed_header.go Outdated
Comment threadtypes/signed_header.go Outdated
@Manav-Aggarwal

Copy link
Copy Markdown
MemberAuthor

New and updated code has new and/or updated testing - this is false. Code is not covered by unit tests at all. I see that there is a subtask in #1031, but IMO creating unit tests should not be separated from implementation.

Added header verification tests as well.

Comment threadtypes/signed_header_test.go
Comment threadtypes/signed_header_test.go

@MSeveyMSevey left a comment

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.

LGTM, all my comments can be follow ups.

@nashqueuenashqueue left a comment

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.

Thank you! LGTM, You can resolve conversations when followups are created

@tzdybaltzdybal left a comment

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.

LGTM

@tzdybal
tzdybal enabled auto-merge July 3, 2023 18:57
@tzdybal
tzdybal added this pull request to the merge queueJul 3, 2023
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Jul 3, 2023
@Manav-Aggarwal
Manav-Aggarwal added this pull request to the merge queueJul 3, 2023
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Jul 3, 2023
@Manav-Aggarwal
Manav-Aggarwal added this pull request to the merge queueJul 3, 2023
Merged via the queue into main with commit dd5e371Jul 3, 2023
@Manav-Aggarwal
Manav-Aggarwal deleted the manav/add_header_verification branch July 3, 2023 21:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:headerT:testingRelated to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create Unit Tests for each step of Header Verification Add verification steps to gossiped signed header

6 participants

@Manav-Aggarwal@nashqueue@tzdybal@S1nus@gupadhyaya@MSevey