Opened 5 years ago

Closed 5 years ago

#13126 closed defect (fixed)

Deprecate smartlist_choose_node_by_bandwidth() ?

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: nickm-patch
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

smartlist_choose_node_by_bandwidth() is a function that chooses a random node from a smartlist, weighted by the advertised bandwidth of each element.

It's the older sister of smartlist_choose_node_by_bandwidth_weights() (see node_sl_choose_by_bandwidth()) which uses advertised bandwidth instead of measured. Its algorithm is old and buggy (#13066) and apparently documented at http://archives.seul.org/or/dev/Jul-2007/msg00056.html.

Now that Tor has measured bandwidth, we should make sure that the function is unused and then maybe consider deprecating it. However, we should make it so that we are still able to gracefully handle a situation where we don't have measured bandwidths or weights.

Child Tickets

Attachments (1)

0001-Set-default-value-for-bw-weights-if-absent-from-cons.patch (1.3 KB) - added by asn 5 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 5 years ago by nickm

Suggested approach:

  • All missing weights default to 1.
  • All missing bandwidths default to 1.


This is possibly not a good idea! Let's think of a better set of defaults, then kill off smartlist_choose_node_by_bandwidth.

comment:2 Changed 5 years ago by nickm

Status: newneeds_review

Branch bug13126 in my public repository removes the code in question and makes the preferred function fail under fewer circumstances.

comment:3 Changed 5 years ago by nickm

Keywords: nickm-patch added

Add the nickm-patch keyword to some needs_review tickets where I wrote or substantially revised the patch. This helps me find which tickets I should review and which I should find reviewers for.

comment:4 Changed 5 years ago by asn

Looks good to me. Have you tested it? If not, tell me and I can try to setup a chutney network to test this next week.

BTW, do you think we should document this behavior and the default val of 30000 to the spec? Or is it an implementation decision?

comment:5 Changed 5 years ago by nickm_mobile

Have tested only a little; more testing would be welcome. Also, documentation would rock here, yeah.

comment:6 Changed 5 years ago by asn

Tested in a chutney network and it seems to work. With no weights in the consensus, clients take the intended code path and seem to be able to do path selection properly.

I'll also attach a torspec patch for this.

comment:7 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks for testing and writing; I've merged these both.

Note: See TracTickets for help on using tickets.