Feat/en 777 refactor p2p #58

iulianpascalau · 2019-02-08T13:36:22Z

Done P2P package refactoring, syntax should be clearer now, peer discovery mechanism was changed to kad-dht instead of mdns, dropped Creator interface, dropped Topic class, added send direct capabilities, added send data throttling capabilities.
~~All it remains to do is start-up bootstrap.~~

interceptors/resolvers refactor adapted integrationTests, node, spos packages

- changed the pubsub system to gossipsub - code cleanup in autodiscovery example

- minor code refactoring on tests

- added TrimConnections to Messenger interface

- added TrimConnections to Messenger interface - minor code changes (removed comments, added struct description)

- every test in libp2pMessenger_test.go will close its created messenger implementations - modified messengerStub to implement Messenger interface (added TrimConnections func)

ccorcoveanu

This is a partial review, new comments will be added

p2p/libp2p/directSender.go

p2p/libp2p/libp2pMessenger.go

ccorcoveanu · 2019-02-13T15:32:20Z

p2p/libp2p/libp2pMessenger.go

+}
+
+// BroadcastData tries to send a byte buffer onto a topic
+func (p2pMes *libp2pMessenger) BroadcastData(pipe string, topic string, buff []byte) {


From what I see in the usage of this function the pipe and topic are (or can be) equivalent. Ca we use the topic as pipe by default and make the signature and the use of this function simpler?
Also, the name can be simply Broadcast

The first iteration of implementing pipes was made as you recommend. When it was time to apply the new implementation on node.GenerateAndSendBulkTransaction, I have decided that those bulk transactions will be sent onto another specially created pipe as to not clog the regular transaction pipe on which the interceptors/resolvers are wired. So that is why there are pipe and topic parameters.
BroadcastData became Broadcast.

Just a suggestion, what if we have:

func (netMes *networkMessenger) Broadcast(topic string, buff []byte) { netMes.BroadcastWithPipe(topic, topic, buff) } func (netMes *networkMessenger) BroadcastWithPipe(pipe string, topic string, buff []byte) {}

I don't have a strong opinion about this but I'm aiming to have the signature as brief as possible for the regular caller. If someone is just interesting in making a call to broadcast maybe he should not care what are those pipes and what he should pass as a parameter there

waiting for @AdoAdoAdo's review.

I incline towards corcos 2 methods: Broadcast(topic, buffer) and just on the "temporary" GenerateAndSendBulk should the call the one with a different topic. All other broadcasts should not know about the pipe.

Ok, will add second method

This is not yet added

p2p/libp2p/libp2pMessenger.go

- some name changes (networkMessengers + constructors), directSender's Send function - minor code optimization (dataThrottle)

ccorcoveanu

Q: Unrelated to this PR necessarily, but I'll leave it here since this changes affect the containing package. In (for ex.) process/block/check.go, we have wrappers over other structs with no added fields. Can't the methods be added directly to the to the original structure so we would avoid initialisation of other types?

p2p/libp2p/netMessenger.go

p2p/libp2p/netMessenger_test.go

p2p/dataThrottle/sendDataThrottle.go

p2p/example/libp2p/autodiscovery/main.go

process/block/interceptors.go

iulianpascalau · 2019-02-14T11:19:54Z

Q: Unrelated to this PR necessarily, but I'll leave it here since this changes affect the containing package. In (for ex.) process/block/check.go, we have wrappers over other structs with no added fields. Can't the methods be added directly to the to the original structure so we would avoid initialisation of other types?

Indeed, this is an issue. Will be taken into account after the architecture will be refactored.

- some name changes (networkMessengers + constructors), directSender's Send function - minor code optimization (dataThrottle, block interceptor), added types for 2 handler functions in netMessenger - minor comments updates in sendDataThrottle, autodiscovery/main.go

…etwork/elrond-go-sandbox into feat/EN-777-refactor-p2p # Conflicts: # node/mock/messengerStub.go # p2p/libp2p/export_test.go # p2p/libp2p/netMessenger.go # p2p/libp2p/netMessenger_test.go # p2p/p2p.go # process/mock/messengerStub.go

raduchis

Reviewed until process/block/interceptors.go.

consensus/spos/sposConsensusWorker.go

consensus/spos/sposConsensusWorker_test.go

integrationTests/block/common.go

integrationTests/transaction/interceptedBulkTx_test.go

p2p/dataThrottle/sendDataThrottle.go

p2p/dataThrottle/sendDataThrottle_test.go

p2p/example/libp2p/autodiscovery/main.go

raduchis

Reviewed P2P changes

process/block/interceptors.go

process/interceptor/topicInterceptor.go

process/resolver/topicResolver.go

- changed the callback mechanism when receiving a message from pointer function to interface - Messenger.ConnectedPeers() will return an unique peer list, added test process: - refactored interceptors/resolvers as to not use the pointer function mechanism for message callback - moved containers inside factory package, created tests - changed interceptorsResolvers factory's name + code refactoring to comply with the new callback mechanism - added interface for a subset of Messenger's methods node: - changed factory option name + added more tests spos: - modified SposConsensWorker to implement p2p.TopicValidatorHandler

raduchis

Second review

cmd/bootnode/main.go

consensus/spos/sposConsensusWorker_test.go

node/node.go

raduchis · 2019-02-20T14:59:49Z

p2p/libp2p/netMessenger.go

@@ -334,14 +334,20 @@ func (netMes *networkMessenger) IsConnected(peerID p2p.PeerID) bool {
 func (netMes *networkMessenger) ConnectedPeers() []p2p.PeerID {
 	peerList := make([]p2p.PeerID, 0)

+	connectedPeers := make(map[p2p.PeerID]struct{})


Was this change really needed? Changing from a list to a map... and then adding the peers to a list?

The map is used for removing p2p.PeerID duplicates. Without it, before every peerList append I should iterate the peerList to check if the peerID needs to be added. This check might prove to be more costly if we have a lot of connections/peers

Could you be connected 2 times to the same peerID?

Technically yes, libp2p might have n connections for the same peerID, as far as I could see in their implementation.

Maybe you could use the Peerstore().Peers() method to get the slice of peers and then check for the connection? I think Peers() should return a unique list of peer ids

Peers() returns all known peers. I might add a peer to peerstore but not connect to it.

raduchis · 2019-02-20T15:06:19Z

p2p/libp2p/netMessenger_test.go

-	err := mes.SetTopicValidator("test", func(message p2p.MessageP2P) error {
-		return nil
-	})
+	err := mes.SetTopicValidator("test", &mock.TopicValidatorHandlerStub{})


I would call the SetTopicValidator AddTopicValidator because there is a list of topic validators.

There is a possibility to call SetTopicValidator("test", nil) in which case it will remove the topic validator and then the next call to SetTopicValidator with a non nil validator will set the new validator for that topic. So, can be left as it is or split in 2 functions: SetTopicValidator and ResetTopicValidator.

I also think it should be called AddTopicValidator or RegisterTopicValidator. Set implies that the only validator will be the one you add at that point.

Ok, so we can follow libp2p's model: RegisterTopicValidator + UnregisterTopicValidator.
Does this sound better?

ok, it sounds better

p2p/p2p.go

process/interface.go

- added HeaderResolver interface - added reference of resolvers in bootstrap struct as to remove the request handlers from node.go - minor name changes

ccorcoveanu

Only couple small changes, the rest looks good

ccorcoveanu · 2019-02-21T11:54:07Z

consensus/spos/sposConsensusWorker.go

-// ReceivedMessage method redirects the received message to the channel which should handle it
-func (sposWorker *SPOSConsensusWorker) ReceivedMessage(message p2p.MessageP2P) error {
+// Validate method redirects the received message to the channel which should handle it
+func (sposWorker *SPOSConsensusWorker) Validate(message p2p.MessageP2P) error {


Why Validate if it actually does something else?

From the P2P perspective, it needs to validate the message. If the Validate func returns error, messenger won't broadcast the message to its peers. Now, since we already parse the message (in case of all interceptors) and check different things like nil fields, signature, etc, why not store the produced object? Otherwise we should have another registered object/function just for storing.

If it's called Validate it should not do anything else. The flow is validate -> do something if the data was correct. If somebody sees a public Validate method and wants to validate that some data is correct it will end up writing in a channel. Now he must look for whoever reads from that channel and sees if it's a desired behaviour or not.

Changed to ProcessReceivedMessage

ccorcoveanu · 2019-02-21T12:52:27Z

p2p/libp2p/netMessenger.go

@@ -334,14 +334,20 @@ func (netMes *networkMessenger) IsConnected(peerID p2p.PeerID) bool {
 func (netMes *networkMessenger) ConnectedPeers() []p2p.PeerID {
 	peerList := make([]p2p.PeerID, 0)

+	connectedPeers := make(map[p2p.PeerID]struct{})


Maybe you could use the Peerstore().Peers() method to get the slice of peers and then check for the connection? I think Peers() should return a unique list of peer ids

p2p/libp2p/netMessenger.go

ccorcoveanu · 2019-02-21T13:02:35Z

p2p/libp2p/netMessenger_test.go

-	err := mes.SetTopicValidator("test", func(message p2p.MessageP2P) error {
-		return nil
-	})
+	err := mes.SetTopicValidator("test", &mock.TopicValidatorHandlerStub{})


I also think it should be called AddTopicValidator or RegisterTopicValidator. Set implies that the only validator will be the one you add at that point.

process/block/interceptors/common.go

- names changed for TopicValidator, Messenger.RegisterTopicValidater and Messenger.UnregisterTopicValidator, loadBalancer

# Conflicts: # cmd/bootnode/main.go # consensus/spos/sposConsensusWorker.go # consensus/spos/sposConsensusWorker_test.go # integrationTests/block/interceptedRequestTxBlockBodyNet_test.go # integrationTests/block/testInitializer.go # integrationTests/transaction/interceptedTxMem_test.go # integrationTests/transaction/testInitializer.go # node/node_test.go # process/block/interceptedBlocks.go # process/block/interceptedBlocks_test.go # process/block/interceptors.go # process/block/interceptors_test.go # process/factory/factory.go # process/factory/factory_test.go # process/transaction/interceptedTransaction.go # process/transaction/interceptor.go # process/transaction/interceptor_test.go

raduchis

Fixed required changes

- changed method names in Messenger (ProcessReceivedMessage, RegisterMessageProcessor, UnregisterMessageProcessor) - changed TopicValidator to MessageProcessor - added new broadcast func in Messenger that sends on the topic's pipe

ccorcoveanu

As discussed, we should add a configuration option to be able to use mdns when desired. This would also help in merging this branch without breaking master

p2p/libp2p/netMessenger.go

iulianpascalau · 2019-02-22T14:54:19Z

As discussed, we should add a configuration option to be able to use mdns when desired. This would also help in merging this branch without breaking master

PR description stated that bootstrap will be unavailabe
Will add option to choose between the discovery mechanism the node should use (for now 2 options will be available: off and mdns)

- adding the possibility of peer discovery using either kad-dht or mdns

- added the possibility of peer discovery using either kad-dht or mdns integrationTests: - added integration test that tests connecting to n peers and ensuring a message is being broadcast to all peers main: - added flag for choosing the peer discovery mode, default is mdns

- fixed messenger test as bootstrapCalled func in mock was nil

iulianpascalau · 2019-02-24T10:34:54Z

As discussed, we should add a configuration option to be able to use mdns when desired. This would also help in merging this branch without breaking master

PR description stated that bootstrap will be unavailabe
Will add option to choose between the discovery mechanism the node should use (for now 2 options will be available: off and mdns)

Added the possibility to chose from "off", "mdns" and "kad-dht" discovery methods. The default is mdns. Added integration test to check if 20 peers connect to each other and a message broadcast by one of the peers should reach all other peers.

- made the peerDiscovery test if all messenger got the message by checking a flag p2p: - changed some tests as Bootstrap initial test may not pass each time

iulianpascalau · 2019-02-24T11:56:15Z

There are 2 tests that do not pass when running with race detector on:

TestLibp2pMessenger_SendDirectWithMockNetToConnectedPeerShouldWork as the bug fix pushed to go-libp2p is not included in a release (yet)
TestPeerDiscoveryAndMessageSending that fails because of how mdns library is implemented.

- typos fixed

ccorcoveanu

Mdns is implemented now but not triggered when the option is used

p2p/libp2p/netMessenger.go

raduchis · 2019-02-25T14:00:18Z

p2p/libp2p/netMessenger.go

@@ -70,7 +77,13 @@ func NewMockNetworkMessenger(ctx context.Context, mockNet mocknet.Mocknet) (*net
 		return nil, err
 	}

-	mes, err := createMessenger(ctx, h, false, loadBalancer.NewOutgoingPipeLoadBalancer())
+	mes, err := createMessenger(


Is there a reason why we have the MockNetworkMessenger inside the networkMessenger struct? I would create a different file for this structure.

will be renamed into NewMemoryMessenger and placed in memMessenger.go

# Conflicts: # consensus/spos/errors.go # consensus/spos/sposConsensusWorker.go # consensus/spos/sposConsensusWorker_test.go # integrationTests/transaction/interceptedTxMem_test.go # node/node.go # process/interface.go # process/sync/block.go # process/sync/block_test.go

p2p: - created memMessenger file, NewMemoryMessenger func

raduchis

Fixed all required changes

Update 2020.12.11

iulianpascalau added 5 commits February 6, 2019 14:20

p2p: done complete refactoring of the package

e335df7

p2p: done complete refactoring of the package

8cea60f

interceptors/resolvers refactor adapted integrationTests, node, spos packages

p2p:

00c3f5d

- changed the pubsub system to gossipsub - code cleanup in autodiscovery example

p2p, process:

a95cb70

- minor code refactoring on tests

integration tests: changed timeout on bulk tx generation to 1min

ed2f388

iulianpascalau added the type:feature New feature or request label Feb 8, 2019

iulianpascalau self-assigned this Feb 8, 2019

iulianpascalau added 4 commits February 11, 2019 17:01

p2p:

a1d71ac

- added TrimConnections to Messenger interface

Merge branch 'master' into feat/EN-777-refactor-p2p

d5d1f80

p2p:

91aa0ed

- added TrimConnections to Messenger interface - minor code changes (removed comments, added struct description)

p2p:

cd252a3

- every test in libp2pMessenger_test.go will close its created messenger implementations - modified messengerStub to implement Messenger interface (added TrimConnections func)

iulianpascalau requested review from AdoAdoAdo and ccorcoveanu February 12, 2019 15:02

ccorcoveanu reviewed Feb 13, 2019

View reviewed changes

p2p - fix after review:

b2809af

- some name changes (networkMessengers + constructors), directSender's Send function - minor code optimization (dataThrottle)

ccorcoveanu reviewed Feb 14, 2019

View reviewed changes

iulianpascalau added 2 commits February 14, 2019 14:09

raduchis self-requested a review February 18, 2019 07:25

raduchis requested changes Feb 18, 2019

View reviewed changes

raduchis requested changes Feb 19, 2019

View reviewed changes

iulianpascalau added 2 commits February 20, 2019 14:13

Gopkg.lock file

168fdaf

raduchis requested changes Feb 21, 2019

View reviewed changes

process:

9124587

- added HeaderResolver interface - added reference of resolvers in bootstrap struct as to remove the request handlers from node.go - minor name changes

ccorcoveanu requested changes Feb 21, 2019

View reviewed changes

raduchis previously approved these changes Feb 21, 2019

View reviewed changes

p2p:

39c4a64

- names changed for TopicValidator, Messenger.RegisterTopicValidater and Messenger.UnregisterTopicValidator, loadBalancer

iulianpascalau dismissed raduchis’s stale review via 39c4a64 February 21, 2019 17:36

iulianpascalau added 2 commits February 21, 2019 20:04

Merged with master modifications

c7ef353

raduchis self-requested a review February 22, 2019 11:38

raduchis previously approved these changes Feb 22, 2019

View reviewed changes

p2p:

de14c57

- changed method names in Messenger (ProcessReceivedMessage, RegisterMessageProcessor, UnregisterMessageProcessor) - changed TopicValidator to MessageProcessor - added new broadcast func in Messenger that sends on the topic's pipe

iulianpascalau dismissed raduchis’s stale review via de14c57 February 22, 2019 13:41

ccorcoveanu requested changes Feb 22, 2019

View reviewed changes

p2p/libp2p/netMessenger.go Outdated Show resolved Hide resolved

ccorcoveanu closed this Feb 22, 2019

ccorcoveanu reopened this Feb 22, 2019

iulianpascalau added 3 commits February 22, 2019 18:14

p2p:

8a00585

- adding the possibility of peer discovery using either kad-dht or mdns

p2p:

5df08c4

- added the possibility of peer discovery using either kad-dht or mdns integrationTests: - added integration test that tests connecting to n peers and ensuring a message is being broadcast to all peers main: - added flag for choosing the peer discovery mode, default is mdns

node:

3188087

- fixed messenger test as bootstrapCalled func in mock was nil

integrationTests:

3092015

- made the peerDiscovery test if all messenger got the message by checking a flag p2p: - changed some tests as Bootstrap initial test may not pass each time

integrationTests:

1add3a7

- typos fixed

raduchis self-requested a review February 25, 2019 08:27

ccorcoveanu requested changes Feb 26, 2019

View reviewed changes

p2p/libp2p/netMessenger.go Show resolved Hide resolved

p2p/libp2p/netMessenger.go Show resolved Hide resolved

raduchis reviewed Feb 26, 2019

View reviewed changes

iulianpascalau added 2 commits February 26, 2019 10:41

Merged with master

7771431

p2p: - created memMessenger file, NewMemoryMessenger func

raduchis approved these changes Feb 26, 2019

View reviewed changes

ccorcoveanu approved these changes Feb 26, 2019

View reviewed changes

iulianpascalau merged commit e078d55 into master Feb 26, 2019

raduchis deleted the feat/EN-777-refactor-p2p branch April 10, 2019 08:50

iulianpascalau added a commit that referenced this pull request Mar 15, 2021

Merge pull request #58 from ElrondNetwork/update-2020.12.11

9d6b220

Update 2020.12.11

Feat/en 777 refactor p2p #58

Feat/en 777 refactor p2p #58

Conversation

iulianpascalau commented Feb 8, 2019 • edited Loading

ccorcoveanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iulianpascalau Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccorcoveanu left a comment

Choose a reason for hiding this comment

iulianpascalau commented Feb 14, 2019

raduchis left a comment

Choose a reason for hiding this comment

raduchis left a comment

Choose a reason for hiding this comment

raduchis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccorcoveanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raduchis left a comment

Choose a reason for hiding this comment

ccorcoveanu left a comment

Choose a reason for hiding this comment

iulianpascalau commented Feb 22, 2019

iulianpascalau commented Feb 24, 2019

iulianpascalau commented Feb 24, 2019 • edited Loading

ccorcoveanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raduchis left a comment

Choose a reason for hiding this comment

iulianpascalau commented Feb 8, 2019 •

edited

Loading

iulianpascalau Feb 14, 2019 •

edited

Loading

iulianpascalau commented Feb 24, 2019 •

edited

Loading