Clients/Relays: Use IPv6 Addresses from microdesc consensus

Client/Relay Implementation for #20916 (moved).

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

changed milestone to %Tor: 0.3.3.x-final

added actualpoints::1 component::core tor/tor ipv6 milestone::Tor: 0.3.3.x-final owner::teor parent::20916 points::0.5 priority::medium resolution::implemented review-group-27 reviewer::dgoulet severity::normal sponsor::V-can status::closed type::enhancement labels

Trac:
Points: N/A to 0.5

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.

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 (moved).

These are all mine

Trac:
Owner: N/A to teor
Status: new to assigned

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

Trac:
Status: assigned to needs_review

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

Trac:
Status: needs_review to needs_revision

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.

Trac:
Status: needs_revision to needs_review
Actualpoints: N/A to 1
Keywords: regression deleted, N/A added

Trac:
Keywords: N/A deleted, review-group-27 added

Trac:
Reviewer: N/A to dgoulet

• (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?

Trac:
Status: needs_review to needs_revision

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 (moved), 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 (moved), 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.

Trac:
Sponsor: N/A to SponsorV-can
Status: needs_revision to needs_review

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

ack.

Trac:
Status: needs_review to merge_ready

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?

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.

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

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 4h

added 8h of time spent

moved to tpo/core/tor#23827 (closed)