webtorrent / bittorrent-protocol

Simple, robust, BitTorrent peer wire protocol implementation
https://webtorrent.io
MIT License
349 stars 71 forks source link

Update PE/MSE implementation #120

Closed ghost closed 1 year ago

ghost commented 1 year ago

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update [X] Bug fix [ ] New feature [ ] Other, please explain:

What changes did you make? (Give an overview) I made some updates to the code that handles protocol encryption/obfuscation to make it more compatible with the specification and known implementations. I also changed code related to stream events.

Which issue (if any) does this pull request address? https://github.com/webtorrent/webtorrent/issues/2594

Is there anything you'd like reviewers to focus on?

I have tested this code with https://github.com/anma-dev/webtorrent/blob/pe-mse-2x/test/node/protocol-encryption.js. This test is passing for WebTorrent 2.x.

I've also done some testing against other implementations. Outgoing encrypted connections work, but incoming ones still need fixing.

welcome[bot] commented 1 year ago

🙌 Thanks for opening this pull request! You're awesome.

ThaUnknown commented 1 year ago

Incoming peers are unable to pass the third step of the handshake (parsePe3) and get stuck.

in practice, what does this mean? does this mean that the peers can never connect to eachother?

ghost commented 1 year ago

This means that the peer can establish outgoing encrypted connections, but cannot accept incoming ones, and needs improvement.

There is a step in the handshake where the peer must block and establish encryption generators before continuing. I think handling this block is the source of the complexity, at least for me. as it is now in the master, it uses a sync block that becomes an infinite loop and blocks not only this handshake step, but blocks the handshake completely.

We can turn this into a draft PR if you prefer, as it is unfinished. I just wanted to get things started.

ThaUnknown commented 1 year ago

which means webtorrent can't ever connect to another webtorrent peer when they both use encryption? yeah then this should be a draft point me towards the loop causing said issue?

ghost commented 1 year ago

which means webtorrent can't ever connect to another webtorrent peer when they both use encryption? yeah then this should be a draft point me towards the loop causing said issue?

Yes, it means that if both peers are using a webtorrent client, there is a problem, but with the code in this pr, if webtorrent connects to another type of bittorrent client, and both webtorrent and the other client use encryption, and even if the other client requires it, encrypted outgoing seeding and encrypted outgoing downloading is possible. This is an improvement because this repo is about tcp/utp connections, so it needs to be interoperable with other clients, and this improves it.

Correct me if I'm wrong, but as I understand it, this repo supports "require outgoing/allow incoming" for encrypted connections. This means that webtorrent with the secure option enabled will still downgrade the encryption to cleartext if the incoming client initiates a cleartext handshake. I believe this is by design to ensure backwards compatibility, and requiring incoming encrypted connections might mean a different pr to enable.

The code that hangs the handshake I've identified is in the parsePe2 function. You can test by running two clients against each other and if the process hangs you can easily note the node process id and attach a debugger with node inspect -p PROCESSID. When I did that, I got a line of code that was hanging and it was in the

https://github.com/webtorrent/bittorrent-protocol/blob/master/index.js#L1156

But also potentially

https://github.com/webtorrent/bittorrent-protocol/blob/master/index.js#L1170

I think a possible solution might be to use async/await or "old school" generators. The async/await is tricky because the _write function that calls the _parser must not be async because it gets a callback.

ThaUnknown commented 1 year ago

nooooo

I haven't had time to look at this, when I had spare time other breaking issues in webtorrent were more pressing like HTTP tracker mangling infohashes, torrent-piece not working correctly and others