GRIN/nym
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove notes

Browse Source
This commit is contained in:
durch
2025-10-23 11:06:20 +02:00
parent f8906c4514
commit 86d8ed0f5a
1 changed files with 0 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files
common/nym-kcp/notes/code_review_summary_20240731.md
-85
View File
@@ -1,85 +0,0 @@
# Nym-KCP Code Review Summary (2024-07-31)
Based on an initial code review, the following potential issues and areas for improvement were identified in the `nym-kcp` crate:
## Potential Bugs / Protocol Deviations
1. **Simplified Windowing (`session.rs: move_queue_to_buf`):**
* **Issue:** ~~Currently only considers the local send window (`snd_wnd`), ignoring the remote receive window (`rmt_wnd`).~~
* **Status:** Confirmed OK. The implementation correctly uses `cwnd = min(snd_wnd, rmt_wnd)`.
* **Impact:** ~~Violates KCP congestion control principles (`cwnd = min(snd_wnd, rmt_wnd)`). Can potentially overwhelm the receiver.~~ **(Initial concern resolved)**
2. **Naive RTO Backoff (`session.rs: flush_outgoing`):**
* **Issue:** ~~Uses a simple linear increase (`rto += max(rto, rx_rto)`) instead of standard exponential backoff.~~
* **Status:** Resolved. Changed to exponential backoff (`rto *= 2`) clamped to 60s.
* **Impact:** ~~Slower recovery from packet loss/congestion compared to standard KCP.~~
3. **Less Robust UNA Update (`session.rs: parse_una`):**
* **Issue:** ~~Uses `self.snd_una = una` instead of `max(self.snd_una, una)`. ~~
* **Status:** Resolved. Changed to use `cmp::max(self.snd_una, una)`.
* **Impact:** ~~Less resilient to out-of-order packets carrying older UNA values.~~
## Areas for Improvement / Robustness
4. **Limited Testing (`session.rs: tests`):**
* **Issue:** Only one test case focusing on out-of-order fragment reassembly.
* **Impact:** Insufficient coverage for loss, retransmissions, windowing, edge cases. Low confidence in overall robustness.
5. **Unimplemented Wask/Wins (`session.rs: input`):**
* **Issue:** `KcpCommand::Wask` and `KcpCommand::Wins` are not handled.
* **Impact:** Session cannot probe or react to dynamic changes in the peer's receive window.
6. **Concurrency Locking (`driver.rs`):**
* **Issue:** `Arc<Mutex<>>` with `try_lock` and exponential backoff loop.
* **Impact:** Potential performance bottleneck under high contention; hardcoded retry limit.
7. **Fragment Reassembly Complexity (`session.rs: move_buf_to_queue`):**
* **Issue:** Logic for reassembling fragments, while plausible, is complex and needs thorough testing.
* **Impact:** Potential for subtle bugs related to sequence numbers, buffer state.
## Next Steps
* ~~Address the windowing logic deviation (Priority 1).~~ (Confirmed OK)
* Enhance test suite significantly.
* Implement Wask/Wins handling.
* ~~Refine RTO backoff mechanism.~~ (Resolved)
* (Optional) Test robustness of UNA update logic against out-of-order packets.
## Code Fixes
* **RTO Backoff:** Updated `flush_outgoing` to use exponential backoff (`rto *= 2`) for segment retransmissions, clamped to a maximum (60s), instead of the previous linear increase. Addresses Review Item #2.
* **UNA Update:** Updated `parse_una` to use `cmp::max(self.snd_una, una)` for more robust handling of out-of-order packets. Addresses Review Item #3.
* **Windowing Logic:** Confirmed that `move_queue_to_buf` correctly calculates `cwnd = min(snd_wnd, rmt_wnd)`. Initial concern in Review Item #1 was based on misunderstanding or outdated code.
## Proposed Testing Enhancements
1. **Windowing Behavior Tests:**
* Verify `cwnd = min(snd_wnd, rmt_wnd)` limit on outgoing segments.
* Verify `Write` trait returns `ErrorKind::WouldBlock` when `cwnd` is full.
2. **Retransmission & RTO Tests:**
* Simulate packet loss and verify retransmission occurs after RTO.
* Verify RTO backoff mechanism (current naive, future standard).
* Verify ACK prevents scheduled retransmission.
3. **ACK & UNA Processing Tests:**
* Verify UNA correctly clears acknowledged segments from `snd_buf`.
* Verify specific ACK removes the correct segment and updates RTT.
* Test robustness against out-of-order ACKs/UNA (requires `parse_una` fix).
4. **More Fragmentation/Reassembly Tests:**
* Test diverse out-of-order delivery patterns.
* Test handling of duplicate fragments.
* Test loss of fragments and subsequent retransmission/reassembly.
## Testing Progress (2024-08-01)
The following tests have been implemented in `session.rs` based on the proposed enhancements:
* `test_congestion_window_limits_send_buffer`: Verifies that the number of segments moved to `snd_buf` respects `cwnd = min(snd_wnd, rmt_wnd)`. (Addresses Windowing Behavior Test 1)
* `test_segment_retransmission_after_rto`: Verifies that a segment is retransmitted if its RTO expires without an ACK. (Addresses Retransmission Test 1)
* `test_ack_removes_segment_from_send_buffer`: Verifies that receiving a specific ACK removes the corresponding segment from `snd_buf`. (Addresses ACK Processing Test 2, first part)
* `test_ack_updates_rtt`: Verifies that receiving a specific ACK updates the session's RTT estimate and RTO. (Addresses ACK Processing Test 2, second part)
* `test_una_clears_send_buffer`: Verifies that receiving a packet with a UNA value clears all segments with `sn < una` from `snd_buf`. (Addresses ACK Processing Test 1)
## Testing Progress (2024-08-02)
* `test_write_fills_send_queue_when_window_full`: Verifies that `Write` limits accepted data based on `snd_wnd` and `update` respects `cwnd` when moving segments. (Partially addresses Windowing Behavior Test 2)
* `test_ack_prevents_retransmission`: Verifies that a segment is not retransmitted if it is ACKed before its RTO expires. (Addresses Retransmission Test 3)
* `test_duplicate_fragment_handling`: Verifies that the receiver correctly ignores duplicate fragments during reassembly. (Addresses Fragmentation Test 2)
* `test_fragment_loss_and_reassembly`: Verifies that a lost fragment is retransmitted after RTO and the receiver can reassemble the message upon receiving it. (Addresses Fragmentation Test 3)