Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions network/wsNetwork.go
Original file line number Diff line number Diff line change
Expand Up @@ -1191,7 +1191,17 @@ func (wn *WebsocketNetwork) maybeSendMessagesOfInterest(peer *wsPeer, messagesOf
messagesOfInterestEnc = wn.messagesOfInterestEnc
wn.messagesOfInterestMu.Unlock()
}

if messagesOfInterestEnc != nil {
// Filter VP tag for peers lacking stateful compression support
// older nodes (<= v4.3) treat unknown tags as protocol violations and disconnect.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been changed for current nodes? I didn't realize that when we spoke offline.
If so, this can removed after next consensus update? Should we comment that way?
Maybe we should have a comment tag for that, it common up a lot.
// UNTILCONSENSUS or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be cool to have a comment like that because we often just leave a TODO message or github issue saying we can remove this later and forget

if !peer.vpackStatefulCompressionSupported() {
tags, err := unmarshallMessageOfInterest(messagesOfInterestEnc)
if err == nil && tags[protocol.VotePackedTag] {
delete(tags, protocol.VotePackedTag)
messagesOfInterestEnc = marshallMessageOfInterestMap(tags)
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent error handling: if unmarshalling fails, the code proceeds without logging or handling the error, potentially sending incompatible MOI messages to legacy peers. Consider logging the error or falling back to a safe default behavior.

Suggested change
messagesOfInterestEnc = marshallMessageOfInterestMap(tags)
messagesOfInterestEnc = marshallMessageOfInterestMap(tags)
} else if err != nil {
wn.log.Warnf("Failed to unmarshal messagesOfInterest for peer %v: %v. Sending original MOI, which may be incompatible.", peer.addr, err)
// Optionally, could skip sending or send a safe default here.

Copilot uses AI. Check for mistakes.
}
Comment on lines +1199 to +1203
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code unmarshalls and remarshalls the MOI message for every legacy peer connection. Consider caching two versions of messagesOfInterestEnc (one with VP, one without) at the network level to avoid repeated serialization overhead when multiple legacy peers connect.

Copilot uses AI. Check for mistakes.
}
peer.sendMessagesOfInterest(messagesOfInterestGeneration, messagesOfInterestEnc)
} else {
wn.log.Infof("msgOfInterest Enc=nil, MOIGen=%d", messagesOfInterestGeneration)
Expand Down
123 changes: 123 additions & 0 deletions network/wsNetwork_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"encoding/json"
"fmt"
"io"
"maps"
"math/rand"
"net"
"net/http"
Expand Down Expand Up @@ -4773,3 +4774,125 @@ func TestPeerComparisonInBroadcast(t *testing.T) {
require.Equal(t, 1, len(testPeer.sendBufferBulk))
require.Equal(t, 0, len(exceptPeer.sendBufferBulk))
}

func TestMaybeSendMessagesOfInterestLegacyPeer(t *testing.T) {
partitiontest.PartitionTest(t)
t.Parallel()

makePeer := func(wn *WebsocketNetwork, features peerFeatureFlag) (*wsPeer, chan sendMessage) {
ch := make(chan sendMessage, 1)
return &wsPeer{
wsPeerCore: makePeerCore(wn.ctx, wn, wn.log, nil, "test-addr", nil, ""),
features: features,
sendBufferHighPrio: ch,
sendBufferBulk: make(chan sendMessage, 1),
closing: make(chan struct{}),
processed: make(chan struct{}, 1),
}, ch
}

newTestNetwork := func(tags map[protocol.Tag]bool) *WebsocketNetwork {
wn := &WebsocketNetwork{
log: logging.TestingLog(t),
}
wn.ctx = context.Background()
cloned := maps.Clone(tags)
wn.messagesOfInterest = cloned
wn.messagesOfInterestEnc = marshallMessageOfInterestMap(cloned)
wn.messagesOfInterestGeneration.Store(1)
return wn
}

t.Run("filters VP for peers without stateful support", func(t *testing.T) {
wn := newTestNetwork(map[protocol.Tag]bool{
protocol.AgreementVoteTag: true,
protocol.VotePackedTag: true,
})

peer, ch := makePeer(wn, pfCompressedProposal|pfCompressedVoteVpack)
wn.maybeSendMessagesOfInterest(peer, nil)

select {
case msg := <-ch:
require.Equal(t, protocol.MsgOfInterestTag, protocol.Tag(msg.data[:2]))

decoded, err := unmarshallMessageOfInterest(msg.data[2:])
require.NoError(t, err)

require.Contains(t, decoded, protocol.AgreementVoteTag)
require.True(t, decoded[protocol.AgreementVoteTag])
_, hasVP := decoded[protocol.VotePackedTag]
require.False(t, hasVP, "VP tag should be filtered for legacy peers")
default:
t.Fatal("expected MOI message for legacy peer")
}
})

t.Run("retains VP for peers with stateful support", func(t *testing.T) {
wn := newTestNetwork(map[protocol.Tag]bool{
protocol.AgreementVoteTag: true,
protocol.VotePackedTag: true,
})

peer, ch := makePeer(wn, pfCompressedProposal|pfCompressedVoteVpack|pfCompressedVoteVpackStateful256)

wn.maybeSendMessagesOfInterest(peer, nil)

select {
case msg := <-ch:
require.Equal(t, protocol.MsgOfInterestTag, protocol.Tag(msg.data[:2]))

decoded, err := unmarshallMessageOfInterest(msg.data[2:])
require.NoError(t, err)

require.Contains(t, decoded, protocol.AgreementVoteTag)
require.True(t, decoded[protocol.AgreementVoteTag])
require.Contains(t, decoded, protocol.VotePackedTag)
require.True(t, decoded[protocol.VotePackedTag], "expected VP tag for peer with stateful support")
default:
t.Fatal("expected MOI message for stateful peer")
}
})

t.Run("gracefully handles configuration without VP tag", func(t *testing.T) {
wn := newTestNetwork(map[protocol.Tag]bool{
protocol.AgreementVoteTag: true,
})

peer, ch := makePeer(wn, pfCompressedProposal|pfCompressedVoteVpack)
wn.maybeSendMessagesOfInterest(peer, nil)

select {
case msg := <-ch:
require.Equal(t, protocol.MsgOfInterestTag, protocol.Tag(msg.data[:2]))

decoded, err := unmarshallMessageOfInterest(msg.data[2:])
require.NoError(t, err)

require.Contains(t, decoded, protocol.AgreementVoteTag)
require.True(t, decoded[protocol.AgreementVoteTag])
_, hasVP := decoded[protocol.VotePackedTag]
require.False(t, hasVP)
default:
t.Fatal("expected MOI message when VP is absent from configuration")
}
})

t.Run("skips sending when peer generation matches", func(t *testing.T) {
wn := newTestNetwork(map[protocol.Tag]bool{
protocol.AgreementVoteTag: true,
protocol.VotePackedTag: true,
})

peer, ch := makePeer(wn, pfCompressedProposal|pfCompressedVoteVpack)
peer.messagesOfInterestGeneration.Store(wn.messagesOfInterestGeneration.Load())

wn.maybeSendMessagesOfInterest(peer, nil)

select {
case <-ch:
t.Fatal("did not expect MOI message when generations already match")
default:
}
})
}