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.

Sign up for GitHub

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

Jump to bottom

chore!: upgrade to celestia-core v1.8.0-tm-v0.34.20 #1023

Merged
merged 4 commits into from
Nov 17, 2022
Merged

chore!: upgrade to celestia-core v1.8.0-tm-v0.34.20 #1023

merged 4 commits into from
Nov 17, 2022

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Nov 16, 2022 •
edited

Closes #1022

Does not rename all instances of message to blob see #995

@rootulp rootulp self-assigned this Nov 16, 2022
@rootulp rootulp changed the title chore: upgrade to celestia-core v1.6.0-tm-v0.34.20 chore: upgrade to celestia-core v1.6.0-tm-v0.34.20 Nov 16, 2022
@rootulp rootulp changed the title chore: upgrade to celestia-core v1.6.0-tm-v0.34.20 chore: upgrade to celestia-core v1.8.0-tm-v0.34.20 Nov 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022 •
edited

Codecov Report

Merging #1023 (3543e6d) into main (f67acce) will increase coverage by 24.00%.
The diff coverage is 58.62%.

@@             Coverage Diff             @@
##             main    #1023       +/-   ##
===========================================
+ Coverage   27.15%   51.16%   +24.00%     
===========================================
  Files          81       71       -10     
  Lines        9021     4372     -4649     
===========================================
- Hits         2450     2237      -213     
+ Misses       6335     1911     -4424     
+ Partials      236      224       -12
Impacted Files Coverage Δ
app/prepare_proposal.go 0.00% <0.00%> (ø)
app/process_proposal.go 0.00% <0.00%> (ø)
app/test_util.go 73.77% <0.00%> (ø)
pkg/shares/utils.go 48.14% <0.00%> (ø)
x/blob/keeper/keeper.go 91.30% <ø> (ø)
x/blob/types/params.go 33.33% <28.57%> (-3.71%) ⬇️
pkg/shares/share_splitting.go 66.01% <70.00%> (ø)
pkg/shares/share_merging.go 76.82% <87.50%> (-0.28%) ⬇️
pkg/prove/proof.go 83.09% <91.66%> (ø)
app/malleate_txs.go 71.87% <100.00%> (ø)
... and 18 more

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

@rootulp rootulp marked this pull request as ready for review November 17, 2022 01:22
@evan-forbes evan-forbes added dependencies Pull requests that update a dependency file consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules labels Nov 17, 2022
@evan-forbes evan-forbes added this to the Q4 2022 milestone Nov 17, 2022
@evan-forbes evan-forbes changed the title chore: upgrade to celestia-core v1.8.0-tm-v0.34.20 chore!: upgrade to celestia-core v1.8.0-tm-v0.34.20 Nov 17, 2022
rootulp
rootulp commented Nov 17, 2022
View reviewed changes
app/test/process_proposal_test.go
@@ -165,7 +165,7 @@ func TestMessageInclusionCheck(t *testing.T) {
// require.NoError(t, err)

// require.NoError(t, err)
// eds, err := da.ExtendShares(input.BlockData.OriginalSquareSize, shares)
// eds, err := da.ExtendShares(input.BlockData.SquareSize, shares)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should be deleting these tests rather than leaving them commented out. Created #1026

evan-forbes
evan-forbes previously approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

left one possible change, but we will likely change it soon anyway, so pls feel free to ignore if it takes too long

app/malleate_txs.go Outdated Show resolved Hide resolved
app/prepare_proposal.go Outdated Show resolved Hide resolved
app/test/prepare_proposal_test.go
firstNS := []byte{2, 2, 2, 2, 2, 2, 2, 2}
firstNamespace := []byte{2, 2, 2, 2, 2, 2, 2, 2}
Copy link
Member

Choose a reason for hiding this comment

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

[non-blocking][for future reference]
is this change related at all? its already completed, so pls keep, but if its 100% not related, in the future I would appreciate it if we try to not arbitrarily change variable names and put them into unrelated PRs. I don't mind this specific change, and I don't wan't to discourage renaming things, and I've been guilty of this too, but if any of us did it would be silly to block on an otherwise perfect PR.

thx 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed completely. This change isn't necessary in this PR. While working on this PR, I noticed that we use two names to refer to the same thing ns and namespace. I started correcting in this PR which was a mistake, should've been a new issue and PR.

@rootulp rootulp enabled auto-merge (squash) November 17, 2022 02:31
@rootulp rootulp merged commit 80037d1 into celestiaorg:main Nov 17, 2022
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 17, 2022
@rootulp @rach-id
chore!: upgrade to celestia-core v1.8.0-tm-v0.34.20 (celestiaorg#1023)
2dac04c 
Closes celestiaorg#1022

Does not rename all instances of `message` to `blob` see
celestiaorg#995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

Assignees

@rootulp rootulp

Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to latest celestia-core release
3 participants
@rootulp @codecov-commenter @evan-forbes