Review format for peer id in announce response

[josecelano]

A comment on this PR originated a discussion and finally this issue.

The HTTP tracker response (non compact response) for an announce request looks like this:

Bencoded bytes:

d8:completei2e10:incompletei0e8:intervali120e12:min intervali120e5:peersld2:ip9:126.0.0.17:peer_id40:2d714230303030303030303030303030303030314:porti8080eeee

And the deserialized data into a Rust struct:

Announce {
    complete: 2,
    incomplete: 0,
    interval: 120,
    min_interval: 120,
    peers: [
        DictionaryPeer {
            ip: "126.0.0.1",
            peer_id: "2d71423030303030303030303030303030303031",
            port: 8080,
        },
    ],
}

Before merging this PR the peer_id field was always empty. This was the commit were it was fixed.

Not it returns the hex string of the peer id.

I think we should not convert the peer ID into an UTF8 string since bencode serialization allows any byte array for what they call a "string".

A byte string (a sequence of bytes, not necessarily characters) is encoded as :. The length is encoded in base 10, like integers, but must be non-negative (zero is allowed); the contents are just the bytes that make up the string. The string "spam" would be encoded as 4:spam. The specification does not deal with encoding of characters outside the ASCII set; to mitigate this, some BitTorrent applications explicitly communicate the encoding (most commonly UTF-8) in various non-standard ways. This is identical to how netstrings work, except that netstrings additionally append a comma suffix after the byte sequence.

Code

This is the function the build the announce response:

https://github.com/torrust/torrust-tracker/blob/develop/src/http/handlers.rs#L134-L169

/// Send announce response
#[allow(clippy::ptr_arg)]
fn send_announce_response(
    announce_request: &request::Announce,
    torrent_stats: &torrent::SwamStats,
    peers: &Vec<peer::Peer>,
    interval: u32,
    interval_min: u32,
) -> WebResult<impl Reply> {
    let http_peers: Vec<response::Peer> = peers
        .iter()
        .map(|peer| response::Peer {
            peer_id: peer.peer_id.to_string(),
            ip: peer.peer_addr.ip(),
            port: peer.peer_addr.port(),
        })
        .collect();

    let res = response::Announce {
        interval,
        interval_min,
        complete: torrent_stats.seeders,
        incomplete: torrent_stats.leechers,
        peers: http_peers,
    };

    // check for compact response request
    if let Some(1) = announce_request.compact {
        match res.write_compact() {
            Ok(body) => Ok(Response::new(body)),
            Err(_) => Err(reject::custom(Error::InternalServer)),
        }
    } else {
        Ok(Response::new(res.write().into()))
    }
}

It uses the torrust_tracker::tracker::peer::to_string() function.

impl std::fmt::Display for Id {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self.to_hex_string() {
            Some(hex) => write!(f, "{hex}"),
            None => write!(f, ""),
        }
    }
}

For a random 20-byte array you cannot always build a UTF8 string but you can always generate the hex representation. I decided to use the hex format for displaying the peer ID.

Review format in the announce reqeust

I think we should return the 20-byte array without converting it into a Rust String.

I have installed a different tracker and confirmed they are returning the 20-byte array:

#162 (comment)

Although for some reason they are returning an extra byte.

Review the Display trait implementation

Even if we change what we return we could keep the Display trait as it is, returning the hex representation, but I do not know if that the right solution.

Prons:

• It always return the same format (hex)
• It's always non empty

Cons:

• The Display trait is intended for user-friendly output. It could lead to misinterpretation since the original value is a 20-byte array.

Proposal:

I think most client uses a sequqence of ASCII bytes that can be represented as a String with this format -qB00000000000000001. That probably what a BitTorrent client will show or other interfaces to read byte sequences like:

00000000: 6438 3a63 6f6d 706c 6574 6569 3165 3130  d8:completei1e10
00000010: 3a69 6e63 6f6d 706c 6574 6569 3065 383a  :incompletei0e8:
00000020: 696e 7465 7276 616c 6936 3030 6535 3a70  intervali600e5:p
00000030: 6565 7273 6c64 323a 6970 333a 3a3a 3137  eersld2:ip3:::17
00000040: 3a70 6565 7220 6964 3230 3a2d 7142 3030  :peer id20:-qB00
00000050: 3030 3030 3030 3030 3030 3030 3030 3134  0000000000000014
00000060: 3a70 6f72 7469 3137 3534 3865 6565 65    :porti17548eeee

I think we could try to build a valid String and show the string if possible or the byte array or hex representation if we cannot convert the byte array into a valid String. But I do not like to display different formats. Maybe we could print the byte array and an extra value if that array is also a valid string, like this;

#[test]
fn should_be_converted_into_string() {
    let id = peer::Id(*b"-qB00000000000000001");
    assert_eq!(id.to_string(), "([2d 71 42 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 31],"-qB00000000000000001"));
}

What do you think @WarmBeer @da2ce7?

[da2ce7]

It is fine for the "Display" function to return a Hex Encoded Array, however best to prefix the Hex String with a ´0x´, to make it explicit.

[josecelano]

This PR fixes this issue but only in the new Axum implementation. But we can keep it open even after merging that PR until we implement @da2ce7's proposal: prefix the Hex String with a ´0x´.