25691/25692: Distinguish "has preferred descriptor" from "has any descriptor" #48

nmathewson · 2018-04-16T15:56:27Z

No description provided.

This patch adds a new node_has_preferred_descriptor() function, and replaces most users of node_has_descriptor() with it. That's an important change, since as of d1874b4 (our fix for #25213), we are willing to say that a node has _some_ descriptor, but not the _right_ descriptor for a particular use case. Part of a fix for 25691 and 25692.

In router_add_running_nodes_to_smartlist(), we had an inline implementation of the logic from node_has_descriptor(), which should be changed to node_has_preferred_descriptor().

In order to fix 25691 and 25692, we need to pass the "direct_conn" flag to more places -- particularly when choosing single-hop tunnels. The right way to do this involves having a couple more functions accept router_crn_flags_t, rather than a big list of boolean arguments. This commit also makes sure that choose_good_exit_server_general() honors the direct_conn flag, to fix 25691 and 25692.

Changing the name of this function should help keep us from misusing it when node_has_preferred_descriptor() would be more appropriate.

It tried to pick nodes for which only routerinfo_t items are set, but without setting UseMicroDescriptors to 0. This won't work any more, now that we're strict about using the right descriptor types due to 25691/25692/25213.

teor2345

There are a few places where I think we should be using node_has_preferred_descriptor(). The large changes should probably be split into other tickets.

Otherwise, it looks good, and it works. Most of my comments ask for comment changes.

teor2345 · 2018-04-18T05:46:04Z

src/or/circuitbuild.c

-    /* Otherwise we need an md. */
-    if (node->rs == NULL || node->md == NULL)
-      return NULL;
+  if (!node_has_preferred_descriptor(node, for_direct_connect)) {


An earlier fix for this bug changed how for_direct_connect works.
Do we need to update this part of the function comment?

May return NULL if there is not enough * info about <b>node</b> to extend to it--for example, if there is no * routerinfo_t or microdesc_t, or if for_direct_connect is true and none of * the node's addresses are allowed by tor's firewall and IP version config.

I think we do. Done with 1d06778.

teor2345 · 2018-04-18T05:46:04Z

src/or/entrynodes.c

@@ -192,7 +192,7 @@ guard_has_descriptor(const entry_guard_t *guard)
  const node_t *node = node_get_by_id(guard->identity);
  if (!node)
    return 0;
-  return node_has_descriptor(node);
+  return node_has_preferred_descriptor(node, 1);


Function comment: "know a preferred descriptor"

cea472f fixes this.

teor2345 · 2018-04-18T05:46:04Z

src/or/hs_common.c

@@ -1281,7 +1281,7 @@ node_has_hsdir_index(const node_t *node)

  /* A node can't have an HSDir index without a descriptor since we need desc
   * to get its ed25519 key */
-  if (!node_has_descriptor(node)) {
+  if (!node_has_preferred_descriptor(node, 0)) {


Note: Using for_direct_connect = 0 is always correct for the HSDir hash ring, because we use non-bridge descriptors for the HSDir hash ring. This is true even though Tor2web connects directly to HSDirs.

Hmm, does this need a comment?

5398bf0 has that comment.

teor2345 · 2018-04-18T05:46:04Z

src/or/hs_common.c

@@ -1617,7 +1617,7 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
    time_t last = hs_lookup_last_hid_serv_request(dir, req_key_str, 0, 0);
    const node_t *node = node_get_by_id(dir->identity_digest);
    if (last + hs_hsdir_requery_period(options) >= now ||
-        !node || !node_has_descriptor(node)) {
+        !node || !node_has_preferred_descriptor(node, 0)) {


Does this need a comment about Tor2web, similar to the last comment?

03494da is the fixup for this.

teor2345 · 2018-04-18T05:46:04Z

src/or/nodelist.c

@@ -1139,6 +1140,27 @@ node_has_descriptor(const node_t *node)
          (node->rs && node->md));
 }

+/** DOCDOC */


Please document the function

80ac049 does that.

teor2345 · 2018-04-18T05:46:04Z

src/or/control.c

@@ -3498,7 +3498,7 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len,
      connection_printf_to_buf(conn, "552 No such router \"%s\"\r\n", n);
      goto done;
    }
-    if (!node_has_descriptor(node)) {
+    if (!node_has_any_descriptor(node)) {


The log message at line 3525 is now incorrect:

"controller tried to connect to a node that doesn't have any " "addresses that are allowed by the firewall configuration; " "circuit marked for closing."

We can fail because we are missing the preferred descriptor for the node, too.
Should we do int first_node = zero_circ; first_node = 0; in this loop, too?

e480c11 does it here.

teor2345 · 2018-04-18T05:46:04Z

src/or/nodelist.c

@@ -1134,7 +1134,7 @@ node_is_dir(const node_t *node)
 /** Return true iff <b>node</b> has either kind of usable descriptor -- that
 * is, a routerdescriptor or a microdescriptor. */


Add to the function comment: You should probably use node_has_preferred_descriptor() instead of this function.

f5a472c does that.

teor2345 · 2018-04-18T05:46:04Z

src/or/nodelist.c

@@ -2243,7 +2243,8 @@ compute_frac_paths_available(const networkstatus_t *consensus,
              nu);

    SMARTLIST_FOREACH_BEGIN(myexits_unflagged, const node_t *, node) {
-      if (node_has_descriptor(node) && node_exit_policy_rejects_all(node)) {
+      if (node_has_any_descriptor(node) &&


count_usable_descriptors() counts mds if we have an md consensus, and ris if we have an ns consensus.
So I think we want node_has_preferred_descriptor(node, 0) here.

looks good; done in 632899c.

teor2345 · 2018-04-18T05:46:04Z

src/or/routerlist.c

@@ -2758,15 +2758,15 @@ frac_nodes_with_descriptors(const smartlist_t *sl,
      total <= 0.0) {
    int n_with_descs = 0;
    SMARTLIST_FOREACH(sl, const node_t *, node, {
-      if (node_has_descriptor(node))
+      if (node_has_any_descriptor(node))


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:

compute_frac_paths_available() only checks guard-flagged nodes, not bridges

even if it did check bridges, they don't have bandwidths

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.

This change might cause bootstrapping issues. Let's defer it to ~~0.3.4~~ Tor: unspecified in another ticket?

Opened trac:25886 for this.

teor2345 · 2018-04-18T05:46:04Z

src/test/test_hs_common.c

@@ -305,7 +305,7 @@ helper_add_hsdir_to_networkstatus(networkstatus_t *ns,
  node_t *node = node_get_mutable_by_id(ri->cache_info.identity_digest);
  tt_assert(node);
  node->rs = rs;
-  /* We need this to exist for node_has_descriptor() to return true. */
+  /* We need this to exist for node_has_any_descriptor() to return true. */


hsdirs use node_has_preferred_descriptor(), so I think this comment is wrong

3455d87 is the fix here.

Change a comment, per Teor's code review.

Fix comment for guard_has_descriptor(), per code review from teor.

Add a comment explaining why for_direct_connect should be zero when building hsdir hash ring.

Another comment about hsdir index and for_direct_connnect.

Document node_has_preferred_descriptor().

Document that you shouldn't use node_has_any_descriptor() for most things.

Fix a search-and-replace mistake.

Suggested by teor during code review for 25691.

Should use has_preferred_descriptor() in count_frac_paths_available (Especially since this usage is about exit nodes)

teor2345 · 2018-04-22T21:11:32Z

5398bf0 has that comment.

Two typos: consensus-indexed cnodes are also briges

teor2345 · 2018-04-22T21:15:10Z

I've added a comment to note this defect as df7eaf8.

Two typos: thar are note the type we would prefer to use

Typo fix.

Other typo fix.

nmathewson · 2018-05-07T19:02:10Z

This was squashed and merged with 6773102

nmathewson added 6 commits Apr 16, 2018

Use node_has_preferred_descriptor() in another case

2d8234e

In router_add_running_nodes_to_smartlist(), we had an inline implementation of the logic from node_has_descriptor(), which should be changed to node_has_preferred_descriptor().

Add a changes file for 25691/25692

030c2a6

Rename node_has_descriptor() to node_has_any_descriptor()

fb700a7

Changing the name of this function should help keep us from misusing it when node_has_preferred_descriptor() would be more appropriate.

Repair the legacy_hs/pick_tor2web_rendezvous_node unit test

fa86000

It tried to pick nodes for which only routerinfo_t items are set, but without setting UseMicroDescriptors to 0. This won't work any more, now that we're strict about using the right descriptor types due to 25691/25692/25213.

teor2345 suggested changes Apr 18, 2018

View changes

nmathewson added 10 commits Apr 22, 2018

fixup! Check for "the right descriptor", not just "any descriptor".

1d06778

Change a comment, per Teor's code review.

fixup! Check for "the right descriptor", not just "any descriptor".

cea472f

Fix comment for guard_has_descriptor(), per code review from teor.

fixup! Check for "the right descriptor", not just "any descriptor".

5398bf0

Add a comment explaining why for_direct_connect should be zero when building hsdir hash ring.

fixup! Check for "the right descriptor", not just "any descriptor".

03494da

Another comment about hsdir index and for_direct_connnect.

fixup! Check for "the right descriptor", not just "any descriptor".

80ac049

Document node_has_preferred_descriptor().

fixup! Rename node_has_descriptor() to node_has_any_descriptor()

f5a472c

Document that you shouldn't use node_has_any_descriptor() for most things.

fixup! Rename node_has_descriptor() to node_has_any_descriptor()

3455d87

Fix a search-and-replace mistake.

Note a defect in count_acceptable_nodes()

df7eaf8

control EXTENDCIRCUIT: check node_has_preferred_descriptor().

e480c11

Suggested by teor during code review for 25691.

fixup! Check for "the right descriptor", not just "any descriptor".

632899c

Should use has_preferred_descriptor() in count_frac_paths_available (Especially since this usage is about exit nodes)

nmathewson added 2 commits Apr 22, 2018

fixup! Check for "the right descriptor", not just "any descriptor".

40e406e

Typo fix.

fixup! Check for "the right descriptor", not just "any descriptor".

6c99a14

Other typo fix.

nmathewson closed this May 7, 2018

neelchauhan referenced this issue in neelchauhan/tor Jun 8, 2018

Make frac_nodes_with_descriptors() take and use for_direct_connect

73c01d7

torproject / tor Public

25691/25692: Distinguish "has preferred descriptor" from "has any descriptor" #48

25691/25692: Distinguish "has preferred descriptor" from "has any descriptor" #48

nmathewson commented Apr 16, 2018

teor2345 left a comment

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018 •

edited

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018 •

edited

teor2345 Apr 18, 2018 •

edited

nmathewson Apr 22, 2018

teor2345 Apr 18, 2018

nmathewson Apr 22, 2018

teor2345 commented Apr 22, 2018

teor2345 commented Apr 22, 2018

nmathewson commented May 7, 2018

		@@ -1134,7 +1134,7 @@ node_is_dir(const node_t *node)
		/** Return true iff <b>node</b> has either kind of usable descriptor -- that
		* is, a routerdescriptor or a microdescriptor. */

torproject / tor Public

25691/25692: Distinguish "has preferred descriptor" from "has any descriptor" #48

25691/25692: Distinguish "has preferred descriptor" from "has any descriptor" #48

Conversation

nmathewson commented Apr 16, 2018

teor2345 left a comment

teor2345 Apr 18, 2018 • edited

teor2345 Apr 18, 2018 • edited

teor2345 Apr 18, 2018 • edited

teor2345 commented Apr 22, 2018

teor2345 commented Apr 22, 2018

nmathewson commented May 7, 2018

teor2345 Apr 18, 2018 •

edited

teor2345 Apr 18, 2018 •

edited

teor2345 Apr 18, 2018 •

edited