network: don't send VP in MOI messages if unsupported by remote peer #6484
Conversation
cce
force-pushed
the
backwards-compatible-moi-vp
branch
from
35b91f1 to
1b3ab73
Compare
cce
force-pushed
the
backwards-compatible-moi-vp
branch
from
0526738 to
efcaf45
Compare
cce
changed the title
cce
force-pushed
the
backwards-compatible-moi-vp
branch
from
3e56b87 to
0ea0302
Compare
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
I thought about changing defaultSendMessageTags but this version looks clearer
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6484 +/- ##
==========================================
- Coverage 47.53% 47.39% -0.14%
==========================================
Files 666 659 -7
Lines 88500 88420 -80
==========================================
- Hits 42069 41910 -159
- Misses 43665 43739 +74
- Partials 2766 2771 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a backward compatibility issue where nodes without stateful vote compression support would disconnect when receiving VP (VotePackedTag) in messages of interest. The solution filters VP from MOI messages when communicating with legacy peers.
Key Changes:
- Modified
maybeSendMessagesOfInterestto conditionally exclude VP tag based on peer capabilities - Added comprehensive test coverage for the VP filtering logic across different peer feature scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| network/wsNetwork.go | Implements conditional filtering of VotePackedTag from MOI messages for peers without stateful compression support |
| network/wsNetwork_test.go | Adds test suite covering VP filtering for legacy peers, VP retention for modern peers, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tags, err := unmarshallMessageOfInterest(messagesOfInterestEnc) | ||
| if err == nil && tags[protocol.VotePackedTag] { | ||
| delete(tags, protocol.VotePackedTag) | ||
| messagesOfInterestEnc = marshallMessageOfInterestMap(tags) | ||
| } |
There was a problem hiding this comment.
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.
| tags, err := unmarshallMessageOfInterest(messagesOfInterestEnc) | ||
| if err == nil && tags[protocol.VotePackedTag] { | ||
| delete(tags, protocol.VotePackedTag) | ||
| messagesOfInterestEnc = marshallMessageOfInterestMap(tags) |
There was a problem hiding this comment.
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.
| 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. |
|
|
||
| if messagesOfInterestEnc != nil { | ||
| // Filter VP tag for peers lacking stateful compression support | ||
| // older nodes (<= v4.3) treat unknown tags as protocol violations and disconnect. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
I think it makes sense. It seems unfortunate to remodify the encoded thing each time its sent, but I guess a) The code can be removed later, and b) do it the other way was a synchronization problem?
4 participants
Summary
In #6466 I added a VP message tag to the default "messages of interest" list, forgetting that this would make nodes that didn't know about VP drop the connection. This updates the MOI system to only send VP if the other side advertises knowledge of the stateful vote compression system using peer features.
Test Plan
Added new test, existing test should pass.