[tor-bugs] #22453 [Core Tor/Tor]: Relays should regularly do a larger bandwidth self-test

[Tor Bug Tracker & Wiki]

#22453: Relays should regularly do a larger bandwidth self-test
-------------------------------------------------+-------------------------
 Reporter:  arma                                 |          Owner:  juga
     Type:  enhancement                          |         Status:
                                                 |  assigned
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  unspecified
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  034-triage-20180328,                 |  Actual Points:
  034-removed-20180328, tor-bwauth               |
Parent ID:  #25925                               |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by teor):

 Replying to [comment:21 juga]:
 > If i understand correctly the code [0]:
 >
 > {{{
 >   int num_cells = (int)(get_options()->BandwidthRate * 10 /
 >                         CELL_MAX_NETWORK_SIZE);
 >   int max_cells = num_cells < CIRCWINDOW_START ?
 >                     num_cells : CIRCWINDOW_START;
 >   int cells_per_circuit = max_cells / num_circs;
 > }}}
 >
 > the 500KB were due to `CIRCWINDOW_START` (1000), the 1st line was not
 having effect.
 >
 > Do we want that the first test to send only 500KB (and then
 AuthDirGuardBWGuarantee*10 in the consecutives ones)?. `max_cells` then it
 just needs a condition.

 Doing something different the first time makes the code more complicated.
 We want to get a good observed bandwidth from new relays and slow relays.
 How does running a small first test help?

 > I'm not sure why there was `BandwidthRate` there, that's 1GB by default
 (bigger than AuthDirGuardBWGuarantee)

 If a relay has a small bandwidth rate, this code limits the self-test
 bandwidth to that rate.

 > and i think the 1st line was also missing to divide by the number of
 circuits (not only the cell size).

 The division by the number of circuits is done later:
 {{{
   int cells_per_circuit = max_cells / num_circs;
 }}}

 > The 10 in the first line is there because of the 10 seconds?

 Yes.

 > [0]
 https://gitweb.torproject.org/tor.git/tree/src/feature/relay/selftest.c?h=release-0.3.5#n278

 Replying to [comment:22 juga]:
 > > > Where in the code should go the `times out after 30-60`?
 > >
 > > When you start the test, schedule a timer for 30-60 seconds that
 cancels the test of it is still going.
 >
 > The cells are queued by [0]:
 > {{{
 >       if (relay_send_command_from_edge(0, TO_CIRCUIT(circ),
 >                                        RELAY_COMMAND_DROP,
 >                                        NULL, 0, circ->cpath->prev)<0) {
 >         return; /* stop if error */
 > }}}

 Yes.

 > And there is an event that expire old circuits every 11 seconds [1]
 >
 > {{{
 >   /* every 11 seconds, so not usually the same second as other such
 events */
 >   circuit_expire_old_circuits_serverside(now);
 >   return 11;
 > }}}
 >
 > So, i guess that would also clean the circuits created by the bandwidth
 test?

 No, this event is only for non-origin circuits:
 https://github.com/torproject/tor/blob/b058f64cc002b44e6dd48616ca3163a01c3f3e14/src/core/or/circuituse.c#L1551

 The self-test circuits are origin circuits, so you want
 circuit_expire_old_circs_as_needed(), which calls
 circuit_expire_old_circuits_clientside():
 https://github.com/torproject/tor/blob/b058f64cc002b44e6dd48616ca3163a01c3f3e14/src/core/or/circuituse.c#L1345
 https://github.com/torproject/tor/blob/b058f64cc002b44e6dd48616ca3163a01c3f3e14/src/core/or/circuituse.c#L1459

 circuit_expire_old_circuits_clientside() expires bandwidth testing
 circuits 10 minutes after they start testing. That is ok. We don't need to
 change anything.

 > [0]
 https://gitweb.torproject.org/tor.git/tree/src/feature/relay/selftest.c?h=release-0.3.5#n294
 > [1]
 https://gitweb.torproject.org/tor.git/tree/src/core/mainloop/mainloop.c?h=release-0.3.5#n2340

 > Where in the code should go ... the 12-24 hours, at random?

 The regular bandwidth test code already exists, but we need to change the
 "bandwidth is low" check from 51200 to AuthDirGuardBWGuarantee:

 {{{
       if (!first_time && me &&
           me->bandwidthcapacity < me->bandwidthrate &&
           me->bandwidthcapacity < 51200) {
         reset_bandwidth_test();
       }
 }}}

 https://gitweb.torproject.org/tor.git/tree/src/core/mainloop/mainloop.c?h=release-0.3.5#n2283

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