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

Fix network issues #448

Merged
merged 2 commits into from
Nov 23, 2021
Merged

Fix network issues #448

merged 2 commits into from
Nov 23, 2021

Conversation

jsdanielh
Copy link
Member

@jsdanielh jsdanielh commented Nov 22, 2021

  • Reduce the number of concurrent dial addresses to 1 in the network,
    which is also the default hence the code won't set any value to it.
    This was detected as the cause of several dials being launch when
    issuing a peer dial and provoking error messages since only one of
    the dial actions succeed while the other fails with messages
    reporting incoming connection errors.
    This fixes Incoming connection failures when a peer is dialed #446.

  • Fix incoming dial failure handling for connection pool behavour.
    Since the recent update of libp2p, this
    change
    introduces
    state to dial request that can be transmitted in
    inject_dial_failure. So there are a couple of cases where the
    function is called but we can ignore the error since the connection
    is still to be completed or is already connected.
    This fixes Dialing a peer sometime results in an error #447.

Pull request checklist

  • All tests pass. Demo project builds and runs.
  • I have resolved any merge conflicts.

Fix incoming dial failure handling for connection pool behavior.
Since the recent update of libp2p, [this
change](libp2p/rust-libp2p#2191) introduces
state to dial request that can be transmitted in
`inject_dial_failure`. So there are a couple of cases where the
function is called but we can ignore the error since the connection
is still to be completed or is already connected.

This fixes #447.
@jsdanielh jsdanielh force-pushed the jsdanielh/network branch 2 times, most recently from f5f27fd to 56b0bc9 Compare November 22, 2021 23:04
@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #448 (56b0bc9) into albatross (03c3c2b) will increase coverage by 0.13%.
The diff coverage is 0.00%.

❗ Current head 56b0bc9 differs from pull request most recent head 3a329d3. Consider uploading reports for the commit 3a329d3 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##           albatross     #448      +/-   ##
=============================================
+ Coverage      29.51%   29.65%   +0.13%     
=============================================
  Files            348      348              
  Lines          33513    33514       +1     
  Branches       15355    15356       +1     
=============================================
+ Hits            9891     9938      +47     
- Misses         15183    15205      +22     
+ Partials        8439     8371      -68     
Flag Coverage Δ
unittests 29.64% <0.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
network-libp2p/src/connection_pool/behaviour.rs 28.06% <0.00%> (+0.14%) ⬆️
network-libp2p/src/network.rs 22.81% <ø> (+0.46%) ⬆️
...rimitives/transaction/src/account/basic_account.rs 41.66% <0.00%> (-4.17%) ⬇️
blockchain/src/blockchain/accounts.rs 53.33% <0.00%> (-2.67%) ⬇️
consensus/src/messages/handlers.rs 23.47% <0.00%> (-2.61%) ⬇️
network-libp2p/src/discovery/behaviour.rs 51.96% <0.00%> (-1.97%) ⬇️
blockchain/src/chain_info.rs 54.90% <0.00%> (-1.97%) ⬇️
primitives/account/src/basic_account.rs 51.88% <0.00%> (-1.89%) ⬇️
network-interface/src/peer_map.rs 39.81% <0.00%> (-1.86%) ⬇️
...imitives/account/src/staking_contract/validator.rs 50.82% <0.00%> (-1.33%) ⬇️
... and 35 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 03c3c2b...3a329d3. Read the comment docs.

Copy link
Contributor

@vlvrd vlvrd left a comment

Choose a reason for hiding this comment

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

LGTM, there's a warning on the network.rs file because the NonZeroU8 import is no longer needed.

Reduce the number of concurrent dial addresses to 1 in the network,
which is also the default hence the code won't set any value to it.
This was detected as the cause of several dials being launch when
issuing a peer dial and provoking error messages since only one of
the dial actions succeed while the other fails with messages
reporting incoming connection errors.

This fixes #446.
@jsdanielh jsdanielh merged commit 3a329d3 into albatross Nov 23, 2021
@jsdanielh jsdanielh deleted the jsdanielh/network branch November 23, 2021 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialing a peer sometime results in an error Incoming connection failures when a peer is dialed
2 participants