Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

feat: use message_id instead of hash for message_filter #2527

Merged
merged 6 commits into from
May 28, 2021

Conversation

maqi
Copy link
Member

@maqi maqi commented May 26, 2021

No description provided.

src/crypto.rs Show resolved Hide resolved
Comment on lines -39 to -40
incoming: LruCache<MessageHash, ()>,
outgoing: LruCache<(MessageHash, XorName), ()>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to filtering outgoing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as each WireMsg will be given a random msg_id, there is no point to filtering outgoing (based on random msg_id).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we filter outgoing previously and if there was a valid reason are we now creating an issue? Or was it always useless?

@@ -466,10 +466,29 @@ async fn handle_message(dispatcher: Arc<Dispatcher>, bytes: Bytes, sender: Socke
};
let _span_guard = span.enter();

let message_type = match WireMsg::deserialize(bytes) {
let wire_msg = match WireMsg::from(bytes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we deserialising the whole message here as the point is not to and just read dst an dif it's us then process it (deserialise) or forward it.
Also msg_id to be read from msg without deserializing the whole message and added to filter / checked we have handled it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WireMsg::from only restore the header part, which contains the msg_id
WireMsg::deserialize or WireMsg::to_message will deserialize the whole message

Comment on lines 45 to 48
pub async fn contains_incoming(&self, msg_id: &MessageId) -> bool {
self.core.lock().await.contains_incoming(msg_id)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly better to have add_to_filter() ->bool if added then cool, if not added then drop message (it's filtered)

src/routing/mod.rs Outdated Show resolved Hide resolved
src/routing/mod.rs Outdated Show resolved Hide resolved
Comment on lines -66 to -67
if self.msg_filter.contains_incoming(&msg) {
trace!("not handling message - already handled: {:?}", msg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not still be the case?

@maqi maqi force-pushed the message_id branch 2 times, most recently from 0029086 to 68859ab Compare May 28, 2021 08:03
@dirvine dirvine merged commit 02dffda into maidsafe:master May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants