Opened 8 months ago

Last modified 7 weeks ago

#28930 needs_revision enhancement

consider reordering PT/proxy phases

Reported by: catalyst Owned by: ahf
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: pt, proxy, 040-deferred-20190220, ex-sponsor-19, network-team-roadmap-2019-Q1Q2, ex-28018-child, bootstrap, ex-sponsor19
Cc: brade, mcs, arma Actual Points: 8.5
Parent ID: Points: 3
Reviewer: catalyst Sponsor: Sponsor28-can

Description

A pluggable transport can itself use a firewall-bypass proxy, so maybe the progression should go more like

  1. conn_pt
  2. conn_proxy
  3. conn_proxy_done
  4. conn_pt_done

Child Tickets

Change History (14)

comment:1 Changed 8 months ago by catalyst

Sponsor: Sponsor8Sponsor19

comment:2 Changed 7 months ago by ahf

Owner: changed from catalyst to ahf

comment:3 Changed 6 months ago by nickm

Keywords: 040-deferred-20190220 added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.

comment:4 Changed 4 months ago by gaba

Points: 3

comment:5 Changed 3 months ago by catalyst

The change would be to reorder the phases so the proxy phases are "inside" the PT phases. Last I checked, tor doesn't know about whether or not the PT client itself uses a proxy, so the PT would have to explicitly tell tor.

We can do the reordering now, or wait until we have a PT client interface that can report intermediate progress when using a PT tunneled through a proxy. ahf, which do you think is better?

There might be reasons to have these phases in any number of slightly different orderings, including the current ordering. I would have to refresh my memory about the current behavior to be sure.

comment:6 Changed 3 months ago by gaba

Keywords: ex-sponsor-19 added

Adding the keyword to mark everything that didn't fit into the time for sponsor 19.

comment:7 Changed 3 months ago by gaba

Keywords: sponsor19-also s8-bootstrap removed
Sponsor: Sponsor19

comment:8 Changed 3 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:9 Changed 3 months ago by catalyst

Keywords: ex-28018-child bootstrap ex-sponsor19 added
Parent ID: #28018

Unparent remaining open children of #28018.

comment:10 Changed 8 weeks ago by ahf

Actual Points: 8.5
Status: assignedneeds_review

Moving this to needs review.

PR in https://github.com/torproject/tor/pull/1151

comment:11 Changed 8 weeks ago by asn

Reviewer: catalyst

comment:12 in reply to:  10 Changed 7 weeks ago by catalyst

Replying to ahf:

Moving this to needs review.

PR in https://github.com/torproject/tor/pull/1151

Thanks!

The stuff to explicitly note whether we're using a PT looks good. I haven't done manual testing of it yet.

I think when we talked about this on IRC we agreed to have separate new phases for "PT client is connecting/connected to its proxy"?

(Summary -- control-spec.txt already is overly specific and refers to the TCP connection to the PT proxy. So my proposed reordering turns out to be wrong. I think having more specific new bootstrap phases also makes troubleshooting easier.)

For the reviewer: Should we change the tests in test_controller_events

to instead of storing a single string each time we have a bootstrap
event, that we instead append to a list and then check if the list is
correct after all the state transitions have happened?

Yes, I think there might places in the unit tests where that would be helpful. Or maybe make the entire "make a state change and assert an expected bootstrap message" a macro?

comment:13 Changed 7 weeks ago by catalyst

Status: needs_reviewneeds_revision

comment:14 Changed 7 weeks ago by catalyst

Sponsor: Sponsor28-can
Note: See TracTickets for help on using tickets.