transmission / transmission

Official Transmission BitTorrent client repository
https://transmissionbt.com
Other
11.67k stars 1.18k forks source link

fix: various pex flag bugs and cleanup #6917

Open tearfur opened 1 week ago

tearfur commented 1 week ago

Fixes #6915:

Fixes #6910: Fixes #5816:

Others:

Notes: Fixed various bugs related to PEX flags.

reardonia commented 1 week ago

This is great. A few thoughts:

1) Would it be more consistent for peer_info::set_seed to set ADDED_F_SEED_FLAG? Just want to avoid any situation where the pex flag is set but peer_info->is_seed_ is not set, and vice-versa. eg, pex flags are saved to resume file but isseed (and most of peer_info object) is not, so you want to ensure that state is always saved?

2) ensure_info_exists currently wipes the pex flags. So isseed is lost for existing peer_info. For example after an announce, each peer is run through tr_peerMgrAddPex and flags reset. Maybe should be peer_info->set_pex_flags(peer_info->pex_flags() | flags) within ensure_info_exists?

3) in parse_ltep_handshake is there a reason some info triggers publish() and other just directly manipulates peer_info?

4) a common case is a peer sending us ipv6 during LTEP handshake. looks like the seed flag is lost, so we immediately try a connection to the ipv6 peer when we shouldn't. also, looks like we should set ADDED_F_CONNECTABLE when we add peer as TR_PEER_FROM_LTEP ?

5) more parse_ltep_handshake : we should also set ADDED_F_HOLEPUNCH if we see that in the LTEP exchange, at the same time that we set ? no reason not to, even if currently unsupported it has value for other peers during PEX.

re 4-6) should parse_ltep_handshake start by initializing the pex->flags to peer_info->pex_flags()? that way you can just or-in the flags you want as you hit upload_only, port, ipv6, holepunch, etc.

tearfur commented 1 week ago
  1. Feel free to read tr_peer_info::pex_flags() and tr_peer_info::set_pex_flags() to convince yourself that setting is_seed_ is equivalent to setting ADDED_F_SEED_FLAG.
  2. Good point. ad8f09288bd58b232ef088985021dcab595c6002
  3. Those that triggers publish() means that we need to the access the swarm, instead of just the peer info object, to properly process it.
  4. I don't see how the seed flag would be lost. Also, we shouldn't set ADDED_F_CONNECTABLE when adding peers from ltep, because by BEP-11's definition, this flag is only set for peers whom we successfully initiated an outgoing connection with.
  5. Good point. 9a51cdb4b3014de61c78b03d92f3c57f97fc146a

re 4-6) should parse_ltep_handshake start by initializing the pex->flags to peer_info->pex_flags()? that way you can just or-in the flags you want as you hit upload_only, port, ipv6, holepunch, etc.

Is there anything wrong with what I'm doing now?

tearfur commented 5 days ago

I am rethinking whether it was a good idea to filter out peers that weren't marked as connectable in tr_peerMgrAddPex(). If the peer turned out to be not very useful, it will (or is supposed to) be dropped by the peer info pulse anyway, so why risk missing out on some good but untested peers?

I went ahead and removed the filter 7fddbd8.

reardonia commented 2 days ago

@tearfur I can't seem to apply this to current head @ ec5296c , can you rebase? (my git-fu is weak, apologies)

tearfur commented 2 days ago

@reardonia Done.