Opened 11 months ago

Closed 7 months ago

#27167 closed enhancement (implemented)

track "first" OR_CONN

Reported by: catalyst Owned by: catalyst
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: usability, ux, ux-team, bootstrap, 035-roadmap-subtask, 035-triaged-in-20180711, s8-bootstrap, 035-deferred-20180930, s8-errors
Cc: brade, mcs, linda, isabela, iry Actual Points:
Parent ID: #27103 Points:
Reviewer: nickm Sponsor: Sponsor8

Description

Right now the first stages of the "first" OR_CONN get reported as BOOTSTRAP_STATUS_CONN_DIR and BOOTSTRAP_STATUS_HANDSHAKE (the latter is a special bootstrap phase that gets translated into BOOTSTRAP_STATUS_HANDSHAKE_DIR or BOOTSTRAP_STATUS_HANDSHAKE_OR depending on how much progress was previously reported. The logic in functions that report these events should be moved up to a new abstraction so lower level code has to track less high-level state.

This also eliminates some logic in control_event_bootstrap() that tries to figure out whether a given handshake attempt corresponds to a directory connection or an application circuit connection.

Child Tickets

Change History (15)

comment:1 Changed 10 months ago by nickm

Keywords: 035-deferred-20180930 added
Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Deferring several items from 0.3.5. I think these are features, not bugfixes, and therefore no longer great for 0.3.5. Please let me know if I'm wrong.

comment:2 Changed 9 months ago by catalyst

Keywords: s8-errors added
Sponsor: Sponsor8

comment:3 Changed 9 months ago by catalyst

This probably depends on #28226 to help disentangle some of the existing deeply buried complicated logic.

comment:4 Changed 9 months ago by catalyst

Type: taskenhancement

comment:5 Changed 8 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:6 Changed 8 months ago by catalyst

WIP branch at https://github.com/tlyu/tor/tree/orconn-tracker which also contains a patch for #27402.

comment:7 Changed 7 months ago by catalyst

Owner: set to catalyst
Status: newassigned

comment:8 Changed 7 months ago by catalyst

The fix for this will probably fix #28884 as a side effect.

comment:9 Changed 7 months ago by catalyst

Status: assignedneeds_review

Pull request at https://github.com/torproject/tor/pull/613

Known issues:

  • the way I tried to detect PT vs firewall-bypass proxy didn't actually work, so PT connections get reported as proxy connections.
  • test coverage isn't great on some things because it would take longer to figure out how to mock all the right things

comment:10 Changed 7 months ago by nickm

Reviewer: nickm

comment:11 Changed 7 months ago by nickm

Status: needs_reviewmerge_ready

This is looking solid; I don't have any must-change items. I have left a couple of questions on the PR, to make sure I understand what's going on.

One more general question: Do you think that eventually all calls to control_event_bootstrap() should come from the btrack subsystem, or do you think that the ones that remain outside it are unproblematic?

Two requests:

  1. Could you please open a ticket to deal with the PT issue?
  1. When I merge this later today, could you please close the tickets that are fixed by it?

comment:12 in reply to:  11 Changed 7 months ago by catalyst

Replying to nickm:

This is looking solid; I don't have any must-change items. I have left a couple of questions on the PR, to make sure I understand what's going on.

Thanks! I've pushed a fixup commit to add the requested comment, and I'll start working on the control-spec.txt changes.

One more general question: Do you think that eventually all calls to control_event_bootstrap() should come from the btrack subsystem, or do you think that the ones that remain outside it are unproblematic?

I think eventually all calls to control_event_bootstrap() should come from the btrack subsystem. The directory progress reporting in particular could benefit a lot from that. Also that way we can add GETINFO handlers to the btrack subsystem to allow applications to query detailed bootstrap state information.

Two requests:

  1. Could you please open a ticket to deal with the PT issue?

Done: #28925

  1. When I merge this later today, could you please close the tickets that are fixed by it?

Will do.

comment:13 Changed 7 months ago by nickm

excellent; it sounds like we're on track here.

comment:14 Changed 7 months ago by nickm

squashed and merged! Please close as appropriate. :)

comment:15 in reply to:  14 Changed 7 months ago by catalyst

Resolution: implemented
Status: merge_readyclosed

Replying to nickm:

squashed and merged! Please close as appropriate. :)

Thanks! Also closing the related tickets as appropriate.

Note: See TracTickets for help on using tickets.