Opened 8 months ago

Closed 7 months ago

#24573 closed defect (fixed)

rewrite_node_address_for_bridge() should set IPv6 preferences even if there is no ri

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.5-alpha
Severity: Normal Keywords: ipv6, tor-bridge-client, easy, intro, review-group-28
Cc: Actual Points:
Parent ID: #20916 Points: 0.5
Reviewer: teor Sponsor: SponsorV-can

Description

rewrite_node_address_for_bridge() should always set IPv6 preferences, even if there is only one of ri or rs. And it should always warn about them.

This goes back to commit c213f277cde in 0.2.8.2-alpha and commit 9e9edf71f7d in 0.2.4.5-alpha.

Child Tickets

TicketStatusOwnerSummaryComponent
#24572closedffmancerarewrite_node_address_for_bridge() doesn't set rs IPv6 addressesCore Tor/Tor

Change History (14)

comment:1 Changed 8 months ago by teor

Keywords: easy intro added

comment:2 Changed 8 months ago by ffmancera

Status: newneeds_review

I think all it's done! Check my github branch bug24573.

comment:3 Changed 8 months ago by teor

Status: needs_reviewneeds_information

The code looks sensible to me.

The next step here is that ffmancera and I need to test this using make check test-network-all

comment:4 Changed 8 months ago by teor

Status: needs_informationneeds_review

comment:5 Changed 8 months ago by ffmancera

All tests passed, it's fine with me!

comment:6 Changed 8 months ago by teor

Reviewer: teor
Status: needs_reviewneeds_revision

Code Structure:

This patch duplicates some code, we should get rid of the duplication when we make ri and rs read-only in node_t.

Backporting:

Using a relay as a bridge is a rare case, and the configured address will only be different for PTs or multi-port or multi-homed relays. I don't think we need to backport this.

Testing:

The code passes make check test-network-all.

Nitpicks:

There is a space at the end of the changes file line:

router information or router status and warns about them. 

The changes file syntax for bugs is slightly different, it references the tor version that the bug was introduced in.

Please make these two changes file fixes, and flip to merge_ready.

Also, feel free to credit yourself in the changes file using "Patch by (preferred name or handle)".

comment:7 Changed 8 months ago by teor

I noticed you also implemented #24572 as part of this patch.
Can you update the changes file to mention that we also rewrite IPv6 addresses in rs?
And reference 24572 as another bug that we fixed?
This might need two entries in the changes file. That's ok.

(Please run "make check-changes" after editing the changes file.)

comment:8 Changed 8 months ago by ffmancera

I edited the change file. Maybe we need to create another ticket for code structure and backporting changes.

All done I hope everything is okay! (For the next time I will create new commit for each review).

comment:9 Changed 8 months ago by ffmancera

Status: needs_revisionmerge_ready

comment:10 Changed 8 months ago by nickm

Keywords: review-group-28 added

comment:11 Changed 7 months ago by nickm

teor, can you confirm that you like this version of the patch?

comment:12 Changed 7 months ago by teor

Yes, the updated version is fine. Let's get it merged.

comment:13 Changed 7 months ago by nickm

Merged it this morning. Please either unparent or close the child ticket as appropriate, so we can close this?

comment:14 Changed 7 months ago by ffmancera

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.