review of file controller_routing

[PaulBernier]

Reviewing file https://github.com/WhoSoup/factom-p2p/blob/master/controller_routing.go

1. If I understand correctly ToNetwork/FromNetwork are the interfaces between the application and the p2p package. I think those communication channels should not use the Parcel struct but its own structure specific to the communication with the application layer; Parcel is an object that belongs to the networking layer and should not leak outside. Here are some elements that brought me to this conclusion:
   -first just from a layering point of view it's better to have things well defined and isolated. It's very similar to the classic DTO vs DAO pattern
   -the only Parcel that should go in/out to the Application must be of type TypeMessage/TypeMessagePart, having a specific type for application communication would definitively rule out any possibility to mistakenly send in/out unwanted Parcel types (prevent bugs ahead thanks to typing mechanism).

• You have a comment next to Address in your Parcel struct "" or nil for broadcast, otherwise the destination peer's hash. but that's not completely true, when coming from the application it is filled with "", "" or "". That's another hint that you may be overloading your Parcel struct with 2 different meanings in 2 different contexts.

2. manageData method: I think it's better if there is as little logic as possible within the cases of the switch. In the TypePing case I see a comment "// Move": I say yes! Let's create a Pong method of peer and just call that in in the switch case (also maybe create a Ping method for Peer for symmetry). Similarly the TypePeerRequest case contains a bit too much logic for my personal taste that would be better encapsulated. It seems you don't need that distinction between TypeMessage and TypeMessagePart any more, I guess you can merge them instead of using fallthrough?

3. Broadcast why does it take a boolean as an argument while the type could be determined within by using the Parcel type? Technically speaking this method could receive inconsistent information between this boolean and the parcel type, by relying solely on the type you protect programmers against this risk.

4. randomPeers and randomPeersConditional return results that are a bit surprising/inconsistent to me in the case you have less elements than count, you will not fulfill your promise of randomness, but still return some peers. It looks like a half-baked results. If the contract is to returned exactly count peers and you cannot meet that demand then make it clear by returning an empty slice (or nil). Also you separate the case len(peers) == 0 but it seems to me it fits the general case, it's not particularly special.

	if uint(len(regular)) < count {
		return append(special, regular...)
	}

This snippet looks suspicious to me as len(special)+len(regular) may end up greater than count, should that be a possibility? I'd find that surprising as a client of this function.

[WhoSoup]

This is an interesting point. Parcel is indeed the structure for both internal network messages and external application messages. The reasoning why they're the same is mostly because Golang is a painful language for these kind of "dynamic" types. It's not an object oriented language but it does have interfaces.

The way I'd implement your vision is to have "Parcel" be an interface with various implementations:

interface Message
  + PeerMessage
  + RandomMessage
  + BroadcastMessage
  + FullBroadcastMessage
  + parcel (unexported)

the interface would look like this:

type Message interface {
	GetType() ParcelType
	GetAddress() string
	GetPayload() []byte
}

This is a rather cumbersome approach that requires a fair amount of copypasting, though it could work.

---

A different approach would be to keep it as it is and just make both parcel itself and parcel.Type unexported. That way, the application would only be able to create it via p2p.NewParcel(target, payload) and be unable to change the type or instantiate an empty Parcel.

It would be much easier than interface juggling, though you'd still have to compare parcel.Address with p2p.Broadcast and the likes.

Thoughts?

[WhoSoup]

2. Let's create a Pong method of peer and just call that in in the switch case (also maybe create a Ping method for Peer for symmetry). Similarly the TypePeerRequest case contains a bit too much logic for my personal taste that would be better encapsulated.

I'm okay with Peer having Ping/Pong functions, PeerRequest would be better suited for the controller.

It seems you don't need that distinction between TypeMessage and TypeMessagePart any more, I guess you can merge them instead of using fallthrough?

Around three years ago, the P2P package split all application messages into 1000 byte "messageparts", though that was deemed unnecessary since TCP will already split large messages into small chunks. Instead of removing the code, they just raised the limit to 1000000000 bytes (1 gigabyte), which means P2P1 will always send out "MessagePart" messages.

P2P2 only sends out "Message" types, and converts "MessagePart"s to regular "Message" types for consistency. There is no functional difference between Message and MessagePart anymore, so they're handled by the same switch case now

[WhoSoup]

This is the new route:

factom-p2p/controller_routing.go

Lines 6 to 39 in 6459677

	func (c *controller) route() {
	for {
	// blocking read on ToNetwork, and c.stopRoute
	select {
	case parcel := <-c.net.ToNetwork:
	switch parcel.Address {
	case FullBroadcast:
	for _, p := range c.peers.Slice() {
	p.Send(parcel)
	}
	case Broadcast:
	selection := c.selectBroadcastPeers(c.net.conf.Fanout)
	for _, p := range selection {
	p.Send(parcel)
	}
	case "":
	fallthrough
	case RandomPeer:
	if random := c.randomPeer(); random != nil {
	random.Send(parcel)
	} else {
	c.logger.Debugf("attempted to send parcel %s to a random peer but no peers are connected", parcel)
	}
	default:
	if p := c.peers.Get(parcel.Address); p != nil {
	p.Send(parcel)
	}
	}
	}
	}
	}

Not sure if it's worth factoring out each switch-case into its own function as before. I'm open to it if you think it's a good idea in regards to potential unit testing. This is already a big improvement over the previous code though, imo.

[WhoSoup]

3. Broadcast why does it take a boolean as an argument while the type could be determined within by using the Parcel type?

Great question! I have no idea and am changing it.

4. randomPeers and randomPeersConditional return results that are a bit surprising/inconsistent to me in the case you have less elements than count, you will not fulfill your promise of randomness, but still return some peers. It looks like a half-baked results.

I originally intended to rename count to max to address this, but after looking at where the code is used, I'm only using the single-case. So instead, I'm renaming the functions to randomPeer and randomPeerConditional, which only select a single random peer rather than shuffling the entire peer list.

[WhoSoup]

5. This snippet looks suspicious to me as len(special)+len(regular) may end up greater than count, should that be a possibility? I'd find that surprising as a client of this function.

Yes, that is intended functionality at the moment, taken over from P2P1: https://github.com/FactomProject/factomd/blob/6f0164ba51dc1776d1ca44a3555ae1dd510bc5fb/p2p/controller.go#L666-L674

If the special peers are counted inside count, there are situations where if you have more special nodes than regular peers, you could end up never broadcasting to non-special peers at all. It also impacts the probability of packet routing, with special peers receiving an abnormal amount of "random" selections.

Tacking special peers on top of regular peers doesn't have either of those problems, although you're paying with more bandwidth. At the moment, the only nodes using special peers are ANOs to establish the backhaul network.

[PaulBernier]

I haven't been clear enough in my comment about fallthrough usage. It was just stylistic comment:

case "A":
    fallthrough
case "B":
    // handle case A and B

is better written in Go:

case "A", "B":
    // handle case A and B

Golang switch supports list of cases.
You used that fallthrough pattern in your new PR too

[PaulBernier]

I still need to think more about 1. I also want to understand how currently the application layer uses the network layer (p2p module).

[WhoSoup]

I haven't been clear enough in my comment about fallthrough usage. It was just stylistic comment:

Ah, my apologies!

[WhoSoup]

I still need to think more about 1. I also want to understand how currently the application layer uses the network layer (p2p module).

The short explanation:

Routing

Factomd has its own simulated p2p network inside the node, which is used to test "simulated nodes" (SimNode). By default, there is only one other simulated "node" in this network: p2pproxy. The proxy bridges the internal factom p2p network with the p2p package.

The messages in the internal network are of type "FactomMessage", the proxy converts them to the p2p network parcels:

https://github.com/FactomProject/factomd/blob/17287fc5c07187fb9fbdb2151414cf11f7be50e2/engine/p2pProxy.go#L210-L231

and vice-versa for incoming p2p parcels, it converts them to factom messages:
https://github.com/FactomProject/factomd/blob/17287fc5c07187fb9fbdb2151414cf11f7be50e2/engine/p2pProxy.go#L233-L248

FactomMessages are just a carrier for internal node messages, summarizing the whole message flow as:

IMsg => (internal network) FactomMessage[IMsg] => (p2p package) Parcel[IMsg] => (tcp) V10Parcel[IMsg]

---

Processing

Factomd checks two things of network messages: duplication and validity. There are some p2p libraries that will have their own duplicate filter, but p2p2 doesn't have one because factomd handles this aspect.

Validity checks the IMsg to ensure it's properly signed, has the right time, is supposed to be a network messages, etc. Only valid messages are then sent back to the network to be rebroadcasted.

[PaulBernier]

For 1. with the lack of generics I couldn't find a good solution not requiring duplicate code. So maybe un-exporting parcel.Type outside of the p2p module may be already a step in the right direction though.

[WhoSoup]

I added a fix for most of the scheduling in 9433d6d

I played around with channel sizes in my testapp and found that the channel most likely to drop parcels was "FromNetwork", meaning the application isn't reading from it fast enough. I added a runtime.Gosched() into that function and it stopped a significant amount of parcels from being dropped.

I ended up just baking runtime.Gosched into the parcel channel itself. With a channel size of 10 (vs 1000 originally) and sending 6k+ messages up and 6k+ messages down, I had a drop rate of ~0.02%, most of which happened during the bootup phase.