add tests for announce extension support stage #25

sarp · 2024-08-21T19:48:12Z

This PR adds a test for announce extension support stage
Added checks for reserved bytes to handshake

It also contains refactoring to support easier passing of parameters to peers goroutine:

renamed listenAndServePeersResponse to listenAndServeTrackerResponse
added TrackerParams and PeerConnectionParams types for easier parameter plumbing
added MagnetTestParams to make it easier to create magnet tests with all parameters

rohitpaulk

Added notes on backward compatibility for older stages

rohitpaulk · 2024-08-22T20:39:10Z

internal/stage_handshake.go

@@ -63,9 +60,29 @@ func testHandshake(stageHarness *test_case_harness.TestCaseHarness) error {
 return err
 }

+ expectedReservedBytes := []byte{0, 0, 0, 0, 0, 0, 0, 0}


@sarp when users work on the later stages, their code will be expected to work against older stages too. Based on this diff, it seems like we're going to fail if the user's handshake indicates support for extensions, no?

good question, yes, I was thinking people would have to build-in the capability to enable / disable extension support for older stages. would be a good thing to call out in the course description if we're going down this path

is there a better way? if we were to become more lenient for the handshake stage tests, say also accept extensions in handshake tests for the base challenge, I think download piece and download file stages would also fail because the sequence of messages is different (you'd need to do the extension handshake once you signal support for extensions)

Generally, we try to keep tests flexible enough to handle later implementations and don't burden the user with having to maintain backward compatibility.

I do see how this could interact with the download_piece/download_file though. Let me think about this a bit... will respond today

@sarp a peer would only initiate the extension handshake if the base handshake received included the extensions bit, yes? In that case I think we're good - users won't have to disable/enable extension support based on the stage they're on (i.e. the command they're handling), they'd just need to do so based on what bits they receive from a peer in the initial handshake.

We'd still want to be tolerant of the user sending their extension bit set in the initial handshake (for any stage), but we don't need to send it in our client's handshake for earlier stages - and thus won't have to expect the extension handshake to take place.

a peer would only initiate the extension handshake if the base handshake received included the extensions bit, yes?

correct

In that case I think we're good - users won't have to disable/enable extension support based on the stage they're on (i.e. the command they're handling), they'd just need to do so based on what bits they receive from a peer in the initial handshake.

problem is that the tracker you've set up on bittorrent-test-tracker.codecrafters.io always indicates during base handshake that it supports extensions, regardless of the other client supporting it or not. and once both parties support it, extension handshake messages will be sent, also for base stages ...

I see two options:

one way could be to have two sets of trackers, one for base stages that is configured without extensions support and one tracker with extension support for magnet links and future challenges that need extensions (like DHT)

other way is that users build the capability to disable extensions for base stages and enable it for magnet links and future extensions (have a flag and enable it for magnet commands)

problem is that the tracker you've setup on bittorrent-test-tracker.codecrafters.io always indicates during base handshake that it supports extensions, regardless of your client supporting it or not. and once both parties support it, extension handshake messages will be sent, also for base stages ...

Ah, yeah true. This too might be fine though - the tracker won't initiate an extension handshake unless it receives the extension bit set in the handshake from the user's peer.

If the user does set the extension bit in earlier stages seems fair to expect that they must handle the extension handshake if the tracker advertises it too.

What I'm trying to avoid here is anything "CodeCrafters-specific" - i.e. code a user would have to write only to pass CodeCrafters tests, but wouldn't have to write when building a real-world implementation. I think the flow described above doesn't count as CodeCrafters-specific - it's just something an implementation would have to handle anyway.

(Maybe this is what you meant by "people would have to build-in the capability to enable / disable extension support for older stages", apologies if so)

No, it seems reasonable to me to have the ability to enable / disable extension support in a BitTorrent client, wouldn't call it "CodeCrafters-specific"

sarp · 2024-08-23T19:19:16Z

How do you want to proceed on this PR?

If we make handshake more lenient (allow both extensions enabled and disabled), download-piece and download-file stages will fail until they send the extension handshake (next stage), so they will have to disable it for base stages anyway. So I don't see the value of making it more lenient.

So I'm in favor of my initial approach, where base stages require extensions to be disabled and magnet links require extensions to be enabled.

rohitpaulk · 2024-08-23T19:30:11Z

If we make handshake more lenient (allow both extensions enabled and disabled), download-piece and download-file stages will fail until they send the extension handshake (next stage), so they will have to disable it for base stages anyway. So I don't see the value of making it more lenient.

Is it okay to assume that when working on the base stages, it's unlikely that a user will intentionally set the extensions bit to true? If that assumption is safe, then leniency should be okay for the base stages - the user won't have to handle the extension handshake in download_file and download_pieces while working on the base stages.

I still think that conditionally disabling/enabling extension protocol support is a CodeCrafters-specific thing, it seems unlikely that you'd want to do this in a real-world client.

To clarify what I mean:

Reasonable: expect a real-world client must only perform an extension handshake if the remote peer advertises extension support too
Unreasonable: expect a real-world client to keep a flag that disables/enables whether that client should advertises extension support
Reasonable: expect a real-world client to allow disabling/enabling a specific extension like magnet URLs (but this isn't what we're talking about here - we're talking about disabling support for the extension protocol entirely)

sarp · 2024-08-23T19:54:32Z

Is it okay to assume that when working on the base stages, it's unlikely that a user will intentionally set the extensions bit to true? If that assumption is safe, then leniency should be okay for the base stages - the user won't have to handle the extension handshake in download_file and download_pieces while working on the base stages.

Yes, this is what our stage instructions say:
eight reserved bytes, which are all set to zero (8 bytes)
And this is how our tester currently checks for all bytes to be zero

If that assumption is safe, then leniency should be okay for the base stages - the user won't have to handle the extension handshake in download_file and download_pieces while working on the base stages.

I don't follow this part ^

I still think that conditionally disabling/enabling extension protocol support is a CodeCrafters-specific thing, it seems unlikely that you'd want to do this in a real-world client.

To clarify what I mean:

Reasonable: expect a real-world client must only perform an extension handshake if the remote peer advertises extension support too

Unreasonable: expect a real-world client to keep a flag that disables/enables whether that client should advertises extension support

Reasonable: expect a real-world client to allow disabling/enabling a specific extension like magnet URLs (but this isn't what we're talking about here - we're talking about disabling support for the extension protocol entirely)

What is your proposal in that case?

only perform an extension handshake if the remote peer advertises extension support too

I've explained that this wouldn't work in our scenario as the peers on our tracker always advertise extensions, and if you don't perform the extension handshake after saying you support extensions, they will drop your connection

expect a real-world client to allow disabling/enabling a specific extension like magnet URLs

This wouldn't help in our scenario either

rohitpaulk · 2024-08-23T20:09:21Z

My concrete proposal is that in the early handshake stage, our tester should accept both "no extensions supported" or "extensions supported" for the reserved bits part.

As long as it does that, I think the last 2 stages should "just work"? Explanation below:

In the last 2 stages
- When working on base stages:
  - everything would just work as-is - the user doesn't advertise extension support, so the remote peer won't perform the extension handshake.
- After the base stages are complete
  - if the user starts to advertise extension support in earlier stages too, their implementation will need to perform the extension handshake (since their code advertised it).
  - If their code doesn't advertise it, same as before - everything should just work

rohitpaulk · 2024-08-23T20:11:31Z

Apologies if I'm missing something here btw! Been a long day and I think I'm making sense but maybe I'm not 😛 Can take a look at this fresh tomorrow if that helps.

sarp · 2024-08-23T20:12:44Z

After the base stages are complete
if the user starts to advertise extension support for earlier stages, their implementation will need to perform the extension handshake (since their code advertised it).

But they haven't implemented it yet, they will implement it in the next 2 stages. This stage is modifying base handshake, next two stages implement extension handshake (they should have given a different name!)

rohitpaulk · 2024-08-23T20:28:53Z

But they haven't implemented it yet, they will implement it in the next 2 stages.

Ahh okay gotcha, true. That’s the part I was missing. Will think fresh tomorrow and reply!

rohitpaulk · 2024-08-24T19:18:57Z

Okay! Had some time to think about this today, this suggestion looks good to me:

one way could be to have two sets of trackers, one for base stages that is configured without extensions support and one tracker with extension support for magnet links and future challenges that need extensions (like DHT)

@sarp just to confirm, it'd be okay to keep the tracker the same and just have different sets of peers, yes? If so, please feel free to proceed assuming this is what we'll do, and I'll try to set this up on Mon/Tue

sarp · 2024-08-26T13:39:03Z

Okay! Had some time to think about this today, this suggestion looks good to me:

one way could be to have two sets of trackers, one for base stages that is configured without extensions support and one tracker with extension support for magnet links and future challenges that need extensions (like DHT)

@sarp just to confirm, it'd be okay to keep the tracker the same and just have different sets of peers, yes? If so, please feel free to proceed assuming this is what we'll do, and I'll try to set this up on Mon/Tue

yes, that should be fine.
only peers without extension support for the base challenge
only peers with extension support for magnet links extension

rohitpaulk · 2024-08-26T13:45:43Z

Sounds good - will set this up tomo!

sarp · 2024-09-05T09:43:56Z

friendly ping, are we good to proceed here?

rohitpaulk · 2024-09-06T09:36:31Z

Ah, sorry this disappeared my list since I submitted a "request changes" review earlier. Looks good!

sarp changed the title ~~Sarp/magnet2~~ add tests for Advertise Extension Support stage Aug 21, 2024

sarp force-pushed the sarp/magnet2 branch 2 times, most recently from 60d140c to 7d89271 Compare August 22, 2024 18:22

sarp changed the title ~~add tests for Advertise Extension Support stage~~ add tests for announce extension support stage Aug 22, 2024

sarp force-pushed the sarp/magnet2 branch 2 times, most recently from fdb2ef5 to a4469c6 Compare August 22, 2024 18:37

add tests for announce extension support stage

5b2c1f7

sarp force-pushed the sarp/magnet2 branch from a4469c6 to 5b2c1f7 Compare August 22, 2024 18:45

sarp requested a review from rohitpaulk August 22, 2024 18:48

sarp marked this pull request as ready for review August 22, 2024 18:49

rohitpaulk requested changes Aug 22, 2024

View reviewed changes

allow both {0,0,0,0,0,0,0,0} and {0,0,0,0,0,16,0,0} during handshake

b48d448

rohitpaulk approved these changes Sep 6, 2024

View reviewed changes

sarp merged commit c0d2197 into main Sep 6, 2024
1 check passed

sarp deleted the sarp/magnet2 branch September 11, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tests for announce extension support stage #25

add tests for announce extension support stage #25

sarp commented Aug 21, 2024 •

edited

Loading

rohitpaulk left a comment

rohitpaulk Aug 22, 2024

sarp Aug 22, 2024 •

edited

Loading

rohitpaulk Aug 23, 2024

rohitpaulk Aug 23, 2024

sarp Aug 23, 2024 •

edited

Loading

rohitpaulk Aug 23, 2024 •

edited

Loading

rohitpaulk Aug 23, 2024 •

edited

Loading

sarp Aug 23, 2024

sarp commented Aug 23, 2024

rohitpaulk commented Aug 23, 2024

sarp commented Aug 23, 2024 •

edited

Loading

rohitpaulk commented Aug 23, 2024 •

edited

Loading

rohitpaulk commented Aug 23, 2024

sarp commented Aug 23, 2024

rohitpaulk commented Aug 23, 2024

rohitpaulk commented Aug 24, 2024

sarp commented Aug 26, 2024

rohitpaulk commented Aug 26, 2024

sarp commented Sep 5, 2024

rohitpaulk commented Sep 6, 2024

add tests for announce extension support stage #25

add tests for announce extension support stage #25

Conversation

sarp commented Aug 21, 2024 • edited Loading

rohitpaulk left a comment

Choose a reason for hiding this comment

rohitpaulk Aug 22, 2024

Choose a reason for hiding this comment

sarp Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2024

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2024

Choose a reason for hiding this comment

sarp Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

rohitpaulk Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

sarp Aug 23, 2024

Choose a reason for hiding this comment

sarp commented Aug 23, 2024

rohitpaulk commented Aug 23, 2024

sarp commented Aug 23, 2024 • edited Loading

rohitpaulk commented Aug 23, 2024 • edited Loading

rohitpaulk commented Aug 23, 2024

sarp commented Aug 23, 2024

rohitpaulk commented Aug 23, 2024

rohitpaulk commented Aug 24, 2024

sarp commented Aug 26, 2024

rohitpaulk commented Aug 26, 2024

sarp commented Sep 5, 2024

rohitpaulk commented Sep 6, 2024

sarp commented Aug 21, 2024 •

edited

Loading

sarp Aug 22, 2024 •

edited

Loading

sarp Aug 23, 2024 •

edited

Loading

rohitpaulk Aug 23, 2024 •

edited

Loading

rohitpaulk Aug 23, 2024 •

edited

Loading

sarp commented Aug 23, 2024 •

edited

Loading

rohitpaulk commented Aug 23, 2024 •

edited

Loading