Pre2P - RainTree

[Olshansk]

Pocket Network's RainTree implementation as described here atop of the Pre2P module.

Changes

General

• Updated Makefile to generate mocks for the bus & network interfaces
• Improve leader election logging
• Added a ValidatorJsonCompatibleWrapper because the protobuf structure cannot be imported from a json Genesis file

Node Configs

• Add UseRainTree and ConnectionType options to Pre2PConfig
• Reordered the private keys in the configs to make ordering easier to understand in development
• Modified LocalNet to use tcp connection and RainTree by default
• Explicitly specify validators in the LocalNet config

Development Configs

• Modified node.consensus node to enable Delve debugging (tested with VSCode but not Goland)

Pre2P

• Moved pre2p/handlers.go in place to reduce # files
• Abstracted out TCP specific functionality in pre2p to a general TransportLayerConn to enable injecting an override for testing purposes
• Extract the previous approach to networking into stdnetwork, which is replaced by raintree but can be toggled back
• Move validator related helpers into their own utils file

RainTree

• Partial implementation of RainTree implementing the Network interface, which is a substitute for the previous std network
  • Just the end-to-end “happy path”
• End-to-end RainTree tests of the P2P module in isolation for a 4 node network and a 9 node network

Testing

Pre2P tests

$ make test_pre2p

All tests

$ make test_all

LocalNet

Follow the instructions in docs/development/README.md

TODOs

TODO - After review

• Test that Delve debugging also works with Goland

TODO - Did not finish

• RainTree features - need to implement and add tests
  • ACK/Adjust/Resend
  • Daisy chain cleanup (i.e. IGYW messages)
  • Add unit tests for partial visibility
  • Add unit tests for dead/faulty nodes (both deterministic and ephemeral)
• Change the raintTreeNetwork specific methods in addrbook_utils to functions that are not tied to a struct instance
• Unit tests for addrbook_utils.go
• Move logs and errors into a shared file

TODO - Future commits

• Completely refactor typesGenesis.GetNodeState which is a poor overall design
• Various TODO types left throughout the code
• Delete stdnetwork altogether and simplify code by embedding Braintree in place
• Improve logging & telemetry
• Look into uber-go/dig or uber-go/fx to use a proper dependency injection network

[Olshansk]

Re various tasks, done the following:

1. Fixed typos
2. Used _ *testing.T in all testing functions
3. ValId removed where unused
4. createConfigs updated
5. FIxed make confilits

Re structure:

1. I consolidated the unit tests and helpers into 1 file
2. I renamed it so that you can tell that they are unit tests at the module level. Not quite integration or E2E, but also higher than just a basic unit test. I tried using reflections and moving it around, but it ended up being too complex.
3. I updated the graph so you can see how we can eventually remove stdnetwork and that it is a dropin for raintree

├── raintree
│   ├── addrbook_utils.go        # AddrBook utilities
│   ├── addrbook_utils_test.go   # AddrBook utilities unit tests
│   ├── network.go               # Implementation of the Network interface using RainTree's specification
│   └── types
│       └── proto
│           └── raintree.proto
├── module_raintree_test.gogo            # RainTree unit tests at the module level
├── stdnetwork                  # This can eventually be deprecated once raintree is 
│   └── network.go              # Implementation of the Network interface using Golang's std networking lib

2. This doesn't render for me - could be missing something

Let me know if the following image helps clarify things

[derrandz]

I feel slightly strongly about some names in here, let me know if you can make those changes.

Overall, here are my thoughts:

In terms of implementation correctness for the networking part, you are now very familiar with most of my comments and they will be addressed in #86 and #54 and some others perhaps, but I think this makes for a very good prototype overall, as the code remedies itself, so to say.

On the other hand, when it comes to abstraction, I think I was not at ease initially with the two abstractions TransportLayerConn and Network.

As I read through, I think it makes for a good base abstraction and separates things well, decoupling logic from networking, so kudos for that.

Tests were good to read through, my issue was that I had to go look for those prepare and test functions outside of the context of the test case, but you can't complain as that DRYs code well enough. Only that I think it would be more semantic to name things to signify important stages of the test: Setup Run Teardown.

The best part of this PR is the rain tree implementation, names are verbose but everything is very clear.

Good work on this @Olshansk I think this is heading a good direction.

[derrandz]

Suggested change

	type TransportLayerConn interface {
	IsListener() bool
	Read() ([]byte, error)
	Write([]byte) error
	Close() error
	}
	type Transport interface {
	IsListener() bool
	IsDialer() bool
	Accept() (net.Conn, error)
	Dial(...) (net.Conn, error)
	Close() error
	}

[derrandz]

This is a suggestion, I think the name can definitely change, in terms of interface methods and behavior, I am not sure we can do it before #86 so we can keep it as is until then.

[Olshansk]

1. I'm open to changing TransportLayerConn to Transport, but can you get a 2nd opinion from someone or a reference showing it's the way to go?

2. Do you think it's a good idea to start adding unused interface functions here before we have a need for them?

[derrandz]

1. Does it need a reference? I think it's obvious enough in my opinion and we should just rename it. We are dealing with a networking layer, that is transport, and this is a sufficient semantic queue to infer other things such as: a transport produces connections, a transport listens or dials, etc... But, since you are interested in references, I remember a nice little microservices go framework that used similar naming. Please check it at this link.

2. No, I think we can reserve this as a TODO until we execute [Pre2P] Refine the P2P module listening and IO behaviour #86

[Olshansk]

Renamed TransportLayerConn to Transport and pushed.

[Olshansk]

@derrandz

Legend for my response on review comments

• Using ACK as neither agreement nor disagreement, but rather as "let's discuss later" for comments outside of the scope of this PR.
• Using DONE where changes were applied
• Using AGGREGATE when multiple comments span the same topic, but we still need to follow up on it afterwards
• Left some open comments inline where appropriate

Some feedback review:

• If something is a rename, and it spans multiple files, it’s better to leave it as a comment than a suggestion. Using Github’s “commit suggestion” feature would make it break.
• As much as I appreciate the positive feedback, leaving too many inline “I like this part” messages added noise to the review. I believe having one left at the top level is sufficient.

Next steps and open questions remaining:

1. If you feel I answered your comment (which I did not resolve), please either resolve it or leave a follow-up comment. If the follow-up comment is non-actionable, please resolve.
2. The reason for NetworkSend and NetworkBroadcast is because they are part of the Network interfaces and not the P2PModules Interface, which has a Broadcast and Send method. I did not want the two to be confused. Lmk what you think

[derrandz]

@Olshansk Thanks for the feedback and for acting on the review.

I've resolved the parts I am okay with. Left open just one that has an actionable comment. There is one resolved conversation in particular in which I left a clarifying comment to make the case for renaming is this.

Thank you.

[Olshansk]

@derrandz @andrewnguyen22 I believe I've addressed all the comments, so please re-review/approve if everything looks good to merge.

[derrandz]

Hey @Olshansk I've created #93 for the changelog task and tagged it as good starter task for the community as we've discussed before in this conversation.

Since that conversation is linked as an origin document to the issue, referencing this issue on the PR will leave good trace-ability.