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: minor improvements to available neighbouring peer search #3598

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Nov 21, 2021

Description

  • Removes peers that were previously offline; a connection was re-attempted and fails.
  • Instead of attempting to replace a disconnected peer in the neighbouring and random peer pools, attempt to reconnect to the disconnected peer. If that fails, the peer is replaced.
  • neighbouring peers ordered by last seen (based on @mikethetike 's PR Add Kademlia buckets to comms layer #2817)
  • Xor distance uses u128 (based on @mikethetike 's PR Add Kademlia buckets to comms layer #2817)
  • add cooldown if encountering many failed connection attempts from DHT pool peers
  • add last_seen directly to peer (w/ migration)

Motivation and Context

By removing peers from the peer manager when offline, the number of peers stored on the network should gradually decrease. This will only happen once all/most peers are upgraded.

How Has This Been Tested?

Memorynet
Manually, base node operates normally and peer list reduces (until peer sync pulls more in again from nodes without this change)

@sdbondi sdbondi force-pushed the comms-connectivity-remove-offline-peers branch 5 times, most recently from 45a0cb4 to 434a984 Compare November 22, 2021 05:04
hansieodendaal
hansieodendaal previously approved these changes Nov 30, 2021
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Logic LGTM - not tested

@sdbondi
Copy link
Member Author

sdbondi commented Nov 30, 2021

Going to work on various improvements to the strategy here, setting as draft

@sdbondi sdbondi marked this pull request as draft November 30, 2021 06:00
@sdbondi sdbondi force-pushed the comms-connectivity-remove-offline-peers branch from 434a984 to ff90b86 Compare December 5, 2021 12:16
@sdbondi sdbondi marked this pull request as ready for review December 5, 2021 12:16
@sdbondi sdbondi force-pushed the comms-connectivity-remove-offline-peers branch 8 times, most recently from c067fc6 to fa598a5 Compare December 5, 2021 13:48
@sdbondi sdbondi changed the title fix: remove peers that are reattempted and still offline fix: minor improvements to available neighbouring peer search Dec 5, 2021
@sdbondi sdbondi force-pushed the comms-connectivity-remove-offline-peers branch from fa598a5 to 5ab6083 Compare December 5, 2021 15:23
@sdbondi sdbondi force-pushed the comms-connectivity-remove-offline-peers branch from 5ab6083 to 4937112 Compare December 6, 2021 04:48
* development:
  feat: bad block list for invalid blocks after sync (tari-project#3637)
Copy link
Contributor

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

LGTM, tested by starting to sync a node and seems good

@aviator-app aviator-app bot merged commit e59d194 into tari-project:development Dec 6, 2021
@sdbondi sdbondi deleted the comms-connectivity-remove-offline-peers branch December 7, 2021 05:08
sdbondi added a commit to sdbondi/tari that referenced this pull request Dec 7, 2021
* development:
  feat(consensus)!: add tari script byte size limit check to validation (tari-project#3640)
  fix: minor improvements to available neighbouring peer search (tari-project#3598)
  feat: bad block list for invalid blocks after sync (tari-project#3637)
@sdbondi sdbondi restored the comms-connectivity-remove-offline-peers branch February 3, 2022 05:27
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.

3 participants