Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

chore: update libp2p to 0.52.1#14429

Merged
paritytech-processbot[bot] merged 53 commits intoparitytech:masterfrom
melekes:anton/libp2p-0.52.0
Jul 25, 2023
Merged

chore: update libp2p to 0.52.1#14429
paritytech-processbot[bot] merged 53 commits intoparitytech:masterfrom
melekes:anton/libp2p-0.52.0

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Jun 21, 2023

@melekes melekes added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 21, 2023
@melekes melekes self-assigned this Jun 21, 2023
};
let reason = match cause {
Some(ConnectionError::IO(_)) => "transport-error",
// Some(ConnectionError::Handler(Either::Left(Either::Left(
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 such variant seems to exist

Copy link
Contributor

Choose a reason for hiding this comment

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

PingFailure doesn't exist?

What does ConnectionError::Handler(Either::Left(Either::Left(Either::Left(Either::Right contain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let reason = match cause {
     |                                        ----- this expression has type `std::option::Option<ConnectionError<either::Either<either::Either<either::Either<either::Either<NotifsHandlerError, either::Either<void::Void, std::io::Error>>, std::io::Error>, void::Void>, void::Void>>>`

std::io::Error

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are sure that this io::Error is handling the case for ping failure, may be we should still report it as "ping-failure" and not as default "protocol-error"?

@melekes melekes requested review from a team July 19, 2023 06:43
@melekes melekes added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Jul 19, 2023
@melekes

This comment was marked as resolved.

in attempt to build cumulus
Comment on lines +1047 to +1050
// Set the Kademlia mode to server so that it can accept incoming requests.
//
// Note: the server mode is set automatically when the node learns its external
// address, but that does not happen in tests => hence we set it manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about another test with one server and one client, just to verify that such a scenario also works?

// };

// handler.on_connection_event(handler::ConnectionEvent::FullyNegotiatedInbound(
// handler::FullyNegotiatedInbound { protocol: (notif_in, 0), info: () },
Copy link
Contributor

Choose a reason for hiding this comment

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

@thomaseizinger

IIUC the reason these were commented out was because something in libp2p was made private. What was the exact reason @melekes?

@thomaseizinger
Copy link

thomaseizinger commented Jul 20, 2023

While this is not my problem anymore, I find it frustrating that every update of a libp2p version seems to require a +1000 -1000 pull request that understandably takes a long time to write, review, and merge.

I find it a bit ridiculous that a very critical feature is blocked on a simple version bump, but this simple version bump comes bundles with tons of breaking changes.

You will appreciate then that lots of engineering hours have already gone and will continue to go into making as few breaking changes as possible:

You can discover more things by following the links in the above issues. Esp. the 0.52 release hardened many many APIs in rust-libp2p that allow us to make improvements without making more breaking changes :)

self.kademlia.on_swarm_event(FromSwarm::NewListenAddr(e));

if let Some(ref mut mdns) = self.mdns {
mdns.on_swarm_event(FromSwarm::NewListenAddr(e));
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use libp2p_swarm::behaviour::toggle::Toggle instead of Option<TokioMdns>. It will remove ifs because they are hidden inside Toggle::on_* methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍 Will do in a separate PR

@melekes
Copy link
Contributor Author

melekes commented Jul 25, 2023

bot merge

@paritytech-processbot
Copy link

Error: Github API says paritytech/cumulus#2838 is not mergeable

@melekes
Copy link
Contributor Author

melekes commented Jul 25, 2023

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/cumulus#2838

@melekes
Copy link
Contributor Author

melekes commented Jul 25, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit d38d176 into paritytech:master Jul 25, 2023
@melekes melekes deleted the anton/libp2p-0.52.0 branch July 25, 2023 11:12
melekes added a commit to melekes/substrate that referenced this pull request Jul 28, 2023
melekes added a commit to melekes/substrate that referenced this pull request Jul 28, 2023
altonen added a commit that referenced this pull request Aug 5, 2023
altonen added a commit that referenced this pull request Aug 6, 2023
altonen added a commit that referenced this pull request Aug 6, 2023
paritytech-processbot bot pushed a commit that referenced this pull request Aug 16, 2023
* Revert "chore: update libp2p to 0.52.1 (#14429)"

This reverts commit d38d176.

* Fix dependencies

* Update dependencies

* Update Cargo.lock
Ank4n pushed a commit that referenced this pull request Aug 20, 2023
* Revert "chore: update libp2p to 0.52.1 (#14429)"

This reverts commit d38d176.

* Fix dependencies

* Update dependencies

* Update Cargo.lock
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants