Prune go.sum #643

faddat · 2021-12-08T11:04:07Z

Description

To explore preformance issues more deeply, we need to prune go.sum as much as possible. This represents a good first start, but is in no way complete. Since when do cosmos projects need four different versions of btcd, for example?

Sure, likely they need one.... but I reckon that we've got four may be causing trouble.

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

…ferent imported libs at a time.

codecov-commenter · 2021-12-08T11:08:37Z

Codecov Report

Merging #643 (d1d3158) into main (97ac2a8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #643   +/-   ##
=======================================
  Coverage   18.42%   18.42%           
=======================================
  Files         172      172           
  Lines       24254    24254           
=======================================
  Hits         4470     4470           
  Misses      19019    19019           
  Partials      765      765

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 97ac2a8...d1d3158. Read the comment docs.

faddat · 2021-12-10T23:45:25Z

go.sum

 github.com/cosmos/go-bip39 v1.0.0 h1:pcomnQdrdH22njcAatO0yWojsUnCO3y2tNoV1cb6hHY=
 github.com/cosmos/go-bip39 v1.0.0/go.mod h1:RNJv0H/pOIVgxw6KS7QeX2a0Uo0aKUlfhZ4xuwvCdJw=
-github.com/cosmos/iavl v0.17.2 h1:BT2u7DUvLLB+RYz9RItn/8n7Bt5xe5rj8QRTkk/PQU0=
-github.com/cosmos/iavl v0.17.2/go.mod h1:prJoErZFABYZGDHka1R6Oay4z9PrNeFFiMKHDAMOi4w=


I think this is also resolved by tm-db 0.6.6

faddat · 2021-12-10T23:45:42Z

go.sum

@@ -832,8 +833,6 @@ github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9dec
 github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
 github.com/zondax/hid v0.9.0 h1:eiT3P6vNxAEVxXMw66eZUAAnU2zD33JBkfG/EnfAKl8=
 github.com/zondax/hid v0.9.0/go.mod h1:l5wttcP0jwtdLjqjMMWFVEE7d1zO0jvSPA9OPZxWpEM=
-go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=


ValarDragon

Hey, thanks for looking into this in such depth!

Aside from the IBC-go bump to v2.0.1, I don't agree with the other changes here. I feel like this replace based approach adds untenable amounts of overheads / safety issues for updating any constituent library. I'm also unclear on where the problem stems from.

I think we do need replaces for SDK, Tendermint, and maybe SDK direct dependencies long term. This is due to our direct forking of an SDK version, and propensity to choose our own tendermint version, and need for compat.

But the rest of the dependencies should be chosen by tendermint / tm-db alone imo. I'd be much more happy to do just a tm-db version replace, and then check tendermint & tm-db in tandem.

ValarDragon · 2021-12-11T08:26:30Z

go.mod

@@ -3,10 +3,10 @@ module github.com/osmosis-labs/osmosis
 go 1.17

 require (
-	github.com/cosmos/cosmos-sdk v0.44.3
+	github.com/cosmos/cosmos-sdk v0.44.5


Lets not bump this, we're really importing v0.44.3 with our osmosis commits

ValarDragon · 2021-12-11T08:28:24Z

go.mod

+	github.com/cosmos/iavl v0.17.3
+	github.com/cosmos/ibc-go/v2 v2.0.1


Not really a fan of bumping minor versions unless we have a reason to do so, or have looked carefully into changes at hand. Its more things to think about for non-determinism / surface area for incompatibility between versions.

In the ibc-go case, seems fine to me. (Looked into it due to this commit)

In IAVL, not sure. Would prefer to not be first to test it, sounds liable to cause slowdowns for parallel queries. (And i think theres a better fix for the bug at hand)

ValarDragon · 2021-12-11T08:29:32Z

go.mod

@@ -120,8 +120,12 @@ require (

 replace (
 	github.com/cosmos/cosmos-sdk => github.com/osmosis-labs/cosmos-sdk v0.43.0-rc3.0.20211119062100-2f81f59f3aa1
+	github.com/cosmos/go-bip39 => github.com/cosmos/go-bip39 v1.0.0


I don't really grok why we had a bip39 issue, especailly if its past 1.0. The bip39 library authors should be guaranteeing API compatibility.

ValarDragon · 2021-12-11T08:30:45Z

go.mod

 	github.com/tendermint/tendermint => github.com/tendermint/tendermint v0.34.14
 	github.com/tendermint/tm-db => github.com/osmosis-labs/tm-db v0.6.5-0.20210911033928-ba9154613417
+	go.etcd.io/bbolt => go.etcd.io/bbolt v1.3.5


The bbolt stuff is odd, but it all only differs in patch version. I don;t want to add maintainence burden on us to check every time we update a dependency if it for some reason depends on a new feature in next minor bump, and we get a subtle error due to this replace.

so, your concern here, was actually the same as mine (regarding the fix)

ValarDragon · 2021-12-11T08:34:08Z

go.mod

 	github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
+	github.com/tecbot/gorocksdb => github.com/cosmos/gorocksdb v0.0.0-20211202124722-2c356d6d98e4


ditto for my comment re: bbolt. Do we have to upgrade this every patch version of TM/SDK/TM-db?

I'm more happy to do this if we decide to give more rocksdb support, but I want to be clear on why were getting multiple versions, and if we can do this replace in (say) our SDK fork, or a tm-db fork.

It seems to me that they've resolved this issue in tm-db v0.6.6

ValarDragon · 2021-12-11T08:40:11Z

Also not sure I agree with the performance improvement aspect of this. (Not at all clear to me how this impacts performance, aside from potentially binary size)

I do agree with the dependency mitigation / security surface area reduction aspects of consideration with the imports / replaces though!

faddat · 2021-12-11T12:12:30Z

Re: performance-- the claim isn't so much that this will improve performance, but instead that it'll allow for investigation into performance issues.

faddat · 2021-12-11T15:17:47Z

@ValarDragon thanks so much for your review and responsiveness during my discovery process on this. I summarized my findings in this thread of tweets:

https://twitter.com/gadikian/status/1469512833809678342

Conclusion: We can actually close this PR and should work to get this:

cosmos/ibc-go#611

merged asap, and we're all good. I'll see if I can get our tm-db updated w/the changes from tendermint/tm-db so all of this is simply eliminated as a concern.

ValarDragon · 2021-12-12T02:18:58Z

Sweet! Thanks for the work on investigating all of the dependencies

Prune go.sum by ensuring we use not more than just one version of dif…

bf6c342

…ferent imported libs at a time.

faddat changed the title ~~Prune go.sum by ensuring we use not more than just one version of different imported libs at a time.~~ Prune go.sum Dec 8, 2021

Merge branch 'main' into go-sum-prune

4fb59b5

faddat mentioned this pull request Dec 10, 2021

Update go.mod #648

Closed

4 tasks

Ensure that rocksdb works

cbf72e7

faddat commented Dec 10, 2021

View reviewed changes

ValarDragon requested changes Dec 11, 2021

View reviewed changes

faddat added 2 commits December 11, 2021 19:14

Update go.mod

4c783bb

removed replaces

d1d3158

faddat closed this Dec 11, 2021

ValarDragon deleted the go-sum-prune branch December 12, 2021 02:19

daniel-farina added this to the 2021 - December Milestone milestone Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune go.sum #643

Prune go.sum #643

faddat commented Dec 8, 2021 •

edited

Loading

codecov-commenter commented Dec 8, 2021 •

edited

Loading

faddat Dec 10, 2021

faddat Dec 11, 2021

faddat Dec 10, 2021

ValarDragon left a comment

ValarDragon Dec 11, 2021

ValarDragon Dec 11, 2021

faddat Dec 11, 2021

ValarDragon Dec 11, 2021

ValarDragon Dec 11, 2021

faddat Dec 11, 2021

ValarDragon Dec 11, 2021

faddat Dec 11, 2021

ValarDragon commented Dec 11, 2021

faddat commented Dec 11, 2021

faddat commented Dec 11, 2021 •

edited

Loading

ValarDragon commented Dec 12, 2021

		github.com/cosmos/iavl v0.17.3
		github.com/cosmos/ibc-go/v2 v2.0.1

		github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
		github.com/tecbot/gorocksdb => github.com/cosmos/gorocksdb v0.0.0-20211202124722-2c356d6d98e4

Prune go.sum #643

Prune go.sum #643

Conversation

faddat commented Dec 8, 2021 • edited Loading

Description

codecov-commenter commented Dec 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Dec 11, 2021

faddat commented Dec 11, 2021

faddat commented Dec 11, 2021 • edited Loading

ValarDragon commented Dec 12, 2021

faddat commented Dec 8, 2021 •

edited

Loading

codecov-commenter commented Dec 8, 2021 •

edited

Loading

faddat commented Dec 11, 2021 •

edited

Loading