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: use go1.19 to prevent mixed runtimes #2016

Closed
wants to merge 3 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jan 13, 2023

The alternative to this is to have the Makefile check the Go version, as
Osmosis did for their v13 release.

This is the lower-effort, safer way to address the issue. Mixing the go
1.18 and 1.19 runtimes has been known to cause issues with state sync and
consensus. Choosing the higher version ensures that no one can use the
lower version.

closes: 2015

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #2016 (39db920) into main (983a9fe) will decrease coverage by 5.66%.
The diff coverage is n/a.

❗ Current head 39db920 differs from pull request most recent head bb00cd7. Consider uploading reports for the commit bb00cd7 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2016      +/-   ##
==========================================
- Coverage   84.85%   79.20%   -5.66%     
==========================================
  Files          21       23       +2     
  Lines        1466     1577     +111     
==========================================
+ Hits         1244     1249       +5     
- Misses        177      274      +97     
- Partials       45       54       +9     
Impacted Files Coverage Δ
app/app.go 72.35% <0.00%> (-4.23%) ⬇️
x/globalfee/module.go 84.90% <0.00%> (-3.78%) ⬇️
app/keepers/keepers.go 97.21% <0.00%> (-1.70%) ⬇️
app/modules.go 100.00% <0.00%> (ø)
app/keepers/keys.go 86.20% <0.00%> (ø)
app/upgrades/v8/upgrades.go 54.73% <0.00%> (ø)
app/upgrades/v7/upgrades.go 0.00% <0.00%> (ø)

@glnro
Copy link
Contributor

glnro commented Jan 13, 2023

Neither ibc-go or sdk versions that v8 will use are opting for 1.19. The docs explicitly mention which version of go is compatible with gaia. Perhaps best to open an issue for future consideration instead.

@faddat
Copy link
Contributor Author

faddat commented Jan 14, 2023

Then I implore that a check be added to the Makefile for the go version.

Normally this does not matter. Between 18 and 19, it matters.

@faddat
Copy link
Contributor Author

faddat commented Jan 17, 2023

Hey I would just like to say once again, that 1.19 will build releases that specify 1.18. this leads to a mixed runtime situation. When there are mixed run times, state sync does not work, and there are rumors that this can break consensus that are being explored in the osmosis validators discord.

The way to entirely prevent this issue, is to specify 1.19 in the go.mod file of version 8 of the cosmos hub. Even adding a check to the makefile like cosmosis did, can still have the result of validators building with the wrong runtime, like:

go install ./...

I know this from experience.

@glnro
Copy link
Contributor

glnro commented Jan 17, 2023

Would you be able add the check to the makefile then instead in this pr? @jtremback

@jtremback
Copy link
Contributor

jtremback commented Jan 17, 2023

@faddat here's how I understand this issue:

  • There are potentially problems related to having some validators compile with 1.18 and some compile with 1.19
  • By specifying 1.19 in the go.mod, nobody will be able to compile with 1.18
  • But we don't to specify 1.19 because we don't want to do a version upgrade right now because we are trying to freeze the code and get a release out, and our dependencies use 1.18
  • So we can add a check in the makefile to refuse to compile with 1.19, solving the potential mixed runtimes problem.

Is that correct?

@faddat
Copy link
Contributor Author

faddat commented Jan 25, 2023

Yeah, but it's way better to just use 1.19, otherwise there can be a bunch of issues.

@faddat
Copy link
Contributor Author

faddat commented Jan 25, 2023

The commentary around state sync here:

could be solved by this.

@sonarqubecloud
Copy link

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

@faddat faddat closed this Jan 25, 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.

3 participants