[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 juga):

 Replying to [comment:23 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?

 Then there should not be the CIRCWINDOW_START condition because it limits
 the number of the cells to 1000.

 >
 > > 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.

 Then the number of cells should be calculated using min(BandwidthRate,
 AuthDirGuardBWGuarantee), not only BandwidthRate?. Otherwise we will be
 sending 10GB if BandwidthRate has not been modified.
 >
 > > 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:24>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online