-
Notifications
You must be signed in to change notification settings - Fork 114
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: split census into multiple types #1510
Conversation
683ff1d
to
aa32159
Compare
Are you planning to implement #1501 ? |
Yes, in future PR. Don't know in what exact form yet (mine, Piper's, both and compare, something else). |
Sounds good I will review the PR after the meeting 🫡 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, I noticed one thing that probably shouldn't be public but other then that things look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! just a few nits to consider
tokio::select! { | ||
peer = history_network.peer_to_process(), if subnetworks.contains(&Subnetwork::History) => { | ||
history_network.process_peer(peer).await; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit is it worth adding an else
here with a panic
/error!
? maybe not bc it'll get a bit convoluted, but then if we don't need an else
do we need this if
? there shouldn't be a case where we try to process a peer from an unactivated subnetwork... but this is very much a nit, fine to leave as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might be misunderstanding.
The if
is not part of the regular code flow, but the special syntax for tokio::select!
(more details here), and there isn't an option for else
.
Unless you were thinking about else
case of the tokio::select!
statement, but then the comment doesn't make sense because code would already panic if else
is not present and code execution would go there.
With the current implementation, this can happen if no subnetworks were provided in the init
function, and now I added a check for that as well.
The if
statements are needed with the current implementation because peer_to_process()
would return None
for uninitialized networks, forcing them to re-initialize and we don't want that for non-enabled subnetworks.
The original implementation was using selectors like this:
Some(Ok(known_enr)) = self.history.peers.next() => { ... }
(basically ignoring those None
values) and re-initialization would be triggered when get_interested_enrs()
would fail, causing that request to take much longer but still return empty set of enrs.
I believe that re-initialization should be detected and done as part of the regular census maintenance (and not be triggered by get_interested_enrs
), and this is the cleanest way I found to do it.
I'm not sure if my explanation is clear. I think good understanding of what tokio::select!
does under the hood is very important here, and I'm not sure how well I was able to explain my reasoning around it.
What was wrong?
It's difficult to add features (e.g. #1501) to the Census in its current form.
Also, getting peers to gossip content to doesn't have to be asynchronous and as such, doesn't have to go over separate channel.
How was it fixed?
Split the census into multiple components, each responsible for small part:
Peers
- contains all peers (behind RwLock)Network
- contains and propagates calls toPeers
Network
) and executing themNew design allows
Peers
andNetwork
(and easilyCensus
) to be cloneable and shareable.This PR shouldn't drastically change the behavior of the census, but it should make adding features easier in the future.
To-Do