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.

Sign up for GitHub

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

Jump to bottom

Update go-libp2p to v0.15.0 #7362

Merged
merged 6 commits into from
Sep 23, 2021
Merged

Update go-libp2p to v0.15.0 #7362

merged 6 commits into from
Sep 23, 2021

Conversation

aarshkshah1992
Copy link
Contributor

go-libp2p-core v0.9.0 and go-libp2p v0.15.0 contains breaking changes.

@aarshkshah1992 aarshkshah1992 requested a review from a team as a code owner September 21, 2021 11:11
@jennijuju
Copy link
Member

Ci is very unhappy..
What’s the breaking changes? What are the potential impact to lotus users? Have we tested it?

Stebalien
Stebalien reviewed Sep 21, 2021
View reviewed changes
go.mod Outdated
@@ -85,7 +85,7 @@ require (
github.com/ipfs/go-ipfs-exchange-interface v0.0.1
github.com/ipfs/go-ipfs-exchange-offline v0.0.1
github.com/ipfs/go-ipfs-files v0.0.8
github.com/ipfs/go-ipfs-http-client v0.0.5
github.com/ipfs/go-ipfs-http-client v0.1.0-update-libp2p
Copy link
Member

Choose a reason for hiding this comment

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

Can you make a PR for this? We might as well update go-ipfs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien Master in github.com/ipfs/go-ipfs-http-client already has the latest mutliaddr stuff but upgrading to it breaks a whole host of other ipfs related stuff in Lotus and I don't wanna do that upgrade as part of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Can we tag v0.0.6 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien This is done. Please can you approve the PR ?

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Sep 21, 2021 •
edited
Loading

@jennijuju

  • The CI failure was a compilation error. I've fixed it.

  • I don't think there's any impact to users. The breaking change is that some libp2p APIs that had been deprecated for a while have finally been completely removed. So, we had to make the change on our side.

  • If we don't upgrade libp2p and punt on this, that will block M2 work for Ignite because we will NOT be able to integrate the go-indexer-core work done by Data systems.

  • If you still have concerns about the libp2p upgrade here, we can have a chat with @Stebalien or @marten-seeman - the maintainer of libp2p.

@Stebalien
Copy link
Member

What’s the breaking changes? What are the potential impact to lotus users? Have we tested it?

The main breaking changes are the removal of deprecated interfaces. That shouldn't affect anything.

Other than that, this has a new mDNS implementation (not used by lotus, IIRC) and a new QUIC version.

The riskiest thing is the new QUIC version.

marten-seemann
marten-seemann approved these changes Sep 21, 2021
View reviewed changes
node/modules/lp2p/addrs.go
@@ -11,19 +11,6 @@ import (
mamask "github.com/whyrusleeping/multiaddr-filter"
)

func AddrFilters(filters []string) func() (opts Libp2pOpts, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this an unused function? Otherwise, you might need to replace it with a connection gater.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was dead code.

@codecov
Copy link

codecov bot commented Sep 21, 2021 •
edited
Loading

Codecov Report

Merging #7362 (7694f89) into master (38f7cbc) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7362      +/-   ##
==========================================
- Coverage   39.18%   39.12%   -0.07%     
==========================================
  Files         614      614              
  Lines       64996    64988       -8     
==========================================
- Hits        25471    25424      -47     
- Misses      35123    35157      +34     
- Partials     4402     4407       +5
Impacted Files Coverage Δ
cmd/lotus-miner/init.go 0.00% <0.00%> (ø)
node/modules/lp2p/addrs.go 22.80% <ø> (+2.80%) ⬆️
itests/kit/ensemble.go 91.92% <100.00%> (ø)
node/modules/lp2p/libp2p.go 36.17% <100.00%> (ø)
chain/events/message_cache.go 87.50% <0.00%> (-12.50%) ⬇️
chain/actors/builtin/miner/diff.go 48.52% <0.00%> (-10.30%) ⬇️
chain/stmgr/call.go 67.87% <0.00%> (-7.28%) ⬇️
extern/sector-storage/manager_calltracker.go 57.70% <0.00%> (-4.85%) ⬇️
chain/exchange/peer_tracker.go 66.66% <0.00%> (-4.31%) ⬇️
storage/wdpost_sched.go 77.22% <0.00%> (-1.99%) ⬇️
... and 15 more

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 38f7cbc...7694f89. Read the comment docs.

Stebalien
Stebalien requested changes Sep 22, 2021
View reviewed changes
testplans/lotus-soup/go.mod Outdated
github.com/ipfs/go-ipfs-files v0.0.8
github.com/ipfs/go-ipld-format v0.2.0
github.com/ipfs/go-log/v2 v2.3.0
github.com/ipfs/go-merkledag v0.3.2
github.com/ipfs/go-unixfs v0.2.6
github.com/ipld/go-car v0.1.1-0.20201119040415-11b6074b6d4d
github.com/ipld/go-car v0.3.1-0.20210601190600-f512dac51e8e
Copy link
Member

Choose a reason for hiding this comment

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

This is not a commit on master, or a tagged commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien This has been a problem with the go-car dep for a while now. Do you just want us to tag this commit and push it in ?

Copy link
Member

Choose a reason for hiding this comment

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

That works. As long as we can be sure it won't disappear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien - This is done. Please can you approve ?

@dirkmc
Copy link
Contributor

dirkmc commented Sep 22, 2021 •
edited
Loading

I ran storage and retrieval compatibility tests between a client and miner running either

Results

  • hybrid client vs hybrid miner

    • Storage deal success
    • Retrieval deal success

  • v1.11.3-rc2 client vs hybrid miner

    • Storage deal success
    • Retrieval deal success

  • hybrid client vs v1.11.3-rc2 miner

    • Storage deal success
    • Retrieval deal success

@magik6k magik6k merged commit 8e0314b into master Sep 23, 2021
@magik6k magik6k deleted the chore/update-libp2p branch September 23, 2021 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magik6k magik6k magik6k approved these changes

@marten-seemann marten-seemann marten-seemann approved these changes

@Stebalien Stebalien Stebalien approved these changes

Assignees

@aarshkshah1992 aarshkshah1992

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aarshkshah1992 @jennijuju @Stebalien @dirkmc @marten-seemann @magik6k