Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#31356 closed defect (fixed)

0.4.1 relays should list Padding=2

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.0.5
Severity: Normal Keywords: wtf-pad, 041-must, 041-backport, dgoulet-merge, nickm-merge
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: asn Sponsor: Sponsor2

Description (last modified by teor)

Somehow we accidentally merged the protover for padding support while doing the incremental merge thing, and 0.4.0 relays are advertising padding that they don't support. This is mostly harmless, because the negotiation will not succeed and then clients will stop, but it will result in those clients emitting a "Middle node did not accept our padding request" protocol warn/info message.

We should just remove this protover field from 0.4.0.x.

At the weekly meeting last week, we decided that we can't remove a protover once it's been released.
Instead, we will:

  • make 0.4.1 and later relays declare Padding=2 (pre-0.4.1 stable)
  • make 0.4.1 and later clients require Padding=2 (padding is not on by default, so we can do this at any time)

Edited to simplify: we don't need to preserve compatibility with alphas.

Child Tickets

TicketStatusOwnerSummaryComponent
#31387closedMake 0.4.1 and later clients require Padding=2Core Tor/Tor
#31392closedExplain Padding 1 and 2 in tor-spec.txtCore Tor/Tor
#31393closedStop declaring support for Padding=1 after 0.4.1-stable is releasedCore Tor/Tor

Change History (19)

comment:1 Changed 2 months ago by teor

Status: newneeds_revision

Travis says:
(edited to reduce the length of the ticket)

Last edited 2 months ago by teor (previous) (diff)

comment:2 Changed 2 months ago by mikeperry

Status: needs_revisionneeds_review

PR that solves this bug: https://github.com/torproject/tor/pull/1227

Do not close #30992 please. This bug only adds logs for it.

comment:3 in reply to:  1 ; Changed 2 months ago by mikeperry

Replying to teor:

Travis says:
(edited to reduce the length of the ticket)

Did I mentiin my PR on any ticket before you posted this message? Was this a good use of your time?

What were you trying to accomplish here? I am honestly curious.

Is there a good reason to watch *all* PRs run for the purpose of exercising CI?

Seriously... This is kind of crazy.. let's discuss.

Anyway, since we're here, we might as well treat this PR as final cause that's how we do, apparently.

Thanks.

Last edited 2 months ago by teor (previous) (diff)

comment:5 in reply to:  3 Changed 2 months ago by teor

Replying to mikeperry:

...
Did I mentiin my PR on any ticket before you posted this message? Was this a good use of your time?

What were you trying to accomplish here? I am honestly curious.

Is there a good reason to watch *all* PRs run for the purpose of exercising CI?

Seriously... This is kind of crazy.. let's discuss.

Anyway, since we're here, we might as well treat this PR as final cause that's how we do, apparently.

Thanks.

Hi Mike,

I've been monitoring CI closely over the last few days, because Appveyor has been broken for about a week, since they upgraded the compiler in their CI image.

That means logging tickets for each new CI issue as they come up, and checking new builds to make sure that the issues are fixed.

I need CI to work, so I can merge the backport backlog.

On Travis, I filter out pull requests. On Appveyor, they're all in the same list. I guess I made a mistake? Or I was trying to be helpful? I'll try to be more careful in future.

I think what you want me to do is:

  • Don't look at or comment on your draft pull requests, or
  • Don't comment on any of your tickets, unless I'm specifically assigned as the reviewer

Just let me know what you want, and I'll try my best to do it in future.

I understand you're concerned or annoyed. But please don't call me words like "crazy". That's totally unnecessary. And please try to assume that I'm doing my best to be helpful. Even if I'm failing at it.

If you want to continue this conversation, let's do it at the monthly retrospective. Or let's talk about it next week. Because I'm just about to finish for the day.

comment:6 Changed 2 months ago by teor

Description: modified (diff)
Keywords: 041-should added
Milestone: Tor: 0.4.1.x-final
Status: needs_reviewneeds_revision
Summary: 0.4.0 relays should not list Padding=10.4.1 relays should list Padding=1,2

At the weekly meeting last week, we decided that we can't remove a protover once it's been released. See the ticket description for details. I'll open another ticket for the client change.

Mike, I'm happy to do this change, if that would help you with your work overload.

comment:7 Changed 2 months ago by teor

The client change is #31387.

comment:8 Changed 2 months ago by mikeperry

Status: needs_revisionneeds_review

I think we should keep this simple, so that it can get in before 0.4.1-stable, with low risk. We do not support alphas once the stable is released. This means that we can just bump the protover without doing all of the extra effort to phase out Padding=1 in stages.

In other words: if this gets merged before 0.4.1-stable, then once 0.4.1-stable is released, it won't matter that our older 0.4.1 alpha clients won't negotiate with newer 0.4.1 relays -- they are unsupported. Similarly, it doesn't matter if 0.4.1-stable clients won't negotiate with 0.4.1-alpha relays -- those relays are also unsupported.

Here is a cleanly organized PR against 0.4.1 that passes CI and is now actually ready for review: https://github.com/torproject/tor/pull/1230

FTR: I didn't call you crazy. I called this entire situation crazy (which I meant to include my response). I meant "crazy" in the sense that what we were doing did not make sense to me and made things confusing to others. The thing that upset me was that I am capable of viewing the CI logs of PRs that I'm still actively rebasing and re-organizing.. I didn't need those logs to occupy space in this ticket, and I didn't want them to, which is why I didn't ask anyone to review it or even mention the branch yet.. That said, I should have reacted more reasonably myself rather than mirroring and exaggerating what I felt were unreasonable actions, which is a bad habit of mine that I have been trying to break. It was the end of a long week.

comment:9 in reply to:  8 Changed 2 months ago by teor

Replying to mikeperry:

I think we should keep this simple, so that it can get in before 0.4.1-stable, with low risk. We do not support alphas once the stable is released.

You're right, that's a new policy that I hadn't quite caught up with.

This means that we can just bump the protover without doing all of the extra effort to phase out Padding=1 in stages.

In other words: if this gets merged before 0.4.1-stable, then once 0.4.1-stable is released, it won't matter that our older 0.4.1 alpha clients won't negotiate with newer 0.4.1 relays -- they are unsupported. Similarly, it doesn't matter if 0.4.1-stable clients won't negotiate with 0.4.1-alpha relays -- those relays are also unsupported.

We don't need to keep Padding=1 working in alphas, so it is much simpler to just bump the protover. (That's different to what we discussed in the meeting and in #tor-dev, but I agree with you now. We're not taking support away from a stable release, and no-one should be relying on the alpha behaviour.)

Here is a cleanly organized PR against 0.4.1 that passes CI and is now actually ready for review: https://github.com/torproject/tor/pull/1230

FTR: I didn't call you crazy. I called this entire situation crazy (which I meant to include my response). I meant "crazy" in the sense that what we were doing did not make sense to me and made things confusing to others. The thing that upset me was that I am capable of viewing the CI logs of PRs that I'm still actively rebasing and re-organizing.. I didn't need those logs to occupy space in this ticket, and I didn't want them to, which is why I didn't ask anyone to review it or even mention the branch yet.. That said, I should have reacted more reasonably myself rather than mirroring and exaggerating what I felt were unreasonable actions, which is a bad habit of mine that I have been trying to break. It was the end of a long week.

Thanks for this explanation. I am also trying to react less to situations which stress me out.

Here's how I'd like a conversation like this to go in future:

teor: does something unnecessary
mikeperry: Please don't do that thing. Can I delete your comment?
teor: Oops sorry. Of course! I will try to remember next time.

I am going to edit my comment with the logs to reduce the space it takes up.

comment:10 Changed 2 months ago by teor

Description: modified (diff)
Summary: 0.4.1 relays should list Padding=1,20.4.1 relays should list Padding=2

I also edited the ticket description to match our discussion, so the reviewer is not confused.

comment:11 Changed 2 months ago by teor

Looks good to me, but I think you probably want asn to review this patch? He's much more familiar with the padding code.

Do you want me to help by writing the tor-spec update for Padding=1 and Padding=2 ? (#31392)

comment:12 Changed 2 months ago by teor

Keywords: 041-must added; 041-should removed
Reviewer: asn

We must get this patch in 0.4.1. If we don't, we need a more complicated patch.
asn said he would review this patch in the team meeting.

comment:13 Changed 2 months ago by asn

Status: needs_reviewmerge_ready

Looks good to me!

I tested this both on the real network (and verified that a client with this patch will consider 041-alpha relays as unsupportive of padding), and also on chutney (and verified that padding will flow between a client and relays with this patch).

Thanks for the work everyone!

comment:14 Changed 2 months ago by teor

Keywords: 041-backport added
Owner: set to mikeperry
Status: merge_readyassigned

Fix owner and backport tags

comment:15 Changed 2 months ago by teor

Keywords: dgoulet-merge nickm-merge added
Status: assignedmerge_ready

Add merge tags. 0.4.1 is not stable yet, so the mainline mergers can merge.

comment:16 Changed 2 months ago by dgoulet

Status: merge_readyneeds_revision

@mikeperry: Can you make me a branch merged on master as well? The merge forward fails due to a conflict in circpadding.c and of course practracker...

https://github.com/torproject/tor/pull/1230

comment:17 Changed 2 months ago by asn

Status: needs_revisionmerge_ready

Let me know how this works. I made a branch that merges on master and resolved conflicts while on it: https://github.com/torproject/tor/pull/1235

comment:18 Changed 2 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:19 in reply to:  17 ; Changed 2 months ago by dgoulet

Replying to asn:

Let me know how this works. I made a branch that merges on master and resolved conflicts while on it: https://github.com/torproject/tor/pull/1235

Again, travis exploded... Not sure what is up here :S...

comment:20 in reply to:  19 Changed 2 months ago by teor

Replying to dgoulet:

Replying to asn:

Let me know how this works. I made a branch that merges on master and resolved conflicts while on it: https://github.com/torproject/tor/pull/1235

Again, travis exploded... Not sure what is up here :S...

The PR CI builds were cancelled when the PR was merged and closed. It was slightly naughty to merge before CI finished :-)

The merged branch had a temporary deb mirror network error in one CI job. The next build worked fine.

Note: See TracTickets for help on using tickets.