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: bump celestia app v0.9.0 #1300

Merged
merged 13 commits into from
Nov 9, 2022

Conversation

evan-forbes
Copy link
Member

bumps to v0.9.0 of celestia-app

this also requires a bump to v0.11.0 of nmt, v0.7.0 of rsmt2d, and v1.4.0 of our fork of the sdk

most of the diff is updating to the new api introduced in rsmt2d and the wrapper

pls feel free to push or edit this branch to fix the remaining bugs

@evan-forbes evan-forbes marked this pull request as draft October 31, 2022 15:27
@walldiss
Copy link
Member

walldiss commented Nov 2, 2022

@evan-forbes Could you please grant me write access to this branch? I've found solution to one of the problems and want to commit it.

@walldiss walldiss force-pushed the evan/bump-celestia-app-v0.9.0 branch from 9fa7035 to 2e6d90b Compare November 3, 2022 16:16
@walldiss walldiss added the kind:deps Pull requests that update a dependency file label Nov 3, 2022
@walldiss walldiss force-pushed the evan/bump-celestia-app-v0.9.0 branch from 6d88a76 to 9cc03b3 Compare November 3, 2022 17:13
@walldiss walldiss force-pushed the evan/bump-celestia-app-v0.9.0 branch from 9cc03b3 to 960e7b0 Compare November 4, 2022 04:44
@walldiss
Copy link
Member

walldiss commented Nov 4, 2022

I've made all tests pass except few that was flaky because of unrelated to PR changes. CI test still could fail due to tests issues that will be fixed in #1314

The problems I've found through tests debugging were:

  • Before updateErrByzantineData returned from rsmt2d was bugged. It should had Shares field populated with pre-build shares, but had used rebuild ones instead. It got fixed in new version, which is nice! That introduced possibility to contain nil shares. We didn't handled nil shares case before, so I've added related checks.
  • Turned outnmt.Batch is not concurrency safe and there is even an open issue for it. The solution was to add external mutex to protect it from parallel operations.

share/eds/byzantine/share_proof.go Show resolved Hide resolved
share/ipld/nmt_adder.go Show resolved Hide resolved
share/ipld/nmt_adder.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1300 (960e7b0) into main (faf8b78) will decrease coverage by 0.57%.
The diff coverage is 36.34%.

@@            Coverage Diff             @@
##             main    #1300      +/-   ##
==========================================
- Coverage   55.81%   55.23%   -0.58%     
==========================================
  Files         170      176       +6     
  Lines       10254    10604     +350     
==========================================
+ Hits         5723     5857     +134     
- Misses       3962     4166     +204     
- Partials      569      581      +12     
Impacted Files Coverage Δ
das/daser.go 65.95% <ø> (ø)
nodebuilder/header/service.go 57.14% <ø> (ø)
nodebuilder/node.go 60.31% <ø> (ø)
nodebuilder/share/service.go 100.00% <ø> (ø)
nodebuilder/share/mocks/api.go 8.16% <8.16%> (ø)
nodebuilder/state/mocks/api.go 11.45% <11.45%> (ø)
nodebuilder/fraud/mocks/api.go 12.50% <12.50%> (ø)
nodebuilder/header/mocks/api.go 12.50% <12.50%> (ø)
nodebuilder/das/mocks/api.go 26.66% <26.66%> (ø)
header/header.go 56.25% <70.00%> (+6.25%) ⬆️
... and 23 more

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

@walldiss walldiss marked this pull request as ready for review November 8, 2022 11:51
Wondertan
Wondertan previously approved these changes Nov 8, 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

distractedm1nd
distractedm1nd previously approved these changes Nov 9, 2022
Copy link
Member

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

great finds with the nmt node adder lock and proof proto!

Copy link
Member

@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.

hopefully quick ones

nodebuilder/tests/fraud_test.go Show resolved Hide resolved
nodebuilder/tests/swamp/config.go Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Show resolved Hide resolved
@renaynay renaynay merged commit 72d8bc4 into celestiaorg:main Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:deps Pull requests that update a dependency file T:dependencies
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants