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

Lock database state globally and implement state watching #117

Merged
merged 5 commits into from
Feb 4, 2025

Conversation

dknopik
Copy link
Member

@dknopik dknopik commented Jan 28, 2025

Issue Addressed

Before this PR, the in-memory database was a bit spooky: As operations (such as adding/removing validators) touch several DashMaps without locking the whole state, readers might read inconsistent data across these maps. Reasoning about these maps and safe orders in accessing/updating them is kind of hard, especially as the client is still evolving rapidly.

Furthermore, other parts of the client are interested in DB updates, but currently have no way of receiving them except polling regularly.

Proposed Changes

Instead of relying on multiple DashMaps and Atomic*, we now lock the whole NetworkState until we are fully done modifying it. We do this by using tokio::sync::watch, which is basically a Arc<RwLock<T>> with some extra infrastructure for task notification if the value changed. We use this notification feature in validator_store to only load new validators if the state changed, and will use it in network to figure out the subnets we want to subscribe to.

@dknopik dknopik added ready-for-review This PR is ready to be reviewed database labels Jan 28, 2025
@Zacholme7
Copy link
Member

small comments/questions, otherwise lgtm gj

@dknopik dknopik mentioned this pull request Jan 28, 2025
@dknopik dknopik merged commit 5afc8f9 into sigp:unstable Feb 4, 2025
10 checks passed
@dknopik dknopik deleted the global-state-lock branch February 5, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database ready-for-review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants