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

Upgrade to tendermint v0.35.4 and cosmos-sdk v0.46.0 #399

Merged
merged 32 commits into from
May 18, 2022

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented May 12, 2022

Description

This PR updates the application to tendermint v0.35.4 and the cosmos-sdk to v0.46.0. While there are a wide variety of changes that included in this PR, each is needed to accommodate the latest version of the cosmos-sdk

blocked by celestiaorg/celestia-core#747
closes #376

related to #291

@evan-forbes evan-forbes added dependencies Pull requests that update a dependency file C: Celestia app labels May 12, 2022
@evan-forbes evan-forbes self-assigned this May 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

Merging #399 (17379f7) into master (6080046) will decrease coverage by 0.99%.
The diff coverage is 43.33%.

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
- Coverage   26.12%   25.13%   -1.00%     
==========================================
  Files          14       14              
  Lines        1860     1858       -2     
==========================================
- Hits          486      467      -19     
- Misses       1336     1353      +17     
  Partials       38       38              
Impacted Files Coverage Δ
x/payment/keeper/keeper.go 62.50% <0.00%> (+6.94%) ⬆️
x/payment/types/builder.go 39.71% <36.84%> (-2.15%) ⬇️
x/payment/types/wirepayfordata.go 68.24% <40.00%> (-1.21%) ⬇️
x/payment/types/codec.go 91.66% <100.00%> (-0.23%) ⬇️
x/payment/types/genesis.go 100.00% <100.00%> (ø)
x/payment/types/tx.pb.go 15.86% <0.00%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6080046...17379f7. Read the comment docs.

app/encoding/encoding.go Show resolved Hide resolved
Comment on lines 10 to 12
func main() {
rootCmd, _ := cosmoscmd.NewRootCmd(
app.Name,
app.AccountAddressPrefix,
app.DefaultNodeHome,
app.Name,
app.ModuleBasics,
appBuilder,
// this line is used by starport scaffolding # root/arguments
)
if err := svrcmd.Execute(rootCmd, app.DefaultNodeHome); err != nil {
rootCmd := NewRootCmd()
if err := svrcmd.Execute(rootCmd, "CELESTIA", app.DefaultNodeHome); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

We replace more starport code with our own infrastructure.

Comment on lines +39 to +41
// NewRootCmd creates a new root command for celestia-appd. It is called once in the
// main function.
func NewRootCmd() *cobra.Command {
Copy link
Member Author

Choose a reason for hiding this comment

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

This command can probably be cleaned up and more organized. I elected to do this in a different PR for time purposes, but if we want I'm happy to refactor to something cleaner and more organized

Comment on lines +200 to +202
// SetupWithGenesisValSet initializes GenesisState with a single validator and genesis accounts
// that also act as delegators.
func GenesisStateWithSingleValidator(t *testing.T, testApp *app.App) (app.GenesisState, *tmtypes.ValidatorSet) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This might fix the issue @renaynay was running into with using a test application in celestia-node, as the network could not be setup due to improper initialization of the staking state/validator set

go.mod Outdated Show resolved Hide resolved
@adlerjohn adlerjohn marked this pull request as draft May 12, 2022 15:38
@evan-forbes evan-forbes marked this pull request as ready for review May 13, 2022 16:46
app/app.go Show resolved Hide resolved
app/app.go Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/app.go Show resolved Hide resolved
app/export.go Show resolved Hide resolved
cmd/celestia-appd/main.go Outdated Show resolved Hide resolved
Comment on lines 18 to 19
sed -i 's/timeout_commit = "5s"/timeout_commit = "1s"/g' ~/.celestia-app/config/config.toml
sed -i 's/timeout_propose = "3s"/timeout_propose = "1s"/g' ~/.celestia-app/config/config.toml
Copy link
Member

Choose a reason for hiding this comment

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

Are we intentionally overwriting this for single node networks?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, tbh, I don't remember why or when we added this

go.mod Show resolved Hide resolved
Comment on lines +45 to +47
Block: &tmproto.BlockParams{
MaxBytes: 200000,
MaxGas: 2000000,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we would keep the blocks so small in tests 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

so far, this test is only used for a few txs, as its only for unit tests. We might need to change this in the near future, depending on when and how we add more tests with large messages and full mempools

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Left a bunch of comments. Looks really good besides a few nits and questions. I did not test this manually.

@evan-forbes evan-forbes mentioned this pull request May 16, 2022
@liamsi
Copy link
Member

liamsi commented May 16, 2022

Surprised to see merge conflicts now. I think we did not change anything on main?

@evan-forbes
Copy link
Member Author

Surprised to see merge conflicts now. I think we did not change anything on main?

we merged #224, so this makes sense. I tried to quickly merge last night, but ran into an issue and haven't time to finish debugging.

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK

@evan-forbes evan-forbes merged commit 5e4a1dc into master May 18, 2022
@evan-forbes evan-forbes deleted the evan/upgrade-core-35.4 branch May 18, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Upgrade to celestia-core v1.2.0-tm-v0.35.4
4 participants