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

chore!: upgrade to celestia-app v0.7.0 #1147

Merged
merged 24 commits into from
Oct 14, 2022

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Sep 21, 2022

Closes #1142
Opens #1231

@rootulp rootulp added area:core_and_app Relationship with Core node and Celestia-App kind:deps Pull requests that update a dependency file T:dependencies labels Sep 21, 2022
@rootulp rootulp self-assigned this Sep 21, 2022
header/pb/extended_header.proto Show resolved Hide resolved
header/store/heightsub.go Outdated Show resolved Hide resolved
ipld/plugin/nmt.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Merging #1147 (44c8d92) into main (793e2b0) will increase coverage by 0.26%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
+ Coverage   55.55%   55.82%   +0.26%     
==========================================
  Files         160      160              
  Lines        9853     9862       +9     
==========================================
+ Hits         5474     5505      +31     
+ Misses       3841     3825      -16     
+ Partials      538      532       -6     
Impacted Files Coverage Δ
header/header.go 50.00% <0.00%> (ø)
header/pb/extended_header.pb.go 36.50% <ø> (ø)
header/serde.go 27.27% <ø> (ø)
header/testing.go 89.56% <ø> (ø)
service/rpc/share.go 0.00% <0.00%> (ø)
share/add.go 77.35% <ø> (ø)
share/availability/cache/availability.go 87.50% <ø> (ø)
share/availability/test/testing.go 67.39% <ø> (ø)
share/byzantine.go 69.23% <ø> (ø)
share/eds/retriever.go 91.97% <ø> (ø)
... and 14 more

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

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 22, 2022

Integ test failed CI. Will debug tomorrow

@Wondertan
Copy link
Member

@rootulp, is it ready for review?

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 22, 2022

@Wondertan nope, sorry I haven't investigated why integ test is failing yet.

@Wondertan
Copy link
Member

Note that we have one flakey integration test that may be annoying for you to debug

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 22, 2022

Noted. I just started a third retry

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 22, 2022

Notes from investigation:

  • This log is suspect: starting Bridge Node: square size is not a power of two: 0 (link). The error appears to originate from celestia-app shares.Split even though the block size in this test is 16 per this line.
  • It seems plausible that the block data used in this test (link) isn't splittable by celestia-app
  • I verified that squareSize is indeed the issue at least for the TestFullReconstructFromBridge integ test. Hacky workaround was to add b.Data.OriginalSquareSize = 16 immediately before this line which makes the test pass.

Synchronous debugging with @evan-forbes

@Wondertan
Copy link
Member

Thanks, @rootulp. Mockable App is something we have wanted for a while already, and it just has never been a priority. From your message, it seems like it is blocking the update now, right?

@rootulp
Copy link
Collaborator Author

rootulp commented Sep 23, 2022

Yep blocking. @evan-forbes is working on a slightly different approach to unblock this PR.

header/store/heightsub.go Outdated Show resolved Hide resolved
service/rpc/share.go Outdated Show resolved Hide resolved
@rootulp
Copy link
Collaborator Author

rootulp commented Sep 29, 2022

Blocked on celestiaorg/celestia-app#770

@Bidon15 Bidon15 mentioned this pull request Oct 5, 2022
4 tasks
@rootulp
Copy link
Collaborator Author

rootulp commented Oct 7, 2022

Update: blocked on #628 and #1160

@Wondertan
Copy link
Member

@rootulp, the #1160 is now merged, so you freely continue here.

Ping if anything is needed from our side

go.mod Outdated Show resolved Hide resolved
@rootulp rootulp changed the title chore: upgrade to celestia-app v0.7.0-rc3 chore: upgrade to celestia-app v0.7.0-rc5 Oct 11, 2022
@rootulp rootulp marked this pull request as ready for review October 13, 2022 04:41
go.mod Outdated Show resolved Hide resolved
nodebuilder/tests/swamp/swamp.go Outdated Show resolved Hide resolved
header/store/heightsub.go Outdated Show resolved Hide resolved
header/store/heightsub.go Outdated Show resolved Hide resolved
nodebuilder/tests/swamp/swamp.go Outdated Show resolved Hide resolved
nodebuilder/tests/swamp/swamp.go Outdated Show resolved Hide resolved
nodebuilder/tests/swamp/swamp_tx.go Outdated Show resolved Hide resolved
share/availability/light/availability_test.go Outdated Show resolved Hide resolved
share/availability/test/testing.go Show resolved Hide resolved
share/availability/light/availability_test.go Show resolved Hide resolved
share/availability/light/availability_test.go Show resolved Hide resolved
header/header.go Show resolved Hide resolved
@rootulp
Copy link
Collaborator Author

rootulp commented Oct 13, 2022

Another instance of #1090 encountered here

rootulp and others added 3 commits October 13, 2022 12:14
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@rootulp rootulp changed the title chore: upgrade to celestia-app v0.7.0-rc5 chore: upgrade to celestia-app v0.7.0 Oct 13, 2022
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the help ❤️

Left two non-blocking optional nits

nodebuilder/tests/swamp/config.go Show resolved Hide resolved
nodebuilder/tests/swamp/swamp.go Show resolved Hide resolved
@evan-forbes evan-forbes merged commit 738b69f into celestiaorg:main Oct 14, 2022
@rootulp rootulp deleted the rp/upgrade-celestia-app branch October 14, 2022 11:26
@rootulp rootulp changed the title chore: upgrade to celestia-app v0.7.0 chore!: upgrade to celestia-app v0.7.0 Oct 14, 2022
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:deps Pull requests that update a dependency file T:dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to celestia-app v0.7.0-rc3
6 participants