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

Don't mark peers as offline if there are no existing connections #1763

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Apr 25, 2020

Description

  • Adding a condition that will prevent peers from being marked as
    offline while there are no active connections, in which case, the node
    itself may have lost connection.
  • To clarify the above: "Offline" means that a peer will not be chosen for the
    propagation of messages. Attempting to send a message directly will still
    be attempted. There is existing protection for marking more than 70%
    of neighbouring peers as offline, so this change is an additional
    protection from both sides considering each other offline.
  • Bug fix where peers are displayed as offline in the list-peers
    command, even though logically they are not considered offline.

Motivation and Context

Nodes could mistakenly be marked as offline by a node experiencing loss of connection. This remedies that by requiring at least one active peer connection before marking any peer as offline. Unfortunately, tor takes a while to drop socks connections once the internet is cut off, TODO: look for settings in tor to fine tune this.

How Has This Been Tested?

No unit/integration tests (TODO). Tested locally by starting a base node, switching off wifi and checking that no peers are marked as offline.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Sorry, something went wrong.

Unverified

This user has not yet uploaded their public signing key.
- Adding a condition that will prevent peers from being marked as
  offline while there are no active connections, in which case, the node
  itself may have lost connection.
- To clarify the above: "Offline" means that a peer will not be chosen for the
  propagation of messages. Attempting to send a message directly will still
  be attempted. There is existing protection for marking more than 70%
  of neighbouring peers as offline, so this change is an additional
  protection from both sides considering each other offline.
- Bug fix where peers are displayed as offline in the `list-peers`
  command, even though logically they are not considered offline.
@@ -394,7 +403,6 @@ impl DhtDiscoveryService {
None,
)
.await?;
peer_manager.set_offline(pubkey, false).await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

Offline already set to false in update_peer above

Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

LGTM

@CjS77 CjS77 merged commit 8ae9df6 into development Apr 25, 2020
CjS77 added a commit that referenced this pull request Apr 25, 2020

Verified

This commit was signed with the committer’s verified signature. The key has expired.
CjS77 Cayle Sharrock
** Major Changes from 0.0.9

**** Store and forward

Peers will hold onto message for recipients that are not online and deliver the messages to them when they appear again.

**** OsX package installer

**** Many documentation improvements

**** Ephemeral keys for private messages

This is a big change that preserves privacy on the network but dramatically reduces the amount of traffic peers have to deal with.

**** Emoji Ids

*** Other changes

- The target difficulty for a specified PoW algorithm is included in the block header. This allows the target difficulty
  of any block height to be calculated by only processing the last set of target difficulty samples up to that height.
- Don't mark peers as offline if there are no existing connections (#1763)
- Add UTXO selection strategy for large txs
- Base node: Dynamically determine build version (#1760)
- Include random peers for liveness ping (#1753)
- RandomX - Version Update (#1754)
- Add generic debug log function to FFI (#1752)
- Lots of logging improvements
- Added list-transactions and cancel-transaction commands (#1746)
- ASCII table output for list-peers and list-connections (#1709)
- Improve Difficulty adjustment manager
- Modular configuration via ConfigLoader and ConfigPath traits
- Fix chain monitoring bug in Transaction Service (#1739)
- Empty Emoji String Bug Fix  (#1736)
- Coin-split base node cli command
- Complete the basic OSX pkg build
- Perform reorgs only on stronger tip accumulated difficulties
- Use filesystem storage for dht.db on libwallet (#1735)
- Fix duplicate message propagation (#1730)
- Introduced accumulated difficulty validators to allow different rules for testing and running running a base node. -
- Changes to peer offline handling (#1716)
- Update Transaction cancellation to work for Inbound and Outbound Txs
- Added oneshot reply to outbound messaging (#1703)
- Add transaction stress test command to CLI
- Implemented basic `make-it-rain` command
- Fix MmrCache rewind issue
- Use ephemeral key for private messages (e.g Discovery) (#1686)
- Limit orphan pool size
- Added a function to list UTXOs in the console (#1678)
- Prevent adding yourself as a peer (#1665)
- Update transaction weights (#1661)
- Fix block period calculation
- Validators will now check the weight of a block when doing validation (#1648)
- Cleaned up duplicate code from the Blockchain db
- The ban peer log will now supply n reason why the peer was banned (#1638)
@sdbondi sdbondi deleted the sb-comms-offline-no-connection branch May 4, 2020 08:51
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.

None yet

2 participants