2 tests demonstrating the overflow bug

[KolbyML]

Details of how I found the bug and information are detailed in a comment linked here #44 (comment)

seq_nr/ack_nr are both u16 this is intential TCP/uTP have intentional overflows so that they can continuously send data

In this demo pr test case test_transfer_65535_packets_in_theory_should_pass sends 65535 1 below the limit and passes

but when the overflow occurs test_transfer_65536_packets_in_theory_should_fail it fails

[KolbyML]

@emhane @jacobkaufmann @njgheorghita

Here is a picture of the test log of the demo working as expected 🥳

[KolbyML]

Looking into it a little there seems to be a few issues. Having init_seq_num isn't very useful instead it should be replaced with something like conn.seq like libutp does. init_seq_num seems like a precursor to a bug like this occuring since it assumes we won't reach the number again (which we will)

Instead we should keep track of the sequence and a current_window_packets (just a count) this is the way it is done in fluffy's uTP and the reference implementation. This way we can track if ack's relatively instead of based off a fixed point, because we do reuse numbers TCP does it too.

I am not done researching for a solution to this problem, but this kind of seems like a starting point. let range = self.seq_num_range(); seems buggy assuming there is only u16 packets ever sent kind of thing. When infinity packets can be sent so knowing the initial seq isn't helpful knowing the sequence and how many packets are in the window are what matter since everything is relative.

I guess I will test more and see what I come up with

[jacobkaufmann]

in your screenshot it looks like the test that was supposed to pass is not terminating. but yea init_seq_num would present a problem for a connection that transmits more than u16::MAX packets because of the seq_num_range method.

I did not anticipate that it would be a problem for its initial use cases. if each DATA packet contains ~500-1000 bytes of data, then you would have to send on the order of tens of megabytes of data to run into that issue. I'm not saying it's not an issue, but I would not expect that issue to come up in practice unless you are doing large (tens of megabytes) data transfers.

[KolbyML]

in your screenshot it looks like the test that was supposed to pass is not terminating.

test stream::test::test_transfer_65535_packets_in_theory_should_pass ... ok ? it did terminate

but yea init_seq_num would present a problem for a connection that transmits more than u16::MAX packets because of the seq_num_range method. I did not anticipate that it would be a problem for its initial use cases. if each DATA packet contains ~500-1000 bytes of data, then you would have to send on the order of tens of megabytes of data to run into that issue. I'm not saying it's not an issue, but I would not expect that issue to come up in practice unless you are doing large (tens of megabytes) data transfers.

From what I know this issue shouldn't affect the history network currently. Though it is the only public utp implementation on crates.io currently being maintained. So I think having a stable library would be beneficial. uTP is just a TCP clone with a "fancier "congestion protocol" on UDP. When I use qBittorrent over half of my connections are often uTP instead of TCP, transfers are often over that possible 32-65mb failure range. So I think that is kind of the feature of uTP to do large transfers of data over 1kb and beyond.

I am just trying to catch them all, as the saying goes. The bug @emhane is tracking will probably bear more fruit for history. Hopefully the collective work though can get this library as stable as libtorrent 💪

I will make a PR resolving this issue

[emhane]

well done @KolbyML this is spot on. no more than 65535 packets can be sent due to the seq num being a u16, which in fact, is just a megabyte of data. I was also wondering if we could either 1: adapt the protocol for ethereum needs making the seq num a larger number or 2: rotate the seq num, as you mention in #62 .

[emhane]

in your screenshot it looks like the test that was supposed to pass is not terminating.

test stream::test::test_transfer_65535_packets_in_theory_should_pass ... ok ? it did terminate

but yea init_seq_num would present a problem for a connection that transmits more than u16::MAX packets because of the seq_num_range method. I did not anticipate that it would be a problem for its initial use cases. if each DATA packet contains ~500-1000 bytes of data, then you would have to send on the order of tens of megabytes of data to run into that issue. I'm not saying it's not an issue, but I would not expect that issue to come up in practice unless you are doing large (tens of megabytes) data transfers.

From what I know this issue shouldn't affect the history network currently. Though it is the only public utp implementation on crates.io currently being maintained. So I think having a stable library would be beneficial. uTP is just a TCP clone with a "fancier "congestion protocol" on UDP. When I use qBittorrent over half of my connections are often uTP instead of TCP, transfers are often over that possible 32-65mb failure range. So I think that is kind of the feature of uTP to do large transfers of data over 1kb and beyond.

I am just trying to catch them all, as the saying goes. The bug @emhane is tracking will probably bear more fruit for history. Hopefully the collective work though can get this library as stable as libtorrent 💪

I will make a PR resolving this issue

I totally agree with your argument @KolbyML. Good observations on the weight of a rust uTP crate in the current time, and the public expectations on how much data the uTP protocol should handle. Indeed, since the other rust uTP crates aren't maintained anymore, it makes the interoperability of this crate with for example libtorrent important. Nice to have a number for common max transfer size for uTP, 32-65 MB, that helps us set the milestone towards which to work. If you have some links or self generated data to show the failure range is 32-65 MB from other non-rust uTP implementations, please share.

[KolbyML]

well done @KolbyML this is spot on. no more than 65535 packets can be sent due to the seq num being a u16, which in fact, is just a megabyte of data. I was also wondering if we could either 1: adapt the protocol for ethereum needs making the seq num a larger number or 2: rotate the seq num, as you mention in #62 .

2 is following the spec, 1 wouldn't be uTP

[KolbyML]

I totally agree with your argument @KolbyML. Good observations on the weight of a rust uTP crate in the current time, and the public expectations on how much data the uTP protocol should handle. Indeed, since the other rust uTP crates aren't maintained anymore, it makes the interoperability of this crate with for example libtorrent important. Nice to have a number for common max transfer size for uTP, 32-65 MB, that helps us set the milestone towards which to work. If you have some links or self generated data to show the failure range is 32-65 MB from other non-rust uTP implementations, please share.

32-65MB is just calculated from @jacobkaufmann range of 500-1000 bytes. 500*65535 = 32MB.

Other uTP implementation's don't have this failure, this is only a failure that happens with our client which is what makes it a bug.

TL:DR

Every other compliant uTP implementation can send infinity amount of data

Our implementation can only send 65535 packets of data before failing because of a bug in implementation design

Sorry if my original message was confusing, but this is a ethereum/utp only bug.

[emhane]

I totally agree with your argument @KolbyML. Good observations on the weight of a rust uTP crate in the current time, and the public expectations on how much data the uTP protocol should handle. Indeed, since the other rust uTP crates aren't maintained anymore, it makes the interoperability of this crate with for example libtorrent important. Nice to have a number for common max transfer size for uTP, 32-65 MB, that helps us set the milestone towards which to work. If you have some links or self generated data to show the failure range is 32-65 MB from other non-rust uTP implementations, please share.

32-65MB is just calculated from @jacobkaufmann range of 500-1000 bytes. 500*65535 = 32MB.

Other uTP implementation's don't have this failure, this is only a failure that happens with our client which is what makes it a bug.

TL:DR

Every other compliant uTP implementation can send infinity amount of data

Our implementation can only send 65535 packets of data before failing because of a bug in implementation design

Sorry if my original message was confusing, but this is a ethereum/utp only bug.

gotcha, then the goal is to transfer infinitely many bytes.

the limit to the amount of data we can transfer on a stream rn on master is 1 MB with 65535 ACKs. so not even close to 32-65 MB.

[KolbyML]

gotcha, then the goal is to transfer infinitely many bytes.

yeppers

the limit to the amount of data we can transfer on a stream rn on master is 1 MB with 65535 ACKs. so not even close to 32-65 MB.

The send buffer limit (the 1 MB you are talking about) doesn't mean we can only send 1 MB per a stream, if you fill pending_sends with 0.9MB you can send over 1MB per a stream and exceed 32-65MB.