Opened 5 years ago

Closed 4 years ago

#12656 closed defect (fixed)

Calculating path selection probabilities shouldn't depend on parsing a new consensus

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

Description

The current implementation in NodeDetailsStatusUpdater calculates guard, middle, and exit probability using bandwidth weights (Wgg, Wgd, etc.) from the most recent of all parsed consensuses. This approach breaks if no consensus was parsed, and as a result, all relays' guard/middle/exit probabilities are set to 0.0. This happend a few days ago, because it took CollecTor more than 10 minutes to fetch and provide a current consensus from the directory authorities, which was too late for the hourly Onionoo run.

This is also bad design, because right now, JSON documents depend on more than what's contained in internal status files. We may want to add a mode of operation that rewrites all JSON documents. That is currently not possible.

A possible fix would be to make all attributes of NodeStatus persistent. The downside is that we may not need those contents in the servlet, so this change would needlessly slow down populating the servlet's search index.

Another fix would be to move all non-persistent attributes from NodeStatus to DetailsStatus. That includes guard/middle/exit probabilities, but maybe also other fields. Not sure about downsides yet.

Child Tickets

Change History (4)

comment:1 Changed 4 years ago by karsten

Cc: iwakeh added

iwakeh, this is the bug I mentioned on #14826. I'm currently testing if this is still an issue after the major refactoring from a few months ago.

comment:2 Changed 4 years ago by karsten

Status: newneeds_review

Please review my branch task-12656 that contains a fix for this issue.

comment:3 Changed 4 years ago by iwakeh

The changes are fine.

Just my 'ceterum censeo' about testing ;-)
Currently, I cannot find any 'updater' tests. Maybe, add a todo-task here?

comment:4 Changed 4 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Great, merged to master. Thanks for looking!

Regarding unit tests, there are some tests for "updater" classes, namely for UptimeStatusUpdater! :) But I don't want to argue that there shouldn't be more tests. I think we should make progress on #13080, and then decide what to write tests for first. I'm sure we'll find something.

I'm closing this ticket, because this bug is fixed. Let's discuss tests more on #13080 or other tickets.

Note: See TracTickets for help on using tickets.