Import repair

[faddat]

Closes: #1064

If we ship current v6.0.0 we'll ship with an iavl that just had race conditions identified on it.

Furthermore, we'd be shipping with a go.sum that imports like 4 versions of iavl.

And we'd be shipping it with Tendermint as far back as v0.34.0-rc0

Personally, I just prefer that we don't do that.

This PR also makes CI use the latest golangci-lint by default, and changes some settings that were making ci fail due to the deprecation of various linters.

You can think of this PR as mainly problem-identification and defense. The real solution here is better coordination throughout the cosmos ecosystem.

Here's a tweet where I summarized findings, and also linked to pull requests that fully resolve the issue and would make the replace statements at the bottom of go.mod unnecessary.

twitter.com/gadikian/status/1469512833809678342

If we wanted to ship as-is everything would most likely be fine. Also, there's a slim chance that my changes actually introduce issues by causing imported modules to use incompatible versions of software, but so far, so good.

Summary of findings:

• Cosmos-SDK v0.44.3 pulls in tendermint as old as v0.34.0-rc4, as well as a tm-db v0.5.* or two and probably shouldn't be used because of this.

• Cosmos-SDK v0.44.5 does not pull in old-tendermint and also does not pull in old tm-db, but should make a release that uses tm-db v0.6.6 in the v0.44.* line.

• IBC-go v2.0.0 pulls in cosmos-sdk v0.44.3 and is fixed here:
  backport #589 #603 to v2.0.x ibc-go#611

• the liquidity module pulls in cosmos-sdk v0.44.3 and is fixed here:
  Fix version import issues Gravity-Devs/liquidity#7

• the ibc router pulls in cosmos-sdk v0.44.3 and is fixed here:
  updated to cosmos-sdk 0.44.5 to make gaia more slim strangelove-ventures/packet-forward-middleware#5

So, if those three PRs were merged pre-launch, I think that'd be a step towards an easier to maintain, possibly more performant, possibly more secure gaia.

[faddat]

this

[faddat]

this

[faddat]

this

[faddat]

?

[faddat]

this

[faddat]

this

[faddat]

@marbar3778 this is of specific concern

[faddat]

?

[faddat]

this

[faddat]

@marbar3778 this too

[faddat]

this

[faddat]

this

[codecov]

Codecov Report

Merging #1099 (3100b48) into main (4533e8a) will decrease coverage by 0.02%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #1099      +/-   ##
==========================================
- Coverage   12.00%   11.97%   -0.03%     
==========================================
  Files           9        9              
  Lines         933      935       +2     
==========================================
  Hits          112      112              
- Misses        816      817       +1     
- Partials        5        6       +1     

[faddat]

There are two solutions to this, and really I figure that it's best that for now we pursue both:

• replace statements
• collaboration in the ecosystem to ensure that we do not import ancient versions of our own software

It's time for sleep, but I can fix the stuff the linter found tomorrow.

[lgtm-com]

This pull request introduces 1 alert when merging 0442e16 into c5e5b1d - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

[faddat]

CosmWasm/wasmd#692

[faddat]

I checked through cosmos-sdk 0.44.3 and 0.44.5. While 0.44.3 exhibits this issue, 0.44.5 does not.

[faddat]

This patch now satisfies the linter, too :)

[tac0turtle]

Hey could you run go mod tidy . Having multiple checksums does not mean we are importing multiple versions -> https://go.dev/ref/mod#minimal-version-selection

[faddat]

on this, or on gaia's main branch?

[tac0turtle]

this

[faddat]

sure :)

[faddat]

exactly like that?

[faddat]

@marbar3778 what I think is happening is that various things that gaia imports, pull in really old software, and build against them, and that's my concern here.

[tac0turtle]

In gaia the bot that we had fixing this was removed, still unclear as to why? I think if we add depndabot back for go modules it should keep modules and versions up to date.

[faddat]

I should mention that this problem is very much not limited to gaia.

I think I am up to six chains, and I think I could easily find it to be near-universal.

See, thing is, it isn't enough to just specify stuff in go.mod because a lot of this comes from the cosmos-sdk. v0.44.3 shows this problem, while 0.44.5 doesn't.

Then, if you import anything that depends on cosmos-sdk v0.44.3 or lower, you end up getting tendermint 0.34.0-rc4 in go.sum and -- please correct me if I'm wrong-- that's no bueno. I am going to try this: go list-m all

MVS starts at the main module (a special vertex in the graph that has no version) and traverses the graph, tracking the highest required version of each module. At the end of the traversal, the highest required versions comprise the build list: they are the minimum versions that satisfy all requirements.

The build list may be inspected with the command go list -m all. Unlike other dependency management systems, the build list is not saved in a “lock” file. MVS is deterministic, and the build list doesn’t change when new versions of dependencies are released, so MVS is used to compute it at the beginning of every module-aware command.

Consider the example in the diagram below. The main module requires module A at version 1.2 or higher and module B at version 1.2 or higher. A 1.2 and B 1.2 require C 1.3 and C 1.4, respectively. C 1.3 and C 1.4 both require D 1.2.

Then, the problem comes back into the main branch of cosmos-sdk here:

cosmos/cosmos-sdk#10570

[faddat]

Well, well.....

@marbar3778 thanks for the teachin. Can you let me know if --

1. this is actually an issue, despite what this command says below

or

2. I was chasing a nothingburger because Go was showing the lowest version that it could build against, then building against a required version?

or

3. something else, which I still don't understand?

main:

 ~/testiepoo (main)> go list -m all | grep tendermint
github.com/tendermint/btcd v0.1.1
github.com/tendermint/crypto v0.0.0-20191022145703-50d29ede1e15
github.com/tendermint/go-amino v0.16.0
github.com/tendermint/tendermint v0.34.14
github.com/tendermint/tm-db v0.6.4

~/testiepoo (main)> go list -m all | grep cosmos
github.com/cosmos/gaia/v6
github.com/cosmos/cosmos-sdk v0.44.3 => github.com/cosmos/cosmos-sdk v0.44.2
github.com/cosmos/go-bip39 v1.0.0
github.com/cosmos/iavl v0.17.1
github.com/cosmos/ibc-go/v2 v2.0.0
github.com/cosmos/ledger-cosmos-go v0.11.1
github.com/cosmos/ledger-go v0.9.2
github.com/regen-network/cosmos-proto v0.3.1

[williambanfield]

Hey @faddat, it looks like what we're seeing here is this behavior of go.sum:

The go.sum file may contain hashes for multiple versions of a module. The go command may need to load go.mod files from multiple versions of a dependency in order to perform minimal version selection.

You can see that a similar stanza exists at the top of the go.sum file containing many different go.mod files correspond to cloud.google.com/go versions. These are stored because the tooling fetched them and used them as part of its calculation for MVS.

The version that the binary will be built with is the version output by go list -m all

[faddat]

Thanks @marbar3778, @williambanfield -- final question-- the difference in go.sum between cosmos-sdk 0.44.3 and 0.44.5 -- also nothing? (that difference, is really quite vast, but it seems I've learned a great deal about Go today)

cloud.google.com/go

refers to go itself?

Thanks again!

[faddat]

Hey, please let me know if this should be closed, left open, or what-- pretty clear that I was at least to some degree chasing a nothingburger based on a misnterpertation of go.sum

[tac0turtle]

The dependency update should be merged imo.

[williambanfield]

Thanks @marbar3778, @williambanfield -- final question-- the difference in go.sum between cosmos-sdk 0.44.3 and 0.44.5 -- also nothing? (that difference, is really quite vast, but it seems I've learned a great deal about Go today)

Correct, the go.sum file does not dictate what's going to be included in a build. It's just some record keeping the tooling does to ensure that it can fetch a consistent version of a dependency. It's effectively a checksum, not a lock file. It has no effect on what is in the dependency graph. It should generally be ignored.

cloud.google.com/go

refers to go itself?

Thanks again!

From go mod graph, that shows as a dependency of a grpc library:

...
google.golang.org/grpc@v1.19.1 cloud.google.com/go@v0.26.0

Repo here.

Thanks for keeping an eye on our imports. If you do spot issues where our versions look out of date, please feel free to open an issue or reach out.

[williambanfield]

I think we can close this for now. The replace statement makes versioning a bit more confusing since you need to manage the version two places in the go.mod and I'm not sure it's necessary right now, although when versioning versions of software that do not use sem ver at all, it can definitely be useful.

[faddat]

You're right about the replace statements for sure. I have to go and let others know ..... Thanks to you both @marbar3778 and @williambanfield for the lesson in go.sum ; I appreciate it.