From 358acbf004897ef411b17d698498179684bf8206 Mon Sep 17 00:00:00 2001 From: notlelouch Date: Fri, 11 Oct 2024 16:44:22 +0530 Subject: [PATCH 1/5] Modified BlockPeer to close peer connections after successful blocking --- nodebuilder/p2p/p2p.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/nodebuilder/p2p/p2p.go b/nodebuilder/p2p/p2p.go index 15491f929a..69d23a6828 100644 --- a/nodebuilder/p2p/p2p.go +++ b/nodebuilder/p2p/p2p.go @@ -145,7 +145,13 @@ func (m *module) NATStatus(context.Context) (network.Reachability, error) { } func (m *module) BlockPeer(_ context.Context, p peer.ID) error { - return m.connGater.BlockPeer(p) + if err := m.connGater.BlockPeer(p); err != nil { + return err + } + + m.host.Network().ClosePeer(p) + + return nil } func (m *module) UnblockPeer(_ context.Context, p peer.ID) error { From 19c36adc8419c30f6597b72ebcb214dc4b3d707d Mon Sep 17 00:00:00 2001 From: notlelouch Date: Fri, 11 Oct 2024 19:45:59 +0530 Subject: [PATCH 2/5] logged the error and updated the godoc of the module --- nodebuilder/p2p/p2p.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/nodebuilder/p2p/p2p.go b/nodebuilder/p2p/p2p.go index 69d23a6828..70692d527e 100644 --- a/nodebuilder/p2p/p2p.go +++ b/nodebuilder/p2p/p2p.go @@ -42,7 +42,8 @@ type Module interface { // NATStatus returns the current NAT status. NATStatus(context.Context) (network.Reachability, error) - // BlockPeer adds a peer to the set of blocked peers. + // BlockPeer adds a peer to the set of blocked peers and + // closes any existing connection to that peer. BlockPeer(ctx context.Context, p peer.ID) error // UnblockPeer removes a peer from the set of blocked peers. UnblockPeer(ctx context.Context, p peer.ID) error @@ -148,9 +149,9 @@ func (m *module) BlockPeer(_ context.Context, p peer.ID) error { if err := m.connGater.BlockPeer(p); err != nil { return err } - - m.host.Network().ClosePeer(p) - + if err := m.host.Network().ClosePeer(p); err != nil { + log.Warnf("failed to close connection to blocked peer %s: %v", p, err) + } return nil } From e3cef4f4c42779e15f7ce2666743b9aa25cd5b78 Mon Sep 17 00:00:00 2001 From: notlelouch Date: Sat, 12 Oct 2024 02:45:41 +0530 Subject: [PATCH 3/5] testa coverage --- nodebuilder/p2p/p2p.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nodebuilder/p2p/p2p.go b/nodebuilder/p2p/p2p.go index 70692d527e..3e2de09596 100644 --- a/nodebuilder/p2p/p2p.go +++ b/nodebuilder/p2p/p2p.go @@ -149,8 +149,10 @@ func (m *module) BlockPeer(_ context.Context, p peer.ID) error { if err := m.connGater.BlockPeer(p); err != nil { return err } - if err := m.host.Network().ClosePeer(p); err != nil { - log.Warnf("failed to close connection to blocked peer %s: %v", p, err) + if m.host != nil { + if err := m.host.Network().ClosePeer(p); err != nil { + log.Warnf("failed to close connection to blocked peer %s: %v", p, err) + } } return nil } From fe3e4b31ce526527261e463123f1364003ada0c2 Mon Sep 17 00:00:00 2001 From: notlelouch Date: Wed, 16 Oct 2024 18:08:06 +0530 Subject: [PATCH 4/5] fully tested the new BlockPeer function --- nodebuilder/p2p/p2p.go | 6 ++---- nodebuilder/p2p/p2p_test.go | 27 ++++++++++++++++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/nodebuilder/p2p/p2p.go b/nodebuilder/p2p/p2p.go index 3e2de09596..70692d527e 100644 --- a/nodebuilder/p2p/p2p.go +++ b/nodebuilder/p2p/p2p.go @@ -149,10 +149,8 @@ func (m *module) BlockPeer(_ context.Context, p peer.ID) error { if err := m.connGater.BlockPeer(p); err != nil { return err } - if m.host != nil { - if err := m.host.Network().ClosePeer(p); err != nil { - log.Warnf("failed to close connection to blocked peer %s: %v", p, err) - } + if err := m.host.Network().ClosePeer(p); err != nil { + log.Warnf("failed to close connection to blocked peer %s: %v", p, err) } return nil } diff --git a/nodebuilder/p2p/p2p_test.go b/nodebuilder/p2p/p2p_test.go index b883889859..031d9c5dda 100644 --- a/nodebuilder/p2p/p2p_test.go +++ b/nodebuilder/p2p/p2p_test.go @@ -218,22 +218,39 @@ func TestP2PModule_Pubsub(t *testing.T) { // TestP2PModule_ConnGater tests P2P Module methods on // the instance of ConnectionGater. func TestP2PModule_ConnGater(t *testing.T) { + net, err := mocknet.FullMeshConnected(2) + require.NoError(t, err) + host, peer := net.Hosts()[0], net.Hosts()[1] + gater, err := connectionGater(datastore.NewMapDatastore()) require.NoError(t, err) - mgr := newModule(nil, nil, gater, nil, nil) + mgr := newModule(host, nil, gater, nil, nil) ctx := context.Background() - assert.NoError(t, mgr.BlockPeer(ctx, "badpeer")) + connectedness, err := mgr.Connectedness(ctx, peer.ID()) + require.NoError(t, err) + + assert.NoError(t, mgr.BlockPeer(ctx, peer.ID())) blocked, err := mgr.ListBlockedPeers(ctx) require.NoError(t, err) - assert.Len(t, blocked, 1) + assert.Contains(t, blocked, peer.ID()) - assert.NoError(t, mgr.UnblockPeer(ctx, "badpeer")) + // Check if the peer is disconnected (or remains disconnected) after blocking + connectedness, err = mgr.Connectedness(ctx, peer.ID()) + require.NoError(t, err) + assert.Equal(t, network.NotConnected, connectedness) + + assert.NoError(t, mgr.UnblockPeer(ctx, peer.ID())) blocked, err = mgr.ListBlockedPeers(ctx) require.NoError(t, err) - assert.Len(t, blocked, 0) + assert.NotContains(t, blocked, peer.ID()) + + // Verify that unblocking doesn't automatically reconnect + connectedness, err = mgr.Connectedness(ctx, peer.ID()) + require.NoError(t, err) + assert.Equal(t, network.NotConnected, connectedness) } // TestP2PModule_ResourceManager tests P2P Module methods on From 1a69f64cd48ea503561416406027091e062ed8e6 Mon Sep 17 00:00:00 2001 From: notlelouch Date: Thu, 17 Oct 2024 12:57:13 +0530 Subject: [PATCH 5/5] retested changes --- nodebuilder/p2p/p2p_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nodebuilder/p2p/p2p_test.go b/nodebuilder/p2p/p2p_test.go index 031d9c5dda..01e53fa553 100644 --- a/nodebuilder/p2p/p2p_test.go +++ b/nodebuilder/p2p/p2p_test.go @@ -229,16 +229,13 @@ func TestP2PModule_ConnGater(t *testing.T) { ctx := context.Background() - connectedness, err := mgr.Connectedness(ctx, peer.ID()) - require.NoError(t, err) - assert.NoError(t, mgr.BlockPeer(ctx, peer.ID())) blocked, err := mgr.ListBlockedPeers(ctx) require.NoError(t, err) assert.Contains(t, blocked, peer.ID()) // Check if the peer is disconnected (or remains disconnected) after blocking - connectedness, err = mgr.Connectedness(ctx, peer.ID()) + connectedness, err := mgr.Connectedness(ctx, peer.ID()) require.NoError(t, err) assert.Equal(t, network.NotConnected, connectedness)