#21637 closed enhancement (fixed)

Include both declared and reachable IPv6 OR addresses

Reported by: teor Owned by: metrics-team
Priority: Medium Milestone: Onionoo-1.7.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords: metrics-2017, ipv6
Cc: Actual Points:
Parent ID: Points:
Reviewer: iwakeh Sponsor:

Description

When a relay declares an IPv6 OR address, it puts it in its descriptor, and it gets placed in the microdescriptor automatically.

But the IPv6 addresses in the full consensus are different: authorities on IPv6 only vote for an IPv6 address if they believe it is reachable.

(There are no IPv6 addresses in the microdesc consensus, see #20916.)

This makes a difference on Atlas tickets like #10401.

(It doesn't make a difference to client behaviour yet, because microdescs are the default. We'll fix that in #20916.)

Child Tickets

Change History (23)

comment:1 Changed 20 months ago by karsten

I believe we're taking addresses from consensuses, not from server descriptors, so we're only including reachable IPv6 OR addresses. And you want to learn about declared and unreachable IPv6 OR addresses, right?

comment:2 in reply to:  1 ; Changed 20 months ago by teor

Replying to karsten:

I believe we're taking addresses from consensuses, not from server descriptors, so we're only including reachable IPv6 OR addresses. And you want to learn about declared and unreachable IPv6 OR addresses, right?

Ok, so that's good: we're only displaying IPv6 addresses that work.

It would be nice to have the IPv6 address from the descriptor, and then we can synthesise a flag if it's missing from the consensus (or different).

comment:3 in reply to:  2 ; Changed 20 months ago by karsten

Replying to teor:

Replying to karsten:

I believe we're taking addresses from consensuses, not from server descriptors, so we're only including reachable IPv6 OR addresses. And you want to learn about declared and unreachable IPv6 OR addresses, right?

Ok, so that's good: we're only displaying IPv6 addresses that work.

Somebody (possibly I) should confirm by belief here by looking at the code.

It would be nice to have the IPv6 address from the descriptor, and then we can synthesise a flag if it's missing from the consensus (or different).

Yes, makes sense.

comment:4 Changed 13 months ago by karsten

Summary: Onionoo should distinguish between declared and reachable IPv6 or_addressesInclude both declared and reachable IPv6 OR addresses

Update summary to reflect what we think needs to be done.

comment:5 Changed 13 months ago by karsten

Keywords: metrics-2018 added

comment:6 Changed 13 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:7 Changed 12 months ago by teor

Keywords: ipv6 added

comment:8 in reply to:  3 ; Changed 12 months ago by karsten

Replying to karsten:

Replying to teor:

Replying to karsten:

I believe we're taking addresses from consensuses, not from server descriptors, so we're only including reachable IPv6 OR addresses. And you want to learn about declared and unreachable IPv6 OR addresses, right?

Ok, so that's good: we're only displaying IPv6 addresses that work.

Somebody (possibly I) should confirm by belief here by looking at the code.

So, I did look at the code and some sample descriptors and now feel more confident to confirm: we're only displaying IPv6 addresses that work.

It would be nice to have the IPv6 address from the descriptor, and then we can synthesise a flag if it's missing from the consensus (or different).

Yes, makes sense.

How about we add a new field "unreachable_or_addresses" that is an optional array of strings and that contains an "Array of IPv4 or IPv6 addresses and TCP ports or port lists where the relay claims to accept onion-routing connections but that the directory authorities failed to confirm as reachable. Addresses are in arbitrary order. IPv6 hex characters are all lower-case. Omitted if empty." If you can think of specifying this more clearly, please feel free to suggest a better text. And if you can think of a better way to include this data, just state it here.

Regarding the suggested flag, I believe that that shouldn't be a relay flag like the existing ones, but it could be a custom notification like Atlas adds for relays running an non-recommended version. In other words, that's for Atlas land, not Onionoo land. Hope that matches your idea. If not, please elaborate.

comment:9 in reply to:  8 ; Changed 12 months ago by teor

Replying to karsten:

Replying to karsten:

Replying to teor:

It would be nice to have the IPv6 address from the descriptor, and then we can synthesise a flag if it's missing from the consensus (or different).

Yes, makes sense.

How about we add a new field "unreachable_or_addresses" that is an optional array of strings and that contains an "Array of IPv4 or IPv6 addresses and TCP ports or port lists where the relay claims to accept onion-routing connections but that the directory authorities failed to confirm as reachable. Addresses are in arbitrary order. IPv6 hex characters are all lower-case. Omitted if empty." If you can think of specifying this more clearly, please feel free to suggest a better text. And if you can think of a better way to include this data, just state it here.

Do we need to explain:

  • claims to accept onion-routing connections in its descriptor
  • the directory authorities failed to confirm as reachable. (If a relay has an unreachable IPv4 address, the relay is removed from the consensus. If a relay has an unreachable IPv6 address, the relay is included in the consensus without the IPv6 address.)

This is how things will work when authorities upgrade to 0.3.3, assuming we merge the IPv6 fixes in #20916.

Regarding the suggested flag, I believe that that shouldn't be a relay flag like the existing ones, but it could be a custom notification like Atlas adds for relays running an non-recommended version. In other words, that's for Atlas land, not Onionoo land. Hope that matches your idea. If not, please elaborate.

Sounds good to me.
This will help us prompt operators when their IPv6 is broken.

comment:10 in reply to:  9 ; Changed 12 months ago by karsten

Replying to teor:

Replying to karsten:

Replying to karsten:

Replying to teor:

It would be nice to have the IPv6 address from the descriptor, and then we can synthesise a flag if it's missing from the consensus (or different).

Yes, makes sense.

How about we add a new field "unreachable_or_addresses" that is an optional array of strings and that contains an "Array of IPv4 or IPv6 addresses and TCP ports or port lists where the relay claims to accept onion-routing connections but that the directory authorities failed to confirm as reachable. Addresses are in arbitrary order. IPv6 hex characters are all lower-case. Omitted if empty." If you can think of specifying this more clearly, please feel free to suggest a better text. And if you can think of a better way to include this data, just state it here.

Do we need to explain:

  • claims to accept onion-routing connections in its descriptor

Sure, we can add the "in its descriptor" part.

  • the directory authorities failed to confirm as reachable. (If a relay has an unreachable IPv4 address, the relay is removed from the consensus. If a relay has an unreachable IPv6 address, the relay is included in the consensus without the IPv6 address.)

Yes, let's try to add that. Maybe we can make the distinction between primary and additional addresses rather than IPv4 and IPv6, just in case it will be possible to use an IPv6 address as primary address or an IPv4 address as additional address in the future.

This is how things will work when authorities upgrade to 0.3.3, assuming we merge the IPv6 fixes in #20916.

Hmm, can you elaborate how either the upgrade or the bugfix will affect us here?

In any case, here's the edited specification with additional parts being written in italics:

"Array of IPv4 or IPv6 addresses and TCP ports or port lists where the relay claims in its descriptor to accept onion-routing connections but that the directory authorities failed to confirm as reachable. Contains only additional addresses of a relay that are found unreachable, whereas relays with an unreachable primary address are excluded entirely. Addresses are in arbitrary order. IPv6 hex characters are all lower-case. Omitted if empty."

Please continue editing or suggesting as necessary!

Regarding the suggested flag, I believe that that shouldn't be a relay flag like the existing ones, but it could be a custom notification like Atlas adds for relays running an non-recommended version. In other words, that's for Atlas land, not Onionoo land. Hope that matches your idea. If not, please elaborate.

Sounds good to me.
This will help us prompt operators when their IPv6 is broken.

Sounds great!

comment:11 Changed 12 months ago by karsten

Status: newneeds_review

My task-21637 branch contains a small patch that adds this new field. Requires a minor protocol version bump when merging.

comment:12 in reply to:  10 ; Changed 12 months ago by teor

Replying to karsten:

Replying to teor:

Replying to karsten:

Replying to karsten:

Replying to teor:

It would be nice to have the IPv6 address from the descriptor, and then we can synthesise a flag if it's missing from the consensus (or different).

Yes, makes sense.

How about we add a new field "unreachable_or_addresses" that is an optional array of strings and that contains an "Array of IPv4 or IPv6 addresses and TCP ports or port lists where the relay claims to accept onion-routing connections but that the directory authorities failed to confirm as reachable. Addresses are in arbitrary order. IPv6 hex characters are all lower-case. Omitted if empty." If you can think of specifying this more clearly, please feel free to suggest a better text. And if you can think of a better way to include this data, just state it here.

Do we need to explain:

  • claims to accept onion-routing connections in its descriptor

Sure, we can add the "in its descriptor" part.

  • the directory authorities failed to confirm as reachable. (If a relay has an unreachable IPv4 address, the relay is removed from the consensus. If a relay has an unreachable IPv6 address, the relay is included in the consensus without the IPv6 address.)

Yes, let's try to add that. Maybe we can make the distinction between primary and additional addresses rather than IPv4 and IPv6, just in case it will be possible to use an IPv6 address as primary address or an IPv4 address as additional address in the future.

Yes. Or some combination of primary and alternate addresses regardless of version.

This is how things will work when authorities upgrade to 0.3.3, assuming we merge the IPv6 fixes in #20916.

Hmm, can you elaborate how either the upgrade or the bugfix will affect us here?

Authorities will vote Running for relays with unreachable IPv6 addresses, but drop the unreachable address from the vote.
At the moment, they vote not Running, and drop the entire relay from the consensus.

In any case, here's the edited specification with additional parts being written in italics:

"Array of IPv4 or IPv6 addresses and TCP ports or port lists where the relay claims in its descriptor to accept onion-routing connections but that the directory authorities failed to confirm as reachable. Contains only additional addresses of a relay that are found unreachable, whereas relays with an unreachable primary address are excluded entirely. Addresses are in arbitrary order. IPv6 hex characters are all lower-case. Omitted if empty."

Please continue editing or suggesting as necessary!

Looks great!

Regarding the suggested flag, I believe that that shouldn't be a relay flag like the existing ones, but it could be a custom notification like Atlas adds for relays running an non-recommended version. In other words, that's for Atlas land, not Onionoo land. Hope that matches your idea. If not, please elaborate.

Sounds good to me.
This will help us prompt operators when their IPv6 is broken.

Sounds great!

comment:13 in reply to:  12 ; Changed 12 months ago by karsten

Replying to teor:

Replying to karsten:

Replying to teor:

This is how things will work when authorities upgrade to 0.3.3, assuming we merge the IPv6 fixes in #20916.

Hmm, can you elaborate how either the upgrade or the bugfix will affect us here?

Authorities will vote Running for relays with unreachable IPv6 addresses, but drop the unreachable address from the vote.
At the moment, they vote not Running, and drop the entire relay from the consensus.

It took me a while to figure out why I'm seeing relays with unreachable IPv6 address and the Running flag in the consensus. Turns out that only tor26 and gabelmoo check whether an IPv6 address is reachable and hence remove the Running flag from relays in their vote. But given that all the other authorities don't perform this check and assign the Running flag to relays, they end up in the consensus as Running. And #20916 will fix the part about votes not containing the Running flag for these relays? If so, okay. This shouldn't block deploying this Onionoo feature, though.

I also found a minor bug in my branch and pushed a fixup commit.

I'd say that my updated task-21637 branch is ready for code review now!

comment:14 Changed 12 months ago by Sebastian

Can someone please summarize this "only some dirauths vote running" stuff and send it to the dirauth list? That seems super broken and we really should get all dirauths on the same page wrt ipv6

comment:15 in reply to:  14 Changed 12 months ago by karsten

Replying to Sebastian:

Can someone please summarize this "only some dirauths vote running" stuff and send it to the dirauth list? That seems super broken and we really should get all dirauths on the same page wrt ipv6

Sure, I'll send my findings to that list, and teor can follow up as needed. (I thought that all of this is already known information. Maybe it is, we'll find out.)

comment:16 in reply to:  14 Changed 12 months ago by teor

Replying to Sebastian:

Can someone please summarize this "only some dirauths vote running" stuff and send it to the dirauth list? That seems super broken and we really should get all dirauths on the same page wrt ipv6

Sure!

I have a draft proposal that summarises the changes here:

https://github.com/teor2345/torspec/blob/bug20916/proposals/xxx-ipv6-in-micro-consensus.txt

It describes the current state of IPv6 voting. But I want to improve the explanation before sending it out.

comment:17 Changed 12 months ago by iwakeh

Milestone: Onionoo-1.7.0

Adding the next milestone as these tickets are close to completion.

comment:18 Changed 12 months ago by iwakeh

Reviewer: iwakeh

comment:19 in reply to:  13 Changed 12 months ago by iwakeh

Replying to karsten:

...
I also found a minor bug in my branch and pushed a fixup commit.

I'd say that my updated task-21637 branch is ready for code review now!

Looks ok.
What about tests for the new field?
Other than that existing checks and tests pass.

comment:20 Changed 12 months ago by karsten

Status: needs_reviewneeds_revision

Hmm, there are no tests for DetailsDocumentWriter yet. I guess we could create a test class. Do you have any preferences for such a test class, or should we just start with a minimal one that only tests this new field?

comment:21 Changed 11 months ago by karsten

Status: needs_revisionneeds_review

Please review the last commit in my updated task-21637 branch which contains a new test class with unit tests for this new field.

comment:22 Changed 11 months ago by iwakeh

Status: needs_reviewmerge_ready

Checks and tests pass, line coverage up by 2% and branch coverage by 1%.
(In the writer package even by 11% and 5%.)

Last edited 11 months ago by iwakeh (previous) (diff)

comment:23 Changed 11 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Added a change log entry, rebased, and pushed to master. Thanks for checking!

Note: See TracTickets for help on using tickets.