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

Perf/randomly drop discovery bucket entry #5966

Merged
merged 6 commits into from
Jul 28, 2023
Merged

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Jul 26, 2023

  • The entry inside the discovery bucket tend to stay fixed and not change. And because we don't do a full dht walk, we are generally limited by the entry within the nodes from the bucket. This causes the percentage of new nodes from neighbour message to stabilize at about 10%.

  • This PR add a random drop when a bucket is full which causes the entry in the bucket to change. This reduces the time to reach 8000 candidate node from 40 minute to 5 without increasing the rate of neighbour request.

  • This is out of spec, but I guess since the density of the bucket based on the XOR distance stays the same, it should be fine. (bucket with lower distance does not get touched much as its much harder to get node to fill it).

  • Run is 20%, 10%, 5%, 2%, no drop, 20%, 10%, 5%, 2%, no drop.
    Screenshot_2023-07-26_14-09-29

Changes

  • Fix a lot of random drop of neighbour message.
  • Add option to randomly drop last node if bucket is full. Default to 5%.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • Optimization

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

Notes on testing

  • Works on ewf.
  • Mainnet can sync fine.

Requires documentation update

  • No

Requires explanation in Release Notes

  • Yes: Improved peer discovery

Remarks

  • Not sure why not just put it at 20% or 10%. but I guess its not great for the network if the entry keep changing maybe.
  • Tried other scheme such as not adding node that is known to be incompatible or drop incompatible peer first. Does not work as effective as just randomly dropping them.

@asdacap asdacap marked this pull request as ready for review July 26, 2023 12:08
@@ -166,25 +168,38 @@ public void ProcessNeighborsMsg(NeighborsMsg? msg)
return;
}

if (_isNeighborsExpected)
if (_lastNeighbourSize + msg.Nodes.Length == 16)
Copy link
Member

Choose a reason for hiding this comment

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

What if its greater than 16? Is that possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory. 16 is enough to fix like 99% of unexpected message though.

Copy link
Member

Choose a reason for hiding this comment

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

I mean should it be:

Suggested change
if (_lastNeighbourSize + msg.Nodes.Length == 16)
if (_lastNeighbourSize + msg.Nodes.Length >= 16)

@asdacap asdacap merged commit 7549b9e into master Jul 28, 2023
61 checks passed
@asdacap asdacap deleted the perf/randomly-drop-bucket branch July 28, 2023 04:39
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