[tor-bugs] #26227 [Core Tor/Stem]: Review existing stem.client code

[Tor Bug Tracker & Wiki]

#26227: Review existing stem.client code
---------------------------+------------------------------
 Reporter:  dmr            |          Owner:  dmr
     Type:  task           |         Status:  needs_review
 Priority:  Medium         |      Milestone:
Component:  Core Tor/Stem  |        Version:
 Severity:  Normal         |     Resolution:
 Keywords:  client         |  Actual Points:
Parent ID:                 |         Points:
 Reviewer:  atagar         |        Sponsor:
---------------------------+------------------------------

Comment (by dmr):

 Replying to [comment:6 atagar]:
 > Thanks Dave. Finally cobbled together some time to begin reviewing this
 and looks great! I really love how you broke these up - that's making it
 far easier to review.
 Thanks! I'll keep trying to do granular commits. (And hopefully push them
 more often!)

 > I've pushed your first five commits with some tweaks, most notably that
 I added a LinkProtocol class that centralizes the constants which vary by
 version. Mind taking a peek to see what you think?
 Overall: Looks good!

 A few comments...

 ==== LinkProtocol `__eq__`, future direction
 w.r.t.
 [[https://gitweb.torproject.org/stem.git/commit/?id=76c17caabc13b21dde7a5930fe30a59ffea9b2fe|76c17caabc13b21dde7a5930fe30a59ffea9b2fe]],
 would it make sense to have the LinkProtocol `__eq__` method allow an
 integer argument to be checked against the LinkProtocol.version attribute?
 (Or e.g. would that be considered non-Pythonic?)

 Also regarding the `LinkProtocol` class - do you envision we might turn it
 into something beyond a `NamedTuple` in the future, for version-specific
 behavior? Or do you expect we'll keep it limited to constants? (this
 touches a bit on #26226)
 ==== circ_id allocation
 I also saw a change to the circ_id allocation algorithm. The spec says
 this is arbitrary ("[[https://gitweb.torproject.org/torspec.git/tree/tor-
 spec.txt?id=4df184021b7c84cc47e2ed19a601b1e790b5b4fb#n914|The CircID for a
 CREATE cell is an arbitrarily chosen nonzero integer,]]"), but I think it
 might be useful to factor this out. It is trivially network-observable
 behavior to the guard, and thus might be used to distinguish stem.client
 connections and treat them differently.

 Maybe the spec //ought to// specify it, at least a SHOULD behavior? I
 suppose it depends on the threat model the spec itself is trying to cover.
 Please let me know your thoughts!
 ==== Cell.unused -> Cell.padding?
 I really liked
 [[https://gitweb.torproject.org/stem.git/commit/?id=84e4e657b4785e4888e567fbc04c8ea29fd43cc4|84e4e657b4785e4888e567fbc04c8ea29fd43cc4]]
 - I was contemplating how we might do that, and having an `unused`
 attribute makes a lot of sense!
 Do you think it might make more sense to call it `padding`, though? There
 was some discussion about terminology in #26228, and it's not fully
 resolved therein, but a few opinions did come up that padding bytes are
 not a payload, even in the `[V]PADDING` cells.
 ==== Testing literals
 For the side point of testing literals you brought up in
 [[https://gitweb.torproject.org/stem.git/commit/?id=7711050619af1a2f8ecf4aa87f774baa5f367b3b|7711050619af1a2f8ecf4aa87f774baa5f367b3b]],
 I filed a separate ticket - see #26420. (I was already planning to file
 this.)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26227#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online