[Tor Trac #25885]: count_acceptable_nodes() would be more accurate using node_has_preferred_descriptor() #271
Conversation
Pull Request Test Coverage Report for Build 2802
|
src/core/or/circuitbuild.c
Outdated
| * 3, return the route length. Else, return 0 to signify that we do not | ||
| * have a complete circuit. | ||
| * | ||
| * Keep in mind that this function can succeed if direct is greater than |
This seems to be an unwanted property....
src/core/or/circuitbuild.c
Outdated
| * Keep in mind that this function can succeed if direct is greater than | ||
| * 1 but indirect isn't because guard nodes can also be middle or exit | ||
| * nodes if configured so on the relay side. */ | ||
| return (direct >= 1) && (direct + indirect >= 3) ? direct + indirect : 0; |
I think this calculation isn't what we actually want.
After all, we only use this function in one place: to tell if we have enough routers to build a path. Maybe the right solution here is to have this function return the direct and indirect counts separately, and let new_route_len() decide whether it likes it?
src/core/or/circuitbuild.c
Outdated
| @@ -2418,17 +2414,29 @@ count_acceptable_nodes, (smartlist_t *nodes)) | |||
| if (! node->is_valid) | |||
| // log_debug(LD_CIRC,"Nope, the directory says %d is not valid.",i); | |||
| continue; | |||
| if (! node_has_any_descriptor(node)) | |||
| /* If we can use the node, check if we can connect directly. */ | |||
| if (node_has_preferred_descriptor(node, 1)) | |||
Here you treat "indirect" and "direct" as having a logical relation to one another, as if it's okay to count every "direct" node as also being "indirect" later on (when you do direct+indirect). That isn't exactly the case, though: for the nodes in the "direct" count, we might not be willing to use them indirectly if we don't have the right descriptor for them.
Probably direct and indirect should be computed separately; maybe "direct" should be an argument to this function?
It's also worth noting that bridges are used as entry nodes (direct) but not middle or end nodes (indirect).
The one exception is when a public relay is set as a bridge.
So would it be okay to make count_acceptable_nodes() take in a direct, and make num_acceptable_routers in new_route_len() a sum of count_acceptable_nodes() with direct as 1 plus count_acceptable_nodes() with direct as 0?
To make a path of length N, tor needs at least 1 direct entry node (guard, or bridge, or any node if not using entry guards) and at least N-1 indirect non-entry nodes.
We need to replace the simple test in new_route_len() with separate tests for direct and indirect, and update the log message as well.
src/core/or/circuitbuild.c
Outdated
|
|
||
| if (num_acceptable_routers < routelen) { | ||
| if (num_acceptable_direct < 1 && num_acceptable_indirect < routelen - 1) { |
Should this be || instead of &&?
I agree, it should be ||.
We need to fail if there are no entry nodes, or if there are not enough non-entry nodes to fill the route.
Looks good, just needs a few minor fixes.
src/core/or/circuitbuild.c
Outdated
|
|
||
| if (num_acceptable_routers < routelen) { | ||
| if (num_acceptable_direct < 1 && num_acceptable_indirect < routelen - 1) { |
I agree, it should be ||.
We need to fail if there are no entry nodes, or if there are not enough non-entry nodes to fill the route.
src/core/or/circuitbuild.c
Outdated
| log_info(LD_CIRC, | ||
| "Not enough acceptable routers (%d/%d). Discarding this circuit.", | ||
| num_acceptable_routers, routelen); | ||
| "Not enough acceptable routers (%d direct and %d indirect " |
Can you make this log entry say %d/%d direct and %d/%d indirect?
…e_has_preferred_descriptor()
For the bug here.
The text was updated successfully, but these errors were encountered: