Opened 16 months ago

Closed 14 months ago

Last modified 11 months ago

#25886 closed defect (implemented)

Have frac_nodes_with_descriptors() take and use for_direct_connect

Reported by: nickm Owned by: neel
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.10-alpha
Severity: Normal Keywords: tor-bridge-client, tor-guard, bootstrap
Cc: teor, neel@… Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description (last modified by teor)

On their review for #25691, teor notes (about for_direct_connect):

We should pass for_direct_conn into this function, and use node_has_preferred_descriptor().

For the mid and exit case:
We won't bootstrap unless we have enough actual mid and exit bandwidth, even if we have mids or exits listed as our bridges.

For the guard case:
The guard case is unchanged for non-bridge clients.

The bridge client case could be tricky, because:

  1. compute_frac_paths_available() only checks guard-flagged nodes, not bridges
  2. even if it did check bridges, they don't have bandwidths only have self-measured bandwidths
  3. even if we used a weight of 1 for each bridge, we don't require 65% of bridges to be up to bootstrap

To workaround this issue, I suggest we make f_guard = 1.0 in compute_frac_paths_available() if we are using bridges, and have at least one bridge with the preferred a full descriptor.

Edit: bridge clients always use full descriptors for bridges
Edit: bridges have self-measured bandwidths

Child Tickets

Change History (19)

comment:1 Changed 16 months ago by teor

Description: modified (diff)

comment:2 Changed 15 months ago by neel

Cc: neel@… added
Owner: set to neel
Status: newassigned

I am interested in this ticket. However, I have a question:

When I add for_direct_conn in frac_nodes_with_descriptors(), should I replace instances of node_has_any_descriptor() with node_has_preferred_descriptor() and pass in for_direct_conn (and modifying instances for frac_nodes_with_descriptors() to add for_direct_conn)? Is there more that I need to do?

comment:3 in reply to:  description Changed 15 months ago by teor

Description: modified (diff)
Keywords: tor-bridge-client tor-guard bootstrap added
Milestone: Tor: unspecifiedTor: 0.3.5.x-final

You also need to rewrite the guard weights when the client is using bridges:

Replying to nickm:

For the guard case:
The guard case is unchanged for non-bridge clients.

The bridge client case could be tricky, because:

  1. compute_frac_paths_available() only checks guard-flagged nodes, not bridges
  2. even if it did check bridges, they don't have bandwidths only have self-measured bandwidths
  3. even if we used a weight of 1 for each bridge, we don't require 65% of bridges to be up to bootstrap

To workaround this issue, I suggest we make f_guard = 1.0 in compute_frac_paths_available() if we are using bridges, and have at least one bridge with the preferred a full descriptor.

Edit: bridge clients always use full descriptors for bridges
Edit: bridges have self-measured bandwidths

comment:4 Changed 15 months ago by neel

My GitHub branch is here: https://github.com/neelchauhan/tor/tree/b25886

CI passes.

comment:5 Changed 15 months ago by teor

Status: assignedneeds_review

comment:6 Changed 15 months ago by asn

Reviewer: dgoulet

comment:7 Changed 15 months ago by dgoulet

Status: needs_reviewneeds_revision

Left some comments in the branch. Neel, next revision, you might want to do a PR instead so it get checked automatically by our CI :). Thanks!

comment:8 Changed 15 months ago by neel

I have a pull request here: https://github.com/torproject/tor/pull/139

I did not include the lines for automatically returning 1.0 on a bridge if we have a preferred descriptor in frac_nodes_with_descriptors() but you (dgoulet) questioned it hence the reason why it's not here. If you want it I can add another commit and/or create a new branch.

comment:9 Changed 15 months ago by dgoulet

So the code makes sense here but I want to come back f_guard workardoun originally proposed by teor.

This branch is missing it and I'm not even sure where it should go... Seems it could be in compute_frac_paths_available where we make that function "bridge aware" somehow. The hack in frac_nodes_with_descriptors() doesn't seem very appealing to me.

I'm not sure how to proceed next neel. I'll point this out to teor.

comment:10 Changed 15 months ago by neel

While I am not committing to a solution for f_guard yet, I am thinking about that in compute_frac_paths_available(), if f_guard is greater than 0 and options->UseBridges is set, we set f_guard to 1.0.

comment:11 in reply to:  10 ; Changed 15 months ago by neel

I have made the changes in a new GitHub PR: https://github.com/torproject/tor/pull/145

This sets f_guard to 1.0 on bridges by checking if UseBridges is set and if the original f_guard is greater than 0 in compute_frac_paths_available(). I do understand that this may not be a perfect solution, and may also be rejected, but it is an alternative (that I thought of) to putting a ugly hack in frac_nodes_with_descriptors() that dgoulet had mentioned.

comment:12 in reply to:  11 Changed 14 months ago by teor

Replying to neel:

I have made the changes in a new GitHub PR: https://github.com/torproject/tor/pull/145

This sets f_guard to 1.0 on bridges by checking if UseBridges is set and if the original f_guard is greater than 0 in compute_frac_paths_available(). I do understand that this may not be a perfect solution, and may also be rejected, but it is an alternative (that I thought of) to putting a ugly hack in frac_nodes_with_descriptors() that dgoulet had mentioned.

We can't check if f_guard is non-zero, because most bridges don't have the guard flag.

I suggest we make f_guard = 1.0 in compute_frac_paths_available() if we are using bridges, and have at least one bridge with the preferred a full descriptor.

Instead, we need to check the number of bridges with full descriptors. There is an existing function in bridges.h that does this check.

comment:13 Changed 14 months ago by neel

Thank you for your response.

I don't fully know what a "full descriptor" is. Does this mean that for bridges:

  • We have node->ri only? (based on node_has_preferred_descriptor())
  • We have node->ri, node->rs, and node->md? (I am guessing this one)

Also, I did not see any function in bridges.h which directly checks for full descriptors (or didn't see them if they were unobvious).

The closest thing I saw were functions which get a list of bridges (bridge_list_get()) so then I get a SMARTLIST, and in each iteration get a node_t from the bridge_info_t and check for a full descriptor. Should I do this? If not, which function should I use?

comment:14 in reply to:  13 Changed 14 months ago by teor

Replying to neel:

Thank you for your response.

I don't fully know what a "full descriptor" is.

A router descriptor, rather than a microdescriptor:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n151

Does this mean that for bridges:

  • We have node->ri only? (based on node_has_preferred_descriptor())

ri (router info) is used to store router descriptors.

  • We have node->ri, node->rs, and node->md? (I am guessing this one)

Bridges don't have rs (router status) or md (microdescriptor), because bridges are not in the consensus.

Also, I did not see any function in bridges.h which directly checks for full descriptors (or didn't see them if they were unobvious).

The closest thing I saw were functions which get a list of bridges (bridge_list_get()) so then I get a SMARTLIST, and in each iteration get a node_t from the bridge_info_t and check for a full descriptor. Should I do this? If not, which function should I use?

num_bridges_usable(0) > 0, which is actually in entrynodes.h :
https://github.com/torproject/tor/blob/5edc72a45b7479f5fe791054aa19f6b3b478c725/src/or/entrynodes.c#L3149

comment:15 Changed 14 months ago by neel

Thank you for the information.

My new PR is here: https://github.com/torproject/tor/pull/153

comment:16 Changed 14 months ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 14 months ago by dgoulet

Status: needs_reviewmerge_ready

Ah wow much simpler and this actually makes more sense! Thanks neel!

I believe that is correct.

comment:18 Changed 14 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Looks good to me too; merging to master!

comment:19 Changed 11 months ago by teor

Version: Tor: 0.2.4.10-alpha

This is actually a fix on commit fcf906ec733 in 0.2.4.10-alpha.

Note: See TracTickets for help on using tickets.