Deprecate uptimes in pong messages (ava-labs#1362)

Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

message/messages_test.go

@@ -59,7 +59,7 @@ func TestMessage(t *testing.T) {
 		bytesSaved       bool // if true, outbound message saved bytes must be non-zero
 	}{
 		{
-			desc: "ping message with no compression",
+			desc: "ping message with no compression no subnet uptimes",
 			op:   PingOp,
 			msg: &p2p.Message{
 				Message: &p2p.Message_Ping{
@@ -84,6 +84,25 @@ func TestMessage(t *testing.T) {
 			bypassThrottling: true,
 			bytesSaved:       false,
 		},
+		{
+			desc: "ping message with no compression and subnet uptimes",
+			op:   PingOp,
+			msg: &p2p.Message{
+				Message: &p2p.Message_Ping{
+					Ping: &p2p.Ping{
+						SubnetUptimes: []*p2p.SubnetUptime{
+							{
+								SubnetId: testID[:],
+								Uptime:   100,
+							},
+						},
+					},
+				},
+			},
+			compressionType:  compression.TypeNone,
+			bypassThrottling: true,
+			bytesSaved:       false,
+		},
 		{
 			desc: "pong message with no compression and subnet uptimes",
 			op:   PongOp,

message/outbound_msg_builder.go

@@ -37,7 +37,10 @@ type OutboundMsgBuilder interface {
 		peerAcks []*p2p.PeerAck,
 	) (OutboundMessage, error)
 
-	Ping() (OutboundMessage, error)
+	Ping(
+		primaryUptime uint32,
+		subnetUptimes []*p2p.SubnetUptime,
+	) (OutboundMessage, error)
 
 	Pong(
 		primaryUptime uint32,
@@ -182,11 +185,17 @@ func newOutboundBuilder(compressionType compression.Type, builder *msgBuilder) O
 	}
 }
 
-func (b *outMsgBuilder) Ping() (OutboundMessage, error) {
+func (b *outMsgBuilder) Ping(
+	primaryUptime uint32,
+	subnetUptimes []*p2p.SubnetUptime,
+) (OutboundMessage, error) {
 	return b.builder.createOutbound(
 		&p2p.Message{
 			Message: &p2p.Message_Ping{
-				Ping: &p2p.Ping{},
+				Ping: &p2p.Ping{
+					Uptime:        primaryUptime,
+					SubnetUptimes: subnetUptimes,
+				},
 			},
 		},
 		compression.TypeNone,

network/peer/message_queue_test.go

@@ -35,7 +35,8 @@ func TestMessageQueue(t *testing.T) {
 	for i := 0; i < numToSend; i++ {
 		testID := ids.GenerateTestID()
 		testID2 := ids.GenerateTestID()
-		m, err := mc.Pong(uint32(i),
+		m, err := mc.Ping(
+			uint32(i),
 			[]*p2p.SubnetUptime{
 				{SubnetId: testID[:], Uptime: uint32(i)},
 				{SubnetId: testID2[:], Uptime: uint32(i)},

network/peer/peer.go

@@ -491,6 +491,7 @@ func (p *peer) writeMessages() {
 	mySignedIP, err := p.IPSigner.GetSignedIP()
 	if err != nil {
 		p.Log.Error("failed to get signed IP",
+			zap.Stringer("nodeID", p.id),
 			zap.Error(err),
 		)
 		return
@@ -508,6 +509,7 @@ func (p *peer) writeMessages() {
 	if err != nil {
 		p.Log.Error("failed to create message",
 			zap.Stringer("messageOp", message.VersionOp),
+			zap.Stringer("nodeID", p.id),
 			zap.Error(err),
 		)
 		return
@@ -648,10 +650,12 @@ func (p *peer) sendNetworkMessages() {
 				}
 			}
 
-			pingMessage, err := p.Config.MessageCreator.Ping()
+			primaryUptime, subnetUptimes := p.getUptimes()
+			pingMessage, err := p.MessageCreator.Ping(primaryUptime, subnetUptimes)
 			if err != nil {
 				p.Log.Error("failed to create message",
 					zap.Stringer("messageOp", message.PingOp),
+					zap.Stringer("nodeID", p.id),
 					zap.Error(err),
 				)
 				return
@@ -702,7 +706,23 @@ func (p *peer) handle(msg message.InboundMessage) {
 	p.Router.HandleInbound(context.Background(), msg)
 }
 
-func (p *peer) handlePing(*p2p.Ping) {
+func (p *peer) handlePing(msg *p2p.Ping) {
+	p.observeUptimes(msg.Uptime, msg.SubnetUptimes)
+
+	primaryUptime, subnetUptimes := p.getUptimes()
+	pongMessage, err := p.MessageCreator.Pong(primaryUptime, subnetUptimes)
+	if err != nil {
+		p.Log.Error("failed to create message",
+			zap.Stringer("messageOp", message.PongOp),
+			zap.Stringer("nodeID", p.id),
+			zap.Error(err),
+		)
+		return
+	}
+	p.Send(p.onClosingCtx, pongMessage)
+}
+
+func (p *peer) getUptimes() (uint32, []*p2p.SubnetUptime) {
 	primaryUptime, err := p.UptimeCalculator.CalculateUptimePercent(
 		p.id,
 		constants.PrimaryNetworkID,
@@ -736,42 +756,57 @@ func (p *peer) handlePing(*p2p.Ping) {
 	}
 
 	primaryUptimePercent := uint32(primaryUptime * 100)
-	msg, err := p.MessageCreator.Pong(primaryUptimePercent, subnetUptimes)
-	if err != nil {
-		p.Log.Error("failed to create message",
-			zap.Stringer("messageOp", message.PongOp),
-			zap.Error(err),
-		)
-		return
-	}
-	p.Send(p.onClosingCtx, msg)
+	return primaryUptimePercent, subnetUptimes
 }
 
 func (p *peer) handlePong(msg *p2p.Pong) {
-	if msg.Uptime > 100 {
-		p.Log.Debug("dropping pong message with invalid uptime",
+	// TODO: Remove once everyone sends uptimes in Ping messages.
+	p.observeUptimes(msg.Uptime, msg.SubnetUptimes)
+}
+
+func (p *peer) observeUptimes(primaryUptime uint32, subnetUptimes []*p2p.SubnetUptime) {
+	// TODO: Remove once everyone sends uptimes in Ping messages.
+	//
+	// If primaryUptime is 0, the message may not include any uptimes. This may
+	// happen with old Ping messages or new Pong messages.
+	if primaryUptime == 0 {
+		return
+	}
+
+	if primaryUptime > 100 {
+		p.Log.Debug("dropping message with invalid uptime",
 			zap.Stringer("nodeID", p.id),
-			zap.Uint32("uptime", msg.Uptime),
+			zap.Stringer("subnetID", constants.PrimaryNetworkID),
+			zap.Uint32("uptime", primaryUptime),
 		)
 		p.StartClose()
 		return
 	}
-	p.observeUptime(constants.PrimaryNetworkID, msg.Uptime)
+	p.observeUptime(constants.PrimaryNetworkID, primaryUptime)
 
-	for _, subnetUptime := range msg.SubnetUptimes {
+	for _, subnetUptime := range subnetUptimes {
 		subnetID, err := ids.ToID(subnetUptime.SubnetId)
 		if err != nil {
-			p.Log.Debug("dropping pong message with invalid subnetID",
+			p.Log.Debug("dropping message with invalid subnetID",
 				zap.Stringer("nodeID", p.id),
 				zap.Error(err),
 			)
 			p.StartClose()
 			return
 		}
 
+		if !p.MySubnets.Contains(subnetID) {
+			p.Log.Debug("dropping message with unexpected subnetID",
+				zap.Stringer("nodeID", p.id),
+				zap.Stringer("subnetID", subnetID),
+			)
+			p.StartClose()
+			return
+		}
+
 		uptime := subnetUptime.Uptime
 		if uptime > 100 {
-			p.Log.Debug("dropping pong message with invalid uptime",
+			p.Log.Debug("dropping message with invalid uptime",
 				zap.Stringer("nodeID", p.id),
 				zap.Stringer("subnetID", subnetID),
 				zap.Uint32("uptime", uptime),
@@ -785,6 +820,8 @@ func (p *peer) handlePong(msg *p2p.Pong) {
 
 // Record that the given peer perceives our uptime for the given [subnetID]
 // to be [uptime].
+// Assumes [uptime] is in the range [0, 100] and [subnetID] is a valid ID of a
+// subnet this peer tracks.
 func (p *peer) observeUptime(subnetID ids.ID, uptime uint32) {
 	p.observedUptimesLock.Lock()
 	p.observedUptimes[subnetID] = uptime // [0, 100] percentage
@@ -1037,8 +1074,8 @@ func (p *peer) handlePeerList(msg *p2p.PeerList) {
 	peerListAckMsg, err := p.Config.MessageCreator.PeerListAck(trackedPeers)
 	if err != nil {
 		p.Log.Error("failed to create message",
-			zap.Stringer("nodeID", p.id),
 			zap.Stringer("messageOp", message.PeerListAckOp),
+			zap.Stringer("nodeID", p.id),
 			zap.Error(err),
 		)
 		return

proto/p2p/p2p.proto

@@ -61,12 +61,16 @@ message Message {
   }
 }
 
-// Message that the local node sends to its remote peers,
-// in order to periodically check its uptime.
+// Message that a node sends to its peers in order to periodically check
+// responsivness and report the local node's uptime measurements of the peer.
 //
-// On receiving "ping", the remote peer responds with the observed
-// uptime value of the message sender in "pong" message.
-message Ping {}
+// On receiving a "ping", the peer should respond with a "pong".
+message Ping {
+  // uptime is the primary network uptime percentage.
+  uint32 uptime = 1;
+  // subnet_uptimes contains subnet uptime percentages.
+  repeated SubnetUptime subnet_uptimes = 2;
+}
 
 // Contains subnet id and the related observed subnet uptime of the message
 // receiver (remote peer).
@@ -79,6 +83,7 @@ message SubnetUptime {
 // from the sender's point of view, in response to "ping" message.
 // Uptimes are expected to be provided as integers ranging in [0, 100].
 message Pong {
+  // Deprecated: remove all these fields in the future, but keep the message.
   // uptime is the primary network uptime percentage.
   uint32 uptime = 1;
   // subnet_uptimes contains subnet uptime percentages.