Wait for subnet peers before broadcasting onto attestation subnet topic (#6893)

* Initial pass

* Add metric to measure success

* Use a subnet RWLock to prevent duplicate requests, give up after 3 attempts

* push latest commented code

* try with non-blocking broadcast

* Add feature flag, ignore parent deadline if any

* Add slot as metadata

* add tests

* gaz

Co-authored-by: nisdas <nishdas93@gmail.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

Author: 3 people

beacon-chain/p2p/BUILD.bazel

@@ -118,6 +118,7 @@ go_test(
         "//beacon-chain/p2p/testing:go_default_library",
         "//proto/beacon/p2p/v1:go_default_library",
         "//proto/testing:go_default_library",
+        "//shared/featureconfig:go_default_library",
         "//shared/iputils:go_default_library",
         "//shared/p2putils:go_default_library",
         "//shared/params:go_default_library",

beacon-chain/p2p/broadcaster.go

@@ -5,11 +5,14 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"time"
 
 	"github.com/gogo/protobuf/proto"
 	"github.com/pkg/errors"
 	eth "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
+	"github.com/prysmaticlabs/prysm/shared/featureconfig"
 	"github.com/prysmaticlabs/prysm/shared/hashutil"
+	"github.com/prysmaticlabs/prysm/shared/params"
 	"github.com/prysmaticlabs/prysm/shared/traceutil"
 	"go.opencensus.io/trace"
 )
@@ -18,10 +21,18 @@ import (
 // GossipTypeMapping.
 var ErrMessageNotMapped = errors.New("message type is not mapped to a PubSub topic")
 
+// Max number of attempts to search the network for a specific subnet.
+const maxSubnetDiscoveryAttempts = 1
+
 // Broadcast a message to the p2p network.
 func (s *Service) Broadcast(ctx context.Context, msg proto.Message) error {
 	ctx, span := trace.StartSpan(ctx, "p2p.Broadcast")
 	defer span.End()
+
+	twoSlots := time.Duration(2*params.BeaconConfig().SecondsPerSlot) * time.Second
+	ctx, cancel := context.WithTimeout(ctx, twoSlots)
+	defer cancel()
+
 	forkDigest, err := s.forkDigest()
 	if err != nil {
 		err := errors.Wrap(err, "could not retrieve fork digest")
@@ -47,7 +58,67 @@ func (s *Service) BroadcastAttestation(ctx context.Context, subnet uint64, att *
 		traceutil.AnnotateError(span, err)
 		return err
 	}
-	return s.broadcastObject(ctx, att, attestationToTopic(subnet, forkDigest))
+
+	if featureconfig.Get().EnableAttBroadcastDiscoveryAttempts {
+		// Non-blocking broadcast.
+		go s.broadcastAttestation(ctx, subnet, att, forkDigest)
+	} else {
+		return s.broadcastObject(ctx, att, attestationToTopic(subnet, forkDigest))
+	}
+
+	return nil
+}
+
+func (s *Service) broadcastAttestation(ctx context.Context, subnet uint64, att *eth.Attestation, forkDigest [4]byte) {
+	ctx, span := trace.StartSpan(ctx, "p2p.broadcastAttestation")
+	defer span.End()
+	ctx = trace.NewContext(context.Background(), span) // clear parent context / deadline.
+
+	oneEpoch := time.Duration(1*params.BeaconConfig().SlotsPerEpoch*params.BeaconConfig().SecondsPerSlot) * time.Second
+	ctx, cancel := context.WithTimeout(ctx, oneEpoch)
+	defer cancel()
+
+	// Ensure we have peers with this subnet.
+	s.subnetLocker(subnet).RLock()
+	hasPeer := s.hasPeerWithSubnet(subnet)
+	s.subnetLocker(subnet).RUnlock()
+
+	span.AddAttributes(
+		trace.BoolAttribute("hasPeer", hasPeer),
+		trace.Int64Attribute("slot", int64(att.Data.Slot)),
+		trace.Int64Attribute("subnet", int64(subnet)),
+	)
+
+	attestationBroadcastAttempts.Inc()
+
+	if !hasPeer {
+		if err := func() error {
+			s.subnetLocker(subnet).Lock()
+			defer s.subnetLocker(subnet).Unlock()
+			for i := 0; i < maxSubnetDiscoveryAttempts; i++ {
+				if err := ctx.Err(); err != nil {
+					return err
+				}
+				ok, err := s.FindPeersWithSubnet(ctx, subnet)
+				if err != nil {
+					return err
+				}
+				if ok {
+					savedAttestationBroadcasts.Inc()
+					break
+				}
+			}
+			return nil
+		}(); err != nil {
+			log.WithError(err).Error("Failed to find peers")
+			traceutil.AnnotateError(span, err)
+		}
+	}
+
+	if err := s.broadcastObject(ctx, att, attestationToTopic(subnet, forkDigest)); err != nil {
+		log.WithError(err).Error("Failed to broadcast attestation")
+		traceutil.AnnotateError(span, err)
+	}
 }
 
 // method to broadcast messages to other peers in our gossip mesh.

beacon-chain/p2p/broadcaster_test.go

@@ -3,17 +3,25 @@ package p2p
 import (
 	"context"
 	"fmt"
+	"net"
 	"reflect"
 	"sync"
 	"testing"
 	"time"
 
+	pb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1"
+
+	"github.com/ethereum/go-ethereum/p2p/discover"
 	"github.com/gogo/protobuf/proto"
+	"github.com/libp2p/go-libp2p-core/host"
 	pubsub "github.com/libp2p/go-libp2p-pubsub"
 	eth "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
+	"github.com/prysmaticlabs/go-bitfield"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
+	"github.com/prysmaticlabs/prysm/beacon-chain/p2p/peers"
 	p2ptest "github.com/prysmaticlabs/prysm/beacon-chain/p2p/testing"
 	testpb "github.com/prysmaticlabs/prysm/proto/testing"
+	"github.com/prysmaticlabs/prysm/shared/featureconfig"
 	"github.com/prysmaticlabs/prysm/shared/testutil"
 	"github.com/prysmaticlabs/prysm/shared/testutil/assert"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
@@ -129,3 +137,261 @@ func TestService_Attestation_Subnet(t *testing.T) {
 		assert.Equal(t, tt.topic, attestationToTopic(subnet, [4]byte{} /* fork digest */), "Wrong topic")
 	}
 }
+
+func TestService_BroadcastAttestation(t *testing.T) {
+	p1 := p2ptest.NewTestP2P(t)
+	p2 := p2ptest.NewTestP2P(t)
+	p1.Connect(p2)
+	if len(p1.BHost.Network().Peers()) == 0 {
+		t.Fatal("No peers")
+	}
+
+	p := &Service{
+		host:                  p1.BHost,
+		pubsub:                p1.PubSub(),
+		joinedTopics:          map[string]*pubsub.Topic{},
+		cfg:                   &Config{},
+		genesisTime:           time.Now(),
+		genesisValidatorsRoot: []byte{'A'},
+		subnetsLock:           make(map[uint64]*sync.RWMutex),
+		subnetsLockLock:       sync.Mutex{},
+		peers: peers.NewStatus(context.Background(), &peers.StatusConfig{
+			ScorerParams: &peers.PeerScorerConfig{},
+		}),
+	}
+
+	msg := &eth.Attestation{
+		AggregationBits: bitfield.NewBitlist(7),
+		Data: &eth.AttestationData{
+			Slot:            0,
+			CommitteeIndex:  0,
+			BeaconBlockRoot: make([]byte, 32),
+			Source: &eth.Checkpoint{
+				Epoch: 0,
+				Root:  make([]byte, 32),
+			},
+			Target: &eth.Checkpoint{
+				Epoch: 0,
+				Root:  make([]byte, 32),
+			},
+		},
+		Signature: make([]byte, 96),
+	}
+
+	subnet := uint64(5)
+
+	topic := AttestationSubnetTopicFormat
+	GossipTypeMapping[reflect.TypeOf(msg)] = topic
+	digest, err := p.forkDigest()
+	require.NoError(t, err)
+	topic = fmt.Sprintf(topic, digest, subnet)
+
+	// External peer subscribes to the topic.
+	topic += p.Encoding().ProtocolSuffix()
+	sub, err := p2.SubscribeToTopic(topic)
+	require.NoError(t, err)
+
+	time.Sleep(50 * time.Millisecond) // libp2p fails without this delay...
+
+	// Async listen for the pubsub, must be before the broadcast.
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func(tt *testing.T) {
+		defer wg.Done()
+		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+		defer cancel()
+
+		incomingMessage, err := sub.Next(ctx)
+		require.NoError(t, err)
+
+		result := &eth.Attestation{}
+		require.NoError(t, p.Encoding().DecodeGossip(incomingMessage.Data, result))
+		if !proto.Equal(result, msg) {
+			tt.Errorf("Did not receive expected message, got %+v, wanted %+v", result, msg)
+		}
+	}(t)
+
+	// Broadcast to peers and wait.
+	require.NoError(t, p.BroadcastAttestation(context.Background(), subnet, msg))
+	if testutil.WaitTimeout(&wg, 1*time.Second) {
+		t.Error("Failed to receive pubsub within 1s")
+	}
+}
+
+func TestService_BroadcastAttestationWithDiscoveryAttempts(t *testing.T) {
+	// Setup bootnode.
+	cfg := &Config{}
+	port := 2000
+	cfg.UDPPort = uint(port)
+	_, pkey := createAddrAndPrivKey(t)
+	ipAddr := net.ParseIP("127.0.0.1")
+	genesisTime := time.Now()
+	genesisValidatorsRoot := make([]byte, 32)
+	s := &Service{
+		cfg:                   cfg,
+		genesisTime:           genesisTime,
+		genesisValidatorsRoot: genesisValidatorsRoot,
+	}
+	bootListener, err := s.createListener(ipAddr, pkey)
+	require.NoError(t, err)
+	defer bootListener.Close()
+
+	// Use shorter period for testing.
+	currentPeriod := pollingPeriod
+	pollingPeriod = 1 * time.Second
+	defer func() {
+		pollingPeriod = currentPeriod
+	}()
+
+	bootNode := bootListener.Self()
+	subnet := uint64(5)
+
+	var listeners []*discover.UDPv5
+	var hosts []host.Host
+	// setup other nodes.
+	cfg = &Config{
+		BootstrapNodeAddr:   []string{bootNode.String()},
+		Discv5BootStrapAddr: []string{bootNode.String()},
+		MaxPeers:            30,
+	}
+	// Setup 2 different hosts
+	for i := 1; i <= 2; i++ {
+		h, pkey, ipAddr := createHost(t, port+i)
+		cfg.UDPPort = uint(port + i)
+		cfg.TCPPort = uint(port + i)
+		s := &Service{
+			cfg:                   cfg,
+			genesisTime:           genesisTime,
+			genesisValidatorsRoot: genesisValidatorsRoot,
+		}
+		listener, err := s.startDiscoveryV5(ipAddr, pkey)
+		// Set for 2nd peer
+		if i == 2 {
+			s.dv5Listener = listener
+			s.metaData = new(pb.MetaData)
+			bitV := bitfield.NewBitvector64()
+			bitV.SetBitAt(subnet, true)
+			s.updateSubnetRecordWithMetadata(bitV)
+		}
+		assert.NoError(t, err, "Could not start discovery for node")
+		listeners = append(listeners, listener)
+		hosts = append(hosts, h)
+	}
+	defer func() {
+		// Close down all peers.
+		for _, listener := range listeners {
+			listener.Close()
+		}
+	}()
+
+	// close peers upon exit of test
+	defer func() {
+		for _, h := range hosts {
+			if err := h.Close(); err != nil {
+				t.Log(err)
+			}
+		}
+	}()
+
+	f := featureconfig.Get()
+	f.EnableAttBroadcastDiscoveryAttempts = true
+	rst := featureconfig.InitWithReset(f)
+	defer rst()
+
+	ps1, err := pubsub.NewFloodSub(context.Background(), hosts[0],
+		pubsub.WithMessageSigning(false),
+		pubsub.WithStrictSignatureVerification(false),
+	)
+	require.NoError(t, err)
+
+	ps2, err := pubsub.NewFloodSub(context.Background(), hosts[1],
+		pubsub.WithMessageSigning(false),
+		pubsub.WithStrictSignatureVerification(false),
+	)
+	require.NoError(t, err)
+
+	p := &Service{
+		host:                  hosts[0],
+		pubsub:                ps1,
+		dv5Listener:           listeners[0],
+		joinedTopics:          map[string]*pubsub.Topic{},
+		cfg:                   &Config{},
+		genesisTime:           time.Now(),
+		genesisValidatorsRoot: []byte{'A'},
+		subnetsLock:           make(map[uint64]*sync.RWMutex),
+		subnetsLockLock:       sync.Mutex{},
+		peers: peers.NewStatus(context.Background(), &peers.StatusConfig{
+			ScorerParams: &peers.PeerScorerConfig{},
+		}),
+	}
+
+	p2 := &Service{
+		host:                  hosts[1],
+		pubsub:                ps2,
+		dv5Listener:           listeners[1],
+		joinedTopics:          map[string]*pubsub.Topic{},
+		cfg:                   &Config{},
+		genesisTime:           time.Now(),
+		genesisValidatorsRoot: []byte{'A'},
+		subnetsLock:           make(map[uint64]*sync.RWMutex),
+		subnetsLockLock:       sync.Mutex{},
+		peers: peers.NewStatus(context.Background(), &peers.StatusConfig{
+			ScorerParams: &peers.PeerScorerConfig{},
+		}),
+	}
+
+	msg := &eth.Attestation{
+		AggregationBits: bitfield.NewBitlist(7),
+		Data: &eth.AttestationData{
+			Slot:            0,
+			CommitteeIndex:  0,
+			BeaconBlockRoot: make([]byte, 32),
+			Source: &eth.Checkpoint{
+				Epoch: 0,
+				Root:  make([]byte, 32),
+			},
+			Target: &eth.Checkpoint{
+				Epoch: 0,
+				Root:  make([]byte, 32),
+			},
+		},
+		Signature: make([]byte, 96),
+	}
+
+	topic := AttestationSubnetTopicFormat
+	GossipTypeMapping[reflect.TypeOf(msg)] = topic
+	digest, err := p.forkDigest()
+	require.NoError(t, err)
+	topic = fmt.Sprintf(topic, digest, subnet)
+
+	// External peer subscribes to the topic.
+	topic += p.Encoding().ProtocolSuffix()
+	sub, err := p2.SubscribeToTopic(topic)
+	require.NoError(t, err)
+
+	time.Sleep(50 * time.Millisecond) // libp2p fails without this delay...
+
+	// Async listen for the pubsub, must be before the broadcast.
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func(tt *testing.T) {
+		defer wg.Done()
+		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+		defer cancel()
+
+		incomingMessage, err := sub.Next(ctx)
+		require.NoError(t, err)
+
+		result := &eth.Attestation{}
+		require.NoError(t, p.Encoding().DecodeGossip(incomingMessage.Data, result))
+		if !proto.Equal(result, msg) {
+			tt.Errorf("Did not receive expected message, got %+v, wanted %+v", result, msg)
+		}
+	}(t)
+
+	// Broadcast to peers and wait.
+	require.NoError(t, p.BroadcastAttestation(context.Background(), subnet, msg))
+	if testutil.WaitTimeout(&wg, 1*time.Second) {
+		t.Error("Failed to receive pubsub within 5s")
+	}
+}