Add a new "outdated" field to distinguish between outdated and too new tor versions

changed milestone to %Onionoo 1.12.0

added component::metrics/onionoo milestone::Onionoo 1.12.0 owner::karsten parent::25199 priority::high resolution::fixed reviewer::iwakeh severity::normal status::closed type::enhancement labels

Replying to arma:

Tor has some somewhat complicated logic for deciding whether a version is obsolete -- see tor_version_is_obsolete(). Is atlas's best plan to try to reproduce Tor's logic? Should we try to specify it, e.g. in version-spec.txt, so external programs have a chance of getting it right?

I opened #24257 (moved) for the Tor side of this fix.

One possible way to fix this would be to change "outdated" to "not recommended" in that Atlas text and let the user figure out whether that means too old or too new. I'd guess that the relay operator knows pretty well whether they're running something experimental or haven't updated their package for a while.

However, if that's a major usability issue, let's add the logic for deciding whether "not recommended" means "outdated" to Onionoo. In that case I wouldn't mind working from a spec.

Trac:
Milestone: N/A to Tor: 0.3.3.x-final

oops; wrong ticket.

Trac:
Milestone: Tor: 0.3.3.x-final to N/A

Replying to karsten:

I'd guess that the relay operator knows pretty well whether they're running something experimental or haven't updated their package for a while.

And if only the relay operator used the atlas page for the relay, that would be enough. I worry that users look at the atlas pages for other relays and get scared.

Yes, agreed. I'm moving this ticket to Onionoo, because that's what we should work on first. Setting to needs_information until there's a spec or until somebody figures that a spec is not necessary, in which case I'd just read the tor code (#24257 (moved)).

Trac:
Status: new to needs_information
Type: defect to enhancement
Summary: atlas complains "outdated tor version" for too new tor versions too to Add a new "outdated" field to distinguish between outdated and too new tor versions
Component: Metrics/Atlas to Metrics/Onionoo

I took a brief look at tor_version_is_obsolete() today. Quite a few cases there. Before I looked at that code I somehow assumed that we could divide the non-recommended bucket into outdated and experimental and be done with it. But it seems like we'd also need an unknown bucket and maybe even more.

If that's really the case we might want to avoid adding separate boolean fields for all possible statuses, like "outdated_version":true, "experimental_version":false, etc., and instead introduce a new string field like "version_status":"outdated" with pre-defined values like "recommended", "outdated" (or "obsolete"?), "experimental", "unknown", etc.

Does that make sense? What statuses would we need, and how would we call them to avoid confusing users when a client application decides to present version status values directly to its users?

I like the "version_status" idea. I was thinking to add the specific reasons why a version is not recommended (the ones that are affected by specific bugs as mentioned in the tor source) but I wonder if those do not ever actually see the consensus anyway and so wouldn't be appearing in Onionoo.

This would perhaps be something to add to Atlas when a whole bunch of relays are just about to disappear to explain to those users that they need to upgrade, but once the relays are not in Onionoo anymore the message would never be seen again.

The atlas side has opened #25199 (moved) as a related ticket.

Replying to karsten:

Setting to needs_information until there's a spec or until somebody figures that a spec is not necessary, in which case I'd just read the tor code (#24257 (moved)).

Looks like #24257 (moved) got closed. There is a new Section 3, "Version status", in https://gitweb.torproject.org/torspec.git/tree/version-spec.txt

Trac:
Status: needs_information to new
Parent: N/A to #25199 (moved)

I started working on this.

Trac:
Owner: metrics-team to karsten
Status: new to accepted

Please review commit 6dd9709 in my task-24256 branch.

Here's what this branch produces when run locally:

$ curl "http://localhost:8080/details?fields=version,version_status&type=relay" | sort | uniq -c
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  391k    0  391k    0     0   193k      0 --:--:--  0:00:02 --:--:--  193k
   1 "bridges":[
   1 "bridges_published":"2018-02-22 19:04:06",
   1 "build_revision":"6dd9709",
   1 "relays":[
   1 "relays_published":"2018-02-22 19:00:00",
   1 ],
   1 ]}
   2 {"version":"0.2.4.19","version_status":"obsolete"},
  15 {"version":"0.2.4.20","version_status":"obsolete"},
   5 {"version":"0.2.4.21","version_status":"obsolete"},
   9 {"version":"0.2.4.22","version_status":"obsolete"},
 148 {"version":"0.2.4.23","version_status":"obsolete"},
   9 {"version":"0.2.4.24","version_status":"obsolete"},
  81 {"version":"0.2.4.27","version_status":"obsolete"},
  96 {"version":"0.2.4.29","version_status":"obsolete"},
  10 {"version":"0.2.5.10","version_status":"obsolete"},
  64 {"version":"0.2.5.12","version_status":"obsolete"},
  57 {"version":"0.2.5.14","version_status":"obsolete"},
   2 {"version":"0.2.5.15","version_status":"obsolete"},
 398 {"version":"0.2.5.16","version_status":"recommended"},
   1 {"version":"0.2.5.16-dev","version_status":"unrecommended"},
   3 {"version":"0.2.6.0-alpha-dev","version_status":"unrecommended"},
  30 {"version":"0.2.6.10","version_status":"unrecommended"},
   1 {"version":"0.2.6.7","version_status":"unrecommended"},
   1 {"version":"0.2.6.8","version_status":"unrecommended"},
   5 {"version":"0.2.6.9","version_status":"unrecommended"},
   1 {"version":"0.2.7.1-alpha","version_status":"unrecommended"},
   1 {"version":"0.2.7.2-alpha","version_status":"unrecommended"},
   1 {"version":"0.2.7.4-rc","version_status":"unrecommended"},
   3 {"version":"0.2.7.5","version_status":"unrecommended"},
 217 {"version":"0.2.7.6","version_status":"unrecommended"},
   1 {"version":"0.2.8.10","version_status":"unrecommended"},
   1 {"version":"0.2.8.10-dev","version_status":"unrecommended"},
   5 {"version":"0.2.8.11","version_status":"unrecommended"},
   6 {"version":"0.2.8.12","version_status":"unrecommended"},
   2 {"version":"0.2.8.2-alpha","version_status":"unrecommended"},
   2 {"version":"0.2.8.6","version_status":"unrecommended"},
   7 {"version":"0.2.8.7","version_status":"unrecommended"},
  11 {"version":"0.2.8.8","version_status":"unrecommended"},
   8 {"version":"0.2.8.9","version_status":"unrecommended"},
  72 {"version":"0.2.9.10","version_status":"unrecommended"},
 176 {"version":"0.2.9.11","version_status":"unrecommended"},
  40 {"version":"0.2.9.12","version_status":"unrecommended"},
  22 {"version":"0.2.9.13","version_status":"unrecommended"},
 893 {"version":"0.2.9.14","version_status":"recommended"},
  10 {"version":"0.2.9.8","version_status":"unrecommended"},
  32 {"version":"0.2.9.9","version_status":"unrecommended"},
   2 {"version":"0.3.0.1-alpha","version_status":"unrecommended"},
 106 {"version":"0.3.0.10","version_status":"unrecommended"},
   2 {"version":"0.3.0.11","version_status":"unrecommended"},
  90 {"version":"0.3.0.13","version_status":"unrecommended"},
   2 {"version":"0.3.0.13-dev","version_status":"unrecommended"},
   1 {"version":"0.3.0.4-rc","version_status":"unrecommended"},
   1 {"version":"0.3.0.6-dev","version_status":"unrecommended"},
   3 {"version":"0.3.0.7","version_status":"unrecommended"},
  10 {"version":"0.3.0.8","version_status":"unrecommended"},
  49 {"version":"0.3.0.9","version_status":"unrecommended"},
   6 {"version":"0.3.1.2-alpha-dev","version_status":"unrecommended"},
   1 {"version":"0.3.1.4-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.1.5-alpha","version_status":"unrecommended"},
 129 {"version":"0.3.1.7","version_status":"unrecommended"},
   1 {"version":"0.3.1.7-dev","version_status":"unrecommended"},
  89 {"version":"0.3.1.8","version_status":"unrecommended"},
   1 {"version":"0.3.1.8-dev","version_status":"unrecommended"},
1522 {"version":"0.3.1.9","version_status":"recommended"},
   1 {"version":"0.3.2.1-alpha","version_status":"unrecommended"},
   2 {"version":"0.3.2.2-alpha","version_status":"unrecommended"},
   6 {"version":"0.3.2.3-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.2.4-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.2.6-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.2.7-rc","version_status":"unrecommended"},
  62 {"version":"0.3.2.8-rc","version_status":"recommended"},
   1 {"version":"0.3.2.8-rc-dev","version_status":"unrecommended"},
   1 {"version":"0.3.2.9","version_status":"recommended"}
2435 {"version":"0.3.2.9","version_status":"recommended"},
  17 {"version":"0.3.2.9-dev","version_status":"new in series"},
   3 {"version":"0.3.3.0-alpha-dev","version_status":"unrecommended"},
  43 {"version":"0.3.3.1-alpha","version_status":"recommended"},
  16 {"version":"0.3.3.1-alpha-dev","version_status":"unrecommended"},
 229 {"version":"0.3.3.2-alpha","version_status":"recommended"},
   8 {"version":"0.3.3.2-alpha-dev","version_status":"experimental"},
  16 {"version":"0.3.4.0-alpha-dev","version_status":"experimental"},
   1 {"version":"5.0",

(Note that the last line, version 5.0, is the Onionoo version, not a relay version.)

Please also check whether the reported "version_status" fields match what's written in version-spec.txt.

Trac:
Status: accepted to needs_review

 398 {"version":"0.2.5.16","version_status":"recommended"},
   1 {"version":"0.2.5.16-dev","version_status":"unrecommended"},

it matches the spec but I find the spec strange and counterintuitive if 0.2.5.16 is recommended I would declare 0.2.5.16-dev experimental, but I guess the spec will not be changed.

actually, by the version spec 0.2.5.16-dev is "new in series"

Replying to cypherpunks:

actually, by the version spec 0.2.5.16-dev is "new in series"

No, 0.2.5.17 is also a recommended version, there's just no relay running that version. And with 0.2.5.16 and 0.2.5.17 being recommended versions, 0.2.5.16-dev is an unrecommended version.

Anyway, if the spec is strange and counterintuitive, let's try to fix it.

Replying to karsten:

Replying to cypherpunks:

actually, by the version spec 0.2.5.16-dev is "new in series"

No, 0.2.5.17 is also a recommended version, there's just no relay running that version. And with 0.2.5.16 and 0.2.5.17 being recommended versions, 0.2.5.16-dev is an unrecommended version. I'm sorry, you are right.

recommended version might include unreleased versions (in preparation for upcoming releases). 0.2.5.17 is an unreleased version.

Anyway, if the spec is strange and counterintuitive, let's try to fix it.

Replying to cypherpunks:

recommended version might include unreleased versions (in preparation for upcoming releases). 0.2.5.17 is an unreleased version.

Oh, I didn't know that 0.2.5.17 is unreleased, yet recommended. But anyway, we have to take what we get in the list of recommended server versions.

Thanks for checking the versions in the example output.

The code is still ready for review.

Trac:
Reviewer: N/A to iwakeh

From reading the code I'd say the use of enum 'Status' is good, but should be extended, i.e., instead of private inside the 'TorVersion' helper class use a public enum 'Status' that is also used elsewhere and the status String should only be used just before serialization. Using enums more reduces memory requirements. The NPE possibly thrown at the beginning of 'TorVersion.of' would stop all of the processing. Is this intended?

Trac:
Status: needs_review to needs_information

Replying to iwakeh:

From reading the code I'd say the use of enum 'Status' is good, but should be extended, i.e., instead of private inside the 'TorVersion' helper class use a public enum 'Status' that is also used elsewhere and the status String should only be used just before serialization. Using enums more reduces memory requirements.

Are there other places where using these specific "states" (recommended, experimental, obsolete, new in series, unrecommended) make any sense? If not, let's rather create new enums for those places.

The NPE possibly thrown at the beginning of 'TorVersion.of' would stop all of the processing. Is this intended?

Hmm, probably not. Can you describe a situation (or even better: add a test) where this would go wrong?

Thanks!

Trac:
Status: needs_information to needs_review

While looking again:

The comparator interface implemented by TorVersion is not really used as such. Maybe remove the comparator interface and instead introduce two methods 'olderThan' and 'youngerThan'? This would also make 'determineVersionStatus' easier to read and save the unused 'hashCode' and 'equals' implementations.
Why not return null, if 'TorVersion.of' receives a null argument? Currently, it is only used once, where the null check is done before calling 'TorVersion.of'.
Making enum 'Status' public would also support de/serialization. The abbreviation could also be part of the enum and replace the switch statement in NodeStatus to, for example nodeStatus.setVersionStatus(Status.ofAbbreviation(parts[25]).toString()) and leave room for any number of new status types. And, the 'knowledge' about abbreviation vs. full description is in one place w/o repetition.

There are also some checkstyle complaints (mostly empty lines and missing javadoc).

Trac:
Status: needs_review to needs_revision

Alright, I made most of the changes/fixes you suggested, except for the first: the reason is that equals() is being used (in recommendedVersions.contains(this)) and that I don't see a good reason against using a standard interface like Comparator in this case. Please take another look at my updated branch. Thanks!

Trac:
Status: needs_revision to needs_review

Replying to karsten:

Alright, I made most of the changes/fixes you suggested, except for the first: the reason is that equals() is being used (in recommendedVersions.contains(this))

Thanks! And true, 'equals' is used and 'hashCode' should give the same result for equal objects.

... and that I don't see a good reason against using a standard interface like Comparator in this case. Please take another look at my updated branch. Thanks!

I don't mind standard interfaces, but here the standard interface is a way bigger promise than we want to keep. It is broken, because it is inconsistent with the implementation of equals and hashCode. I put up a very simple example test here demonstrating this (not for merge; this should be replaced by tests checking the intended behavior).

We only need an order for valid tor versions for deciding the status of valid tor versions. Invalid versions are never recommendable, I assume, but currently they are classified as obsolete. Is that intended?

Another question is, if tor versions like '01.01' and '1.1' have the same 'age'? Method compareTo would currently return 0. If that is correct, these should be equal, too. Could it help to 'normalize' valid versions, i.e., turn '1.01' into '1.1'?

Trac:
Status: needs_review to needs_revision

Addendum: For the sorted set SortedSet a comparator only for valid versions should be given separately.

Ah, I see where this is getting. Agreed, there are cases where the current implementation is broken. Let's fix it.

Invalid versions are never recommendable, and we could simply say that they are "unrecommended". So, we could change TorVersion#of to return null whenever it is given an invalid tor version.

And "01.01" should be the same as "1.1".

I pushed a fixup commit with something that might get us closer to completion, though I'm not sure if it resolves all open issues. (I might be smarter about this tomorrow, but I ran out of brains for today.)

Leaving on needs_revision. If you want to revise what's there, feel free! Otherwise, I'll take another look tomorrow. Thanks!

Perhaps it's time to lighten up a little? Absent a really serious bug, just why is anyone talking about flagging relays for a modestly obsolete version? As often as these versions change, I would think that "3ea version obsolete" would be a reasonable flagging standard.

What Tor using now is not. And I'd be very surprised if such hijinks aren't the reason many relay operators give up and walk away.

Is there a "Rain Man" quality to some of these rules? A dedication to self destruction in pursuit of some unattainable ideal?

Tor's relay count is dropping. How does Tor manage that within an environment of constant news warnings concerning the perils of centralized internet?

Trac:
Username: subscriptionblocker

We discover a serious bug in tor every few months: https://trac.torproject.org/projects/tor/wiki/TROVE

We obsolete old Tor versions every year due to lack of features, or accumulated minor bugs: https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

Adding metrics-team to cc

Trac:
Cc: N/A to metrics-team

Please review my updated task-24256 branch with some more tweaks. I think it's okay now. Or at least I don't know what else to revise at this point. If something remains, please let me know.

Trac:
Status: needs_revision to needs_review

Looks fine. Maybe consider adding tests cases for the consistency of hash-map, equals, and Comparator? Merge ready.

Trac:
Status: needs_review to merge_ready

Good idea, and thanks for checking everything! Added some more tests for those three methods, squashed, and pushed to master. Closing. Thanks!

Trac:
Status: merge_ready to closed
Milestone: N/A to Onionoo 1.12.0
Resolution: N/A to fixed

Oops, I found two bugs while preparing the release that are both related to this enhancement. Please review commit 1ade07e in my task-24256 branch. Setting priority to high, so that we can still put out the release today.

Trac:
Status: closed to reopened
Resolution: fixed to N/A
Priority: Medium to High

Trac:
Status: reopened to needs_review

Ready for merge, if you confirm that version status cannot be null in the line added to DetailsDocumentWriter.

Replying to iwakeh:

Ready for merge, if you confirm that version status cannot be null in the line added to DetailsDocumentWriter.

I had that same thought after seeing both changes in that commit (which I found and fixed separately). Actually the version status there can be null, but that's okay, because we're not invoking a method on it. Alright, merging to master and re-closing. Thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Replying to karsten:

Replying to iwakeh:

Ready for merge, if you confirm that version status cannot be null in the line added to DetailsDocumentWriter.

I had that same thought after seeing both changes in that commit (which I found and fixed separately). Actually the version status there can be null, but that's okay, because we're not invoking a method on it. Alright, merging to master and re-closing. Thanks!

I'm not concerned about NPEs, but the string "null" appearing somewhere, where it shouldn't be.

Replying to iwakeh:

I'm not concerned about NPEs, but the string "null" appearing somewhere, where it shouldn't be.

Ah, Gson simply leaves out fields with null values, unless told otherwise.

closed

mentioned in issue #24257 (moved)

mentioned in issue #25199 (moved)

mentioned in issue #25558 (moved)

mentioned in issue #25711 (moved)

mentioned in issue tpo/core/tor#24257 (closed)

Add a new "outdated" field to distinguish between outdated and too new tor versions

Child items ...

Activity