MAID-1675 Accumulate messages in the first hop #1200
Conversation
r? @Fraser999 |
@afck: no appropriate reviewer found, use r? to override |
@@ -148,12 +156,12 @@ impl DirectMessage { | |||
#[derive(RustcEncodable, RustcDecodable)] | |||
pub struct HopMessage { | |||
/// Wrapped signed message. | |||
content: SignedMessage, | |||
pub content: SignedMessage, |
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.
Were these fields made pub
mainly to allow destructuring? I guess I'd favour leaving these private if possible (e.g. add a wrapper HopMessage::add_group_list()
if that's the only mutating call we make to member fields).
If these remain pub
, I guess the content()
getter can be removed?
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.
No, to avoid having to clone messages. So the alternative would be to make everything private again and add into_signed_message(self)
. Not sure whether that's preferable.
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.
Nah - no probs - I'll leave as is, but remove the content()
getter.
let signed_bytes = try!(serialise(&self.content)); | ||
for (pub_id, sig) in &self.signatures { | ||
if !sign::verify_detached(sig, &signed_bytes, pub_id.signing_public_key()) { | ||
return Err(RoutingError::FailedSignature); |
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 guess this error variant might need more info to become useful later (e.g. at least the PublicId
which failed)?
Also, I need to finish understanding the rest of this PR, but at the moment, it looks like a single malicious node could block the entire group from sending a message via any route by just providing a bad signature from itself? I'm thinking that this function maybe needs to return the number of valid signatures (so we can check quorum are good), or make this function aware of how to calculate quorum, or augment this function with another which takes into account quorum, or (probably best solution) disallow invalid signatures from being added to self.signatures
?
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.
Yes, you're right, a bad signature should not stop us from handling the message.
self.grp_lists.push(group_list); | ||
} | ||
|
||
/// Adds the given signature, if it is valid and new. |
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 we should validate the signature here, but it looks like we don't? Or maybe that would be best done in SignatureAccumulator::add_signature()
and SignatureAccumulator::add_message()
, in which case this comment should maybe make it clear the signature has already been validated?
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.
The only concern I have with validating here is that we serialise the whole message for every signature we add, whereas if we validate later, we can serialise it once and then check all the signatures.
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.
So you'd prefer something more like SignatureAccumulator::remove_if_complete()
to be changed to do:
if !msg.is_fully_signed() {
return None;
}
msg.remove_invalid_signatures()
if !msg.is_fully_signed() {
return 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.
You mean because otherwise we do serialise it every time we add a signature?
That's a good point. And maybe that's all premature optimisation. I'd say go with the more elegant solution, if in doubt. We can optimise later.
|
||
/// Returns whether there are enough valid signatures from the sender. | ||
pub fn is_fully_signed(&self) -> bool { | ||
if self.content.src.is_client() { |
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.
Why do we have this separate check (with a more stringent condition requiring len() == 1
) for clients?
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.
You're right, it should be only one signature for a single sender, too.
content: content, | ||
}) | ||
/// Returns a `DirectMessage::MessageSignature` for this message. | ||
pub fn to_signature(&self, sign_key: &sign::SecretKey) -> Result<DirectMessage, RoutingError> { |
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 guess since "sign" is a verb, I'd have opted for secret_key
or signing_key
here, but not a big deal.
use std::collections::hash_map::Entry; | ||
use std::time::Instant; | ||
|
||
/// Time (in seconds) after which a joining node will get dropped from the map of joining nodes. |
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.
Copy-paste error?
debug!("{:?} - Unhandled direct message: {:?}", | ||
self, | ||
direct_message); | ||
msg @ DirectMessage::BootstrapIdentify { .. } | |
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.
Ahhhh - finally :)
|
||
let raw_bytes = try!(self.to_hop_bytes(signed_msg.clone(), route, new_sent_to.clone())); | ||
let send_sig = | ||
sent_by_us && hop == self.name() && src.is_group() && |
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.
sent_by_us
already checks for hop == self.name()
.
Continued in #1213 |
No description provided.