-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
I'm wondering if we can already remove support for the fallback noise ix config with this PR? It would take out one negotiation round-trip from the connection setup. |
As far as I can tell, the spec compliant handshake was introduced back in May with #6064 and should thus be supported by the majority of nodes. With that in mind, I don't see why not to remove the legacy fallback. Is there something I am missing? |
Maybe not, I'm just never quite up-to-date w.r.t. which versions are running where and I think it is not uncommon for there to be a few months between a merged substrate PR and a polkadot deployment that includes these changes, so I better ask twice. If @tomaka also thinks we can remove it, I will go ahead and do it! |
@@ -368,7 +397,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> { | |||
|
|||
// Add external addresses. | |||
for addr in ¶ms.network_config.public_addresses { | |||
Swarm::<B, H>::add_external_address(&mut swarm, addr.clone()); | |||
Swarm::<B, H>::add_external_address(&mut swarm, addr.clone(), AddressScore::Infinite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the patch for #7518.
According to this comment, we have already broken compatibility with nodes that don't have #6064, and so we can remove the legacy noise handshake. |
Could you also update the burn-in PR? |
Of course, done in paritytech/polkadot@4020626. |
The "request answer time", in other words the time before receiving a response to a request that the node has sent out, has been divided by two, as predicted. The "request serving time", which is the other way around, has also been divided by two, and I don't understand why. Looking at the source code, this measures the time elapsed between Since nodes only perform one concurrent request at a time, a consequence of the serving time being reduced is that more requests are being served per second, and the bandwidth usage increased. The number of failed connection attempts has increased, maybe because of the legacy Noise handshake. It is unclear whether this is caused by older nodes. |
This is indeed surprising. I compared the First off, to proof that the above phenomenon is not just unrelated to this pull request and instead related to a change on When comparing the 95th percentile instead of the median the difference becomes less obvious: With that in mind, a wild guess from me would be: Given that the handshake time decreased, peers can ask for blocks more frequently. Asking for blocks more frequently implies smaller replies which in turn implies shorter request handling time. |
But the handshake time hasn't decreased for requests emitted by remotes. |
Maybe I'm missing something, but if we have the inbound request handling like so
i.e. a single round-trip, isn't it expected that this is quicker than
? |
Oh, I guess it is because other nodes don't use this branch yet that this is unexpected. |
Following up on the increased dialing attempt errors, I am not sure this is related to this pull request. Again comparing Query: Looking at the dialing attempt errors of the canary node over the last 30 days, this issue seems to have been introduced earlier this month around the 19th of November: Query: |
Let's merge this PR, then? |
bot merge |
Trying merge. |
* CI: build docs after test; publish docs after build (paritytech#7591) docs time test/build success on master pub * node-template: add aura to light block import pipeline (paritytech#7595) added aura to block import pipeline * Fix notifications sometimes not being sent (paritytech#7594) * Fix notifications sometimes not being sent * Add comment * Bump rpassword from 4.0.5 to 5.0.0 (paritytech#7597) Bumps [rpassword](https://github.com/conradkleinespel/rpassword) from 4.0.5 to 5.0.0. - [Release notes](https://github.com/conradkleinespel/rpassword/releases) - [Commits](conradkleinespel/rpassword@v4.0.5...v5.0.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remove std feature flags for assert macros (paritytech#7600) * remove std feature flags for assert macros * re-add note about availability in no_std envs * Add small header cache (paritytech#7516) * Remove header query * Header cache * Fix potential race issue * Simplify status query * Inform sync explicitly about new best block (paritytech#7604) * Inform sync explicitly about new best block Instead of "fishing" the new best block out of the processed blocks, we now tell sync directly that there is a new best block. It also makes sure that we update the corresponding sync handshake to the new best block. This is required for parachains as they first import blocks and declare the new best block after being made aware of it by the relay chain. * Adds test * Make sure async stuff had time to run * Bump directories from 2.0.2 to 3.0.1 (paritytech#7609) Bumps [directories](https://github.com/soc/directories-rs) from 2.0.2 to 3.0.1. - [Release notes](https://github.com/soc/directories-rs/releases) - [Commits](https://github.com/soc/directories-rs/commits) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove `RpcMetrics` weirdness (paritytech#7608) * Remove `RpcMetrics` weirdness The metrics was returning an error when prometheus was not given. This was a really weird setup, especially when compared to all other metrics that just do nothing if there is no registry. * Fix browser build * Upgrade to libp2p-0.31. (paritytech#7606) * Upgrade to libp2p-0.31. * Address line width. * Add generous incoming connection limit. * Remove old noise configuration. * Add Key Subcommand to node-template (paritytech#7615) * Forward storage changes in manual seal (paritytech#7614) This prevents nodes from executing the same block 2 times. * chore/error: remove from str conversion and add deprecation notificat… (paritytech#7472) * chore/error: remove from str conversion and add deprecation notifications * fixup changes * fix test looking for gone ::Msg variant * another test fix * one is duplicate, the other is not, so duplicates reported are n-1 * darn spaces Co-authored-by: Andronik Ordian <[email protected]> * remove pointless doc comments of error variants without any value * low hanging fruits (for a tall person) * moar error type variants * avoid the storage modules for now They are in need of a refactor, and the pain is rather large removing all String error and DefaultError occurences. * chore remove pointless error generic * fix test for mocks, add a bunch of non_exhaustive * max line width * test fixes due to error changes * fin * error outputs... again * undo stderr adjustments * Update client/consensus/slots/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * remove closure clutter Co-authored-by: Bastian Köcher <[email protected]> * more error types * introduce ApiError * extract Mock error * ApiError refactor * even more error types * the last for now * chore unused deps * another extraction * reduce should panic, due to extended error messages * error test happiness * shift error lines by one * doc tests * white space Co-authored-by: Bastian Köcher <[email protected]> * Into -> From Co-authored-by: Bastian Köcher <[email protected]> * remove pointless codec Co-authored-by: Bastian Köcher <[email protected]> * avoid pointless self import Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> * network: don't force send block announcements (paritytech#7601) * Change TRACING_SET to static (paritytech#7607) * change TRACING_SET to static * Update primitives/io/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * modify test with nested spans Co-authored-by: Bastian Köcher <[email protected]> * sc-network: Log outgoing notifications too (paritytech#7624) * Log outgoing notifications too * Update client/network/src/protocol/generic_proto/handler.rs Co-authored-by: Max Inden <[email protected]> Co-authored-by: Addie Wagenknecht <[email protected]> Co-authored-by: Max Inden <[email protected]> * Bump console_log from 0.1.2 to 0.2.0 (paritytech#7623) Bumps [console_log](https://github.com/iamcodemaker/console_log) from 0.1.2 to 0.2.0. - [Release notes](https://github.com/iamcodemaker/console_log/releases) - [Commits](https://github.com/iamcodemaker/console_log/commits) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * `sudo_as` should return a result (paritytech#7620) * Fix wrong value put for pending_opening (paritytech#7633) * Fix wrong value put for pending_opening * Oops, didn't even try compiling it * Rename pallet trait `Trait` to `Config` (paritytech#7599) * rename Trait to Config * add test asserting using Trait is still valid. * fix ui tests * resolve unresolved error nits of paritytech#7617 (paritytech#7631) * handle executor should_panic test better * Revert "reduce should panic, due to extended error messages" This reverts commit c080594. * remove excessive constraints * remove duplicate documentation messages for error variants * reduce T: constraints to the abs minimum * whoops * fewer bounds again Co-authored-by: Bernhard Schuster <[email protected]> * Fix bad state transition with DisabledPendingEnable+OpenDesiredByRemote (paritytech#7638) * Renames of `Trait` to `Config` in README.md, weight templates and few minor ones (paritytech#7636) * manual rename * renamse in README.md * fix template * Fix CI Link Check (paritytech#7639) * fix trigger fingers * more * Update frame/example-offchain-worker/README.md Co-authored-by: Guillaume Thiolliere <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]> * Fix cargo clippy warning in peerset. (paritytech#7641) * Fix cargo clippy warning in peerset. * Update client/peerset/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * Apply suggestions from code review Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Denis Pisarev <[email protected]> Co-authored-by: André Silva <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Alexander Popiak <[email protected]> Co-authored-by: Arkadiy Paronyan <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Roman Borschel <[email protected]> Co-authored-by: Benjamin Kampmann <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Andrew Plaza <[email protected]> Co-authored-by: Addie Wagenknecht <[email protected]> Co-authored-by: Max Inden <[email protected]> Co-authored-by: Shawn Tabrizi <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]> Co-authored-by: jolestar <[email protected]>
* Upgrade to libp2p-0.31. * Address line width. * Add generous incoming connection limit. * Remove old noise configuration.
This PR upgrades substrate to
libp2p-0.31
. Highlights:ls
response encoding formultistream-select
. This will eventually be followed-up by [multistream-select] Re-enable use ofls
for the dialer aka "parallel negotiation". libp2p/rust-libp2p#1847.Swarm
with an "infinite" retention. Thereby closes Addresses passed with--public-addr
get purged after a while #7518.V1Lazy
multistream-select negotiation variant is now interoperable withV1
on the wire and configured to be used exclusively in substrate with this PR, both for transport and substream protocol negotiation ([multistream] Make the lazy variant more interoperable. libp2p/rust-libp2p#1855, [swarm] Permit configuration override for the substream upgrade protocol to use. libp2p/rust-libp2p#1858). This can avoid having the initiator of a connection or substream wait for every single protocol confirmation before it can send protocol data.