Add gossipable trait + relevant logic #45

Sanghren · 2023-09-14T07:24:03Z

Summary

This pull request is an initial attempt to address the issue: #29

Changes

Added traits for Gossipable and Set.
Implemented logic for Gossiper and Handler (first dab at it).
Build rust source from the sdk.proto
Dummy Client implementation (real implementation out of scope of this PR, imho)
Basic Handler implemenation.

ToDos

[ ] More tests
[ ] Logic review (feel like i can improve stuff)

Questions

Test File Structure: Currently, I've included tests within the same file as the logic they're intended to test. Is it better to separate them into their own files?
Dependency Choices: I opted for using the probabilistic-collections crate for its BloomFilter implementation. Is there a preferred alternative that I should consider?
File Placement: I'm uncertain where to place the sdk.proto file and its corresponding Rust-generated file. Could you please provide guidance on this?

Would love to have feedback on this. First dive into avalanche-rs so am not really familiar with it.

richardpringle · 2023-09-15T02:18:47Z

@Sanghren, thanks for the contribution! I took a quick look and I think things look pretty good. I'll leave a couple of comments next week!

rkuris · 2023-09-26T20:43:48Z

core/network/examples/peer_gossip.rs

+        let n = socket.read(&mut buf).await.unwrap();
+
+        // Empty, wait 5 sec before next attempt
+        if n == 0 {


When socket.read returns 0, that means the client has disconnected. You will forever get 0, so this is an infinite loop.

I think if the client disconnects, the read call will error and the program will panic on that unwrap. If wouldn't infinite loop.

That's not true. Example program: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d809a48e2983d4fe94833852a8a28961

Client drops the tcp_stream on line 35, and waits for the server to finish up before exiting. While it's in this state, read() repeatedly returns Ok(0)

richardpringle

Cool! Awesome job if some of those examples work!

I'm happy to keep reviewing your code and give you some Rust-specific pointers, but as @exdx mentioned, it might be tough getting this merged in as the P2P stuff isn't stable yet.

Right now, there's quite a bit of code that could panic in spots where you wouldn't want code panicking.

If you're just looking to learn, I'm happy to help as much as I can, but if it's contributions that you want, it might be a better idea to take a look at something smaller. In either case, I'm here to help!

richardpringle · 2023-09-26T18:07:33Z

core/network/Cargo.toml

@@ -12,25 +12,42 @@ readme = "README.md"

 [dependencies]
 avalanche-types = { path = "../../crates/avalanche-types", features = ["message"] }
+async-trait = { version = "0.1.73", features = [] }


🤔, usually libraries re-export async-trait such that the trait definition remains consistent with the trait implementation.

I know I'll get my answer below, but are we defining traits with the macro or as we implementing traits with the macro? Or both?

richardpringle · 2023-09-26T18:08:01Z

core/network/Cargo.toml

 [dev-dependencies]
 env_logger = "0.10.0"
+mockall = "0.11.4"
+proptest = "1.2.0"


richardpringle · 2023-09-26T18:09:12Z

core/network/Cargo.toml

 random-manager = "0.0.5"
-tokio = { version = "1.32.0", features = ["full"] }
+testing_logger = "0.1.1"


🤔, not sure we need this as it looks like it's unmaintained. I guess I'll see where it's used.

richardpringle · 2023-09-26T18:14:35Z

core/network/examples/peer_gossip.rs

+use std::error::Error;
+use std::hash::Hash;
+use tokio::sync::Mutex;
+use std::sync::Arc;
+use std::time::Duration;
+use async_trait::async_trait;
+use tokio::net::{TcpListener, TcpStream};
+use tokio::io::{AsyncReadExt, AsyncWriteExt};
+use network::p2p::gossip::gossip::{Config, Gossiper};
+use tokio::sync::mpsc::{channel};
+use avalanche_types::ids::Id;
+use network::p2p::client::{AppResponseCallback, Client};
+use network::p2p::gossip::{Gossipable, Set};
+use network::p2p::gossip::handler::{Handler, HandlerConfig, new_handler};
+use network::p2p::handler::Handler as TraitHandler;


Prefer merged imports as it's easier to keep them consistent when using automatic imports from tools like rust-analyzer.

Suggested change

use std::error::Error;

use std::hash::Hash;

use tokio::sync::Mutex;

use std::sync::Arc;

use std::time::Duration;

use async_trait::async_trait;

use tokio::net::{TcpListener, TcpStream};

use tokio::io::{AsyncReadExt, AsyncWriteExt};

use network::p2p::gossip::gossip::{Config, Gossiper};

use tokio::sync::mpsc::{channel};

use avalanche_types::ids::Id;

use network::p2p::client::{AppResponseCallback, Client};

use network::p2p::gossip::{Gossipable, Set};

use network::p2p::gossip::handler::{Handler, HandlerConfig, new_handler};

use network::p2p::handler::Handler as TraitHandler;

use async_trait::async_trait;

use avalanche_types::ids::Id;

use network::p2p::{

client::{AppResponseCallback, Client},

gossip::{

gossip::{Config, Gossiper},

handler::{new_handler, Handler, HandlerConfig},

Gossipable, Set,

},

handler::Handler as TraitHandler,

};

use std::{error::Error, hash::Hash, sync::Arc, time::Duration};

use tokio::{

io::{AsyncReadExt, AsyncWriteExt},

net::{TcpListener, TcpStream},

sync::{mpsc::channel, Mutex},

};

Oh I overlooked this, should check how you can configure this in CLion .

richardpringle · 2023-09-26T18:16:32Z

core/network/examples/peer_gossip.rs

+#[allow(unused_variables)]
+impl Client for TestClient {
+    async fn app_gossip(&mut self, request_bytes: Vec<u8>) {
+        unimplemented!()


FYI, todo! is a little shorter, and they pretty much do the same thing.

richardpringle · 2023-09-26T21:21:58Z

core/network/src/p2p/gossip/gossip.rs

+                });
+
+                let mut guard = self.client.try_lock().expect("Failed to acquire a lock on client");
+                guard.app_request_any(msg_bytes.clone(), on_response).await;


I don't think you need to clone msg_bytes here.

Suggested change

guard.app_request_any(msg_bytes.clone(), on_response).await;

guard.app_request_any(msg_bytes, on_response).await;

richardpringle · 2023-09-26T21:23:24Z

core/network/src/p2p/client.rs

+use std::sync::Arc;
+use async_trait::async_trait;
+
+pub type AppResponseCallback = Arc<dyn Fn(Vec<u8>) + Send + Sync>;


Why do you have to wrap the callback in an Arc?

richardpringle · 2023-09-26T21:25:01Z

core/network/src/p2p/gossip/handler.rs

+}
+
+#[async_trait]
+#[allow(unused_variables)]


You shouldn't really ever have this on a trait. Just use the _ prefix where necessary.

richardpringle · 2023-09-26T21:26:03Z

core/network/src/p2p/gossip/handler.rs

+        let mut response_size = 0_usize;
+        let mut gossip_bytes: Vec<Vec<u8>> = Vec::new();
+        let guard = self.set.try_lock().expect("Lock failed on set");
+        guard.iterate(&|gossipable| {


Suggested change

guard.iterate(&|gossipable| {

guard.iterate(|gossipable| {

richardpringle · 2023-09-26T21:39:08Z

core/network/src/p2p/gossip/gossip.rs

+    set: Arc<Mutex<S>>,
+    client: Arc<Client>,
+    stop_rx: Receiver<()>,
+    phantom: PhantomData<T>, // Had to use this to please the compiler about T not being used.


Quick lesson on when to use associated types vs. generics (in case you don't already know this):

A generic type represents many types, right? So, would it make sense to have many different trait implementations, one for each concrete type? OR... is there a single type that should be associated with this particular trait implementation?

Say the Iterator trait did not exist, and we wanted to create an Iterator trait and implement it for Vec<T> (not actually how this works, but I think the example will be clear). Would we want to have a different implementation for a Vec<String> vs. a Vec<u8>? Definitely not! Here's where we want an associated type.

A good rule of thumb is to actually start with an associated type and see if you can get it to work the way you want; it will become clear very quickly if you actually need to make your trait generic.

Sanghren · 2023-09-28T07:28:08Z

Cool! Awesome job if some of those examples work!

I'm happy to keep reviewing your code and give you some Rust-specific pointers, but as @exdx mentioned, it might be tough getting this merged in as the P2P stuff isn't stable yet.

Right now, there's quite a bit of code that could panic in spots where you wouldn't want code panicking.

If you're just looking to learn, I'm happy to help as much as I can, but if it's contributions that you want, it might be a better idea to take a look at something smaller. In either case, I'm here to help!

Thanks for all the feedbacks ! My goal here is more to learn something (never used rust for this kind of work, so definitely lot of things I don't know) than to have something merged in. Am gonna aim for something working, would be a nice achievement ;)

richardpringle · 2023-09-29T19:07:52Z

@Sanghren, hit the "Re-request review" button when you're ready for another round! I can't promise I'll get to it right away, but I will get there when I get a chance!

Sanghren · 2023-10-02T13:26:47Z

@Sanghren, hit the "Re-request review" button when you're ready for another round! I can't promise I'll get to it right away, but I will get there when I get a chance!

Thank you so much to take time to review this, helping me a lot and learning tons of stuff.
I tried to cover all your remarks, there is still some wonky stuff going on with the lock at the end of my start_fake_node function, that's why I left the scope around the asserts . Without the scope it sometimes hang. Trying to find out why.

vikinatora · 2023-12-11T07:51:24Z

Hey @Sanghren, do you still intend to work on this PR in the near future? Our team can hop in and try to finish the great work you've started.

Sanghren · 2023-12-18T12:43:27Z

Hey @Sanghren, do you still intend to work on this PR in the near future? Our team can hop in and try to finish the great work you've started.

eyh o/ Well I submitted some stuff and was waiting for a review (kinda lost track of this tbh). Please do, not sure my code is worth it though, I am learning here.

sanghren and others added 28 commits September 7, 2023 17:29

feat: add gossipable trait

5cbdd66

feat: add gossip

a0e662a

feat: add channel to gossiper

aa7f63c

feat: add gossipable trait

ed23c1e

feat: add gossip

0c35d21

feat: add channel to gossiper

86580a6

Merge remote-tracking branch 'origin/add_gossipable' into add_gossipable

bb397b9

feat: add bloom

10657e0

feat: add test for bloom

baf2923

feat: add test for bloom

1fea689

Merge branch 'ava-labs:main' into add_gossipable

9f1ab3b

Merge branch 'ava-labs:main' into add_gossipable

f5a464b

feat: add proto

7ba33ea

feat: add proto generated file

cc7edeb

feat: some cleanup

231d93b

feat: some cleanup

c1b42c2

feat: add Namespace to Config

1f57923

feat: add test

db4cf30

feat: add test

e52b0a1

Merge branch 'ava-labs:main' into add_gossipable

94085a6

feat: remove warning debug print

3c3e138

feat: add handler.rs

c63269c

feat: cargo.toml dep

476016a

feat: cargo.toml dep

139dacb

feat: lint

b9fc9ec

feat: finish handler accept_request

a606351

feat: unused app-request param

573db11

feat: put tests under mod test

f1b752a

Sanghren mentioned this pull request Sep 14, 2023

Implement Mempool #29

Open

sanghren and others added 4 commits September 18, 2023 16:41

feat: address warnings

b676024

Merge branch 'ava-labs:main' into add_gossipable

acb1de0

feat: implement an example that run gossip logic

3bec776

feat: implement an example that run gossip logic

e046013

rkuris reviewed Sep 26, 2023

View reviewed changes

richardpringle reviewed Sep 26, 2023

View reviewed changes

feat: make things work - wip

13205e7

Merge branch 'ava-labs:main' into add_gossipable

7b1e8eb

sanghren added 13 commits October 1, 2023 14:11

feat: wip

f56ab25

feat: wip

884f1fd

feat: wip

c5446e6

feat: wip

e4b3400

feat: wip

fb0359e

feat: wip

ccce15b

feat: wip

774ca6e

feat: wip

ec6767a

feat: clean

7db76bc

feat: clean

36c40dd

feat: clean

f6e8baf

feat: clean

672c728

feat: remove arc for callback

b89abd9

Sanghren requested a review from richardpringle October 2, 2023 13:20

sanghren added 3 commits October 2, 2023 17:40

feat: format

3faedf9

feat: format

ff65229

feat: format

65e8a14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gossipable trait + relevant logic #45

Add gossipable trait + relevant logic #45

Sanghren commented Sep 14, 2023

richardpringle commented Sep 15, 2023

rkuris Sep 26, 2023

richardpringle Sep 26, 2023

rkuris Oct 2, 2023

richardpringle left a comment

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

Sanghren Oct 2, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

richardpringle Sep 26, 2023

Sanghren commented Sep 28, 2023

richardpringle commented Sep 29, 2023

Sanghren commented Oct 2, 2023 •

edited

Loading

vikinatora commented Dec 11, 2023

Sanghren commented Dec 18, 2023

	guard.app_request_any(msg_bytes.clone(), on_response).await;
	guard.app_request_any(msg_bytes, on_response).await;

Add gossipable trait + relevant logic #45

Are you sure you want to change the base?

Add gossipable trait + relevant logic #45

Conversation

Sanghren commented Sep 14, 2023

Summary

Changes

ToDos

Questions

richardpringle commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle left a 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

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

Choose a reason for hiding this comment

Sanghren commented Sep 28, 2023

richardpringle commented Sep 29, 2023

Sanghren commented Oct 2, 2023 • edited Loading

vikinatora commented Dec 11, 2023

Sanghren commented Dec 18, 2023

Sanghren commented Oct 2, 2023 •

edited

Loading