Opened 6 months ago

Last modified 2 weeks ago

#22261 needs_revision defect

Remove the $ from family fingerprints

Reported by: teor Owned by: karsten
Priority: Medium Milestone: Onionoo-2.0.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords: metrics-2017
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

It's not needed, and it causes issues for naïve shell users.

Child Tickets

Change History (17)

comment:1 Changed 6 months ago by cypherpunks

The dollar signs are added by Onionoo (see https://onionoo.torproject.org/details?fields=effective_family&limit=10) so it would probably be better to fix Onionoo instead.

Keep this ticket open though because Atlas needs some cleaning up once Onionoo fixes the issue.

comment:2 Changed 2 months ago by karsten

Summary: Please remove the $ from atlas family fingerprintsRemove the $ from family fingerprints

Shorten the summary bit. (Thanks for asking nicely anyway!)

comment:3 Changed 8 weeks ago by karsten

Keywords: metrics-2018 added

comment:4 Changed 8 weeks ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:5 in reply to:  1 Changed 8 weeks ago by karsten

Replying to cypherpunks:

The dollar signs are added by Onionoo (see https://onionoo.torproject.org/details?fields=effective_family&limit=10) so it would probably be better to fix Onionoo instead.

Hmm, to be precise, the dollar signs are not added by Onionoo. It simply copies over entries from relays‘ „family“ lines. See also dir-spec.txt. But I have to admit that we‘re not including dollar signs in other fingerprints, including „fingerprint“ and „hashed_fingerprint“. Maybe we should indeed remove the dollar signs in Onionoo. That requires a major protocol change though, because we‘d be modifying an existing field. Should we do it?

Keep this ticket open though because Atlas needs some cleaning up once Onionoo fixes the issue.

What cleaning up is that? Mind if we move this ticket to Metrics/Onionoo until the feature is implemented?

comment:6 Changed 8 weeks ago by irl

Component: Metrics/AtlasMetrics/Onionoo

I would maybe discuss this at tor-project@ or metrics-team@ (where do Onionoo users hang out?), but it's definitely not an Atlas specific issue.

The Atlas issue, if it exists, would be to make sure that we are happy with either $ or no $. i.e. we remove $ signs where needed but we don't just blindly remove the first character. I've made a note to check if this problem exists and will file an Atlas ticket if it does.

comment:7 Changed 5 weeks ago by karsten

Status: newneeds_review

I think we can just decide whether we want to make this change or not, without going out and asking users. After all, Onionoo users are developers, not end users. If they'd prefer to keep the $ for their users, they could easily add it back to any hex string of 40 characters contained in the three family-related fields. But I somehow doubt that. In particular the part about other fingerprints and hashed fingerprint not starting with $ is pretty convincing. Let's remove the last remaining $ from family fingerprints, too.

I wrote a simple patch in my task-22261 branch that needs review.

Regarding documentation, I suggest we simply remove the "$-prefixed" part in the specification, e.g., "Array of $-prefixed fingerprints of relays that are in an effective, mutual family relationship with this relay. These relays are part of this relay's family and they consider this relay to be part of their family. Omitted if empty or if descriptor containing this information cannot be found. Removed $ from fingerprints on October XY, 2017."

And just as a reminder: when we deploy it, we'll need to do a major protocol version bump. That means announcing the protocol change at least 1 month before releasing the change.

comment:9 Changed 5 weeks ago by irl

Owner: changed from irl to karsten
Status: needs_reviewassigned

Reassigning to karsten as this is an Onionoo issue now.

comment:10 Changed 3 weeks ago by iwakeh

Status: assignedmerge_ready

This looks fine.

comment:11 Changed 3 weeks ago by karsten

Status: merge_readyneeds_information

Thanks for looking! Pushed a change to schedule the next major version update for November 30 in the hope that we'll get most of the Onionoo changes merged this month. We can always push back the date if needed. Setting to needs_information until that time has passed.

comment:12 Changed 3 weeks ago by karsten

Note to self: add a change log entry before merging.

comment:13 in reply to:  12 Changed 3 weeks ago by iwakeh

Replying to karsten:

Note to self: add a change log entry before merging.

Oh, I overlooked the missing changelog entry. Setting to needs revision ;-)
Added milestone 1.7.0.
This ticket should be closed when the changelog is updated. Ticket #24059 makes sure the web site protocol description is updated accordingly.

Last edited 3 weeks ago by iwakeh (previous) (diff)

comment:14 Changed 3 weeks ago by iwakeh

Status: needs_informationneeds_revision

comment:15 Changed 3 weeks ago by karsten

Setting to needs_revision for the missing change log entry is fine, but even once we have that we cannot merge to master for a couple of weeks until we're ready for the next major protocol version.

comment:16 Changed 2 weeks ago by iwakeh

Milestone: Onionoo-2.0.0

Adding the major milestone 2.0.0 for protocol 5.0 change.
(This can be set to 'merge-ready' once the changes are completed.)

comment:17 Changed 2 weeks ago by karsten

Why 2.0.0 and not 1.8.0? When we bumped the protocol version to 4.0 we only bumped the implementation version to 1.2.0, too. No need to pick a final version today, I'm just wondering why we would need a major (implementation) version change here.

Note: See TracTickets for help on using tickets.