network: add VP message type for stateful vote compression #6466
Conversation
network/wsPeer_test.go
Outdated
| filtered := allTags[:0] | ||
| for _, tag := range allTags { | ||
| if _, ok := ignored[tag]; ok { |
There was a problem hiding this comment.
does it really work? append below modifies allTags' backing array while iterating
There was a problem hiding this comment.
it should work because it's filtering and so the append is always writing alltags[i] back to the slice or to an earlier position but it is too complex and should just use the slices package, will update
network/wsNetwork.go
Outdated
| // Announce our maximum supported dynamic table size | ||
| // Both sides will independently calculate min(ourSize, theirSize) | ||
| // Only advertise dynamic features if stateless compression is enabled | ||
| switch dtSize := uint32(meta.Config().VoteCompressionDynamicTableSize); { |
There was a problem hiding this comment.
since only these values are supported, shouldn't config run some prevalidation? Or at least log a warning here saying it is disabled b/c of unsupported table value
network/wsNetwork.go
Outdated
|
|
||
| // PeerFeatureVoteVpackDynamic* are values for PeerFeaturesHeader indicating peer | ||
| // supports specific dynamic table sizes for vpack compression | ||
| const PeerFeatureVoteVpackDynamic1024 = "avvpack1024" |
There was a problem hiding this comment.
looks like can be unexported
| ctx: context.Background(), | ||
| } | ||
|
|
||
| return wp.sendControlMessage(sm) |
There was a problem hiding this comment.
control message but data VP tag?
There was a problem hiding this comment.
yes — maybe I should just call it sendHighPrioMessage or something — I was trying to generalize the code used for MOI messages to send the VP abort message
Co-authored-by: Pavel Zbitskiy <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6466 +/- ##
==========================================
- Coverage 51.13% 46.24% -4.90%
==========================================
Files 668 661 -7
Lines 112101 112249 +148
==========================================
- Hits 57327 51912 -5415
- Misses 51907 57578 +5671
+ Partials 2867 2759 -108 ☔ View full report in Codecov by Sentry. |
| // Initialize stateful compression negotiation details if both nodes support it | ||
| // Stateful compression requires stateless compression to be available since VP messages | ||
| // decompress in two stages: VP → stateless-compressed → raw vote | ||
| if wp.enableVoteCompression && wp.voteCompressionDynamicTableSize > 0 && |
There was a problem hiding this comment.
Should these conditions be behind a dedicated function for checking them?
| return !peerBtoA.msgCodec.statefulVoteEncEnabled.Load() | ||
| }, 2*time.Second, 50*time.Millisecond, "VP encoding not disabled on B->A after receiving abort message") | ||
|
|
||
| // Verify connection is still up after abort |
There was a problem hiding this comment.
Worth sending a non-compressed message to finish test?
| networkP2PReceivedUncompressedBytesByTag.Add(string(tag[:]), uint64(len(msg.Data)+2)) | ||
| networkP2PReceivedUncompressedBytesByTag.Add(string(msg.Tag), uint64(len(msg.Data)+2)) | ||
| } | ||
| if originalTag == protocol.VotePackedTag { |
3 participants
Summary
This follows #6351 by integrating it with the WebsocketNetwork implementation, using a new VP message tag.
Test Plan
New tests added to exercise table size negotiation & failure scenarios. Still need to run some large-scale performance tests.