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

refactor: extract kbuckets into separate structure #1494

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

morph-dev
Copy link
Collaborator

@morph-dev morph-dev commented Sep 28, 2024

What was wrong?

The kbuckets are widely used and they are wrapped in Arc<RwLock<>> for asynchronous access.

This can be dangerous combination as it's easy to hold the kbuckets lock and try to acquire some other lock or do .await. This was the root problem of the #1390 .

How was it fixed?

I wrapped Arc<RwLock<KBucketsTable<NodeId, Node>>> in a structure (that I named SharedKBucketsTable).
Every time lock is acquired, it's held only for the minimum amount of time, during one function call.

This approached is recommended in https://draft.ryhl.io/blog/shared-mutable-state/ , which is referenced from: https://tokio.rs/tokio/tutorial/shared-state#restructure-your-code-to-not-hold-the-lock-across-an-await

I tried to minimize functionality changes. Here are highlights:

  • interested_enrs and batch_interested_enrs are just moved from gossip.rs
  • insert_or_update_discovered_nodes - re-implementation of previous OverlayService::process_discovered_enrs
    • original implementation was handling a case that is not possible so I removed it (for reference, inserted nodes can't be inserted as pending because they are inserted as disconnected)
  • nodes_by_distance - original implementation wasn't working correctly
    • it would call KBucketsTable::nodes_by_distances but it would potentially later filter entries, resulting in returning less results than desired

Note: While this PR is big, I would say that at least 2/3 are test related changes. If desired, I can try to split it up into smaller PRs for easier review.

To-Do

@morph-dev morph-dev self-assigned this Sep 28, 2024
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: looks good I didn't see anything wrong when reviewing. I also ran the PR through hive and all tests pass there as well.

@morph-dev morph-dev merged commit bf54a8a into ethereum:master Oct 2, 2024
9 checks passed
@morph-dev morph-dev deleted the kbuckets_lock branch October 2, 2024 18:25
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.

2 participants