Have frac_nodes_with_descriptors() take and use for_direct_connect

On their review for #25691 (moved), 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

changed milestone to %Tor: 0.3.5.x-final

added bootstrap component::core tor/tor milestone::Tor: 0.3.5.x-final owner::neel priority::medium resolution::implemented reviewer::dgoulet severity::normal status::closed tor-bridge-client tor-guard type::defect version::tor 0.2.4.10-alpha labels

Trac:
Description: On their review for #25691 (moved), 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
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 descriptor.

to

On their review for #25691 (moved), 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
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

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?

Trac:
Status: new to assigned
Cc: teor to teor, neel@neelc.org
Owner: N/A to neel

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

Trac:
Description: On their review for #25691 (moved), 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
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

to

On their review for #25691 (moved), 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
Keywords: N/A deleted, tor-bridge-client, bootstrap, tor-guard added
Milestone: Tor: unspecified to Tor: 0.3.5.x-final

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

CI passes.

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to dgoulet

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!

Trac:
Status: needs_review to needs_revision

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.

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.

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.

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.

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.

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?

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

Thank you for the information.

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

Trac:
Status: needs_revision to needs_review

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

I believe that is correct.

Trac:
Status: needs_review to merge_ready

Looks good to me too; merging to master!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

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

Trac:
Version: N/A to Tor: 0.2.4.10-alpha

closed

mentioned in issue #27236 (moved)

moved to tpo/core/tor#25886 (closed)