Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#2411 closed enhancement (implemented)

expand getinfo circuit-status to show build_state flags

Reported by: arma Owned by: rransom
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #2114 Points:
Reviewer: Sponsor:

Description

We've got a usability bug where users are misinterpreting the circuit listing on Vidalia's network map. In #2114 I suggest that Vidalia should start giving the user more information.

But Tor really doesn't export enough information for Vidalia to know what to say. Right now we export a PURPOSE= argument, but the string we send isn't very useful. Also, we don't export some flags, like onehup_tunnel, which Vidalia needs to know to tell the user that the circuit in question is probably for fetching directory information.

We should come up with a spec change and implement it. Shouldn't be too hard.

Child Tickets

Change History (21)

comment:1 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:2 Changed 8 years ago by arma

Priority: normalmajor

To solve #1090 more fully, we'll want Vidalia to be able to tell the user what types of circuits she's looking at. We can't do that until we close this bug.

comment:3 Changed 8 years ago by rransom

Owner: set to rransom
Status: newaccepted
Type: defectenhancement

comment:4 Changed 8 years ago by nickm

We should also expose the onehop_tunnel flag, the is_internal flag, and maybe expand the array of states that we export. Anything else? We should be specific.

comment:5 Changed 8 years ago by nickm

(err, by "expand the array of states", I mean, "list more useful purposes if we are squashing purposes")

comment:6 Changed 8 years ago by rransom

Status: acceptedneeds_review

See bug2411a ( git://git.torproject.org/rransom/torspec.git bug2411a ) for documentation for the existing PURPOSE field of CIRC events, and some other minor fixups.

For both our hidden service performance-measurement tools and an improved circuit list in Vidalia, we will need to expose more information about circuit states. For the HS performance-measurement tools, we will also need to expose some new event types; I asked on the tor-dev mailing list whether we could add new event types to the CIRC event, but no one answered. I could assume that that silence means that no non-toy controllers would break, but it's more likely that the controller developers/maintainers aren't paying attention to the tor-dev list (or will claim to have not been paying attention if I make that change).

comment:7 in reply to:  6 Changed 8 years ago by rransom

Replying to rransom:

See bug2411a ( git://git.torproject.org/rransom/torspec.git bug2411a ) for documentation for the existing PURPOSE field of CIRC events, and some other minor fixups.

I just force-updated this branch to remove commit 5a4db0706ccd0211e6567368af1d5817c7792dec (Warn that CIRC, STREAM, ORCONN, BW, and STATUS_* ‘extended’ events aren't), because the specification of extended events in section 4.1 (a) is not actually a valid specification of anything, although it looked like one until I tried to figure out what it actually meant, and (b) isn't actually used anywhere (or rather, the extra-lines part isn't used; someone tried (and mostly failed) to specify extra arguments on single-line events there as well).

comment:8 Changed 8 years ago by rransom

See feature2411-v2 ( git://git.torproject.org/rransom/tor.git feature2411-v2 ) for a BUILDFLAGS field in CIRC events and GETINFO circuit-status. This branch is not a complete solution for this ticket; it still needs documentation in a torspec patch, and I need several other pieces of information in CIRC events. The ticket is in ‘needs_review’ status for the bug2411a torspec branch above.

comment:9 in reply to:  8 Changed 8 years ago by rransom

Replying to rransom:

See feature2411-v2 ( git://git.torproject.org/rransom/tor.git feature2411-v2 ) for a BUILDFLAGS field in CIRC events and GETINFO circuit-status. This branch is not a complete solution for this ticket; it still needs documentation in a torspec patch, and I need several other pieces of information in CIRC events. The ticket is in ‘needs_review’ status for the bug2411a torspec branch above.

I'm done with the code changes for this ticket now. I need to add new circuit-related control-port events to Tor, but they don't belong here.

See feature2411-v3 ( git://git.torproject.org/rransom/tor.git feature2411-v3 ) for a pre-squashed version of feature2411-v2; -v3 will be easier to review than -v2 if you haven't read previous commits on -v2.

comment:10 Changed 8 years ago by rransom

My bug2411a branch now contains documentation for the new fields provided by my feature2411-v2 and feature2411-v3 branches.

comment:11 Changed 8 years ago by nickm

Noting here so that it doesn't get lost:

The patch looks good. Before we merge it, though, there's a problem with controllers: at least pytorctl has the problem that if we insert new fields in CIRC events *before* the reason fields, it won't understand REASON fields any more.

Our options are:

  • Fix pytorctl, give everybody plenty time to upgrade, then merge this.
  • Change this patch so that the new CIRC fields come last, then merge it. In parallel, fix pytorctl so we don't have to do this nonsense in the future.

I favor the second; rransom (I believe) favors the first.

comment:12 in reply to:  11 Changed 8 years ago by rransom

Replying to nickm:

Noting here so that it doesn't get lost:

The patch looks good. Before we merge it, though, there's a problem with controllers: at least pytorctl has the problem that if we insert new fields in CIRC events *before* the reason fields, it won't understand REASON fields any more.

Our options are:

  • Fix pytorctl, give everybody plenty time to upgrade, then merge this.
  • Change this patch so that the new CIRC fields come last, then merge it. In parallel, fix pytorctl so we don't have to do this nonsense in the future.

I favor the second; rransom (I believe) favors the first.

Some of the reasons I don't want to change this patch are:

  • Rearranging the code to put event-specific flags for FAILED and CLOSED events in the middle of the keywords describing a circuit's current state would significantly uglify Tor's code.
  • The PURPOSE event extension was added after the REASON and REMOTE_REASON fields were, so the spec has previously been interpreted as allowing new event extensions to be placed anywhere after the non-extended event. PyTorCtl is thus quite broken.

comment:13 Changed 8 years ago by rransom

See bug2411a-v2 ( git://git.torproject.org/rransom/torspec.git bug2411a-v2 ) for a pre-squashed version of my bug2411a torspec branch.

comment:14 Changed 8 years ago by nickm

Replying to rransom:

  • Rearranging the code to put event-specific flags for FAILED and CLOSED events in the middle of the keywords describing a circuit's current state would significantly uglify Tor's code.

Not greatly IMO. I'd just have the function that generates a state description take an optional reason_flags argument, and make an XXX note that those events should get moved to the end later.

  • The PURPOSE event extension was added after the REASON and REMOTE_REASON fields were, so the spec has previously been interpreted as allowing new event extensions to be placed anywhere after the non-extended event. PyTorCtl is thus quite broken.

Like I said on IRC, I don't believe that the spec was "interpreted" on that point when I added PURPOSE back in 2008: I think I just ignored the possibility that stuff would break based on a tighter/looser reading of the spec, and nobody caught it in time. In fact, I think that stuff might have broken when I added it, which wasted people's time.

Regardless of whether we think that that pytorctl's breakage is conformant or nonconformant, the fact remains that this change will break pytorctl as-is, which means that we need one of the two options suggested above. I'd rather take the "fix in parallel" approach so we don't need to delay this feature, but if you're okay with holding off on merging it until pytorctl and its users won't break, I can live with that.

comment:15 Changed 8 years ago by atagar

Change to address this for pytorctl:
https://trac.torproject.org/projects/tor/ticket/3679

comment:16 Changed 8 years ago by mikeperry

FYI: The TorCtl changes seem to be breaking the bw auths mysteriously (#4015)

comment:17 Changed 8 years ago by nickm

Note: I'm planning to merge this some time in December; 6 months is long enough for tools to be expected to become compliant.

comment:18 Changed 8 years ago by rransom

See feature2411-v4 ( https://git.torproject.org/rransom/tor.git feature2411-v4 ) for a branch rebased onto current master.

comment:19 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merging as threatenedWpromised. Let's hope everything works, and fix whatever doesn't.

comment:20 Changed 7 years ago by nickm

Keywords: tor-client added

comment:21 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.