Opened 16 months ago

Closed 10 months ago

Last modified 6 days 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: Tor: 0.2.3.1-alpha
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 (17)

comment:1 Changed 16 months ago by nickm

Cc: teor added

comment:2 Changed 13 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 13 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 13 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:6 Changed 12 months ago by neel

Status: assignedneeds_review

comment:7 Changed 12 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 10 months ago by dgoulet

Reviewer: nickm

comment:9 Changed 10 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 10 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 10 months ago by neel

Status: needs_revisionneeds_review

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

comment:12 Changed 10 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 10 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 10 months ago by neel

Status: needs_revisionneeds_review

Made the changes and pushed it to the PR.

comment:15 Changed 10 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 10 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)

comment:17 Changed 6 days ago by teor

Version: Tor: 0.2.3.1-alpha

The changes file for this release said "bugfix on 0.3.6.1-alpha", which does not exist. I fixed it in the ChangeLog and ReleaseNotes in #31461.

Note: See TracTickets for help on using tickets.