Opened 2 months ago

Closed 7 weeks ago

Last modified 4 weeks ago

#33633 closed task (implemented)

Move extend and reachability code to the relay module

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, prop311, technical-debt, network-team-roadmap-2020Q1
Cc: Actual Points: 1.0
Parent ID: #33048 Points: 0.5
Reviewer: nickm Sponsor: Sponsor55-must

Description

Most of the extend and reachability code is already in the relay module.

But some code was left behind in src/core/or/circuitbuild.c.

Child Tickets

Change History (14)

comment:1 Changed 2 months ago by teor

Status: assignedneeds_review

See my PR:

Should be a quick one, it's mostly code movement.

It's not urgent, and it's not a blocker. I will just continue working off the branch.

comment:2 Changed 2 months ago by teor

Did a bit more refactoring, and added a changes file.

comment:3 Changed 2 months ago by nickm

(Looking now. There's a travis build failure, but it seems to be a bogus network timeout during apt-get.)

comment:4 Changed 2 months ago by teor

It's not quite #32804, but I've added it to the list there, for my next email to Travis.

comment:5 Changed 2 months ago by nickm

Looks good on first glance. I need to take a second look at these commits, since they are nontrivial.

  • relay: Split link specifier checks from circuit_extend()
  • relay: Split out opening a connection for an extend

In the meantime, is it reasonable to try to get test coverage here, particularly on the new check functions introduced in those two commits?

edit: typo

Last edited 2 months ago by nickm (previous) (diff)

comment:6 Changed 2 months ago by nickm

Status: needs_reviewneeds_revision

(Also I left a note on the PR)

comment:7 Changed 2 months ago by teor

Actual Points: 0.51.0

Yes, I think you're right about the test coverage. I knew there was something I had forgotten!

I'll need good test coverage here, because these are some of the functions I'll be modifying as part of Sponsor 55. (For IPv6 extends, and reachability checks.)

And I don't want to depend on chutney to test all these special cases.

comment:8 Changed 2 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

Add all the tickets from sponsor 55 that are done and being worked on to the keyword #network-team-roadmap-2020Q1 so I can look at them in the wiki page...

comment:9 Changed 2 months ago by teor

(I fixed a linking issue in my PR, when relay mode was disabled.)

comment:10 Changed 8 weeks ago by teor

(I wrote a unit test for the first function, 5-ish to go.)

comment:11 Changed 7 weeks ago by teor

Status: needs_revisionneeds_review

I wrote comprehensive unit tests for the circuit_extend() function and helpers. During the rewrite, I re-ordered and fixed many of the existing commits.

I deferred the rest of test_onionskin_answer() to #33860 (part of #33221).

See my PR:

I want to work off this branch for #33817, which is my next task. If you'd like me to make major changes to this code, please let me know as soon as possible. I'd like to avoid conflicts with #33817, #33818, and the rest of #33220.

Otherwise, I don't mind when the review or merge happens. I'm happy to work off un-merged branches.

comment:12 Changed 7 weeks ago by nickm

Status: needs_reviewmerge_ready

I had a question, but it was a false positive. This looks good to me, and CI is passing. Github hasn't noticed that CI is passing yet, though, so I had to hunt down the coveralls output by hand. That looks reasonable (https://coveralls.io/builds/29955459): Coverage is up, and though coveralls is reporting "new uncovered lines", I believe I can confirm that they are all lines that moved here.

comment:13 Changed 7 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master. Nice work!

comment:14 Changed 4 weeks ago by teor

Parent ID: #33220#33048
Note: See TracTickets for help on using tickets.