Opened 13 months ago

Closed 7 months ago

#25885 closed defect (implemented)

count_acceptable_nodes() would be more accurate using node_has_preferred_descriptor()

Reported by: nickm Owned by: neel
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: teor, neel@… Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

On a github review for #25691, Teor notes about count_acceptable_nodes():

I think this code is correct, but it's not obvious:

  1. new_route_len() is the only caller
  2. new_route_len() will fail if count_acceptable_nodes() is less than the route length.
  3. later functions will fail if there is no guard
  4. later functions will fail if there are not enough subsequent nodes for the rest of the route

So this check can spuriously succeed when we have no guards, or when we have many bridges, and no subsequent nodes. But it can never fail when there are actually enough nodes for the path. So this is just an optimisation to stop us trying to build lots of circuits when we have no descriptors.

We could return the number that pass for_direct_connect = 1 and for_direct_connect = 0, but that seems unnecessarily complicated.

I think that we should probably adjust this function to do the careful count, but it's safe to defer.

Child Tickets

Change History (16)

comment:1 Changed 13 months ago by nickm

Cc: teor added

comment:2 Changed 10 months ago by neel

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

I am interested in fixing this.

I have a question: what is a "careful count"?

comment:3 in reply to:  2 Changed 10 months ago by teor

Replying to neel:

I am interested in fixing this.

I have a question: what is a "careful count"?

The function counts all nodes, and then checks if there are enough nodes for a path.

But it should check that there is at least 1 guard node, and at least 2 non-guard nodes.

comment:5 Changed 10 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:6 Changed 9 months ago by neel

Status: assignedneeds_review

comment:7 Changed 9 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

I'm putting this in 0.3.6, because changes to these functions have caused bootstrap issues in the past.

comment:8 Changed 7 months ago by dgoulet

Reviewer: nickm

comment:9 Changed 7 months ago by nickm

Status: needs_reviewneeds_revision

I've made some notes on the PR. I'd also like Teor's opinion about this one, to see if it's what they had in mind.

comment:10 Changed 7 months ago by teor

I agree with nickm's comments, and I also found another edge case: direct nodes can be bridges, which are not used as middles or exits.

I'd like to review the revised code as well.

comment:11 Changed 7 months ago by neel

Status: needs_revisionneeds_review

I have made the changes and pushed it. Same branch/PR.

comment:12 Changed 7 months ago by nickm

Status: needs_reviewneeds_revision

The changes look good to me, except that I think you mean "or" instead of "and" at one point. (Commented on the PR. I am not 100% sure I'm right.)

Teor, what do you think?

comment:13 Changed 7 months ago by teor

Oh, I missed this ticket. Sorry!

I think you're right. I think we want to fail if we can't find enough entry nodes, or if we can't find enough non-entry nodes.

Also, it would be useful for the log message to say how many of each type of node we want.

comment:14 Changed 7 months ago by neel

Status: needs_revisionneeds_review

Made the changes and pushed it to the PR.

comment:15 Changed 7 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:16 Changed 7 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

This looks good to me now; thank you! I've merged it to master. (There's a travis failure, but it looks like an intermittent rust dl failure)

Note: See TracTickets for help on using tickets.