bugfix(op-node): syncClient incorrectly removes peer issue (#50)

* bugfix: only when no connection is available, we can remove the peer from syncClient * add unit test to prove effectiveness of the fixing code * use EventBus replace of notify * remove * change to old version of solution which just adds a condition to disconnected event --------- Co-authored-by: Welkin <[email protected]>
bnb-chain · Oct 17, 2023 · 965e3a1 · 965e3a1
1 parent 6421d48
commit 965e3a1
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 1 deletion.
diff --git a/op-node/p2p/node.go b/op-node/p2p/node.go
@@ -97,7 +97,10 @@ func (n *NodeP2P) init(resourcesCtx context.Context, rollupCfg *rollup.Config, l
 					n.syncCl.AddPeer(conn.RemotePeer())
 				},
 				DisconnectedF: func(nw network.Network, conn network.Conn) {
-					n.syncCl.RemovePeer(conn.RemotePeer())
+					// only when no connection is available, we can remove the peer
+					if nw.Connectedness(conn.RemotePeer()) == network.NotConnected {
+						n.syncCl.RemovePeer(conn.RemotePeer())
+					}
 				},
 			})
 			n.syncCl.Start()

diff --git a/op-node/p2p/sync_test.go b/op-node/p2p/sync_test.go
@@ -288,3 +288,57 @@ func TestMultiPeerSync(t *testing.T) {
 		require.Equal(t, exp.BlockHash, p.BlockHash, "expecting the correct payload")
 	}
 }
+
+func TestNetworkNotifyAddPeerAndRemovePeer(t *testing.T) {
+	t.Parallel()
+	log := testlog.Logger(t, log.LvlDebug)
+
+	cfg, _ := setupSyncTestData(25)
+
+	confA := TestingConfig(t)
+	confB := TestingConfig(t)
+	hostA, err := confA.Host(log.New("host", "A"), nil, metrics.NoopMetrics)
+	require.NoError(t, err, "failed to launch host A")
+	defer hostA.Close()
+	hostB, err := confB.Host(log.New("host", "B"), nil, metrics.NoopMetrics)
+	require.NoError(t, err, "failed to launch host B")
+	defer hostB.Close()
+
+	syncCl := NewSyncClient(log, cfg, hostA.NewStream, func(ctx context.Context, from peer.ID, payload *eth.ExecutionPayload) error {
+		return nil
+	}, metrics.NoopMetrics)
+
+	waitChan := make(chan struct{}, 1)
+	hostA.Network().Notify(&network.NotifyBundle{
+		ConnectedF: func(nw network.Network, conn network.Conn) {
+			syncCl.AddPeer(conn.RemotePeer())
+			waitChan <- struct{}{}
+		},
+		DisconnectedF: func(nw network.Network, conn network.Conn) {
+			// only when no connection is available, we can remove the peer
+			if nw.Connectedness(conn.RemotePeer()) == network.NotConnected {
+				syncCl.RemovePeer(conn.RemotePeer())
+			}
+			waitChan <- struct{}{}
+		},
+	})
+	syncCl.Start()
+
+	err = hostA.Connect(context.Background(), peer.AddrInfo{ID: hostB.ID(), Addrs: hostB.Addrs()})
+	require.NoError(t, err, "failed to connect to peer B from peer A")
+	require.Equal(t, hostA.Network().Connectedness(hostB.ID()), network.Connected)
+
+	//wait for async add process done
+	<-waitChan
+	_, ok := syncCl.peers[hostB.ID()]
+	require.True(t, ok, "peerB should exist in syncClient")
+
+	err = hostA.Network().ClosePeer(hostB.ID())
+	require.NoError(t, err, "close peer fail")
+
+	//wait for async removing process done
+	<-waitChan
+	_, peerBExist3 := syncCl.peers[hostB.ID()]
+	require.True(t, !peerBExist3, "peerB should not exist in syncClient")
+
+}