Vishal/refactor network #48

vishalchangrani · 2020-10-06T22:02:44Z

This PR does the following:

Subscribe the network to the epoch transition events.
Refactor some the network test utils (added testUtil.go which should eventually replace handler.go).
Added two test cases to test node addition and node removal during epoch but have marked both them to skip since the discovery mechanism is yet to be updated to discover new nodes and evict unwanted ones.

vishalchangrani · 2020-10-06T22:04:33Z

cmd/scaffold.go

@@ -195,12 +189,15 @@ func (fnb *FlowNodeBuilder) enqueueNetworkInit() {
 			return nil, fmt.Errorf("could not create topology: %w", err)
 		}

-		net, err := libp2p.NewNetwork(fnb.Logger, codec, participants, fnb.Me, fnb.Middleware, 10e6, nodeTopology, fnb.Metrics.Network)
+		net, err := libp2p.NewNetwork(fnb.Logger, codec, fnb.State, fnb.Me, fnb.Middleware, 10e6, nodeTopology, fnb.Metrics.Network)


network now needs the State instead of the participants list since the participant list changes epoch to epoch.

vishalchangrani · 2020-10-06T22:04:51Z

cmd/scaffold.go

 		if err != nil {
 			return nil, fmt.Errorf("could not initialize network: %w", err)
 		}

 		fnb.Network = net
+
+		fnb.ProtocolEvents.AddConsumer(net)


Added the network as a consumer of the epoch events

vishalchangrani · 2020-10-06T22:09:35Z

go.mod

@@ -34,6 +34,7 @@ require (
 	github.com/onflow/flow-go/crypto v0.9.4
 	github.com/onflow/flow/protobuf/go/flow v0.1.7
 	github.com/opentracing/opentracing-go v1.2.0
+	github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2


In the past, when the network unit test ran, the ports were automatically selected when the libp2p host started by specifying the listen address of 0.0.0.0:0. While that was convenient in terms of getting a free local port to listen on, the test setup became overly complex. The test had to create a half baked network, start the libp2p host and go back and update the peer ids.
I changed it so that the test asks for open ports on a system using this third party library and uses those passing it down as the flow.Identity from network -> middleware -> libp2p.

vishalchangrani · 2020-10-06T22:10:40Z

network/gossip/libp2p/discovery.go

 }

-func NewDiscovery(log zerolog.Logger, overlay middleware.Overlay, me flow.Identifier, done chan struct{}) *Discovery {
-	d := &Discovery{overlay: overlay, log: log, me: me, done: done}


done is replaced by a context

vishalchangrani · 2020-10-06T22:12:25Z

network/gossip/libp2p/middleware/middleware.go

-	// Topology returns the identities of a uniform subset of nodes in protocol state
-	Topology() (map[flow.Identifier]flow.Identity, error)
+	// Topology returns an identity list of nodes which this node should be directly connected to as peers
+	Topology() (flow.IdentityList, error)


returning a simple flow.IdentityList instead of a map.

vishalchangrani · 2020-10-06T22:14:24Z

network/gossip/libp2p/test/epochtransition_test.go

+
+// TestNewNodeAdded tests that an additional node in a new epoch get connected to other nodes and can exchange messages
+func (ts *EpochTransitionTestSuite) TestNewNodeAdded() {
+	ts.T().Skip("skipping till discovery is updated to discover newly added nodes")


these test fail since currently there is no way to trigger the discovery on demand. Once https://github.com/dapperlabs/flow-go/issues/4893 is implemented, these should pass.

vishalchangrani · 2020-10-06T22:15:32Z

network/gossip/libp2p/test/testUtil.go

@@ -0,0 +1,191 @@
+package test


the new and improved helper to setup the network for network related unit tests

network/gossip/libp2p/libp2pNode.go

jordanschalm · 2020-10-07T18:57:00Z

network/gossip/libp2p/network.go

@@ -450,3 +477,16 @@ func (n *Network) queueSubmitFunc(message interface{}) {
 			Msg("failed to process message")
 	}
 }
+
+func (n *Network) EpochTransition(newEpoch uint64, first *flow.Header) {


We should be listening to EpochSetupPhaseStarted event instead of this one, so that nodes are valid network members prior to their epoch starting. We will also need to account for starting up after this event has been emitted.

// handle the case that we are running when this phase change occurs func (n *Network) EpochSetupPhaseStarted(...) { n.ids = n.state.Final.Identities().Union(n.state.Final().Epochs().Next().InitialIdentities()) } // in the network initialization -- handle the case where we start up after the phase change has occurred // since we won't get the event when this happens phase := n.state.Final().Phase() if phase == flow.EpochPhaseSetup || phase == flow.EpochPhaseCommitted { // include next epoch identitites }

I'm doing something similar so implemented Union in #51

jordanschalm · 2020-10-07T19:02:18Z

network/gossip/libp2p/test/helper.go

+		if dryrun {
+			state = generateStateSnapshot(ids)
+		} else {
+			identities := make(flow.IdentityList, 0)
+			state = generateStateSnapshot(identities)
+		}


When dryrun is specified we use the passed-in ids and otherwise we generate an empty list? Kind of seems backward to me 🤔

so we first start the network with empty identities if dryrun is false, then when the network is up and running we modify the identities with the actual identities with real ip and port. But, as you pointed out - this is confusing and also does not depict the real usage where the ids will already have IP and port.
I have removed helper.go all together now.

jordanschalm · 2020-10-07T19:06:45Z

network/gossip/libp2p/test/testUtil.go

+
+	"github.com/phayes/freeport"
+	"github.com/rs/zerolog"
+	mock2 "github.com/stretchr/testify/mock"


Goland auto-rename? Probably want to alias /module/mock to module or mockmodule

network/gossip/libp2p/test/testUtil.go

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

…och' into vishal/refactor_network

…rk; refactor

…nto vishal/refactor_network

yhassanzadeh13

Nice job! Just added a few comments, please consider them before merging.

yhassanzadeh13 · 2020-10-10T00:20:52Z

network/gossip/libp2p/discovery.go

+func (d *Discovery) Advertise(ctx context.Context, _ string, _ ...discovery.Option) (time.Duration, error) {
+	err := ctx.Err()
+	if err != nil {
+		return 0, err


would be good to append some metadata information on the place where the error happens in both this and subsequent functions: return 0, fmt.Errorf("could not advertise the node: %w", err). Since if such an error happens we are sending a zero delay for the next subsequent call. So it would be good to have a clue on it.

yhassanzadeh13 · 2020-10-10T00:22:32Z

network/gossip/libp2p/libp2pNode.go

+func (p *P2PNode) IsConnected(address NodeAddress) (bool, error) {
+	pInfo, err := GetPeerInfo(address)
+	if err != nil {
+		return false, err


same error appending here.

yhassanzadeh13 · 2020-10-10T00:23:24Z

network/gossip/libp2p/libp2pNode.go

+	}
+	// query libp2p for connectedness status of this peer
+	isConnected := p.libP2PHost.Network().Connectedness(pInfo.ID) == network.Connected
+	return isConnected, nil


maybe do this: return p.libP2PHost.Network().Connectedness(pInfo.ID) == network.Connected, nil

I think both of us think alike - I had exactly that way and Jordan suggested adding a new variable isConnected. I think, the additional variable makes the code a little more readable.

yhassanzadeh13 · 2020-10-10T00:28:04Z

network/gossip/libp2p/network.go

+	n.Unlock()
+
+	// update the allow list
+	err := n.mw.UpdateAllowList()


shouldn't we also cover this method with a Lock?

UpdateAllowList calls back network.Identity which in turn does a lock. Hence, it would end up in deadlock if I covered UpdateAllowList with it.

yhassanzadeh13 · 2020-10-10T00:32:14Z

network/gossip/libp2p/network.go

+	// current epoch phase
+	phase, err := state.Final().Phase()
+	if err != nil {
+		log.Err(err).Msg("failed to retrieve epoch phase")


shouldn't we return an error at this phase and do not continue?

yhassanzadeh13 · 2020-10-10T00:37:58Z

network/gossip/libp2p/test/testUtil.go

+var rootBlockID = unittest.IdentifierFixture().String()
+
+// generateIDs generate flow Identities with a valid port and networking key
+func generateIDs(n int) ([]*flow.Identity, []crypto.PrivateKey, error) {


I would suggest passing a t *testing.T here as a parameter. Then instead of returning the error, you can fail the test at the line it happens. It would be easier to track.

makes sense..will change it to accept t and not return an error

yhassanzadeh13 · 2020-10-10T00:38:44Z

network/gossip/libp2p/test/testUtil.go

+}
+
+// generateMiddlewares creates and initializes middleware instances for all the identities
+func generateMiddlewares(log zerolog.Logger, identities []*flow.Identity, keys []crypto.PrivateKey) ([]*libp2p.Middleware, error) {


Same here for *testing.T and the rest.

…nto vishal/refactor_network

jordanschalm

Tried to clear up how and when the node identities are added with a diagram. Maybe we can have a quick call to make sure we're on the same page and I can backfill more details into the notion doc.

The implementation looks correct to me, but the test case makes me think there's some miscommunication about the flow of nodes being added/removed.

network/gossip/libp2p/network.go

jordanschalm · 2020-10-13T12:52:48Z

network/gossip/libp2p/test/epochtransition_test.go

+	newNetworks := append(ts.nets, nets...)
+
+	// increment epoch
+	ts.currentEpoch = ts.currentEpoch + 1


I don't think this test case is testing a circumstance we expect to happen. It's testing this flow:

start in epoch k with identities I

transition to the next epoch (k+1), where we have one additional node identity N

trigger EpochSetupPhaseStarted in the next epoch

expect that N is added

This diagram shows when we add and remove nodes. We always add nodes for epoch N before epoch N begins. By necessity this means that all node identities added to the networking identity list are not yet in the current epoch identity list (state.Final().Identities()). They're always in the epoch information for the next epoch state.Final().Epochs().Next().InitialIdentities().

Translating this test case to fit this scheme, it would look something like:

start in epoch k with identities I

transition to epoch setup phase within epoch k

this mimics the smart contract emitting the service event containing the identity list for the next (k+1) epoch

we mock out the protocol state Epochs().Next().InitialIdentities() to include N

trigger EpochSetupPhaseStarted for epoch k

expect that N is added

This test utility might be useful for the updated test case:

flow-go/utils/unittest/mocks/epoch_query.go

Line 12 in 63b5172

type EpochQuery struct {

@jordanschalm thanks for those details. My understanding was the same as well but I haven't yet updated the test after my initial set of changes since the test fail as peer discovery cannot be triggered on demand right now. That is something what I am working on right now. Eventually thought, the test should do what you described - add a node from the next epoch and ensure that it is part of the network.

Having said that 😃 , I will update the test and still keep it marked as skipped.

Does this change make sense though - https://github.com/onflow/flow-go/pull/48/files#diff-e313509e481ce199478c3001443850d7880761b01f8f93ba677fab52a2176812R497 ?

Yes, that part LGTM. It was just this test case that threw me off

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

AlexHentschel

Was reading your code to get a bit more insight on how the networking could interface with the protocol state.

I have a couple of recommendations:

I think Network should not implement protocol.Consumer:
- There will be multiple conditions under which the identity table changes. Monitoring these conditions is IMHO not the business of the networking layer
- I like how the networking layer exposes SetIDs. I think this is sufficient.
- I would recommend implementing protocol.Consumer as its own struct, which calls into Network.SetIDs. I think the networking layer cold be completely oblivious to the existence of state

I found it very difficult to understand the used concurrency pattern Network, specifically non-atomicity of SetIDs and Topology. I am worried there are potential edge-cases (commented on the ones I noticed).
- In my opinion the reason for the complexity is that Network need to update the Middleware when the identity list changes, but the Middleware calls back into Network (through the Overlay interface). This kind of creates a circular information flow, which is very hard to understand.
- I don't think connGater is concurrency safe in any way, is it? Yet, don't we update and read its field peerIDAllowlist concurrently ⚠️ :
  
  flow-go/network/gossip/libp2p/libp2pNode.go
  
  Line 525 in b18fafa
  
  p.connGater.update(whilelistPInfos)

I think both points would be addressed with a stronger hierarchical update

Specifically, I would suggest to remove Network.Topology() and Network.Identity() (including from the Overlay interface).
Instead of the Middleware always querying Network, I would suggest that Network pushes updates to the identity list to Middleware:
- For example, Middleware could expose a setter
```
SetOverlay(authorizedNodes map[flow.Identifier]flow.Identity, topology flow.IdentityList) error
```
  Middleware can then cache the result until the next SetOverlay overrides the values. Furthermore, Middleware can propagate
  - allowListAddrs (computed from authorizedNodes) to the connGater component
  - new topology to the Discovery component

AlexHentschel · 2020-10-13T20:34:19Z

network/gossip/libp2p/network.go

+	fanout := n.fanout()
+	n.RLock()
+	defer n.RUnlock()
+	subset, err := n.top.Subset(n.ids, fanout)


⚠️ 🕷️ Uncovered Edge case

I think this is an uncovered edge case:

there are two inputs to computing the Topology: n.ids and fanout, where fanout also depends on the number of nodes, i.e. len(n.ids)

The values are computed in independently locked code blocks! For example, consider the case where we compute fanout but before line 178, the internal values of n.ids are concurrently updated. We would then compute the wrong topology.

Instead, the topology needs to be computed in a atomic operation.

Thanks for that detailed review!
the fanout currently has no effect. The topology ignores it. But I agree with you, I will change this to make the whole operation atomic.

…rency safe; removing fanout function

vishalchangrani · 2020-10-14T23:52:05Z

Updated the PR as per our discussion:

Added a new struct called NodeIDRefresher which listens for epoch events and updates the network by calling network.SetID callback.
Removed network.fanout() all together.
Added mutex to connGator
Created this issue to simplify middleware and overlay interfaces and implementation and remove circular information dependency.
Updated the EpochTransitionTest to work with epoch phases and query (although the test itself is still marked to be skipped since it will fail till discovery is implemented)

jordanschalm · 2020-10-20T15:22:49Z

network/gossip/libp2p/test/epochtransition_test.go


 	// create the engine for the new node
 	newEngine := generateEngines(ts.T(), nets)
 	newEngines := append(ts.engines, newEngine...)

+	// increment epoch
+	ts.currentEpoch = ts.currentEpoch + 1


Should you do ts.epochQuery.Transition() here?

so I am still not testing transition from one epoch to the other, I am testing changes of epoch phases. If I call ts.epochQuery.Transition() it will make the epochQuery point to the next epoch while I am testing against the current epoch.

Ah I see, I think the variable name is throwing me a bit. If we aren't doing an epoch transition here, ts.currentEpoch refers to the counter of the next epoch once we increment it. Maybe we can do:

// update epoch query to return new IDs for next epoch ts.addEpoch(ts.currentEpoch+1, newIDs)

jordanschalm · 2020-10-20T15:23:45Z

network/gossip/libp2p/test/epochtransition_test.go

 	// setup state related mocks
 	ts.state = new(protocol.ReadOnlyState)
 	ts.snapshot = new(protocol.Snapshot)
+	ts.snapshot.On("Identities", mock2.Anything).Return(ids, nil)


goland auto-rename

jordanschalm · 2020-10-20T15:26:32Z

network/gossip/libp2p/nodeIDRefresher.go

+	}
+}
+
+// IDsFromState returns the identities that the network should be using based on the current epoch phase


I think it is worth adding a note here about how this works and the limitations, because it's a bit non-obvious.

For example:

it will currently omit nodes from previous epoch if we restart during StakingPhase

it includes nodes from previous epoch otherwise because we wait to call this function until the transition to setup phase, before that transition we retain what was set during the previous epoch

jordanschalm · 2020-10-20T15:27:48Z

network/gossip/libp2p/nodeIDRefresher.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to retrieve epoch phase: %w", err)
+	}
+


Eventually we will want something like:

if phase == flow.EpochPhaseStaking { ids = ids.Union(state.Final().EPochs().Previous().InitialIdentities()) }

Updated comment on this function to reflect that

…nto vishal/refactor_network

jordanschalm · 2020-10-21T17:27:51Z

network/gossip/libp2p/nodeIDRefresher.go

@@ -46,7 +46,16 @@ func (listener *NodeIDRefresher) EpochSetupPhaseStarted(newEpoch uint64, first *
 	}
 }

-// IDsFromState returns the identities that the network should be using based on the current epoch phase
+// IDsFromState returns the identities that the network should be using based on the current epoch phase as following:
+// ----------------------------------------------------------------------------------------------------


jordanschalm · 2020-10-21T17:33:55Z

network/gossip/libp2p/test/epochtransition_test.go


 	// create the engine for the new node
 	newEngine := generateEngines(ts.T(), nets)
 	newEngines := append(ts.engines, newEngine...)

+	// increment epoch
+	ts.currentEpoch = ts.currentEpoch + 1


Ah I see, I think the variable name is throwing me a bit. If we aren't doing an epoch transition here, ts.currentEpoch refers to the counter of the next epoch once we increment it. Maybe we can do:

// update epoch query to return new IDs for next epoch ts.addEpoch(ts.currentEpoch+1, newIDs)

…nto vishal/refactor_network

vishalchangrani added 13 commits September 30, 2020 12:34

disconnecting from peers not part of the new topology

55a7328

wip

60cf911

wip

9e70c00

wip

30f68c2

using collection topology in test

25249d8

wip

e87115b

creating a new way to create network mocks

2c14597

wip

f70458d

wip

bac1694

adding network as protocol event listener in scaffold

391ad5c

subscribing to dummy topic to trigger discovery via libp2p discovery

8be8148

wip

2723e55

reverting dummy topic subscription logic

88bfe16

vishalchangrani requested review from jordanschalm and yhassanzadeh13 October 6, 2020 22:02

vishalchangrani self-assigned this Oct 6, 2020

vishalchangrani commented Oct 6, 2020

View reviewed changes

Merge branch 'master' into vishal/refactor_network

103c95f

jordanschalm reviewed Oct 7, 2020

View reviewed changes

vishalchangrani and others added 5 commits October 7, 2020 12:52

Update network/gossip/libp2p/test/testUtil.go

433d305

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Update network/gossip/libp2p/libp2pNode.go

e0704c4

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

fixing middleware_test

585bdcc

Merge remote-tracking branch 'origin/jordan/2594-broadcast-to-next-ep…

eac83be

…och' into vishal/refactor_network

removing helper.go; listening for EpochSetupPhaseStarted in the netwo…

8ddeb7b

…rk; refactor

vishalchangrani added 7 commits October 7, 2020 19:00

conditional skip to network epoch test

4556b19

Merge branch 'vishal/refactor_network' of github.com:onflow/flow-go i…

71b1370

…nto vishal/refactor_network

Merge branch 'master' into vishal/refactor_network

703d2e9

changing how free ports are derived

34b0c0e

Merge branch 'vishal/refactor_network' of github.com:onflow/flow-go i…

2366b67

…nto vishal/refactor_network

Merge branch 'master' into vishal/refactor_network

4d2a5fc

Merge branch 'master' into vishal/refactor_network

525a320

yhassanzadeh13 approved these changes Oct 10, 2020

View reviewed changes

vishalchangrani added 4 commits October 10, 2020 00:30

adding missing return

b2da736

Merge branch 'vishal/refactor_network' of github.com:onflow/flow-go i…

bb9f48f

…nto vishal/refactor_network

incorporating review comments

fe1f949

Merge branch 'master' into vishal/refactor_network

cc04818

jordanschalm requested changes Oct 13, 2020

View reviewed changes

Update network/gossip/libp2p/network.go

b18fafa

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

AlexHentschel reviewed Oct 13, 2020

View reviewed changes

vishalchangrani added 2 commits October 14, 2020 12:33

fixing the epoch transition test

514e12c

sepearting epoch consumer to NodeIDRefresher; making conngator concur…

a2d5e47

…rency safe; removing fanout function

vishalchangrani mentioned this pull request Oct 18, 2020

Vishal/discovery #78

Merged

Merge branch 'master' into vishal/refactor_network

a2a8747

jordanschalm reviewed Oct 20, 2020

View reviewed changes

vishalchangrani added 3 commits October 20, 2020 16:20

incorporating review comments

64be110

Merge branch 'vishal/refactor_network' of github.com:onflow/flow-go i…

21a4c7a

…nto vishal/refactor_network

Merge branch 'master' into vishal/refactor_network

baafb04

jordanschalm reviewed Oct 21, 2020

View reviewed changes

jordanschalm approved these changes Oct 21, 2020

View reviewed changes

vishalchangrani added 2 commits October 21, 2020 11:25

fixing test flakyness

20fa2f8

Merge branch 'vishal/refactor_network' of github.com:onflow/flow-go i…

a0ff783

…nto vishal/refactor_network

vishalchangrani merged commit add3a38 into master Oct 21, 2020

vishalchangrani deleted the vishal/refactor_network branch October 21, 2020 23:19

Vishal/refactor network #48

Vishal/refactor network #48

Conversation

vishalchangrani commented Oct 6, 2020

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

yhassanzadeh13 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

jordanschalm 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

AlexHentschel left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

⚠️ 🕷️ Uncovered Edge case

Choose a reason for hiding this comment

vishalchangrani commented Oct 14, 2020 • edited

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

AlexHentschel left a comment •

edited

vishalchangrani commented Oct 14, 2020 •

edited