kademlia, hive: rate limiting on gossiping peers (send and receive) #1654

istae · 2021-05-04T13:55:33Z

This change is

anatollupacescu · 2021-05-04T15:14:14Z

pkg/topology/kademlia/kademlia.go


 	if len(addrs) == 0 {
 		return nil
 	}

 	err := k.discovery.BroadcastPeers(ctx, peer, addrs...)
 	if err != nil {
+		fmt.Println("DISCONNECT")


do we want to keep this line or was it intended to be something temp?

mrekucci · 2021-05-05T07:44:07Z

pkg/hive/hive.go

 type Service struct {
 	streamer        p2p.Streamer
 	addressBook     addressbook.GetPutter
 	addPeersHandler func(context.Context, ...swarm.Address) error
 	networkID       uint64
 	logger          logging.Logger
 	metrics         metrics
+	limiter         map[string]*rate.Limiter
+	limiterLock     sync.Mutex


r: this line isn't needed, the zero value of sync.Mutex is already useful

mrekucci · 2021-05-05T07:49:39Z

pkg/hive/hive.go

@@ -169,3 +188,23 @@ func (s *Service) peersHandler(ctx context.Context, peer p2p.Peer, stream p2p.St

 	return nil
 }
+
+func (s *Service) rateLimitPeer(ctx context.Context, peer swarm.Address, count int) error {


The ctx parameter isn't used in the method, so it should be eighter used or removed.

mrekucci · 2021-05-05T08:28:34Z

pkg/topology/kademlia/kademlia.go

-	_ = k.connectedPeers.EachBinRev(func(connectedPeer swarm.Address, _ uint8) (bool, bool, error) {
-		if connectedPeer.Equal(peer) {
-			return false, false, nil
+	//r := rand.New(securerand.NewSource())


Is this needed here?

mrekucci · 2021-05-05T09:00:19Z

pkg/topology/kademlia/kademlia.go

-			return false, false, nil
+	//r := rand.New(securerand.NewSource())
+
+	for bin := uint8(0); bin < swarm.MaxBins; bin++ {


r: @acud is there a reason why swarm.MaxBins is a typed constant? If not, I'll remove the type and change the loop: for bin := 0; bin < swarm.MaxBins; bin++ {... (also in the affected code in the pkg/swarm/proximity.go file), more on this topic: https://blog.golang.org/constants

mrekucci · 2021-05-05T09:01:12Z

pkg/topology/kademlia/kademlia.go

@@ -668,28 +672,31 @@ func (k *Kad) connect(ctx context.Context, peer swarm.Address, ma ma.Multiaddr,
 func (k *Kad) Announce(ctx context.Context, peer swarm.Address) error {
 	addrs := []swarm.Address{}


r: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

mrekucci · 2021-05-05T09:12:35Z

pkg/topology/kademlia/kademlia.go

+
+func randomSubset(addrs []swarm.Address, count int) ([]swarm.Address, error) {
+
+	if count >= len(addrs) {


The addresses aren't shuffled when the count count >= len(addrs), is this expected behavior?

mrekucci · 2021-05-05T09:15:18Z

pkg/topology/pslice/pslice.go

@@ -95,6 +95,28 @@ func (s *PSlice) EachBinRev(pf topology.EachPeerFunc) error {
 	return nil
 }

+func (s *PSlice) BinPeers(bin uint8) []swarm.Address {


r: if you remove the type from swarm.MaxBin constant as recommended above, then the bin can be simply an integer and the casting on L:102 wouldn't be necessary

acud

Reviewed 7 of 8 files at r1.
Reviewable status: 7 of 8 files reviewed, 11 unresolved discussions (waiting on @esadakar, @mrekucci, and @zelig)

go.mod, line 69 at r1 (raw file):

	golang.org/x/term v0.0.0-20201210144234-2321bbc49cbf
	golang.org/x/text v0.3.4 // indirect
	golang.org/x/time v0.0.0-20191024005414-555d28b269f0

this change is probably not needed and can be reverted

pkg/hive/hive.go, line 40 at r1 (raw file):

var (
	ErrRateLimitExceed = errors.New("rate limit exceeded")

r: maybe ErrRateLimit or ErrRateLimitExceeded

pkg/hive/hive.go, line 41 at r1 (raw file):

var (
	ErrRateLimitExceed = errors.New("rate limit exceeded")
	limitBurst         = 4 * int(swarm.MaxBins)

this is still quite a bit (128 in a minute, 7700 in an hour, 180K a day), so the address spam vector is still there. that being said, i'm not sure I have a better idea. maybe limiting to 4 peers in a bin in a minute?

pkg/hive/hive.go, line 201 at r1 (raw file):

	limiter, ok := s.limiter[addr]
	if !ok {
		limiter = rate.NewLimiter(limitRate, limitBurst)

you add to the map but never clean up. this is a clear memory leak. you'd need to leverage the DisconnectIn and DisconnectOut protocol helpers to detect that the peer went away (or we decided to disconnect from it) in order to clean up the entry for the peer

pkg/topology/kademlia/kademlia.go, line 677 at r1 (raw file):

Previously, mrekucci (Peter Mrekaj) wrote…

r: @acud is there a reason why swarm.MaxBins is a typed constant? If not, I'll remove the type and change the loop: for bin := 0; bin < swarm.MaxBins; bin++ {... (also in the affected code in the pkg/swarm/proximity.go file), more on this topic: https://blog.golang.org/constants

it is typed since everything which is a PO is intended to be a uint8. Going with the untyped approach might make daily life a tad bit easier but I think that having it typed provides good assurances and a strong consideration for developers to actually keep in mind the upper bound when actually writing code

pkg/topology/kademlia/kademlia_test.go, line 1244 at r1 (raw file):

	t.Helper()

	timeout := time.After(5 * time.Second)

is the change needed?

pkg/topology/pslice/pslice_test.go, line 260 at r1 (raw file):

	} {

		t.Run("", func(t *testing.T) {

test label missing. we usually add a short strong in the actual tc struct that has the string label which is used here

pkg/topology/pslice/pslice_test.go, line 284 at r1 (raw file):

			bins := ps.BinPeers(uint8(len(tc.peersCount)))
			if bins != nil {
				t.Fatal("peers must be nil for out of bound bind")

out of bound bin?

zelig

LGTM, heed @mrekucci 's comments

istae

Dismissed @acud from 6 discussions.
Reviewable status: 2 of 8 files reviewed, 6 unresolved discussions (waiting on @acud and @esadakar)

go.mod, line 69 at r1 (raw file):