Re: [tor-bugs] #19043 [Core Tor/Tor]: prop224: Implementation of ESTABLISH_INTRO cell

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#19043: prop224: Implementation of ESTABLISH_INTRO cell
-------------------------------------------------+-------------------------
 Reporter:  alec.heif                            |          Owner:  asn
     Type:  enhancement                          |         Status:
                                                 |  merge_ready
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.0.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-hs, prop224, 6.s194,             |  Actual Points:
  TorCoreTeam201611, review-group-12             |
Parent ID:  #17241                               |         Points:  6
 Reviewer:  asn, dgoulet                         |        Sponsor:
                                                 |  SponsorR-must
-------------------------------------------------+-------------------------

Comment (by chelseakomlo):

 All of the new modules look great!

 A couple minor points, feel free to take/leave any:

 - There are some magic numbers in encode_establish_intro_cell_legacy- some
 of these could be extracted into defines
 - the_hs_circuitmap might be renamed for specificity- the_ is a bit
 unclear.
 - In the comment for hs_circuits_have_same_token we use introduction and
 rendezvous tokens, but in the function we use first_token and
 second_token. Maybe introduction/rendezvous naming can be consistently
 used.
 - In commit f4db1ab5fcf3e48c6b4011e9f1cbae6db1aa8c5b it says that
 hs_received_establish_intro is the new entry point, but I think this is
 meant to be hs_intro_received_establish_intro
 - In general, it can be useful to write multiple unit tests per function
 under test if there are branches in the code. For example, it looks like
 we just test one path for generate_establish_intro_cell, but it might be
 worth writing additional unit test to cover error cases.
 - encode_establish_intro_cell_legacy might be able to be unit tested
 directly. It looks like it is tested indirectly but we could have several
 unit tests to test the different branches of this directly.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19043#comment:36>
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