Merge pull request #10674 from yyforyongyu/bugfix/10671-bolt1-ping-ignore

lnwire+peer: ignore BOLT 1 no-reply pings

Author: yyforyongyu

docs/release-notes/release-notes-0.21.0.md

@@ -56,6 +56,12 @@
   transitions during startup, avoiding lost unlocks during slow database
   initialization.
 
+* [Fixed handling of BOLT 1 pings requesting 65532 or more pong
+  bytes](https://github.com/lightningnetwork/lnd/pull/10674). LND now ignores
+  these valid no-reply pings instead of disconnecting peers, restoring
+  compatibility with implementations that pad `channel_reestablish` messages
+  with them.
+
 # New Features
 
 - [Basic Support](https://github.com/lightningnetwork/lnd/pull/9868) for onion

lnwire/ping.go

@@ -47,10 +47,8 @@ func (p *Ping) Decode(r io.Reader, pver uint32) error {
 		return err
 	}
 
-	if p.NumPongBytes > MaxPongBytes {
-		return ErrMaxPongBytesExceeded
-	}
-
+	// Values above MaxPongBytes are still valid on the wire. Per BOLT 1,
+	// receivers must ignore those pings rather than fail deserialization.
 	return nil
 }
 

lnwire/ping_test.go

@@ -0,0 +1,51 @@
+package lnwire
+
+import (
+	"bytes"
+	"strconv"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestPingDecodeAllowsNoReplyPongSizes asserts that ping messages using the
+// BOLT 1 no-reply sentinel range still deserialize successfully.
+func TestPingDecodeAllowsNoReplyPongSizes(t *testing.T) {
+	t.Parallel()
+
+	// Arrange: Pick values from the BOLT 1 no-reply range. These
+	// pings are valid on the wire and should decode successfully
+	// even though they do not require a pong response.
+	testCases := []uint16{65532, 65535}
+
+	for _, numPongBytes := range testCases {
+		numPongBytes := numPongBytes
+		testName := strconv.FormatUint(uint64(numPongBytes), 10)
+
+		t.Run(testName, func(t *testing.T) {
+			// Arrange: Encode a ping carrying a no-reply pong
+			// size together with a small payload so we exercise
+			// the normal wire format.
+			var buf bytes.Buffer
+
+			want := &Ping{
+				NumPongBytes: numPongBytes,
+				PaddingBytes: PingPayload{1, 2, 3},
+			}
+
+			_, err := WriteMessage(&buf, want, 0)
+			require.NoError(t, err)
+
+			// Act: Decode the serialized message through the
+			// standard parser.
+			msg, err := ReadMessage(bytes.NewReader(buf.Bytes()), 0)
+			require.NoError(t, err)
+
+			// Assert: The decoded ping matches the original
+			// input exactly.
+			got, ok := msg.(*Ping)
+			require.True(t, ok)
+			require.Equal(t, want, got)
+		})
+	}
+}

lnwire/pong.go

@@ -2,7 +2,6 @@ package lnwire
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 )
 
@@ -11,10 +10,6 @@ import (
 // 2 bytes, leaving 65531 bytes.
 const MaxPongBytes = 65531
 
-// ErrMaxPongBytesExceeded indicates that the NumPongBytes field from the ping
-// message has exceeded MaxPongBytes.
-var ErrMaxPongBytesExceeded = fmt.Errorf("pong bytes exceeded")
-
 // PongPayload is a set of opaque bytes sent in response to a ping message.
 type PongPayload []byte
 

lnwire/test_message.go

@@ -1541,7 +1541,7 @@ var _ TestMessage = (*Ping)(nil)
 //
 // This is part of the TestMessage interface.
 func (p *Ping) RandTestMessage(t *rapid.T) Message {
-	numPongBytes := uint16(rapid.IntRange(0, int(MaxPongBytes)).Draw(
+	numPongBytes := uint16(rapid.IntRange(0, math.MaxUint16).Draw(
 		t, "numPongBytes"),
 	)
 

peer/brontide.go

@@ -2220,6 +2220,13 @@ out:
 			// the relevant atomic variable.
 			p.lastPingPayload.Store(msg.PaddingBytes[:])
 
+			// BOLT 1 requires us to ignore pings requesting 65532
+			// or more pong bytes instead of replying or
+			// disconnecting.
+			if msg.NumPongBytes > lnwire.MaxPongBytes {
+				continue
+			}
+
 			// Next, we'll send over the amount of specified pong
 			// bytes.
 			pong := lnwire.NewPong(p.cfg.PongBuf[0:msg.NumPongBytes])
@@ -2591,7 +2598,8 @@ func messageSummary(msg lnwire.Message) string {
 			msg.NodeID, time.Unix(int64(msg.Timestamp), 0))
 
 	case *lnwire.Ping:
-		return fmt.Sprintf("ping_bytes=%x", msg.PaddingBytes[:])
+		return fmt.Sprintf("num_pong_bytes=%d, len(ping_bytes)=%d",
+			msg.NumPongBytes, len(msg.PaddingBytes[:]))
 
 	case *lnwire.Pong:
 		return fmt.Sprintf("len(pong_bytes)=%d", len(msg.PongBytes[:]))

peer/brontide_test.go

@@ -1073,6 +1073,96 @@ func TestPeerCustomMessage(t *testing.T) {
 	require.Equal(t, receivedCustomMsg, &receivedCustom.msg)
 }
 
+// TestPeerIgnoresPingWithoutPongReply ensures we keep the connection alive for
+// pings using the BOLT 1 no-reply sentinel range.
+func TestPeerIgnoresPingWithoutPongReply(t *testing.T) {
+	t.Parallel()
+
+	// Arrange: Start a peer using the mock connection so we can
+	// inject incoming pings and observe any outgoing responses.
+	params := createTestPeer(t)
+
+	var (
+		mockConn  = params.mockConn
+		alicePeer = params.peer
+	)
+
+	startPeerDone := startPeer(t, mockConn, alicePeer)
+	_, err := fn.RecvOrTimeout(startPeerDone, 2*timeout)
+	require.NoError(t, err)
+
+	writePing := func(msg *lnwire.Ping) {
+		t.Helper()
+
+		var b bytes.Buffer
+		_, err := lnwire.WriteMessage(&b, msg, 0)
+		require.NoError(t, err)
+
+		select {
+		case mockConn.readMessages <- b.Bytes():
+		case <-time.After(timeout):
+			t.Fatal("timeout sending ping to peer")
+		}
+	}
+
+	// Act: Deliver a ping in the BOLT 1 no-reply range.
+	ignoredPayload := []byte{1, 2, 3}
+	writePing(&lnwire.Ping{
+		NumPongBytes: 65535,
+		PaddingBytes: ignoredPayload,
+	})
+
+	// Assert: The peer records the latest ping payload for observability.
+	require.Eventually(t, func() bool {
+		return bytes.Equal(
+			alicePeer.LastRemotePingPayload(), ignoredPayload,
+		)
+	}, timeout, 10*time.Millisecond)
+
+	// Assert: No pong is sent for the no-reply sentinel range.
+	select {
+	case rawMsg := <-mockConn.writtenMessages:
+		t.Fatalf("expected no pong reply, got %x", rawMsg)
+	case <-time.After(100 * time.Millisecond):
+	}
+
+	// Act: Send a normal ping afterward to prove the peer
+	// stayed connected and still handles standard ping/pong
+	// traffic.
+	writePing(&lnwire.Ping{NumPongBytes: 1})
+
+	rawMsg, err := fn.RecvOrTimeout(mockConn.writtenMessages, timeout)
+	require.NoError(t, err)
+
+	msg, err := lnwire.ReadMessage(bytes.NewReader(rawMsg), 0)
+	require.NoError(t, err)
+
+	// Assert: The follow-up ping receives the requested pong reply.
+	pong, ok := msg.(*lnwire.Pong)
+	require.True(t, ok)
+	require.Len(t, pong.PongBytes, 1)
+}
+
+// TestMessageSummaryPingIncludesNumPongBytes ensures the debug summary for a
+// ping exposes the requested pong size, which makes ignored no-reply pings
+// visible without requiring trace-level logging.
+func TestMessageSummaryPingIncludesNumPongBytes(t *testing.T) {
+	t.Parallel()
+
+	// Arrange: Build a ping that uses the BOLT 1 no-reply sentinel range.
+	msg := &lnwire.Ping{
+		NumPongBytes: 65535,
+		PaddingBytes: []byte{1, 2, 3},
+	}
+
+	// Act: Generate the human-readable message summary.
+	summary := messageSummary(msg)
+
+	// Assert: The summary includes both the requested pong size and payload
+	// length so debug logs can explain why no pong was sent.
+	require.Equal(t, "num_pong_bytes=65535, len(ping_bytes)=3", summary)
+}
+
 // TestUpdateNextRevocation checks that the method `updateNextRevocation` is
 // behave as expected.
 func TestUpdateNextRevocation(t *testing.T) {