diff --git a/common/nym-kcp/notes/code_review_summary_20240731.md b/common/nym-kcp/notes/code_review_summary_20240731.md deleted file mode 100644 index fcfc87affb..0000000000 --- a/common/nym-kcp/notes/code_review_summary_20240731.md +++ /dev/null @@ -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>` 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) \ No newline at end of file