WIP: Refactor pchain network engine #15

ong-jonas · 2023-10-10T09:04:14Z

Changes

network subscribes to Vec<Topic> from config::fullnode_topics(local_public_address) instead of being a separate argument to start
Message types are determined via message.topic.as_str() which can be matched to: "consensus", "mempool" , "droppedTx" and public address string. The message data can then be deserialised according to the type of message it is:
- hotstuff_rs::messages::Message
- pchain_types::blockchain::TransactionV1
- DroppedTxMessage
constant inputs like protocol name and heartbeat seconds were being passed as arguments to Behaviour::new() which were subsequently passed as arguments to kad_config() and gossipsub_config. Removed these redundant arguments and used them in the lowest level functions instead.

manngayin612 · 2023-10-11T01:23:40Z

src/behaviour.rs

                .max_transmit_size(MAX_TRANSMIT_SIZE * MEGABYTES) // block size is limitted to 2 MB. Multiply by factor of safety = 2.
                .message_id_fn(build_msg_id)
-                .heartbeat_interval(Duration::from_secs(heartbeat_secs))
+                .heartbeat_interval(Duration::from_secs(10))


I think we can take this magic number out and put them with the constant up there. (Not a must, just think it might be cleaner)

manngayin612 · 2023-10-11T01:24:52Z

src/behaviour.rs


 pub(crate) const MAX_TRANSMIT_SIZE: usize = 4;
 pub(crate) const MEGABYTES: usize = 1048576;
+pub(crate) const PROTOCOL_NAME: &str = "/pchain_p2p/1.0.0";


Protocol name is no longer a constant, it is a field in Config. That's on me, I forgot to remove it from constant.rs.

Both identity config and kademlia config uses protocol name, however, while kademlia accepts multiple names, identity accepts only 1. So in the event we have multiple protocol names, which one do we use for identity?

I think in our Config, we can change the protocol name into a single String instead of a Vec, just like what we used to do. Only put it into a vector in Kad config.

manngayin612 · 2023-10-11T01:28:20Z

src/engine.rs

-        &local_keypair,
-        10,
-        &config.kademlia_protocol_names, //TODO jonas
+        &local_keypair


As mentioned previously, protocol name is no longer a constant, its a configurable field in Config, so we might have to pass in the Config instance.

manngayin612 · 2023-10-11T01:45:51Z

src/engine.rs

                                    // TryFrom trait for Vec<u8> to Message, implement a function that takes in the Message Topic to help
                                    // converting Vec<u8> to Message. You can refer to fullnode/mempool messagegate to see how to 
                                    // deserialise each Message type. 
+                                    match message.topic.as_str() {


I understand you couldn't do branches like Topic::Mempool.hash() in match statement. But comparing the topic using string doesn't seem a good idea.

let topic = fullnode_topics(public_addr).into_iter().find(|t| t.hash() == message.topic);

Will this way of identifying the topic work? I haven't verified, you can try.

…nd heartbeat interval

manngayin612

Added comments on the functions for deserializing Vec to pchain_network::message::Message

manngayin612 · 2023-10-11T03:47:11Z

src/engine.rs

    // 3. Subscribe to Topic
-    //TODO jonas
-    swarm.behaviour_mut().subscribe()?;
+    swarm.behaviour_mut().subscribe(config::fullnode_topics(local_public_address))?;


I don't think all applications that uses pchain_network will always subscribe to all topics. For example, fullnode doesn't subscribe to droppedTx. So the topics to be subscribed should be based on the Config we passed in.

manngayin612 · 2023-10-11T06:34:02Z

src/engine.rs

+                                        match topic {
+                                            HotStuffRsBroadcast => {
+                                                let hotstuff_message = 
+                                                hotstuff_rs::messages::Message::deserialize(&mut message.data.as_slice())


What's the reason of getting the de-serialized result of type Result<> and wrap them with Optional and check if it is Some() or None later? Can't we do something directly when the deserialisation gives out Error?

Also, you gotta return the deserialised message out after this match statement. You should be about to return pchain_network::message::Message.

You might consider getting this part out into a separate helper function to make it look cleaner.

ong-jonas · 2023-10-12T02:02:00Z

I have completed the changes according to your suggestions, but i still think we should match to the topic hash instead of topic.

The conditional function below checks whether the local peer is subscribed to the message's topic hash.

if swarm.behaviour().is_subscribed(&message)

And after checking, we repeat the check just to get the Topic type.

    let topic = config::fullnode_topics(local_public_address)
                                    .into_iter()
                                    .find(|t| t.clone().hash() == message.topic);

Additionally, config::fullnode_topics(local_public_address) limits mailbox topics to only contain HotStuffRsSend(address) of the local peer.

manngayin612 · 2023-10-12T02:19:23Z

I understand the problem you mentioned, but not really clear about your solution. Have you implemented your thoughts in the recent commit? If not, please do it the way you think it works, doesn't have to follow my suggestions 100%.

ong-jonas · 2023-10-12T02:27:15Z

the current implementation works for v0.5, will work on my solution for future updates.

manngayin612

Resolve the conflicts then this branch will be ready to be merged.

ong-jonas added 3 commits October 10, 2023 11:09

network subscribes to fullnode topics declared in config

47046ea

filter types of messages to pass into handlers

8359198

removed constant inputs that were redundantly passed into functions

89af337

manngayin612 reviewed Oct 11, 2023

View reviewed changes

changed method of identifying message topics, updated protocol name a…

06d7fd1

…nd heartbeat interval

manngayin612 reviewed Oct 11, 2023

View reviewed changes

improve deserialization of messages

34dd150

manngayin612 suggested changes Oct 12, 2023

View reviewed changes

Merge branch 'refactor' into refactor

309a0d6

ong-jonas marked this pull request as ready for review October 12, 2023 02:43

manngayin612 merged commit e4e4b1b into parallelchain-io:refactor Oct 12, 2023

ong-jonas deleted the refactor branch October 12, 2023 02:47

WIP: Refactor pchain network engine #15

WIP: Refactor pchain network engine #15

Uh oh!

Conversation

ong-jonas commented Oct 10, 2023

Changes

Uh oh!

manngayin612 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

manngayin612 Oct 11, 2023 • 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!

ong-jonas Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

manngayin612 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

manngayin612 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

manngayin612 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

manngayin612 left a comment

Choose a reason for hiding this comment

Uh oh!

manngayin612 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

manngayin612 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

ong-jonas commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manngayin612 commented Oct 12, 2023

Uh oh!

ong-jonas commented Oct 12, 2023

Uh oh!

manngayin612 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

manngayin612 Oct 11, 2023 •

edited

Loading

ong-jonas commented Oct 12, 2023 •

edited

Loading