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

feat(header): Check app version in eh validation and return err on mismatch #2138

Merged
merged 5 commits into from
Jun 21, 2023

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Apr 27, 2023

As recommended by @cmwaters, we should ensure (as a rudimentary check) that the headers being passed to us from core match in app versions as upgraded app versions can indicate breaking changes.

We should error out if the app version does not match.

This PR is still draft until celestia-app has a version out where they export the AppVersion as a const that we can depend on.

Eventually we should support several versions via #2137 but for now, this is a good enough check.

TODO:

  • depend on const from app
  • test

@renaynay renaynay added area:core_and_app Relationship with Core node and Celestia-App kind:feat Attached to feature PRs labels Apr 27, 2023
@renaynay renaynay self-assigned this Apr 27, 2023
@renaynay renaynay force-pushed the app-check branch 2 times, most recently from b354158 to 8bd61c7 Compare June 2, 2023 14:59
@renaynay renaynay marked this pull request as ready for review June 15, 2023 14:26
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Yeah this works for now

Wondertan
Wondertan previously approved these changes Jun 15, 2023
distractedm1nd
distractedm1nd previously approved these changes Jun 16, 2023
@renaynay
Copy link
Member Author

Wait a sec

--- FAIL: TestBlobService_Get (0.01s)
    service_test.go:222:
        	Error Trace:	/Users/rene/go/src/github.com/renaynay/celestia-node/blob/service_test.go:222
        	            				/Users/rene/go/src/github.com/renaynay/celestia-node/blob/service_test.go:295
        	Error:      	Received unexpected error:
        	            	share data must be 512 bytes, got 0
        	Test:       	TestBlobService_Get
    --- FAIL: TestBlobService_Get/not_included (0.00s)
        testing.go:1471: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
FAIL

@renaynay
Copy link
Member Author

--- FAIL: TestListenerWithNonEmptyBlocks (60.02s)
    listener_test.go:105:
        	Error Trace:	/Users/rene/go/src/github.com/renaynay/celestia-node/core/listener_test.go:105
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestListenerWithNonEmptyBlocks
    listener_test.go:181:
        	Error Trace:	/Users/rene/go/src/github.com/renaynay/celestia-node/core/listener_test.go:181
        	            				/opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1150
        	            				/opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1328
        	            				/opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1570
        	            				/opt/homebrew/Cellar/go/1.20.3/libexec/src/runtime/panic.go:522
        	            				/opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:980
        	            				/Users/rene/go/src/github.com/renaynay/celestia-node/core/listener_test.go:105
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestListenerWithNonEmptyBlocks
FAIL

will investigate

@renaynay renaynay marked this pull request as draft June 16, 2023 10:37
@renaynay
Copy link
Member Author

waiting on @cmwaters to update testnode in core to set app version

@renaynay renaynay dismissed stale reviews from distractedm1nd and Wondertan via 7d1aa38 June 16, 2023 13:40
@renaynay renaynay marked this pull request as ready for review June 16, 2023 13:40
@Wondertan
Copy link
Member

It's also a dep update PR

@renaynay renaynay enabled auto-merge June 20, 2023 13:06
@renaynay renaynay added this pull request to the merge queue Jun 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 20, 2023
@Wondertan Wondertan added this pull request to the merge queue Jun 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 20, 2023
@Wondertan Wondertan added this pull request to the merge queue Jun 20, 2023
@Wondertan Wondertan removed this pull request from the merge queue due to a manual request Jun 20, 2023
@Wondertan Wondertan enabled auto-merge June 21, 2023 15:24
@Wondertan Wondertan added this pull request to the merge queue Jun 21, 2023
Merged via the queue into celestiaorg:main with commit ce8ccb3 Jun 21, 2023
9 of 13 checks passed
@Wondertan Wondertan deleted the app-check branch June 21, 2023 15:35
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 kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants