[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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

#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:8 atagar]:
 > LinkProtocol `__eq__`, future direction
 >
 > Sure. All attributes of the LinkProtocol are derived from its integer
 version, so it would be perfectly fine to do equality and hash on that.

 Sounds good. Filed as #26432.

 > > I also saw a change to the circ_id allocation algorithm.
 >
 > If you the LinkProtocol addition then nope, I didn't change it. Just
 moved it. The part that concerns us is a
 [https://gitweb.torproject.org/torspec.git/tree/tor-
 spec.txt?id=4df184021b7c84cc47e2ed19a601b1e790b5b4fb#n930 few paragraphs
 later]...
 > [snip]

 Ah, sorry, I meant that the allocation algorithm switched in how it would
 assign circ_ids.
 Previous code was:
 {{{
 # __init__.py
       circ_id = 0x80000000 if self.link_protocol > 3 else 0x01

       while circ_id in self._circuits:
         circ_id += 1
 }}}
 and now is:
 {{{
 # __init__.py
       circ_id = max(self._circuits) + 1 if self._circuits else
 self.link_protocol.first_circ_id

 # datatype.py - LinkProtocol.first_circ_id
       first_circ_id = 0x80000000 if version > 3 else 0x01
 }}}
 There's a subtle difference here - the former would reuse circuit ids
 after a circuit closed, grabbing from the lowest available.
 The latter still reuses circuit ids, but only if //all circuits are
 closed// - then it resets because `self._circuits` is empty.

 Really, the spec says this is arbitrary, especially the `MAY` as you
 pointed out for `link_protocol <= 3`, but it does represent network-
 observable behavior that could be used to distinguish different client
 implementations, fairly trivially.
 (Hence: maybe the spec ought to specify a `SHOULD` here?)

 That's starting to get into threat modeling, for `stem.client` and for the
 spec at large. I think this should be covered within a broader ticket.
 Filed one for `stem.client`: #26431. We can decide after resolving that
 whether the spec needs updates.

 > > I really liked 84e4e657b4785e4888e567fbc04c8ea29fd43cc4 - I was
 contemplating how we might do that, and having an unused attribute makes a
 lot of sense!
 >
 > Neat, glad ya like it!
 >
 > > Do you think it might make more sense to call it padding, though?
 >
 > Actually, initially I did call it that until I realized the name
 conflicted with PADDING and VPADDING cells. I kinda like the name 'unused'
 since it makes it clear that it's bytes that have no impact.

 While I agree that `unused` is very clear that those bytes are incidental
 to the cell, I guess what I was trying to say by referencing #26228 is
 that ... whether PADDING and VPADDING carry a `payload` or whether what
 they carry should be `padding` ... was discussed a bit therein; see my
 opening post, [[ticket:26228#comment:1|teor's comment]], and
 [[ticket:26228#comment:3|arma's comment]].

 This is a bit bikeshed-y and really doesn't affect the implementation
 much, but I just wanted to make sure I represented those voices. I'm fine
 with moving forward with `payload` and `unused`, and re-assessing this
 later if needed.

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs