Hi David,
On 15 May 2020, at 20:53, David Goulet dgoulet@torproject.org wrote:
On 15 May (13:58:06), teor wrote:
Nick and I were talking about how we remove legacy features in tor, and their corresponding subprotocol versions.
Here is a list of the current subprotocol versions: https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt#n2049
Here's a recent protocol version proposal, which deals with recommending and requiring new features: https://gitweb.torproject.org/torspec.git/tree/proposals/303-protover-remova...
But we don't have a similar proposal for removing support for older protocol versions from the tor codebase.
For an example of what that proposal could look like, see our proposal for deprecating consensus methods: https://gitweb.torproject.org/torspec.git/tree/proposals/290-deprecate-conse...
Here's the original conversation Nick and I had: https://github.com/torproject/tor/pull/1874#discussion_r423713579
But after reading our consensus methods deprecation proposal, I've changed my mind. I think that we should check for "protocol N, and any later version" by default.
I agree that this is the right approach imo as well.
That's what we do for consensus methods, and it seems to work well. We can drop the earliest consensus methods, and recent tor versions just keep working.
If we need an incompatible change, we can make it another protocol version, and recommend then require support for it.
So here's an edited version of my notes on that ticket:
There are a few instances of ">=" and "=" confusion in protocol versions. We should try to fix them all.
It only matters when we remove protocol versions. We haven't really specified, tested, or exercised this functionality in practice. And so our reviewers lack experience. (And when we did discover a need for it with NSS and LinkAuth, it was more complicated than we expected.)
I'd like to see a proposal that tells us how to check future protocol versions as they are added. Along with a migration plan for disabling protocol versions.
So let's also open a ticket to check for "any future version". We should replace all "=" checks with ">=". Let's make sure we check all the places where we use protocol versions, even if they don't have a summary flag.
Overall, I think it would be helpful if future protocol versions were orthogonal. Or if they depend on earlier features, that dependency should be clearly documented. (For example, Relay=3 IPv6 extends depends on Relay=2 EXTEND2 cells. So if we were checking EXTEND2 cell support, it would be Relay=2 or Relay=3.)
At the moment, they do depend between each other last time I had that discussion with Nick. As in the later in your example.
Which means that supporting protocol version with a ">=" is consistent with our "non-written expectations" that we have now.
That's how most protocol versions work right now.
The following protocol versions are exceptions: * Link all Link protocols introduce incompatible changes, but a shared Link protocol is dynamically negotiated at runtime * LinkAuth=3 might not support LinkAuth=1 (RSA link authentication) * Padding=1 accidentally enabled for relays that don't support circuit-level padding
Link is a special case because it's the lowest-level protocol, and negotiated at runtime. (And using an unknown feature terminates the connection.)
LinkAuth became incompatible when we introduced NSS support. But a better design might have been: * LinkAuth=1 RSA only * LinkAuth=3 ed25519 and RSA * LinkAuth=4 ed25519 only
Padding was a mistake :-(
So if I understand correctly, we'll need to add a new protocol version let say N+1 in order to deprecate anything <= N ?
As an example, Relay=4 could mean "deprecate Relay=2 and use only Relay=3"
I'm +1 on this if iiuc.
Yes, I think that's what we should do in future.
So we should: * write a proposal or spec update, and * change the current code to use "protocol_list_supports_protocol_or_later() in most cases.
I'm going to wait for asn or nickm to weigh in, before opening tickets.
T