#23827 closed enhancement (implemented)

Clients/Relays: Use IPv6 Addresses from microdesc consensus

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, review-group-27
Cc: Actual Points: 1
Parent ID: #20916 Points: 0.5
Reviewer: dgoulet Sponsor: SponsorV-can

Description

Client/Relay Implementation for #20916.

We need to use the IPv6 addresses from consensuses with versions that implement #23826, and ignore microdesc IPv6 addresses.

Child Tickets

TicketTypeStatusOwnerSummary
#19610defectclosedteorIPv6-only clients fetch microdescriptors from a small number of IPv6 fallbacks

Change History (16)

comment:1 Changed 16 months ago by teor

Points: 0.5

comment:2 Changed 16 months ago by teor

The existing code from 0.2.8 will behave the right way once IPv6 addresses are moved into the microdesc consensus.

It checks addresses in this order:

  • routerinfo / descriptor / rewrite bridge address
  • routerstatus / consensus
  • microdesc

There are some minor changes needed to IPv6-only bootstrap (skipping the special case where the IPv6 microdescs come from fallbacks).

We should probably revise node_get_pref_ipv6_orport() so that it's a bit smarter, but let's do that in a different ticket.

comment:3 in reply to:  2 Changed 16 months ago by teor

Replying to teor:

We should probably revise node_get_pref_ipv6_orport() so that it's a bit smarter, but let's do that in a different ticket.

This is now #23975.

comment:4 Changed 16 months ago by teor

Owner: set to teor
Status: newassigned

These are all mine

comment:5 Changed 16 months ago by teor

Status: assignedneeds_review

These have code and a proposal in progress on tor-dev@

comment:6 Changed 16 months ago by teor

Status: needs_reviewneeds_revision

These have code in bug20916, but it needs to be chopped out and refactored after the other children of #20916 merge

comment:7 Changed 15 months ago by teor

Actual Points: 1
Keywords: regression removed
Status: needs_revisionneeds_review

Please see my branch bug23827_tree, which uses the new consensus methods from bug23826-23828.

We will probably need to do some squashing and rebasing before merge.

comment:8 Changed 15 months ago by nickm

Keywords: review-group-27 added

comment:9 Changed 15 months ago by dgoulet

Reviewer: dgoulet

comment:10 Changed 15 months ago by dgoulet

Status: needs_reviewneeds_revision
  • (Nitpick) For some more safety, I would const this variable:
networkstatus_t *cons = networkstatus_get_reasonably_live_consensus(

Apart from that lgtm;

Although a question, node_awaiting_ipv6() doesn't check for the descriptor (node->ri) anymore and the only trace for a reason of this I can find is in the commit message:

    If node_is_a_configured_bridge(), stop waiting for its IPv6 address in
    a microdescriptor. The previous check for ri was inaccurate.

So was it specifically done before because of bridges and turned out to be inaccurate? Or because now node_has_ipv6_addr() before takes care of looking at the descriptor if one?

comment:11 in reply to:  10 Changed 15 months ago by teor

Sponsor: SponsorV-can
Status: needs_revisionneeds_review

Replying to dgoulet:

  • (Nitpick) For some more safety, I would const this variable:
networkstatus_t *cons = networkstatus_get_reasonably_live_consensus(

Fixed in

[bug23827_tree 35f350a8f7] fixup! fixup! Add networkstatus_consensus_has_ipv6() and unit tests

Apart from that lgtm;

Although a question, node_awaiting_ipv6() doesn't check for the descriptor (node->ri) anymore and the only trace for a reason of this I can find is in the commit message:

    If node_is_a_configured_bridge(), stop waiting for its IPv6 address in
    a microdescriptor. The previous check for ri was inaccurate.

There is also the changes file entry for that commit:

    - Make IPv6-only clients wait for microdescs for relays, even if we were
      previously using descriptors (or were using them as a bridge) and have
      a cached descriptor for them.


So was it specifically done before because of bridges and turned out to be inaccurate?

It was specifically done because of bridges, but it would also stop waiting for mds if we had cached descriptors. It works in 0.3.2 and earlier, because we always check ri, rs, and md. But then we would sometimes chose the wrong address. (Remember that bridge bug?)

It won't work after we merge #23975, because we check which directory documents we can use, then look for the address in them.

Or because now node_has_ipv6_addr() before takes care of looking at the descriptor if one?

node_has_ipv6_addr() has similar issues with looking everywhere, and choosing the wrong address.

After we merge #23975, node_has_ipv6_addr() will also check which directory documents we can use, then look for the address in them.

I edited the comment to explain better in:

[bug23827_tree 270a604297] fixup! squash! Stop waiting for microdescs if the consensus supports IPv6 ORPorts

I also updated the commit message when I rebased and squashed.
The rebase was complex, because we squashed my branch bug23826-23828 before merging it.

Please merge my branch bug23827-v2, which has the same code as bug23827_tree.

comment:12 Changed 15 months ago by teor

(I also updated both branches with some check-spaces and check-changes fixups. Oops!)

comment:13 Changed 15 months ago by dgoulet

Status: needs_reviewmerge_ready

ack.

comment:14 Changed 15 months ago by nickm

Please tell me a little about integration testing here. How sure are we that this will work correctly once the authorities start updating to 0.3.3?

comment:15 Changed 15 months ago by teor

It passes all the 'make test-network-all' integration tests, including the mixed test with tor-old at 0.3.1.9 (or 0.3.1.8).

All the integration tests except mixed run with consensus method 28, which only puts IPv6 addresses in the microdesc consensus.
Mixed runs at consensus method 26, which is the current public network method, with IPv6 addresses in microdescs.

comment:16 Changed 15 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Sounds good to me; code looks good too. Merging to master!

Note: See TracTickets for help on using tickets.