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

refactor(header): Validate should check that the raw header's app version is not greater than latest app version #3589

Merged
merged 11 commits into from
Aug 8, 2024

Conversation

renaynay
Copy link
Member

In preparation for V2, we should change the header validation logic to ensure that we support validation for V2 and below.

@renaynay renaynay added area:header Extended header area:core_and_app Relationship with Core node and Celestia-App kind:chore labels Jul 23, 2024
@renaynay renaynay self-assigned this Jul 23, 2024
@renaynay renaynay requested a review from cmwaters July 23, 2024 13:59
Copy link
Member Author

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

feel like we should also ensure app version is not 0 since that's not valid

@renaynay renaynay added the v0.16.0 Intended for v0.16.0 release label Jul 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.51%. Comparing base (2469e7a) to head (09bf68d).
Report is 156 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3589      +/-   ##
==========================================
+ Coverage   44.83%   45.51%   +0.68%     
==========================================
  Files         265      281      +16     
  Lines       14620    16053    +1433     
==========================================
+ Hits         6555     7307     +752     
- Misses       7313     7901     +588     
- Partials      752      845      +93     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

header/header.go Outdated Show resolved Hide resolved
@renaynay renaynay requested a review from cmwaters July 24, 2024 13:05
vgonkivs
vgonkivs previously approved these changes Jul 24, 2024
cristaloleg
cristaloleg previously approved these changes Jul 25, 2024
@renaynay renaynay dismissed stale reviews from cristaloleg and vgonkivs via 80e20f3 July 29, 2024 13:24
cristaloleg
cristaloleg previously approved these changes Jul 29, 2024
Wondertan
Wondertan previously approved these changes Jul 30, 2024
walldiss
walldiss previously approved these changes Jul 30, 2024
rootulp added a commit to rootulp/celestia-node that referenced this pull request Aug 1, 2024
renaynay and others added 3 commits August 8, 2024 09:38
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
@renaynay renaynay dismissed stale reviews from walldiss, Wondertan, and cristaloleg via 38ec710 August 8, 2024 07:38
@renaynay renaynay force-pushed the app-version-check branch from 80e20f3 to 38ec710 Compare August 8, 2024 07:38
header/header.go Outdated Show resolved Hide resolved
cristaloleg
cristaloleg previously approved these changes Aug 8, 2024
vgonkivs
vgonkivs previously approved these changes Aug 8, 2024
@renaynay renaynay enabled auto-merge (squash) August 8, 2024 09:49
@rootulp rootulp dismissed stale reviews from vgonkivs and cristaloleg via 0fee5be August 8, 2024 14:31
@rootulp
Copy link
Contributor

rootulp commented Aug 8, 2024

Fraud integration tests failed on CI but pass locally so I'm assuming it's a flake:

$ make test-integration TAGS=fraud
--> Running integrations tests  -tags=fraud
ok  	github.com/celestiaorg/celestia-node/nodebuilder/tests	64.643s

@rootulp
Copy link
Contributor

rootulp commented Aug 8, 2024

wow fully green CI 🟢

Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

Nice catch with v2!

@renaynay renaynay merged commit 6032885 into celestiaorg:main Aug 8, 2024
21 of 25 checks passed
@Wondertan Wondertan deleted the app-version-check branch August 8, 2024 18:56
sebasti810 pushed a commit to sebasti810/celestia-node that referenced this pull request Aug 14, 2024
…ersion is not greater than latest app version (celestiaorg#3589)

Co-authored-by: Callum Waters <cmwaters19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core_and_app Relationship with Core node and Celestia-App area:header Extended header kind:chore v0.16.0 Intended for v0.16.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants