Opened 10 months ago

Last modified 5 months ago

#32822 merge_ready enhancement

Make authorities add their own IPv6 address to trusted dir servers

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, 044-can
Cc: Actual Points: 0.4
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor:

Description

Authorities add themselves to trusted dir servers, but they don't add their own IPv6 addresses.

Child Tickets

Change History (12)

comment:1 Changed 10 months ago by teor

Status: assignedneeds_review

This ticket depends on a new function added in #32588.

See my PR:

comment:2 Changed 10 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

One CI build fails with asserts. I just seen that one in another ticket of yours. Might not be related to the fix itself?

In any case, putting back in needs revision to figure that one out.

comment:3 Changed 10 months ago by teor

Actual Points: 0.20.3
Status: needs_revisionneeds_review

I also forgot to check for an empty IPv6 address in this patch (in a different way to the last one).

I re-did the patch, so I could rewrite the commit message.

I still need to write some tests, but I'd like a review :-)

comment:4 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_revision

Couple comments on PR. Nothing crazy! looks good!

comment:5 Changed 10 months ago by teor

I made some fixes, but I still need to do unit tests, so I'm leaving it in needs_revision.

comment:6 Changed 9 months ago by teor

Actual Points: 0.30.4

I added some tests for authority IPv6 ORPort config parsing. They don't actually use the authority IPv6 ORPort self-add feature, but they do increase the coverage of the related config code.

comment:7 Changed 9 months ago by teor

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

Let's do this in 0.4.4: it's a feature, and there's no need to add it at the last minute.

comment:8 Changed 8 months ago by teor

Keywords: 044-should added

I'll look at this ticket again some time during 0.4.4.

comment:9 Changed 5 months ago by teor

Status: needs_revisionneeds_review

It's unlikely that I'll ever write tests for this feature, but someone else is welcome to do that, and then merge.

(Or merge without tests, if they think the code is simple enough.)

Putting this in needs_review to see what we think here.

comment:10 Changed 5 months ago by dgoulet

Status: needs_reviewmerge_ready

This is fine by me tbh. I think we could merge this. Ideal world is unit tests but there is just so much we can do I guess.

comment:11 Changed 5 months ago by nickm

hm, it looks like this is based on #32588, which is already merged in master. Dgoulet, do you have a sense which of these patches we should merge and how we should take them?

comment:12 Changed 5 months ago by nickm

Keywords: 044-can added; 044-should removed
Note: See TracTickets for help on using tickets.