Opened 7 months ago

Closed 6 months ago

#34200 closed defect (implemented)

Refactor tor's circuit path node selection checks

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, extra-review, prop311, technical-debt, extra-review
Cc: nickm Actual Points: 1.5
Parent ID: #33221 Points: 1
Reviewer: nickm Sponsor: Sponsor55-can

Description

In #33222, we added an extra "can extend over IPv6" check to tor's circuit path node selection code.

To make sure it's applied consistently, I did a refactor of that code, so all those checks are in one function.

Child Tickets

Change History (4)

comment:1 Changed 7 months ago by teor

Cc: nickm added
Status: assignedneeds_review

See my PR, which is based on #33222:

This PR still needs unit tests and changes files for the extra refactor on top of #33222.

The stem tests will fail due to:

Reverting to the most recent stem release, or stem commit 13e401d, should resolve this issue.

comment:2 Changed 6 months ago by teor

Actual Points: 11.5
Keywords: extra-review extra-review added
Reviewer: nickm

I have rebased this PR on top of #33222, and added a changes file:

To see the changes without #33222:

This PR makes some of tor's node selection code a lot simpler. It fixes a bunch of subtle bugs and edge cases. The code was almost impossible to unit test before the refactor. After the refactor, it's a lot more consistent and modular.

Here are the features and functions that don't have unit tests:

  • node selection
    • choose_good_exit_server_general()
    • count_acceptable_nodes()
    • router_choose_random_node_helper()
    • router_choose_random_node()
    • router_can_choose_node()
    • router_add_running_nodes_to_smartlist()

Some of these functions are also modified in #33222. It should be easier to write the #33222 node selection unit tests on top of this refactor.

This code also runs in chutney, as part of Tor's CI. But chutney doesn't fully test node selection.

I suggest that we manually review the refactoring in this PR. We should also write unit tests for these functions (or open another ticket for the unit tests).

comment:3 Changed 6 months ago by nickm

Milestone: Tor: 0.4.4.x-finalTor: 0.4.5.x-final
Status: needs_reviewmerge_ready

Yes, this looks fine. On our current timeline it makes sense to take this (and the rest of the remaining S55 code) in 0.4.5.

comment:4 Changed 6 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

(Merged to master)

Note: See TracTickets for help on using tickets.