Network/protocol refactoring

[damip]

Network refactoring

Rationale

The current Network/Protocol stack is suboptimal and has accumulated legacy:

• can't change the protocol port
• vulnerability to some attacks
• can't easily switch to other ways of connecting (eg. QUIC)
• performance problems
• hard to debug

In this proposal, we describe a new design for Massa's network stack.

General design

Key points:

• layerization: have interfaces allowing support for multiple transport methods
• no more async: easier to debug, threads to improve performance
• compartmentalization: have separate modules for each behavior (eg. block propagation, operation propagation etc...)

Layers

Endpoint

To connect towards an endpoint, we use the OutEndpoint structure containing:

• a protocol identifier (eg. TCP, QUIC etc...)
• a route (eg. an IP + port)

When receiving inbound connections, we should be able to know the InEndpoint containing:

• a protocol identifier
• some identifier (like the IP) to prevent flooding by limiting the number of incoming connections from the same entity

Transport

A transport follows the following structure:

• has the possibility to (cancellably) wait for an incoming connection
• has the possibility to try to connect towards an OutEndpoint
• once a connection is established, it has the possibility to send and receive full framed messages

Transports include framing, and guarantee the integrity and ordering of sent data.

Transports need to include rate limiting. A simple way to rate-limit in a sync thread is the following:

• define a window duration: window_duration = typically one second
• define a max amount of data to read during that window: max_window_data
• set read_window_data = 0
• window_start = now()
• loop:
  • read available data from socket using https://doc.rust-lang.org/std/io/trait.Read.html#tymethod.read with a buffer of size min(buf_max_size, max_window_data-window_byte_count) where buf_max_size is about 100 kilobytes
  • window_byte_count += number_of_bytes_read
  • if window_byte_count >= max_window_data:
    • window_start += window_duration
    • window_byte_count = 0
    • sleep until window_start
    • continue loop
  • if now() >= window_start + window_duration:
    • window_start += window_duration
    • window_byte_count = number_of_bytes_read // this is not perfect as it might rate-limit too heavily in certain cases but it is safe and lightweight

Thread organization

Separate every aspect (eg. operation propagation, block retrieval etc...) into their own threads.

DNS support

DNS resolution should be supported out of the box (eg. https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html ).
This will help with docker containers, local networks, dynDNS etc...

Peer announcements

• a PeerAnnouncement contains:
  • NodeId
  • timestamp
  • an optional endpoint on which the peer can be reached (typically protocol, ip/dns, port). If None, the peer is announced as non-routable
  • the signature of the PeerAnnouncement using the private key associated to the NodeID
• the peer database should have HashMap<NodeId, NodeInfo> where NodeInfo would contain the peer info as well as a copy of the latest signed PeerAnnouncement for that peer
• whenever our IP or routability changes, we can announce the change to our peers
• when we receive a PeerAnnouncement, we send it to the robot that will check:
  • if the announcement is for a peer that we already have in our db, and the announcement is equally or more old than the one we already have in our db => ignore the announcement
  • otherwise, the robot can verify the signature of the announcement, and put the peer in the list of peers to try. It's only once peers are tried successfully that they are added to the peer db or updated there.
• when a peer A connects to a peer B:
  • B tells A its peer ID and the IP of A as seen by B (this can be used by A to auto-detect its own IP)
  • if B is full, it quits by sending some random PeerAnnouncements it has to A
  • A sends its own PeerAnnouncement to B
  • then normal procedure
• connected peers notify each other about updates on their peer lists (only updates, not sending the whole list every time !)

[sydhds]

What lib do you plan to use for QUIC ?

[damip]

Dunno yet :)

[aoudiamoncef]

Possible candidates ?

• Quiche
• Quinn

[damip]

@gterzian a basis of discussion

[gterzian]

@damip thanks! Will review and come back tomorrow...

[gterzian]

As a general note, I think we should:

1. Avoid compartmentalizing per thread per se. Threading should be used for parallelism, or dealing with blocking calls, while compartmentalization can be simply achieved with well-defined modules. For example if many "threads" as drawn above used the same shared-state, they probably should run on the same thread, simply as different modules.
2. Avoid shared-state were we can, for example by thinking more in terms of a pipeline, where each stage would have it's own internal state.

Regarding the top diagram: I think we should not spend too much time on the internals of the "networking module" in this discussion, because that is likely to be influenced by, and differ based on, the underlying protocol used. I propose we instead focus the interface it would offer to other modules, such as:

• A way to send a message to a connection.
• A way to ban/whitelist a connection.
• A way to get a channel on which messages from a connection can be received(or rather one channel for all connections), as well as new and closed connections are notified on.
• A way to start a bootstrap server, and/or connect to one(?)

Regarding the bottom diagram:

• I think "Peer management" should belong inside the networking module(could be a separate thread inside of it).
• I think we only need one thread for block propagation, and propagating "other stuff", while splitting it up in two modules.

In conclusion: I propose switching to a more "pipeline-centric" approach, consisting of two separate components, like:

• Network, which sends new incoming messages, new, and dropped connections to Protocol.(no need for sharing acting connections, if Protocol has banned one, it can ignore incoming messages.
• Protocol, which sends outgoing messages, connection bans, to Network(again no need to share active connections, Network can ignore outgoing messages for connections that have been dropped).

(I could add a diagram but the above bullet points seem simple enough).

Regarding the (not shared) data:

• Peer database hidden inside Protocol(with changes communicated to Network as they occur)
• Active Connections hidden inside Network(with changes communicated to Network as they occur)

Please let me know what you think @damip @massalabs/core-team

[Eitu33]

First of all my apologies for writing such a late reply on this. I have some questions concerning choices and incoherences with what I have seen so far in the code. This is written taking into account the initial spec, the above comment, and the first associated PR (#3272).

As a general note, I think we should:

1. Avoid compartmentalizing per thread per se. Threading should be used for parallelism, or dealing with blocking calls, while compartmentalization can be simply achieved with well-defined modules. For example if many "threads" as drawn above used the same shared-state, they probably should run on the same thread, simply as different modules.
2. Avoid shared-state were we can, for example by thinking more in terms of a pipeline, where each stage would have it's own internal state.

I agree that some of the modules (such as the connector) do not require a thread, however for modules such as the one handling an established connection I find it to be necessary. I also think that the argument of saying "same shared-state = no extra thread required" is not something that can be applied on every case as a general rule. I do see that you chose to keep a separate thread for blocks, and was wondering what was the reason for it being the only one deserving of it, and if you changed your mind since you wrote this or if I misunderstood something as there is a thread for handshake management in your PR.

When you write "they probably should run on the same thread, simply as different modules", it is unclear for me how this traduces to code, does running as different modules without separate threads refer to the blocking async tasks such as this one?

I also do not understand your concept of pipeline, you describe it as an alternative approach to the one using multiple threads:

In conclusion: I propose switching to a more "pipeline-centric" approach, consisting of two separate components, like:

• Network, which sends new incoming messages, new, and dropped connections to Protocol (no need for sharing acting connections, if Protocol has banned one, it can ignore incoming messages).
• Protocol, which sends outgoing messages, connection bans, to Network (again no need to share active connections, Network can ignore outgoing messages for connections that have been dropped).

But after reading this I see no real difference (or maybe don't understand it) between the current behaviour and what you are proposing, and what has been done in the PR seems to follow a different idea as well.

I also disagree with the fact that we do not need to share connections because of protocol refusing data coming from network if it has banned one, since they could just keep sending information that we do not wish to read at all, and network would make no difference in the way that it treats it.

Regarding the first iteration that was made concerning this (treating this subject here as well as I feel it is intimately related with the more general matters, could be moved to the PR discussion), I can understand why some asynchronous code had to be used even if it was the base premise to remove any occurrences entirely. But is this the right approach? The asynchronous tasks (for handshakes in this context) are condemned to be blocked at some point, and the workflow of protocol & network did not become more straight forward. As a third evaluation point, I am unable to determine how optimised your approach is compared to the initial proposal.

To make a conclusion, I find that this solution solves the debugging issue only partially and does not make the workflow more understandable. What could justify it would be the compared performance, which I can't estimate since I lack specific knowledge on the subject and the implementation details of the other proposal.

[AurelienFT]

@damip After reading about rate limiting I saw that there two main basic algorithms : Leaky bucket and Token bucket. The difference (and simple explanations) is well explained here : https://qr.ae/pr94Tx and a basic implementation of token bucket algorithm in python is here : https://dev.to/satrobit/rate-limiting-using-the-token-bucket-algorithm-3cjh

The crate async_speed_limit that we used in async use the token bucket algorithm

[litchipi]

I think it is essential to write the scope of the networking stack for our use case:

• Very long connections with Peers
• Continuous traffic, a lot of redundant messages incoming
• Have to be able to debug the code, benchmark it, add new features and refactor parts of it easily

For these reasons:

• Connecting to a new Peer can be "slow" (relatively to a "normal" network stack), it's not something we do 5 times per seconds
• Internal state management have to be really fast (to filter redundant messages)
• Peer management (which peer is faulty, is it worth connecting a new peer ?) have to be fast as well
• The code has to be very modular, very well separated (very generic), and in general, do only one thing, but do it very well.

I'm basing my analysis on the fact that creating a parallel thread is slow, so is only worth it if the thread has a long time to live, or allows to parallelize work such that the overall "time cost" is in our favor.
Also, shared states (like Mutex) are slow operations and have to be used only if unavoidable when dealing with real-time processes.

Summing up these thoughts, about the architecture and implementation I personally think:

• Internal state and peer management have to be optimized for speed, only spawn new threads if really needed, prefer passing messages over channels than have a shared state
• The initialization of a connection with a new Peer can be slow, so any handshakes, monitoring of the peer quality, bootstraping can be done in parallel before we add the new Peer to the internal state and goes to the normal routine

Related to the code organization:

• Use modules and factorize the code as much as possible, abuse enum to treat any kind of variants, messages to pass, states to share
• Separate domains, and layer them:
  • Connection to a single Peer: init connection algorithms, rate limit, transmit raw messages, connections variants
  • Management of Peers: which peer to drop, messages propagation, filter redundancy, implement basic protocol bricks
  • Protocol over a pool of Peers: interact with an interface over the pool of peers, forward blocks, expect event, etc ...

Each of these layers should have their internal state, and communicate the necessary data (aka events) to the other layer to make them update their state.
On some rare occasions, Mutex may be better than channels (and this is where benchmarks will be very useful), but in this architecture it'll be much easier to go from channels to shared state than the reverse.
I feel the real threat here is to have parts of the protocol in the network manager, have peer management stuff in the connection handler, etc... and finish with a messy blob. If we want an optimized code, we need it to be minimal, modular and generic.

[AurelienFT]

Connecting to a new Peer can be "slow" (relatively to a "normal" network stack), it's not something we do 5 times per seconds

It should be enough fast to be done more than 5 times per seconds.

prefer passing messages over channels than have a shared state

In read only cases when we have a lot of threads that read and not often write on only one thread: a shared-state in RwLock could be efficient also.

I have proposed a new way to manage the peers which is more modular and don't have a shared state for this to @damip this afternoon. I will update the spec about it. I can inform you also.

Otherwise, I'm pretty much agree with you and it seems to match what we described in the spec + what we are implementing. Maybe I have miss something in your message but we can discuss when we will work together on that.

[damip]

You're right that we need to write down what is expected and what you listed matches well to our design choices.

I would add that ease of understanding, maintenance, robustness to attacks and future-proofing are also crucial.