Mixnet message handle #435

al8n · 2023-09-25T06:06:58Z

Refactor the mixnet fan-in. There are no locks anymore, only one message handle thread. All messages will be sent to the message thread handler, and the message thread will handle send and retry logic.

mixnet/node/src/lib.rs

zeegomo · 2023-09-25T09:21:34Z

mixnet/node/src/lib.rs

+                if msg.retry_count < self.config.max_retries {
+                    msg.retry_count += 1;
+                    return Ok(Some(msg));
+                }
+
+                tracing::error!(
+                    "failed to forward msg to {}: reach the maximum retries",
+                    msg.target,
+                );
+                Err(MixnetNodeError::Protocol(ProtocolError::ReachMaxRetries(
+                    self.config.max_retries,
+                )))


we have this check twice at the beginning of the function

zeegomo · 2023-09-25T09:28:32Z

mixnet/node/src/lib.rs

+        if let std::collections::hash_map::Entry::Vacant(e) = self.connections.entry(msg.target) {
+            match TcpStream::connect(msg.target).await {
+                Ok(tcp) => {
+                    e.insert(tcp);
+                }
+                Err(e) => {
+                    tracing::error!("failed to connect to {}: {e}", msg.target);
+                    return Ok(Some(msg));
+                }
+            }
+        }
+
+        let tcp = self.connections.get_mut(&msg.target).unwrap();


let tcp = self.connections.entry(msg.target).or_insert_with(|| {TcpStream::connect(...)}).

This seems cannot be done by this way, TcpStream::connect is an async fn.

my bad, you're right!

zeegomo · 2023-09-25T09:32:01Z

mixnet/node/src/lib.rs

+    /// Send a message to the remote node,
+    /// return Ok(Some((Duration, Message))) if the message is not sent and the error is retryable
+    /// return Ok(None) if the message is sent successfully
+    /// return Err(e) if the message is not sent and the error is not retryable
+    async fn send(&mut self, mut msg: TargetedMessage) -> Result<Option<TargetedMessage>> {


This is hard to read even with a doc comment. I would suggest we create a new enum with more descriptive variantas

zeegomo · 2023-09-25T09:33:03Z

mixnet/node/src/lib.rs

+                            Ok(Some(msg)) => {
+                                let _ = self.message_tx.send(msg);
+                            }


This is an example of why the signature of the function it's hard to read, also, a comment explaining this re-queues the message would be helpful, as it's not explicit where the other half of the channel is

…omos-node into mixnet-message-handle

zeegomo · 2023-09-25T11:03:59Z

mixnet/node/src/lib.rs

                    }
                }
+            }
+            Err(e) => self.handle_connection_failure(e, target, body, retry_count),


nit: handle_connection_failure and handle_retry are quite similar, can we reduce duplication?

One idea could be to have something like this but you can get creative!

match self.get_connection(target).await.map(|..| body.write(tcp)) { // async map exists somewhere Err(e) if matches!(err.kind(), ...) | Ok(Err(e)) if e.kind() != ErrorKind::Unsupported => { // do the retry with a single shared function } Ok(Ok(_)) => happy case // maybe other cases }

Also, I don't think we need to make Message as enum. I guess we can leave it as it was.

mixnet/node/src/lib.rs

youngjoon-lee · 2023-09-25T15:31:08Z

mixnet/node/src/lib.rs

+    async fn handle_msg(&mut self, msg: Message) {
+        self.send(msg.target, msg.body, msg.retry_count).await
+    }
+
+    async fn send(&mut self, target: SocketAddr, body: Body, retry_count: usize) {


Since we have the Message struct, what do you think about having only the send function which accepts a Message? Also, I think we can simplify the signature of the handle_retry function below in the same way.

I have changed the signature, but do not know why GitHub does not show the changes.

mixnet/node/src/lib.rs

youngjoon-lee

It looks like this works as intended, but I think some things can be polished.

mixnet/node/src/lib.rs

youngjoon-lee · 2023-09-26T02:41:50Z

mixnet/node/src/lib.rs

+pub struct Message {
+    target: SocketAddr,
+    body: Body,
+    retry_count: usize,
+}


In the mixnet-related crates, I would like to suggest to have clearly separated terminologies for 'message' and 'packet'. As we've seen so far, we call the object, that the mixnet end-user wants to send, "message". The message is splitted into "packet"s by the mixclient, and these packets are sent to mixnodes. The mixnode forwards packets to other mixnodes, not messages (Only one additional case is that the mixnode also forwards "final payloads" to the destination mixnode).

That is, we don't need to use the terminology "message" in the mixnode layer. In this PR, these two or three terminologies are mixed. This will cause confusions in the future.

Yeah, the thread is PacketHandler but the struct is "message", which is misleading. I change all "message" to "packet".

zeegomo · 2023-09-26T09:24:34Z

mixnet/node/src/lib.rs

            .await
-            .map_err(Into::into);
+            .map_err(From::from)


nit: Ok(body.write(...).await?) should also work

zeegomo · 2023-09-26T09:28:24Z

mixnet/node/src/lib.rs

+                    }
+                },
+                _ = tokio::signal::ctrl_c() => {
+                    tracing::info!("Shutting down packet forwarder thread...");


nit: it's not a thread, it's a task

zeegomo · 2023-09-26T09:28:29Z

mixnet/node/src/lib.rs

+                        self.send(pkt).await;
+                    } else {
+                        // Channel closed, we should shutdown the packet forwarder thread
+                        tracing::info!("Channel closed, shutting down packet forwarder thread...");


zeegomo · 2023-09-26T09:29:49Z

mixnet/node/src/lib.rs

+                    if let Some(pkt) = pkt {
+                        self.send(pkt).await;
+                    } else {
+                        // Channel closed, we should shutdown the packet forwarder thread


nit: not sure it's possible since PacketForwarder internally stores the sending half

al8n added the mixnet label Sep 25, 2023

al8n added this to the Mixnet milestone Sep 25, 2023

al8n requested review from danielSanchezQ, youngjoon-lee and zeegomo September 25, 2023 06:06

al8n self-assigned this Sep 25, 2023

al8n changed the base branch from master to mixnet-error September 25, 2023 06:07

Base automatically changed from mixnet-error to master September 25, 2023 06:21

finish refactor message sending model

f6476e8

al8n force-pushed the mixnet-message-handle branch from c35ec81 to f6476e8 Compare September 25, 2023 06:30

remove unused dependency

dd1acf1

youngjoon-lee reviewed Sep 25, 2023

View reviewed changes

mixnet/node/src/lib.rs Outdated Show resolved Hide resolved

mixnet/node/src/lib.rs Outdated Show resolved Hide resolved

mixnet/node/src/lib.rs Outdated Show resolved Hide resolved

change name and do not retry on connection failure

49e9dac

zeegomo requested changes Sep 25, 2023

View reviewed changes

Refactor retry and message handle

5845531

al8n requested review from youngjoon-lee and zeegomo September 25, 2023 10:45

al8n added 3 commits September 25, 2023 18:45

Merge branch 'master' into mixnet-message-handle

14f8996

add missing await

e509317

Merge branch 'mixnet-message-handle' of https://github.com/logos-co/n…

d69e7cb

…omos-node into mixnet-message-handle

zeegomo reviewed Sep 25, 2023

View reviewed changes

al8n added 2 commits September 25, 2023 19:26

remove duplicated code

0d55642

change message back to a struct

e76019b

al8n requested a review from zeegomo September 25, 2023 11:35

fix clippy

a0826b6

youngjoon-lee reviewed Sep 25, 2023

View reviewed changes

cleanup code

97acc99

al8n requested a review from youngjoon-lee September 25, 2023 17:15

Merge branch 'master' into mixnet-message-handle

595c5cf

youngjoon-lee approved these changes Sep 26, 2023

View reviewed changes

rename Message to Packet

7cade36

zeegomo approved these changes Sep 26, 2023

View reviewed changes

al8n added 2 commits September 26, 2023 18:11

cleanup

0f8a3bd

Merge branch 'master' into mixnet-message-handle

739a847

al8n merged commit 8ea341d into master Sep 26, 2023
11 checks passed

al8n deleted the mixnet-message-handle branch September 26, 2023 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mixnet message handle #435

Mixnet message handle #435

al8n commented Sep 25, 2023

zeegomo Sep 25, 2023

zeegomo Sep 25, 2023

al8n Sep 25, 2023

zeegomo Sep 25, 2023

zeegomo Sep 25, 2023

zeegomo Sep 25, 2023

zeegomo Sep 25, 2023 •

edited

Loading

youngjoon-lee Sep 25, 2023

youngjoon-lee Sep 25, 2023

al8n Sep 26, 2023

youngjoon-lee left a comment

youngjoon-lee Sep 26, 2023 •

edited

Loading

al8n Sep 26, 2023

zeegomo Sep 26, 2023

zeegomo Sep 26, 2023

zeegomo Sep 26, 2023

zeegomo Sep 26, 2023

Mixnet message handle #435

Mixnet message handle #435

Conversation

al8n commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeegomo Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngjoon-lee left a comment

Choose a reason for hiding this comment

youngjoon-lee Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeegomo Sep 25, 2023 •

edited

Loading

youngjoon-lee Sep 26, 2023 •

edited

Loading