Opened 4 months ago

Closed 6 days ago

#24494 closed defect (fixed)

Specification says nickname is optional in documents but it's always there

Reported by: irl Owned by: karsten
Priority: Medium Milestone: Onionoo 1.11.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: iwakeh Sponsor:



Specification says:

Relay nickname consisting of 1–19 alphanumerical characters. Omitted if the relay nickname is "Unnamed".

I think I would prefer to fix the specification than to fix the Onionoo behaviour, as it's useful to have only one code path in Relay Search instead of more code paths depending on whether or not the field is present.

Child Tickets

Change History (13)

comment:1 Changed 4 months ago by karsten

Agreed, we should take out that optimization. It seemed like a good idea to save some bandwidth at the beginning, but we added so many more fields that this one doesn't really matter anymore. Reducing complexity seems more important. Let's do it.

comment:2 Changed 6 weeks ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted

I'll look into this.

comment:3 Changed 6 weeks ago by irl

Can I safely assume that nickname will always be present for Relay Search? This means I get to strip out a conditional and make the results table a little faster.

comment:4 Changed 6 weeks ago by karsten

Don't assume it just yet. But I'll take that question as a request and will try to make it so.

comment:5 Changed 6 weeks ago by karsten

Status: acceptedneeds_review

Please review commit 81c72b0 in my Onionoo task-24494 branch and commit d2178c3 in my metrics-web task-24494 branch. Yet untested, except for unit tests.

irl, if these reviews go through, you can safely assume that nickname will always be present for Relay Search. In fact, you can already do that for details documents, just not for summary documents. But feel free to wait for this change and new protocol version 5.1 to be deployed.

comment:6 Changed 6 weeks ago by irl

The patch for metrics-web looks to be what I was expecting.

Relay Search does not actually use the summary documents anymore, and instead only makes one fetch for the details document (instead of the summary document followed by n fetches of details documents).

comment:7 Changed 5 weeks ago by karsten

Reviewer: iwakeh

Okay, that leaves the Onionoo branch for review, which is probably iwakeh's part. Setting them as reviewer.

comment:8 Changed 5 weeks ago by irl

Reviewer: iwakeh

Relay Search is now assuming that nicknames are always present.

comment:9 Changed 5 weeks ago by irl

Reviewer: iwakeh

comment:10 Changed 2 weeks ago by iwakeh

Status: needs_reviewmerge_ready

Onionoo changes look fine and pass all tests and checks. The protocol description makes sense.
Always good to simplify things when possible.

comment:11 Changed 11 days ago by karsten

Great! Pushed to master together with another commit that bumps the protocol version to 5.1. Not closing yet, because we'll still need to merge the metrics-web patch when we put out the next Onionoo release. Leaving as merge_ready.

comment:12 Changed 7 days ago by karsten

Milestone: Onionoo 1.11.0

This will be part of the next milestone.

comment:13 Changed 6 days ago by karsten

Resolution: fixed
Status: merge_readyclosed

Moving the specification part to #25476 and closing this ticket.

Note: See TracTickets for help on using tickets.