-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Handle overlay network events #110
Handle overlay network events #110
Conversation
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.
From a quick glance, it looks good and also fixes issues I saw in #103. So I support this PR
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! A couple suggestions about refactoring that I feel do need to get handled, but can be addressed in a subsequent pr if you'd prefer
@@ -194,114 +172,3 @@ async fn proxy_query_to_state_subnet( | |||
None => Err("No response from state subnetwork".to_string()), | |||
} | |||
} | |||
|
|||
impl PortalnetEvents { |
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'm not sure protocol
is an accurate name for this module anymore after removing this code...
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.
Yeah, what is left is the json-rpc handlers. I created a new jsonrpc
folder and refactored a bit the jsonrpc files.
}) | ||
// Spawn main event handlers | ||
//ToDo: Probably we can find a better way to refactor those event handler initializations. | ||
// Initializing them together with the json-rpc handlers above may be a good idea. |
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 strongly support this. Ideally, when it comes to this refactor, we can package the logic neatly in such a way that it can be shared between this main()
and the subnet main()
used when they are run individually, so we don't need to maintain the same logic in multiple places. I'll create an issue to track this after this pr gets merged
|
||
tokio::spawn(history_events.process_requests()); | ||
} | ||
None => { |
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.
Yeah, it seems like refactoring would also help eliminate some of these weird edge cases we need to handle right now.
pub event_rx: UnboundedReceiver<TalkRequest>, | ||
} | ||
|
||
impl HistoryEvents { |
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'm a little nervous at how similar this and StateEvents
are - besides some log messages, I don't notice any significant differences. Which means as we continue development, any change to one will likely need to be made on the other - and they could fall out of sync very quickly. Could/did you try to implement a common datatype that can be used in each subnetwork? If you want to handle that in a subsequent pr though, that's cool
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 moved the logic for handling the common requests (currently all requests are common) inside the overlay protocol, which probably makes perfect sense because every network implements the overlay protocol. This was actually the idea from the beginning but sometimes I get lost in refactoring :)
9f4643d
to
b58bedc
Compare
What does this PR do?
This is PR following #103.
It adds support for running event handlers for state and history network in parallel.
Any specific implementation changes that would benefit highlighting?
The general idea is to have one main portal network event handler and a network event handler per network.
The main event handler will dispatch via unbounded channels all events to the specific network handlers by checking the
protocol
header in discv5talk_req
.This implementation was tested with the dummy node from the testing framework branch by sending three ping requests, one for every network(state, history) and one for a network we don't support. Here are the results: