feat: replace peer ID recognition with tdyne-peer-id-registry

[si14]

See also: #475

I made this PR as small as possible. I had to change the test, as -qB00000000000000000 is not a valid qBittorent peer ID (it has to have another dash), so I amended it.

Do you want me to append the detected version to the string .get_client_name() returns? It can become qBittorrent 2.3.4 instead, which is on the one hand more convenient for people, but on the other might interfere with grouping somewhere.

If you're open to it, I'll experiment with extracting more logic from Id into tdyne-peer-id (and maybe InfoHash, see #360, although it's trickier to design because of v2) to make it possible to share types directly and make a separate PR.

[codecov]

Codecov Report

Merging #477 (cb914da) into develop (5ecdde5) will increase coverage by 0.75%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #477      +/-   ##
===========================================
+ Coverage    84.52%   85.27%   +0.75%     
===========================================
  Files          100      100              
  Lines         7119     7050      -69     
===========================================
- Hits          6017     6012       -5     
+ Misses        1102     1038      -64     

Files	Coverage Δ
.../servers/apis/v1/context/torrent/resources/peer.rs	100.00% <100.00%> (ø)
src/tracker/peer.rs	94.70% <100.00%> (+27.42%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[josecelano]

Hi @si14 thank you very much!

See also: #475

I made this PR as small as possible. I had to change the test, as -qB00000000000000000 is not a valid qBittorent peer ID (it has to have another dash), so I amended it.

Why is not a valid Peer ID?

Do you want me to append the detected version to the string .get_client_name() returns? It can become qBittorrent 2.3.4 instead, which is on the one hand more convenient for people, but on the other might interfere with grouping somewhere.

I do not think it would interfere, but I would keep it as it is to avoid incompatibility issues. I would prefer to add new methods like:

• get_client_version
• get_client_full_name (name + version)

That could be a new PR.

If you're open to it, I'll experiment with extracting more logic from Id into tdyne-peer-id (and maybe InfoHash, see #360, although it's trickier to design because of v2) to make it possible to share types directly and make a separate PR.

For the PeerID I think it would be fine. We should not have specific code/behavior. For the InfoHash I think we should first extract our package and then check what it's generic and what is specific code in our domain. If we have a specific code, we can move it elsewhere until we have a real generic InfoHash model. In some places (UDP tracker) we also use aquatic_udp_protocol::InfoHash([0u8; 20]), but I guess we have to keep it for the time being. I think there is also a lot of stuff to extract to common packages in the UDP tracker.

By the way, I found this in JavaScript: https://github.com/webtorrent/bittorrent-peerid.

[josecelano]

One more thing @si14 could you sign the commit? We have some rules for branches and we want all commits to be signed. If you are busy I can rebase the PR and sign it, but I think it's a good practice for you to sign all commits. We like promoting it: https://secure-git.guide/

[josecelano]

Pending to sign commit to merge.

[si14]

Why is not a valid Peer ID?

Strictly speaking any 20-byte array is a valid peer ID, but per BEP 20:

A number of clients begin the peer id with a dash followed by two characters to identify the client implementation, four ascii digits to denote version number, and a dash [emphasis mine]. As with mainline, the remaining bytes are random. An example is -AZ2060-

It's often called "azureus style" and that's what qBit does.

I would prefer to add new methods like…

Got you. I guess it depends on where you want to expose them. I'm also considering opening a non-allocating API with a more structured version (I only allocate when turning things into strings), the current API just has the least chance of needing to have backwards incompatible changes. I'll probably open it after I start passing Transmission test suite. Using that would help avoiding unnecessary version parsing.

We should not have specific code/behavior.

I was primarily thinking of adding serde feature flag with serialisers/deserialisers. The main issue is that there are several representations that people might need depending on the circumstances (byte array, hex string, escaped ASCII/base62-ish string), and I'm on the fence on how to handle it. This unfortunately can't be delegated to users of the library because of the orphan rules (well, unless people do newtype, which is inconvenient).

Fair enough re: extracting generic code first, as things stand we basically have parallel implementations and it's great as it makes easier to see what can be refactored out.

I found this in JavaScript

Yes! That's what I was using as my primary inspiration for the parsing logic, which they in turn inherited from older Java code. I had to fix a bunch of problems there (for example, they don't handle Transmission versions correctly) and add a few new clients (like BiglyBT and its Android version), so tdyne-peer-id-registry is strictly a superset of what they can handle. I included the set of peer IDs they test in the test suite even.

Thank you for the heads up re: signing! Will figure it out later today and amend the PR.

[josecelano]

Why is not a valid Peer ID?

Strictly speaking any 20-byte array is a valid peer ID, but per BEP 20:

A number of clients begin the peer id with a dash followed by two characters to identify the client implementation, four ascii digits to denote version number, and a dash [emphasis mine]. As with mainline, the remaining bytes are random. An example is -AZ2060-

It's often called "azureus style" and that's what qBit does.

OK, I did not know about it. Maybe I should read all BEPs. I think that's the reason why it makes sense to extract packages. Even the most simple domain has its own complexity when you dive into it.

I would prefer to add new methods like…

Got you. I guess it depends on where you want to expose them. I'm also considering opening a non-allocating API with a more structured version (I only allocate when turning things into strings), the current API just has the least chance of needing to have backwards incompatible changes. I'll probably open it after I start passing Transmission test suite. Using that would help avoiding unnecessary version parsing.

We should not have specific code/behavior.

I was primarily thinking of adding serde feature flag with serialisers/deserialisers. The main issue is that there are several representations that people might need depending on the circumstances (byte array, hex string, escaped ASCII/base62-ish string), and I'm on the fence on how to handle it. This unfortunately can't be delegated to users of the library because of the orphan rules (well, unless people do newtype, which is inconvenient).

Cool! I did not know either about "the orphan rules". I'm a newbie with Rust. I think in this case, I would implement all of them, since I guess there are not many more cases and the package is very specific for Peer Id, so I do not see a problem that the package can exceed its responsibility. Maybe I would even make sense to have new types for the other representations: HexPeerId, Base62PeerId, etc. It they need to have associated logic. But I do not see it for the time being. I think it would be enough with some named constructors.

Fair enough re: extracting generic code first, as things stand we basically have parallel implementations and it's great as it makes easier to see what can be refactored out.

I found this in JavaScript

Yes! That's what I was using as my primary inspiration for the parsing logic, which they in turn inherited from older Java code. I had to fix a bunch of problems there (for example, they don't handle Transmission versions correctly) and add a few new clients (like BiglyBT and its Android version), so tdyne-peer-id-registry is strictly a superset of what they can handle. I included the set of peer IDs they test in the test suite even.

I think that's the best way to abstract away the logic, from two concrete cases.

Thank you for the heads up re: signing! Will figure it out later today and amend the PR.

Thank you!!

[si14]

Just force-pushed a signed commit!

Even the most simple domain has its own complexity when you dive into it.

Haha yes I spent waaaayyy longer on this than I expected initially

[josecelano]

Hi @si14 This needs a rebase. Besides, @da2ce7 has removed the minor version from Cargo.toml

[si14]

@josecelano seems that CI/coverage didn't pick up the force push, is there anything I can do to help you guys move this forward?

[josecelano]

ACK cb914da

[josecelano]

@josecelano seems that CI/coverage didn't pick up the force push, is there anything I can do to help you guys move this forward?

Hi @si14 ; Yes, I've just run the workflow again, and I could merge it. Thanks! These days I'm a little bit sick, so it might take me longer to reply.