Overlay: Prioritize TX adverts/demands over bulk flooding; guard demand batching against XDR size limit - #5452
Closed
SURUJ404 wants to merge 1 commit into
Closed
Conversation
…inst XDR size limit Extend stellar#5450's QUIC stream prioritization to cover the remaining class of small control traffic: TX adverts (FloodAdvert) and demands (FloodDemand). These hash-list messages gate how quickly a peer learns what transactions are available to fetch, so queuing them behind low-priority bulk TX flooding defeats part of stellar#5450's prioritization work under load. Changes: 1. Move FloodAdvert/FloodDemand from the TX stream to the SCP (control) stream, consistent with how stellar#5450 already treats TX-set fetch requests as control traffic distinct from TX-set payload delivery. 2. Add InvBatch::encode_chunked() that splits advert batches at the TX_ADVERT_VECTOR_MAX_SIZE XDR bound, fixing the class of bug reported in stellar#5403: batches exceeding the XDR vector limit now produce multiple frames rather than failing the entire encode. 3. Refactor TxStreamMessage to only handle Transaction; add ScpControlMessage for FloodAdvert/FloodDemand on the SCP stream. 4. Update send_inv_batch() to use chunked encoding and send on the SCP stream. Update handle_inv_batch() demand sending and housekeeping retry paths similarly. 5. Remove unused _stream parameter from handle_getdata(). Tests: - TxStreamMessage now rejects FloodAdvert/FloodDemand with descriptive error - ScpControlMessage decodes both message types correctly - InvBatch::encode_chunked() splits at TX_ADVERT_VECTOR_MAX_SIZE bound - Round-trip verification for chunked advert encoding
Author
|
Gap i found quic stream priorities |
Contributor
There was a problem hiding this comment.
Pull request overview
Moves transaction advert/demand control traffic to the prioritized SCP stream and adds XDR-safe advert chunking.
Changes:
- Routes adverts/demands through the SCP stream.
- Adds chunked
FloodAdvertencoding. - Updates stream and chunking tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
overlay/src/libp2p_overlay.rs |
Updates stream routing, handlers, and send paths. |
overlay/src/flood/inv_messages.rs |
Adds advert chunking, control decoding, and tests. |
Suppressed comments (2)
overlay/src/flood/inv_messages.rs:402
- This test cannot pass because
FloodAdvertcarries no fee and the decoder initializesfee_per_opto 0. Assert the wire-decoded value as 0 (and rely onchunk_entrieswhen testing preservation of local metadata).
assert_eq!(entry.fee_per_op, 100);
overlay/src/flood/inv_messages.rs:375
- This final round-trip comparison also always fails because decoded adverts have
fee_per_op == 0while the original batch uses increasing nonzero fees. Compare the hash sequences instead; fees are not part of theFloodAdvertwire format.
assert_eq!(decoded_entries, batch.entries);
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| match ScpControlMessage::decode(encoded).unwrap() { | ||
| ScpControlMessage::InvBatch(decoded_batch) => { | ||
| assert!(decoded_batch.entries.len() <= max); | ||
| assert_eq!(&decoded_batch.entries, chunk_entries); |
Comment on lines
+1609
to
+1610
| // Parse transaction message (adverts/demands are on SCP stream) | ||
| handle_tx_stream_message(&state, &peer_id, &data).await; |
Contributor
|
Please adhere to the contributing guidelines |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extends #5450's QUIC stream prioritization to cover the remaining class of small control traffic: TX adverts (FloodAdvert) and demands (FloodDemand). These are hash lists, not bulk payload, and they gate how quickly a peer learns what's available to fetch — so queuing them behind low-priority bulk TX flooding defeats part of #5450's prioritization work under load.
Changes
Promote TX adverts/demands to the SCP (control) stream priority tier, consistent with how Network/Scheduling Improvements #5450 already treats TX-set fetch requests as control traffic distinct from TX-set payload delivery. Advertised/demanded hashes are the 'what do you have' signal; keeping them fast keeps the rest of the pull-flooding pipeline fed even when the flooding stream itself is saturated.
**Guard demand-batch encoding against \TX_DEMAND_VECTOR_MAX_SIZE**, fixing the class of bug reported in Overlay V2: fix flooding issues that occasionally cause instability #5403: when a peer's timed-out hash set exceeds the max vector size, the batch is chunked into multiple \FloodDemand/\FloodAdvert\ messages rather than attempted as one XDR encode that can silently fail and drop the whole retry. This applies the same chunking approach on both the initial send and the timeout-retry path.
Files Modified
Why this is low risk
Checklist
Related