Refactor DHT node #25

NamsooCho · 2018-02-26T09:27:43Z

No description provided.

kpp · 2018-02-26T09:38:00Z

src/toxcore/dht_new/client.rs

+    /// relay NatPingRequest/NatPingResponse to target peer
+    pub fn relay_nat_ping_packet(&self, addr: &SocketAddr, request: DhtRequest) -> IoFuture<()> {
+        let packet = DhtPacket::DhtRequest(request);
+        Box::new(self.tx.clone() // clone tx sender for 1 send only


Create method send_to as method send but with explicit addr-argument. Method send should use send_to inside it passing self.addr as addr.

kpp · 2018-02-26T09:51:49Z

examples/dht_node_new.rs

+        // Create a channel for this peer
+        let (tx, rx) = mpsc::unbounded();
+        // Add or update an entry for this `Peer` in the connected client hashmap.
+        let client = node.borrow_mut().add_client(&addr, &packet, tx);


replace tx with tx_sink_c. You don't need to create let (tx, rx) = mpsc::unbounded();, pass tx into add_client, create let writer = rx future. Just use tx_sink_c.

kpp · 2018-02-26T09:54:38Z

examples/dht_node_new.rs

+        .build()
+    ;
+
+    let relay = rx_sink


rename relay -> writer

coveralls · 2018-02-26T09:55:15Z

Pull Request Test Coverage Report for Build 388

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
44 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-1.5%) to 95.62%

Files with Coverage Reduction	New Missed Lines	%
toxcore/dht_new/codec.rs	3	93.18%
toxcore/dht_new/kbucket.rs	11	97.13%
toxcore/dht_new/packet.rs	30	95.26%

Totals
Change from base Build 377:	-1.5%
Covered Lines:	4082
Relevant Lines:	4269

💛 - Coveralls

kpp · 2018-02-26T10:32:52Z

src/toxcore/dht_new/dht_node.rs

+    }
+
+    /// add new client entry in connected_clients hashmap
+    pub fn add_client(&mut self, addr: &SocketAddr, packet: &Option<DhtPacket>, tx: Tx) -> Client {


Do you actually need to add client? To create it? It worth to keep clients in tcp server, because TCP connection is stateful and we actually need to keep tx for each client to send the data into appropriate TCP connection linked to this client. On the other hand, we do have 1 UDP connection and we send data through it with the only one difference: addr arg.

Good catch.
I was missing it.
UDP connection doesn't need to be saved.

kpp · 2018-02-26T10:33:12Z

src/toxcore/dht_new/dht_node.rs

+                }
+                client.clone()
+            } else {
+                let precomputed_key = encrypt_precompute(&pk, &self.sk);


Calling encrypt_precompute only once is a good performance optimization, but do we really need this optimization for now? We may do this optimization next PR

kpp · 2018-02-26T10:36:39Z

examples/dht_node_new.rs

+        // Add or update an entry for this `Peer` in the connected client hashmap.
+        let client = node.borrow_mut().add_client(&addr, &packet, tx);
+
+        let _ = node.borrow_mut().handle_packet(&(addr, packet.unwrap()), client);


Why do you pass &(addr, packet) by ref? Try to pass values by value.

kpp · 2018-02-26T10:40:55Z

src/toxcore/dht_new/mod.rs

 pub mod kbucket;
 pub mod packed_node;
 pub mod codec;
+pub mod dht_node;


Does it have to be named as dht_node while the next line is client? Shouldn't it be server?

kurnevsky · 2018-02-27T06:10:09Z

src/toxcore/dht_new/client.rs

+    /// socket address of peer
+    pub addr: SocketAddr,
+    /// tx split of channel to send packet to this peer via udp socket
+    pub tx: Tx,


I would say the server struct should store tx but not the client since it's shared between all clients. And the same for send/send_to methods.

kurnevsky · 2018-02-27T06:11:17Z

src/toxcore/dht_new/server.rs

+                if let DhtRequestPayload::NatPingResponse(nat_ping_resp_payload) = packet.get_payload(&alice.sk).unwrap() {
+                    assert_eq!(nat_ping_resp_payload.id, nat_req.id);
+                } else {
+                    panic!("can not occur");


unreachable! :)

kurnevsky · 2018-02-27T06:16:34Z

src/toxcore/dht_new/server.rs

+            DhtPacket::NodesRequest(nodes_request) => nodes_request.pk,
+            DhtPacket::NodesResponse(nodes_response) => nodes_response.pk,
+            DhtPacket::DhtRequest(dht_request) => dht_request.spk,
+            _ => unreachable!("Received packet is not a DhtPacket"),


Actually it's not unreachable because it depends on how this method is called, so panic is preferable in such cases.

But why we can't just get pk when we need it from the packet directly?

But why we can't just get pk when we need it from the packet directly?

How?

It means that we should get it only after matching on packet kind but not before. So we can try to move the client inside the server and get exact client by pk from packet in each handle_* method if we need it. Not sure how exact it will look like though, but perhaps it worth to try.

Good Idea.
But I would like to try this idea on next PR.:)

kurnevsky · 2018-02-27T06:28:37Z

examples/dht_node_new.rs

+    let handler = stream.for_each(move |(addr, packet)| {
+        let tx_c = tx.clone();
+        let pk = Server::get_pk(&packet.clone().unwrap());
+        let client = node.borrow_mut().create_client(&addr, pk, tx_c);


It seems to me that server should take only packet and maybe socket address but not the whole client. It should manage clients inside so the user of server api wouldn't even know about it.

kpp · 2018-02-27T07:19:08Z

src/toxcore/dht_new/server.rs

+}
+
+#[cfg(test)]
+mod test {


mod tests

kpp · 2018-02-27T09:01:38Z

src/toxcore/dht_new/server.rs

+        }
+    }
+
+    // handle_ping_resp()


You forgot to mark this function as #[test]

kpp · 2018-02-28T06:22:41Z

src/toxcore/dht_new/client.rs

+            pk: pk,
+            addr: addr,
+            precomputed_key: precomputed_key,
+            ping_id: 0xFFFFFFFFFFFFFFFF,


Because, can't set ping_id of Client to desired value.
So, can't check in test code.

But why did you chose such a value? Why not 0?

As you can see at https://github.com/NamsooCho/tox-1/blob/7293bfb67bfd02087f4463a438c2a64fcf01df1a/src/toxcore/dht_new/server.rs#L217-L223

0 is checked

https://github.com/NamsooCho/tox-1/blob/7293bfb67bfd02087f4463a438c2a64fcf01df1a/src/toxcore/dht_new/server.rs#L466-L468

From implementing perspective, Client is useful for containing Tx(mpsc), SocketAddr, pk, ping_id,...
Current kbucket contains only SocektAddr, PublicKey.
Maybe It is better to extend kbucket.

From implementing perspective, Client is useful for containing Tx(mpsc), SocketAddr, pk, ping_id,...

And why does it contain ping_id? It's useless if you have new client for every packet.

Got it!!
Thanks for your comments.
I will try to remove Client struct.

No problem. It's not necessary have to be removed though. Just perhaps if it contains few fields we can just pass them as args.

Good job, guys. @NamsooCho And please, don't init ping_id with 0xffff

kpp · 2018-02-28T06:25:09Z

src/toxcore/dht_new/client.rs

+        let nat_res = NatPingResponse { id: random_u64() };
+        let nat_payload = DhtRequestPayload::NatPingResponse(nat_res);
+        let dht_req = DhtRequest::new(&client.precomputed_key, &client.pk, &client.pk, nat_payload.clone());
+        client.relay_nat_ping_packet(tx, &client.addr, dht_req).wait().unwrap();


Why relay_nat_ping_packet?

It is used both for nat_ping_request/response.

I still do not understand why you used relay word here.

if the rpk is not same as this DHT node, it relay to proper DHT node.

kpp · 2018-02-28T06:29:41Z

src/toxcore/dht_new/server.rs

+    Function to handle incoming packets. If there is a response packet,
+    send back it to the peer.
+    */
+    pub fn handle_packet(&mut self, packet: DhtUdpPacket) -> IoFuture<()>


A DhtUdpPacket is a tuple of (packet, addr). Please destructure it right in arg, so there will be no packet.1 or packet.0 calls.

kpp · 2018-02-28T07:09:45Z

src/toxcore/dht_new/server.rs

+                       "Crypto initialization failed."));
+        }
+
+        let (pk, sk) = gen_keypair();


You have to get this info via args, because we want to start a node with the given keypair.

kpp · 2018-02-28T11:24:44Z

src/toxcore/dht_new/server.rs

+    pub fn handle_packet(&mut self, packet: DhtUdpPacket) -> IoFuture<()>
+    {
+        let pk = Server::get_pk(&packet.1.clone());
+        let client = self.create_client(&packet.0, pk);


Have a look at #33 . You logic is incorrect for BootstrapInfo packet

kpp · 2018-03-05T12:23:01Z

src/toxcore/dht_new/server.rs

+            },
+            DhtPacket::PingResponse(packet) => {
+                debug!("Received ping response");
+                let client = self.get_client(&packet.pk);


It is not 100% for sure the client will be in cache

kpp · 2018-03-05T12:23:06Z

src/toxcore/dht_new/server.rs

+            },
+            DhtPacket::NodesResponse(packet) => {
+                debug!("Received NodesResponse");
+                let client = self.get_client(&packet.pk);


It is not 100% for sure the client will be in cache

kpp · 2018-03-05T12:23:23Z

src/toxcore/dht_new/server.rs

+                    },
+                    Ok(DhtRequestPayload::NatPingResponse(pl)) => {
+                        debug!("Received nat ping response");
+                        let client = self.get_client(&dr.spk);


It is not 100% for sure the client will be in cache

kpp · 2018-03-06T00:58:46Z

src/toxcore/dht_new/server.rs

+        Client::new(precomputed_key, pk, addr.clone(), self.tx.clone())
+    }
+    /// get client from cache
+    pub fn get_client(&self, pk: &PublicKey) -> Option<Client> {


create_client const you almost zero CPU. Please, don't precache clients for now.

send PingRequest to peer A with ping_id = 0x01.
send PingRequest to peer B with ping_id = 0x02.
received PingResponse from peer B -> handle_packet() need client.

kpp · 2018-03-06T08:31:37Z

src/toxcore/dht_new/client.rs

+
+
+/*!
+Hold peer info. connected.


kpp

Approve for now. There is a lot of TODOs, but the seems like a good start

kpp reviewed Feb 26, 2018

View reviewed changes

examples/dht_node_new.rs Outdated

.build()

;

let relay = rx_sink

Copy link

Member

kpp Feb 26, 2018

Choose a reason for hiding this comment

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

rename relay -> writer

kpp reviewed Feb 26, 2018

View reviewed changes

kurnevsky reviewed Feb 27, 2018

View reviewed changes

kpp reviewed Feb 27, 2018

View reviewed changes

src/toxcore/dht_new/server.rs Outdated

}

#[cfg(test)]

mod test {

Copy link

Member

kpp Feb 27, 2018

Choose a reason for hiding this comment

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

mod tests

kpp reviewed Feb 27, 2018

View reviewed changes

src/toxcore/dht_new/server.rs

}

}

// handle_ping_resp()

Copy link

Member

kpp Feb 27, 2018

Choose a reason for hiding this comment

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

You forgot to mark this function as #[test]

This was referenced Feb 27, 2018

Add dht_node_new.rs #15

Closed

refactor dht_node #6

Closed

kpp reviewed Feb 28, 2018

View reviewed changes

tox-rs deleted a comment from NamsooCho Feb 28, 2018

kpp reviewed Feb 28, 2018

View reviewed changes

kpp reviewed Mar 5, 2018

View reviewed changes

kpp reviewed Mar 6, 2018

View reviewed changes

NamsooCho added 4 commits March 6, 2018 10:23

refactor(dht): add client.rs

06a52cf

refactor(dht): add server.rs

70a9cad

refactor(dht): add struct derivation

5b8453e

refactor(dht): add get_node()

3cb8b1a

NamsooCho added 5 commits March 6, 2018 10:23

refactor(dht): add server, client module

b86d477

refactor(dht): add test code

5a93409

refactor(dht): move impls to super

ae19b5d

refactor(dht): add dht_node_new.rs

252b263

refactor(dht): not use get_client()

f51b1bb

kpp reviewed Mar 6, 2018

View reviewed changes

src/toxcore/dht_new/client.rs Outdated

/*!

Hold peer info. connected.

Copy link

Member

kpp Mar 6, 2018

Choose a reason for hiding this comment

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

wtf

refactor(dht): rewords comments in client.rs

a7adc64

kpp approved these changes Mar 6, 2018

View reviewed changes

kurnevsky approved these changes Mar 6, 2018

View reviewed changes

kpp merged commit 09fb4ca into tox-rs:master Mar 6, 2018

Refactor DHT node #25

Refactor DHT node #25

Uh oh!

Conversation

NamsooCho commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 388

💛 - Coveralls

Uh oh!

kpp Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpp Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpp Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpp Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Feb 26, 2018 •

edited

Loading

kpp Feb 26, 2018 •

edited

Loading

kpp Feb 28, 2018 •

edited

Loading

kpp Mar 1, 2018 •

edited

Loading

kpp Feb 28, 2018 •

edited

Loading

kpp Feb 28, 2018 •

edited

Loading

kpp Feb 28, 2018 •

edited

Loading

NamsooCho Mar 6, 2018 •

edited

Loading