Modify uTP selective ACKs to conform to BEP #7071
Conversation
|
uTorrent always send exactly 4 bytes: https://github.com/bittorrent/libutp/blob/master/utp_internal.cpp#L815 As far as I can tell, it doesn't require the SACK bitfield to be a multiple of 4 bytes. |
simulation/test_utp.cpp
Outdated
| @@ -139,7 +139,7 @@ TORRENT_TEST(utp_pmtud) | |||
| TEST_EQUAL(metric(cnt, "utp.utp_packets_in"), 593); | |||
| TEST_EQUAL(metric(cnt, "utp.utp_payload_pkts_in"), 66); | |||
|
|
|||
| TEST_EQUAL(metric(cnt, "utp.utp_packets_out"), 603); | |||
| TEST_EQUAL(metric(cnt, "utp.utp_packets_out"), 602); | |||
There was a problem hiding this comment.
it's not clear to me why this change would result in one less packet to be sent. If anything, a larger SACK header would eat into the size of the payload that would fit in the packet
There was a problem hiding this comment.
I'd only updated that packet count in order to see if the other tests would still pass, which they seem to do.
I'm yet to work out why that particular test has one less packet, but that was my first thought as well - with the extra 3 bytes possible in the header, if anything it should be the other way round.
There was a problem hiding this comment.
There might be an issue when picking up a Nagled packet with a SACK already attached if the sack size has increased since it was held back. It appears to keep the old size, rather than growing it if needed. It looks like an existing issue that just happened to trigger on that one test with this PR compared to without.
In this case, the unpatched version misses out on acking a packet (the 1 byte SACK payload needs to grow to hold 9 bits in this case but doesn't), while the patched version already has enough padding to grow (although it will be an issue growing a Nagled SACK from 32-33 bits). As a result, the unpatched version sends an extra ack packet later on to catch up.
When comparing utp debug logfiles of the failing test using this PR against those without they appear to diverge right after that happens.
Hopefully if that gets fixed, then that test will pass without needing to update the packet count.
There was a problem hiding this comment.
Can you attach the two pcap files: one with 603 packets, one with 602?
…conform to BEP 29
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Are you sure this is indeed code used by mutorrent? Also it is not like it is not full of bugs. Will this break compatibility with mutorrent? |
|
Why is this bot closing stale pull requests and issues? That is not serious. |
|
I just realised that BEP 29 was written by @arvidn probably by reading utorrent open source parts. So that means it is just a typo. Errata not possible in BEP? |
The size of libtorrent's uTP selective ACK payload appears to be rounded up to the nearest byte, however BEP 29 defines it as 'a bitmask of at least 32 bits, in multiples of 32 bits.'
This potential change conforms to the BEP by rounding up to the nearest 4 byte multiple. Note that
sackis the SACK payload size in bytes (ielenfrom the BEP which 'must be at least 4, and in multiples of 4').I'm not sure if this is actually an issue in practice at all, or if there are any clients out there in the wild that might have issues receiving SACKs that don't strictly conform.
See #7068