whosoup / factom-p2p Goto Github PK

This one I have a mix of thoughts, suggestions and questions: https://github.com/WhoSoup/factom-p2p/blob/master/peer.go#L121

1. From a OO design point of view I am not sure StartWithHandshake should be a method of Peer, it feels to me that this is the method that should give birth to a fully functional Peer. Right now newPeer creates a shallow, non-functional peer, that is then immediately modified by side effects within this function to become, or not, a valid peer. I personally prefer when instances are created in a complete consistent state. Right now, depending on the exit point of StartWithHandshake, you may end up with a Peer instance with 3 different states of populated fields, I think that has potential for confusion and inconsistencies. Maybe Handshake handling should just be a method of the controller.

2. I am not sure I understand completely the incoming concept. I get it it distinguish the case when our node Dials (incoming = false) vs another node dial us (incoming = true), but that distinction is only necessary at the time of the handshake, so why it is persisted in Peer object afterwards? Once a new peer is added, the relation between the 2 becomes completely symmetrical, regardless of who initiated the connection, isn't it? Also in the case incoming = true, what happens to decoder.Decode(&reply)? Is a response really expected even though it was already the other node that initiated the connection?

3. You test reply.Header.Type for TypeRejectAlternative, should you go further and test that this header type is not an unexpected type during a handshake? Going even further, check that you get an TypeRejectAlternative solely in case of incoming = false? Basically I would tighten the validation around reply.Header.Type to definitely rule out potential inconsistent states.

4. I am confuse by ep.Port = reply.Header.PeerPort. We have established a TCP connection with a given IP and port already, what does this PeerPort returned mean and why does it override the real port?

5. is registered attribute really necessary? Wouldn't it be safer to handle the status of a Peer in a single place? When having 2 different data representing the same state, you may have consistency issues, so preferably if you can maintain this info in a single place it's better.

6. In bootstrapProtocol should you delete this hsParcel code or are you expecting to still use it later?

7. Should banned endpoints be handled as part of the Handshake? Right now we go through a handshake that is meant to say "we can do business together" and then immediately after drop the peer without explanation, kind of rude! :)

8. [not Handshake related be in the same file] in statLoop that 5 should really be a variable :)

https://github.com/WhoSoup/factom-p2p/blob/master/controller_cat.go#L72
2 minor comments about that function:

• it is used solely with shuffle boolean being true. If there is no use case for not shuffling, do you really need that parametrization?
• the function is called trimShare but also shuffle. And because the shuffle is enabled by a boolean, it's hard to guess from just reading the client call what that boolean means (because the function name says nothing about its potential meaning)

Given the comments above, maybe rename the function trimShuffleShare without a shuffle parameter?

Commenting on https://github.com/WhoSoup/factom-p2p/blob/master/controller_cat.go#L10

	if time.Since(c.lastRound) < c.net.conf.RoundTime {
		return
	}
	c.lastRound = time.Now()

I think we can improve that pattern: why not have runCatRound in its own gorountine with an infinite loop that is blocked by a Ticker set to RoundTime? That would be cleaner (avoid unecessary evaluations) and more idiomatic I think and avoid doing this manual handling of event loop.

2. c.net.conf.Drop took me too long to understand it was the target to drop to and not the number of connections to drop (yeah I saw there is a comment on that line but brain was hard wired to not believe it because of the name :D). Then I looked at https://github.com/WhoSoup/factom-p2p/blob/master/configuration.go#L53 and realized I cannot make sense of most of the CAT parameters without digging the code. e.g Max what? I'd appreciate to see a short comment next to each of those CAT parameters explaining them.

https://github.com/WhoSoup/factom-p2p/blob/master/peer.go#L240
Why not propagate the boolean about success? I can see at least 1 place where it could be used: https://github.com/WhoSoup/factom-p2p/blob/master/controller_cat.go#L142 in case the message doesn't go through you could skip the 5s timeout

I think I have reviewed at least once every file... I will still take a deep breadth and a step back to see the big picture again, but I think I am basically done with the review.

Here are just a few leftover comments that I batched together:

1. It doesn't sound right that the Controller does the parseSpecial logic from a separation of concerns point of view, it looks like this is the job of a configuration parser that should happen ahead. The bootstrap endpoints don't come as raw strings, they are provided already parsed, so why not the same for special endpoints?

2. I was thinking that ToNetwork and FromNetwork may need separate capacity configuration because the rate of inbound vs outbound messages can be very different. I was also thinking it would be nice at add the FillRatio of both ToNetwork/FromNetwork to Prometheus metrics. (bonus)

3. I am not sure you need that configuration copy here: https://github.com/WhoSoup/factom-p2p/blob/master/network.go#L41 because the function argument is already a fresh copy you can work on. Also on line 67/68 you work on the old conf variable instead of your copy myconf which is odd. So I'd suggest sticking to the conf argument all the way?

Generally open source projects add headers to files, so I did that.

More useful, this allows comments on the whole project.

I noticed you have a select with a single case and no default: https://github.com/WhoSoup/factom-p2p/blob/master/controller_routing.go#L31

I think the select is unecessary in this case while adding a layer of indentation.

Comments on https://github.com/WhoSoup/factom-p2p/blob/master/controller_cat.go#L42

1. Shouldn't you return early line 46 after the error?
2. I'd say a peer sending bad data warrant a warning for logging, as it seems the only reason for that would be a rogue node, so I think it should stand out
3. It discrepency of validation between Endpoint#Valid and NewEndpoint() is a bit odd... NewEndpoint allows the creation of an Endpoint with invalid port, and Valid() allows an endpoints with an invalid IP format. It's a bit odd and deceptive, what I'd do is have Valid do a full validation (aka add parsing of the IP address) and then have NewEndpoint rely on this solid Valid to be sure to instanciate a valid endpoint.
4. Why is the Prometheus gauge KnownPeers refreshed at the end? Maybe I am overlooking a side effect, but it doesn't seem that c.peers.Total() would have been modified during the execution of this function?

Commenting on https://github.com/WhoSoup/factom-p2p/blob/master/controller_persistence.go

I don't think Persist is a really good name for the struct: 1. it's generic/abstract, it doesn't say anything of what is being persisted or for what purpose. 2. It's also a verb, that lead the creation of functions like persistData which, unexpectedly, doesn't persist any data :) I was thinking about PeerSnapshot, but feel free to propose something else. Also the boostrap field describe

What is your file naming convention?
I see you have controller, controller_cat, controller_run...
but also parcel, parcelType, parcelChannel...

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.

Can you share a bit more why you feel a CRC32 is needed in your protocol v10? Aren't the error checks of the lower layers good enough for factomd use case?

https://softwareengineering.stackexchange.com/questions/378724/has-crc-check-on-top-of-tcp-any-meaning

I haven't read all the parts of the code yet, but I was wondering if you took into account the case factomd is sitting behind a reverse proxy, for instance a simple nginx? In this case RemoteAddr will be the same for all peers (different ports of course though), could that be a problem anywhere in your code? It may be a problem with the "ban" feature as it becomes impossible to ban a specific remote IP? Probably the existing code has the same "problems", just wondering if you thought about that case.

https://github.com/WhoSoup/factom-p2p/blob/master/networkId.go#L27

Beause it's implemented on the pointer, fmt.Println(MainNet) will print 4276993775 which is probably not what we want. The Stringer interface should be implemented on NetworkID directly:

func (n NetworkID) String() string {
	switch n {
	case MainNet:
		return "MainNet"
	case TestNet:
		return "TestNet"
	case LocalNet:
		return "LocalNet"
	default:
		return fmt.Sprintf("CustomNet ID: %x\n", uint32(n))
	}
}

from https://golang.org/src/encoding/gob/decoder.go

// The Decoder does only basic sanity checking on decoded input sizes,
// and its limits are not configurable. Take caution when decoding gob data
// from untrusted sources.

It seems that the decoder will read up to 8Gb of data before considering it too big. Is there any safe guard against an attack that would submit huge messages? Does the current p2p protect itself from that? What do they mean by Take caution when decoding gob data exactly if limits are not configurable? 🤔

This condition doesn't depend on any variable being changed in the loop: https://github.com/WhoSoup/factom-p2p/blob/master/controller_cat.go#L96 which makes it suspicious to me.
I think what you wanted to do is break if list is long enough, instead of the condition being on tmp?

Unrelated but in the same function:

• I don't think you need to declare i outside of the loop, it should just be defined at the loop scope.
• Maybe rename tmp to peers?

comments on this function: https://github.com/WhoSoup/factom-p2p/blob/master/controller_cat.go#L104

1. peer == nil I am all for defensive coding, but this is the only function you adopt this strategy, is there a reason? Especially if you check the only place this is called, it's impossible for peer to be nil (otherwise it would have paniced earlier anyway). So for consistency, I'd say remove this check, or add it to all the other functions receiving a *Peer.
2. should list need to be an argument of the function? couldnt the signature just be sharePeers(peer *Peer) and then inside just

payload, err := peer.prot.MakePeerShare(c.makePeerShare(peer.Endpoint))

This would actually make your comment // CAT select n random active peers more accurate, because within just the context of this function you do no now that the list you have been passed is n randomly selected active peers, it could be anything.

Collect and Read/Write set counter values in different goroutines, so I believe they should use atomic counters? https://gobyexample.com/atomic-counters

Commenting on https://github.com/WhoSoup/factom-p2p/blob/master/controller_run.go#L9

What made you tie together sequentially those 3 functions (runCatRound, runMetrics, runPing) in the same event loop, while other functions got the right to get their own go routines ? (I say sequentially, but it's not completely true as runMetrics actually spawns its own goroutine)

Also do you really need to constantly poll metrics? I don't see any hook yet, what did you have in mind? Why can't metrics be requested on demand only when necessary by calling makeMetrics()? I guess the answer depends on where/when you were thinking this will be used.

https://github.com/WhoSoup/factom-p2p/blob/master/peer.go#L161
https://golang.org/pkg/bytes/#Equal

Can you also explain me when the loopback case may happen?