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

Upgrade libp2p to 0.52.4 #1631

Merged
merged 58 commits into from
Jun 25, 2024
Merged

Upgrade libp2p to 0.52.4 #1631

merged 58 commits into from
Jun 25, 2024

Conversation

altonen
Copy link
Contributor

@altonen altonen commented Sep 19, 2023

Upgrade libp2p to 0.52.4, including a fix:

TODO

  • Fix 3 zombienet tests failing:
    • zombienet-substrate-0002-validators-warp-sync
    • zombienet-polkadot-functional-0005-parachains-disputes-past-session The test is also flaky in other PRs and is not required for CI to succeed.
    • zombienet-polkadot-functional-0009-approval-voting-coalescing
  • Uncomment and update to the actual libp2p API tests in substrate/client/network/src/protocol/notifications/handler.rs.
  • When upgrading multihash crate as part of libp2p upgrade to version v0.19.1, uncomment the conversion code at
    // TODO: uncomment this after upgrading `multihash` crate to v0.19.1.
  • Perform a burn-in.

altonen and others added 4 commits September 19, 2023 10:35
Co-authored-by: Anton <anton.kalyaev@gmail.com>
Recent optimization in `V1Lazy` caused syncing to stall if if a request
was sent to a peer who didn't support the request-response protocol.
Addresses getting removed from Kademlia for completely innocuous reasons
such as keep-alive timeouts are causing the node to have a lot of dial
failures. The dial failures happen because when `ProtocolController`
tries to connect to a node, and when the address is trying to be fetched
from from `Discovery`, it returns an empty vector since `Kademlia` no
longer holds any addresses for the peer.
@altonen altonen added the T0-node This PR/Issue is related to the topic “node”. label Sep 19, 2023
@altonen altonen requested a review from a team September 19, 2023 09:09
@altonen
Copy link
Contributor Author

altonen commented Sep 19, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 19, 2023

@altonen https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3740116 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-85cce569-2ba9-445a-a57d-f7f1e8bde6a5 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 19, 2023

@altonen Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3740116 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3740116/artifacts/download.

Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Looks good! Could you remind why we had to comment-out the tests in Notifications?

substrate/client/network/src/discovery.rs Outdated Show resolved Hide resolved
substrate/client/network/src/discovery.rs Outdated Show resolved Hide resolved
substrate/client/network/src/discovery.rs Outdated Show resolved Hide resolved
@dmitry-markin dmitry-markin requested a review from a team September 19, 2023 10:19
altonen and others added 2 commits September 19, 2023 15:51
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@altonen
Copy link
Contributor Author

altonen commented Sep 27, 2023

@xermicus @athei

This libp2p upgrade updates wat which breaks pallet-contracts tests:

---- wasm::tests::balance stdout ----
thread 'wasm::tests::balance' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: Wast(Error { inner: ErrorInner { text: Some(Text { line: 11, col: 5, snippet: "                (get_local 0)" }), file: None, span: Span { offset: 242 }, kind: Custom("unknown operator or unexpected token") } }) }', substrate/frame/contracts/src/wasm/mod.rs:758:40

This can also be reproduced if cargo update -p wat is run on master. Any tips on what's the best way go about fixing these? I'm not familiar with this code at all.

Copy link

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looking forward for this to land. Ping me if you need any help!

substrate/client/network/src/discovery.rs Outdated Show resolved Hide resolved
substrate/client/network/src/transport.rs Outdated Show resolved Hide resolved
@xermicus
Copy link
Member

@altonen it appears that the wat crate released a patch version that was actually breaking. In master, wat version 1.0.70 is in the Cargo.lock, which still supports this syntax. Which was changed in version 1.0.72, seemingly only supporting local.get 0

@altonen altonen requested a review from athei as a code owner October 16, 2023 21:18
@altonen altonen requested a review from a team October 16, 2023 21:18
@altonen altonen requested a review from koute as a code owner October 16, 2023 21:18
@dmitry-markin dmitry-markin requested a review from alexggh June 18, 2024 11:27
@nazar-pc
Copy link
Contributor

This PR is now a security issue due to dalek-cryptography/curve25519-dalek#659

@dmitry-markin
Copy link
Contributor

This PR is now a security issue due to dalek-cryptography/curve25519-dalek#659

Thanks for reporting! Bumped curve25519-dalek to the latest patched version (4.1.3).

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6503079

substrate/client/mixnet/src/sync_with_runtime.rs Outdated Show resolved Hide resolved
substrate/client/network/src/behaviour.rs Show resolved Hide resolved
substrate/client/network/src/service.rs Outdated Show resolved Hide resolved
substrate/client/network/src/service.rs Outdated Show resolved Hide resolved
substrate/client/network/src/service.rs Outdated Show resolved Hide resolved
substrate/client/network/src/service.rs Outdated Show resolved Hide resolved
substrate/client/network/src/service.rs Outdated Show resolved Hide resolved
substrate/client/network/src/discovery.rs Outdated Show resolved Hide resolved
substrate/client/network/src/discovery.rs Outdated Show resolved Hide resolved
@dmitry-markin dmitry-markin requested a review from lexnv June 24, 2024 14:34
self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e));

// Manually confirm all external address candidates.
// TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like a good proposal, do we have an issue for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure we need it — this would change the polkadot protocol, and at the same time we are able to detect external addresses with the heuristic litep2p uses: if the address is reported by 3 or more peers, it's considered external.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be we can create an issue for implementing this heuristic with libp2p backend, but I don't think it's a high priority. And we would need to handle Identify protocol events directly instead of using libp2p mechanism of external address detection, if we go this route.

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nicely done here 🚀

@dmitry-markin dmitry-markin added this pull request to the merge queue Jun 25, 2024
Merged via the queue into master with commit 414a8fc Jun 25, 2024
164 of 166 checks passed
@dmitry-markin dmitry-markin deleted the altonen-upgrade-libp2p branch June 25, 2024 14:19
@jxs
Copy link

jxs commented Jun 25, 2024

great work folks! 🚀 hopefully the upgrade to 0.53 will be easier 😁

TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…ritytech#4198)

This PR introduces custom types / substrate wrappers for `Multiaddr`,
`multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types
like errors and iterators.

This is needed to unblock `libp2p` upgrade PR
paritytech#1631 after
paritytech#2944 was merged.
`libp2p` and `litep2p` currently depend on different versions of
`multiaddr` crate, and introduction of this "common ground" types is
needed to support independent version upgrades of `multiaddr` and
dependent crates in `libp2p` & `litep2p`.

While being just convenient to not tie versions of `libp2p` & `litep2p`
dependencies together, it's currently not even possible to keep `libp2p`
& `litep2p` dependencies updated to the same versions as `multiaddr` in
`libp2p` depends on `libp2p-identity` that we can't include as a
dependency of `litep2p`, which has it's own `PeerId` type. In the
future, to keep things updated on `litep2p` side, we will likely need to
fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of
`/p2p/...` protocol.

With these changes, common code in substrate uses these custom types,
and `litep2p` & `libp2p` backends use corresponding libraries types.
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Upgrade libp2p to 0.52.4, including a fix: 

* Set Kademlia to server mode
(paritytech/substrate#14703)

### TODO
- [x] Fix 3 zombienet tests failing:
  - [x] `zombienet-substrate-0002-validators-warp-sync`
- [ ]
~`zombienet-polkadot-functional-0005-parachains-disputes-past-session`~
The test is also flaky in other PRs and is not required for CI to
succeed.
  - [x] `zombienet-polkadot-functional-0009-approval-voting-coalescing`
- [x] Uncomment and update to the actual libp2p API tests in
[`substrate/client/network/src/protocol/notifications/handler.rs`](https://github.com/paritytech/polkadot-sdk/blob/7331f1796f1a0b0e9fb0cd7bf441239ad9664595/substrate/client/network/src/protocol/notifications/handler.rs#L1009).
- [x] When upgrading `multihash` crate as part of libp2p upgrade to
version v0.19.1, uncomment the conversion code at
https://github.com/paritytech/polkadot-sdk/blob/7547c4942a887029c11cbcfd5103f6d8315ab95c/substrate/client/network/types/src/multihash.rs#L159
- [x] Perform a burn-in.

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Bastian Köcher <git@kchr.de>
sfffaaa pushed a commit to peaqnetwork/polkadot-sdk that referenced this pull request Dec 27, 2024
Upgrade libp2p to 0.52.4, including a fix: 

* Set Kademlia to server mode
(paritytech/substrate#14703)

### TODO
- [x] Fix 3 zombienet tests failing:
  - [x] `zombienet-substrate-0002-validators-warp-sync`
- [ ]
~`zombienet-polkadot-functional-0005-parachains-disputes-past-session`~
The test is also flaky in other PRs and is not required for CI to
succeed.
  - [x] `zombienet-polkadot-functional-0009-approval-voting-coalescing`
- [x] Uncomment and update to the actual libp2p API tests in
[`substrate/client/network/src/protocol/notifications/handler.rs`](https://github.com/paritytech/polkadot-sdk/blob/7331f1796f1a0b0e9fb0cd7bf441239ad9664595/substrate/client/network/src/protocol/notifications/handler.rs#L1009).
- [x] When upgrading `multihash` crate as part of libp2p upgrade to
version v0.19.1, uncomment the conversion code at
https://github.com/paritytech/polkadot-sdk/blob/7547c4942a887029c11cbcfd5103f6d8315ab95c/substrate/client/network/types/src/multihash.rs#L159
- [x] Perform a burn-in.

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Blocked ⛔️
Development

Successfully merging this pull request may close these issues.