Conversation
dknopik
force-pushed
the
handle-validated-messages
branch
from
02b0b4d to
c9a32e3
Compare
anchor/network/src/network.rs
Outdated
| qbft_manager: Option<Arc<QbftManager>>, | ||
| signature_collector: Option<Arc<SignatureCollectorManager>>, |
There was a problem hiding this comment.
We could create a new crate message_receiver and pass it to the network. Then we'd move the validation and message routing there. It'd make the network less aware of specifics, especially now that it handles only bytes, and hopefully help with the clippy::too_many_arguments - it's probably a sign it's doing too much.
There was a problem hiding this comment.
ooh, sounds good!
this new crate could also then handle filtering of relevant messages - which, as i have realized just now, is not implemented for the signature collector I think.
Thanks!
There was a problem hiding this comment.
What do you think about moving the validation to the receiver? Then, we could have only a channel reporting Reject/Ignore to the Network. For Acceptance, we'd forward the message to the manager, and the Network would receive the result from the Sender.
There was a problem hiding this comment.
It'd reduce copying and moving messages around through channels.
There was a problem hiding this comment.
@diegomrsantos can you please take a look if I implemented it as you meant?
# Conflicts: # anchor/network/src/network.rs
dknopik
force-pushed
the
handle-validated-messages
branch
from
e21f90d to
69d4e27
Compare
|
@jking-aus thanks - before you merge this, lets wait for diego's feedback |
diegomrsantos
left a comment
diegomrsantos
left a comment
There was a problem hiding this comment.
Looks great! Thanks for that. Only two minor comments.
| const RECEIVER_NAME: &str = "message_receiver"; | ||
|
|
||
| /// A message receiver that passes messages to responsible managers. | ||
| pub struct ManagerMessageReceiver<V: ValidatorService + 'static> { |
There was a problem hiding this comment.
nit: should manager be at the end?
There was a problem hiding this comment.
The reason why I put it in front is that it is a receiver, that forwards messages to the managers, not a receiver that is a manager.
But I am also not happy with the name. I just wanted to avoid MessageReceiverImpl at all costs. :D
There was a problem hiding this comment.
Maybe NetworkMessageReceiver or SSVMessageReceiver?
| match SignedSSVMessage::from_ssz_bytes(&message_data) { | ||
| Ok(deserialized_message) => { | ||
| trace!(msg = ?deserialized_message, "SignedSSVMessage deserialized"); | ||
| self.do_validate(&deserialized_message)?; |
There was a problem hiding this comment.
I deleted this fn as it's doing nothing
There was a problem hiding this comment.
Yeah, as noted there will be merge conflicts anyway. So you can remove it then.
3 participants
This PR implements passing the validated messages to the QBFT manager or signature collector.