[STALE - DO NOT REVIEW][P2P] Improve RainTree Testing Framework (Issue#85)

[Olshansk]

Description

Add documentation and improve the P2P RainTree testing framework.

Issue

Incremental step to fixing issue/85

Type of change

Please mark the options that are relevant.

• New feature, functionality or library
• Bug fix (non-breaking change which fixes an issue)
• Code health or cleanup
• Major breaking change
• Documentation
• Other: <REPLACE_ME>

List of changes

Primary Changes

• Add a telemetry event for RainTree send which can be used for:

  • Telemetry of how RainTree is performing
  • More deterministic testing (i.e. matching on exact # messages received and sent over the network)

• Add a bit of documentation on how the test framework works in the README

• Add inline documentation related to details of the test framework

• Reorganize helpers for the testing framework so its easier to follow

• Single source of truth for the serviceURL between the shared code and the p2p testing framework

Flyby changes

• Add a few TODOs throughout the code
• Remove p2pConfig in the p2p module struct
• Group p2p tests together in Makefile
• Remove some legacy pre2p code

Testing

• make test_p2p
• make test_all
• LocalNet w/ all of the steps outlined in the README

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling
• If applicable, I have made corresponding changes to related local or global README
• If applicable, I have added new diagrams using mermaid.js
• If applicable, I have added tests that prove my fix is effective or that my feature works

[Olshansk]

@andrewnguyen22 Per your request to add documentation around how the test framework works, I realized there are ways to make the framework:

• Easier to understand
• More robust and deterministic
• More extensible
• More visibility by adding telemetry, which can also be used for testing

I think that this PR will require a fair amount of thought and effort to understand, so please spend some time to look over it when you have a chance. I think it makes our testing much more robust and will give us the power to track concrete visibility into how messages are being sent.

Once this is reviewed and merged into main, we can continue iterating on the redundancy layer in a follow up PR. You might need to update TestRainTreeCommConfig to account for redundancy logic or simply update the # network reads/writes appropriately.

An example (from the tests):

func TestRainTreeNetworkCompleteTwentySevenNodes(t *testing.T) {
	// 	                                                                                                                    val_1
	// 	                                     ┌────────────────────────────────────────────────────────────────────────────────┴───────────────────────────────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────┐
	//                                    val_10                                                                                                                   val_1                                                                                                       val_19
	//            ┌──────────────────────────┴──────────────┬──────────────────────────────────────┐                                         ┌────────────────────────┴────────────┬──────────────────────────────────┐                                  ┌────────────────────────┴──────────────┬────────────────────────────────────┐
	//          val_16                                    val_10                                 val_22                                     val_7                                 val_1                             val_13                             val_25                                  val_19                                val_4
	//   ┌────────┴─────┬───────────┐              ┌────────┴─────┬───────────┐           ┌────────┴─────┬───────────┐              ┌────────┴────┬──────────┐             ┌───────┴────┬─────────┐          ┌────────┴─────┬──────────┐         ┌───────┴─────┬──────────┐             ┌────────┴─────┬───────────┐          ┌───────┴────┬──────────┐
	// val_20         val_16      val_24         val_14         val_10      val_18      val_26         val_22      val_12         val_11        val_7      val_15        val_5        val_1     val_9      val_17         val_13     val_3     val_2         val_25     val_6         val_23         val_19      val_27     val_8        val_4      val_21
	originatorNode := validatorId(1)
	var expectedCalls = TestRainTreeCommConfig{
		originatorNode:  {0, 6}, // {num_messages_received_over_network, num_messages_sent_over network}
		validatorId(2):  {1, 0},
		validatorId(3):  {1, 0},
		validatorId(4):  {1, 2},
		validatorId(5):  {1, 0},
		validatorId(6):  {1, 0},
		validatorId(7):  {1, 2},
		validatorId(8):  {1, 0},
		validatorId(9):  {1, 0},
		validatorId(10): {1, 4},
		validatorId(11): {1, 0},
		validatorId(12): {1, 0},
		validatorId(13): {1, 2},
		validatorId(14): {1, 0},
		validatorId(15): {1, 0},
		validatorId(16): {1, 2},
		validatorId(17): {1, 0},
		validatorId(18): {1, 0},
		validatorId(19): {1, 4},
		validatorId(20): {1, 0},
		validatorId(21): {1, 0},
		validatorId(22): {1, 2},
		validatorId(23): {1, 0},
		validatorId(24): {1, 0},
		validatorId(25): {1, 2},
		validatorId(26): {1, 0},
		validatorId(27): {1, 0},
	}
	testRainTreeCalls(t, originatorNode, expectedCalls)
}

[andrewnguyen22]

I see the documentation and this is a step in the right direction, the mocking is difficult for me to follow the path.

Can we discuss offline sometime in order to move this forward?

[Olshansk]

Can we discuss offline sometime in order to move this forward?

Yea, definitely. I might not be able to do so this week but we'll make time to go through it.

the mocking is difficult for me to follow the path.

Since I know you're blocked by my reviews, and we can't sync offline on it yet, I was hoping to make a request that you try to dive deeper into the library in the meantime.

For example, pull the branch and:

• Modify unclear variable names so they're more semantic and easy to understand
• Modify/refactor function names that are unclear
• Add comments / TODOs that don't make sense

I realize this ask is a bit pedantic and very non-fun, but would really appreciate if you do it. The main reasoning is that I tried to make it easily interpretable and understandable, but if you cannot understand it, then I don't think anyone else will. Given that the consensus module level testing follows a similar approach, this is an extremely big problem for the long-term health of the codebase.

[andrewnguyen22]

Review when you can

[Olshansk]

@andrewnguyen22 Thanks for the questions/comments/modifications. Made me rethink the readability after you asked, so I think we're heading in the right direction here.

Aside from

// TODO (Olshansk) why did these values change in between commits?

I made a modification to the business logic of TestRainTreeConfig. In particular, the way I was computing numNetworkWrites was incorrect. Rather than counting the number of writes that node makes to other nodes, it was computing the number of writes other nodes due to it, making it always equal to num reads.

// TODO (Olshansk) explain how these expected values are derived

The way we derive the number of reads & writes is a function of how RainTree works. E.g. if we have N nodes, with some names, and we determine who the originator is, we have to manually craft how many reads and writes each node does. 

Hence, the little tree diagram above each test.

I used my python simulator to help me craft the tests. Do you think we should productionize it?

[andrewnguyen22]

Discussed offline recently in protocol hour - this PR is stale right now.

Need to see changes and conflict free before review

[codecov-commenter]

Codecov Report

Base: 0.00% // Head: 43.91% // Increases project coverage by +43.91% 🎉

Coverage data is based on head (bd88f47) compared to base (983f480).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##           main     #179       +/-   ##
=========================================
+ Coverage      0   43.91%   +43.91%     
=========================================
  Files         0       24       +24     
  Lines         0     1749     +1749     
=========================================
+ Hits          0      768      +768     
- Misses        0      941      +941     
- Partials      0       40       +40     

Impacted Files	Coverage Δ
p2p/raintree/network.go	20.40% <0.00%> (ø)
utility/types/vote.go	100.00% <0.00%> (ø)
persistence/types/base_actor.go	0.00% <0.00%> (ø)
persistence/types/util.go	0.00% <0.00%> (ø)
utility/types/message.go	72.00% <0.00%> (ø)
utility/types/mempool.go	0.00% <0.00%> (ø)
shared/indexer/indexer.go	59.40% <0.00%> (ø)
utility/types/transaction.go	60.91% <0.00%> (ø)
p2p/raintree/addrbook_utils.go	86.66% <0.00%> (ø)
p2p/raintree/peers_manager.go	89.62% <0.00%> (ø)
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Olshansk]

This will be descoped into several different issues:

1. Adding the Jupyter notebook to generate tests
2. Improving the counters & adding documentation (in [P2P] Improve RainTree counter mechanism in testing framework #304)
3. Improving / adding tests after (1) and (2) are done

[Olshansk]

cc @jessicadaugherty to help with create tickets for (1) and (3) above