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

v7.0.x-final #2079

Closed
wants to merge 26 commits into from
Closed

v7.0.x-final #2079

wants to merge 26 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jan 25, 2023

This PR updates gaia v7.0.x. It is an omnibus.

Closes:

Changes:

  • use informal's tendermint fork and bump to v0.34.26
  • bump sdk to v0.45.12
  • bump ibc (not state breaking) to v3.0.2
  • fix state sync script
  • don't use a t-word docker image registry
  • markdownlint
  • fix golangci-lint
  • use iavl fast node by default

These are combined here, because this is to be a final cut of release/v7.0.x

I do wish to politely request that we begin to maintain all pieces of cosmoshub-4, beginning with v4.2.x, once we have v8 shipping

To test state sync:

bash contrib/statesync.bash

test-liveness is expected to fail until this is merged. At that time, we can add to readme.md, the invocation:

bash contrib/statesync.bash

and we shall witness its passage.

This PR should sync from the first block of v7 to the last, to ensure that it is good.

@faddat faddat mentioned this pull request Jan 25, 2023
8 tasks
@faddat faddat marked this pull request as ready for review January 25, 2023 15:01
@faddat faddat marked this pull request as draft January 25, 2023 15:05
@faddat faddat marked this pull request as ready for review January 25, 2023 15:42
contrib/testnets/local/gaiadnode/Dockerfile Show resolved Hide resolved
@@ -82,6 +82,7 @@ func initAppConfig() (string, interface{}) {
srvCfg := serverconfig.DefaultConfig()
srvCfg.StateSync.SnapshotInterval = 1000
srvCfg.StateSync.SnapshotKeepRecent = 10
srvCfg.BaseConfig.IAVLDisableFastNode = false // enable fastnode by default
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @faddat, can you describe a bit more about the IAVLDisableFastNode ? what is the benefit and concerns if we set it true or false. Is there some docs i can check? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we set it to false here, then we should set it to false in v8. I recommend false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

false means we will use fast node. I recommend using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

fast node means fast sync ? Do you know why the fafault is set to be true ? If we change it, what would be the impact on a node ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it refers to iavl fast node

.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
@yaruwangway
Copy link
Contributor

Hi @faddat, I am not sure this will be included in v7, depends if we still release v7 line, also sdk0.45.12 might need a coordinate upgrade.
but this PR has some changes which can improve v8, we can include it in v8 (the docker, the state-sync docs and other docs improvement) ? @mmulji-ic @mpoke @jtremback

@faddat
Copy link
Contributor Author

faddat commented Jan 26, 2023

I also think that given the release notes on 45.12, that it probably makes sense to stay with 45.11. what say you?

@faddat
Copy link
Contributor Author

faddat commented Jan 26, 2023

@yaruwangway I need to inform you that the ICA issue with qs happened precisely because we did not maintain previous versions. We were only accepting pull requests at the tip, and this harmed maintenance.

Therefore, I advocate maintaining all versions of Gaia that are necessary to sync Gaia, that would be v4.2.0.x-v8.

All of those should be maintained. Otherwise, verifiability of archive state becomes incredibly difficult.

Essentially, ever since I wrote a certain issue that anthropomorphized the hub, I have wanted to roll through the hub's releases like:

  • v4
  • v5
  • v6
  • v7
  • v8

And format / optimize each of these. This would allow us to run speed races, and ensure that the chain syncs as rapidly as possible.

@sonarcloud
Copy link

sonarcloud bot commented Jan 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yaruwangway
Copy link
Contributor

Hi @faddat, if you can split this PR into two PRs? one only updates fast node, the rest goes to another PR, and make both PRs against main ?

or you can change your present PR against this branch and I can make some changes before it merged ?

Thank you!

@mmulji-ic
Copy link
Contributor

mmulji-ic commented Feb 3, 2023

@yaruwangway I need to inform you that the ICA issue with qs happened precisely because we did not maintain previous versions. We were only accepting pull requests at the tip, and this harmed maintenance.

Therefore, I advocate maintaining all versions of Gaia that are necessary to sync Gaia, that would be v4.2.0.x-v8.

All of those should be maintained. Otherwise, verifiability of archive state becomes incredibly difficult.

Essentially, ever since I wrote a certain issue that anthropomorphized the hub, I have wanted to roll through the hub's releases like:

Thanks @faddat for this PR. Currently we're following the Major Release Maintenence policy, so the current plan is to stop updates for v6 and lower when Rho is out. We should schedule a discussion if you strongly believe that we should support older versions to understand the impact for node operators.

We will review this, as @yaruwangway suggested splitting this into 2 would be helpful.

As a sidenote, the qs issue was caused by constant upstream delays and also the time squeeze that happened because of ICS. In the future we will target smaller releases and release whats ready rather than wait for an agreed feature set which might be delayed. Hopefully this will help with maintenance and security issues fixed sooner.

@faddat
Copy link
Contributor Author

faddat commented Feb 3, 2023

We should not "freeze" old gaias.

@yaruwangway I've removed the iavl fast node by default here, I think this PR is good to go.

The qs issue was caused by a lack of work on the hub, not by upstream delays. There is no other production chain that I'm aware of using that version of ibc-go.

@yaruwangway
Copy link
Contributor

Hi @faddat, how about updating v8 first so we can include this pr to the upcoming releases ? this might need some update of the go.mod in this pr to match v8.
let me know if you prefer to do it, otherwise, i can merge this pr to a branch i checked out and change the version there. Thank you!

@faddat
Copy link
Contributor Author

faddat commented Feb 3, 2023

@yaruwangway

Hi, so this is not the kind of thing that we would merge into the tip, because many of these changes are already in the tip.

Thing is, we do not backport them, and they are version-sensitive. With state sync, for example, we will need to rewrite that after the upgrade.

In fact I'm interested in going even more backwards.

@faddat
Copy link
Contributor Author

faddat commented Feb 4, 2023

@mmulji-ic

https://github.com/cosmos/ibc-go/releases

Please count the number of compatible releases that upstream shipped while hub used v3.0.0

I came up with a number that makes your claim impossible, unfortunately.

I would like to suggest that we try to keep historical gaias up to date with all of our latest standards and practices, while not breaking state.

@yaruwangway
Copy link
Contributor

a release of v7.1.2 ? @mmulji-ic

@faddat
Copy link
Contributor Author

faddat commented Feb 6, 2023

@yaruwangway

I'll break out few very old issues:

You'll note that I've brought in 2 osmosis issues here, and 1 sdk issue.

Currently it is kinda nearly impossible to make archive nodes so everyone just uses quicksync.io but now that's the canonical archive state of the hub, eg: possible long range attack or just plain old state corruption. We also know that we can dramatically improve sync speed. So the idea is to bundle these, and actually begin from v4.2.0, and work our way upwards.

cosmoshub-4 is:

v4.x
v5.x
v6.x
v7.x
v8.x
v9.x

-- not only the tip. So we can't really just make PR's into the tip because for reproducibility we need to walk through the different versions. Ultimate issue: that is simply too time consuming right now so no one does it.

@faddat
Copy link
Contributor Author

faddat commented Feb 6, 2023

Note the v0.45.12 update -- interestingly, we don't know if it is state breaking or not -- it maybe is, according to its release notes. So then we would want to walk through prior releases, and do a full sync, so we can know for sure.

Does it make sense?

@glnro
Copy link
Contributor

glnro commented Feb 6, 2023

Note the v0.45.12 update -- interestingly, we don't know if it is state breaking or not -- it maybe is, according to its release notes. So then we would want to walk through prior releases, and do a full sync, so we can know for sure.

Does it make sense?

The advice is to bump to v0.45.12 in a coordinated upgrade, which the hub will be doing with v8.0.0 https://github.com/cosmos/gaia/releases/tag/v8.0.0

@faddat
Copy link
Contributor Author

faddat commented Feb 6, 2023

@glnro indeed. I want to test this with the full state of v7 before merging.

Then we know.

There have been a bunch of cases where we guess, and I figure it's better for us to know for sure.

@faddat faddat marked this pull request as draft February 6, 2023 21:36
This reverts commit 1c7ad43.
@faddat
Copy link
Contributor Author

faddat commented Feb 6, 2023

Sorry, since we want to use this, and v0.45.12 would delay that, I've reverted that commit. Can test that at a later time.

@faddat faddat marked this pull request as ready for review February 6, 2023 22:21
@faddat
Copy link
Contributor Author

faddat commented Feb 14, 2023

@glnro -- I think that we should bump to v0.45.13 in v8

@mpoke
Copy link
Contributor

mpoke commented Mar 16, 2023

Closing as v7.0.x is no longer supported.

@mpoke mpoke closed this Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants