Opened 3 years ago

Closed 3 years ago

#20765 closed enhancement (fixed)

adapt to new lines in votes and consensus and make the adaption to protocol changes easier

Reported by: iwakeh Owned by: metrics-team
Priority: Medium Milestone: metrics-lib 1.6.0
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

New protocol versions 23 and 25 introduce new lines for votes and consensus; e.g. vote from moria

...
recommended-relay-protocols Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 Link=4 LinkAuth=1 Microdesc=1-2 Relay=2
recommended-client-protocols Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 Link=4 LinkAuth=1 Microdesc=1-2 Relay=2
required-relay-protocols Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 Link=3-4 LinkAuth=1 Microdesc=1 Relay=1-2
required-client-protocols Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 Link=4 LinkAuth=1 Microdesc=1-2 Relay=2
...
shared-rand-participate
shared-rand-commit 1 sha3-256 D586D18309DED4CD6D57C18FDB97EFA96D330566 AAAAAFg3fwANZSmhaatp83nojq97N/eLHSCShOFJqiR1Skc9lO/dXA== AAAAAFg3fwDxNhND8l9+/S4fxn+yeCKNgaZp3yJ8qWSkg8NICmZ+PA==

These should not be treated as unknown lines by metrics-lib.

In addition, the process of recognizing lines should be improved to make accommodation of new protocol versions a lot easier, ideally without code changes, i.e. w/o having to compile metrics-lib for such changes.

(should be tackled together with #17861, #19640, #19607)

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by karsten

See also #19634 here for the shared-rand-* lines.

Regarding making these changes without compiling, I'm not certain what the benefit would be. We'll have to put out a new release to make sense of these new lines anyway. And we'll want to include tests.

Speaking of, I think we accumulated enough changes for a new metrics-lib release in December. Let's discuss that either here or at the next meeting.

comment:2 in reply to:  1 ; Changed 3 years ago by iwakeh

Replying to karsten:

See also #19634 here for the shared-rand-* lines.

Regarding making these changes without compiling, I'm not certain what the benefit would be. We'll have to put out a new release to make sense of these new lines anyway. And we'll want to include tests.

Yes, that's true. Still it might be useful (especially for testing) to have something other than a bunch of if-else-if statements; maybe in form of an enum.

Speaking of, I think we accumulated enough changes for a new metrics-lib release in December. Let's discuss that either here or at the next meeting.

Yes, a December metrics-lib release makes sense!

comment:3 in reply to:  2 Changed 3 years ago by karsten

Replying to iwakeh:

Replying to karsten:

See also #19634 here for the shared-rand-* lines.

Regarding making these changes without compiling, I'm not certain what the benefit would be. We'll have to put out a new release to make sense of these new lines anyway. And we'll want to include tests.

Yes, that's true. Still it might be useful (especially for testing) to have something other than a bunch of if-else-if statements; maybe in form of an enum.

Agreed, having cleaner code there would be good. Maybe that comes together with rewriting that code for better performance (lazy parsing, better control over parsed-descriptor buffer).

Speaking of, I think we accumulated enough changes for a new metrics-lib release in December. Let's discuss that either here or at the next meeting.

Yes, a December metrics-lib release makes sense!

Cool!

I started identifying the code that needs changing by throwing CollecTor's November tarballs into a descriptor reader and handling any unrecognized lines:

diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
index f528033..dd5fac7 100644
--- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
@@ -95,6 +95,9 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry {
         case "v":
           this.parseVLine(line, parts);
           break;
+        case "pr":
+          // TODO
+          break;
         case "w":
           this.parseWLine(line, parts);
           break;
diff --git a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java
index 2bf8e88..e0cfdb7 100644
--- a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java
@@ -117,6 +117,18 @@ public class RelayNetworkStatusVoteImpl extends NetworkStatusImpl
         case "server-versions":
           this.parseServerVersionsLine(line, parts);
           break;
+        case "recommended-relay-protocols":
+          // TODO
+          break;
+        case "recommended-client-protocols":
+          // TODO
+          break;
+        case "required-relay-protocols":
+          // TODO
+          break;
+        case "required-client-protocols":
+          // TODO
+          break;
         case "package":
           this.parsePackageLine(line, parts);
           break;
@@ -135,6 +147,15 @@ public class RelayNetworkStatusVoteImpl extends NetworkStatusImpl
         case "contact":
           this.parseContactLine(line, parts);
           break;
+        case "shared-rand-participate":
+          // TODO
+          break;
+        case "shared-rand-commit":
+          // TODO
+          break;
+        case "shared-rand-current-value":
+          // TODO
+          break;
         case "dir-key-certificate-version":
           this.parseDirKeyCertificateVersionLine(line, parts);
           break;
diff --git a/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java b/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java
index 622e9a4..4c5b00b 100644
--- a/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java
@@ -80,6 +80,9 @@ public abstract class ServerDescriptorImpl extends DescriptorImpl
         case "platform":
           this.parsePlatformLine(line, lineNoOpt, partsNoOpt);
           break;
+        case "proto":
+          // TODO
+          break;
         case "published":
           this.parsePublishedLine(line, lineNoOpt, partsNoOpt);
           break;

Note that the code above does not handle any new lines in consensuses (or microdesc-consensuses) that will only be added when enough votes support the new lines. We'll have to look at proposals and/or dir-spec.txt. But the code above might help us identify proposals that we should read.

comment:4 Changed 3 years ago by karsten

Status: newneeds_review

Please review this commit for the first half of the changes above. Shared randomness follows later today, I hope.

comment:5 Changed 3 years ago by karsten

And there's now another commit for the second half of the changes above. Please review!

comment:6 in reply to:  4 ; Changed 3 years ago by iwakeh

Replying to karsten:

Please review this commit for the first half of the changes above. Shared randomness follows later today, I hope.

Tests and checks pass.
For checking the upper limit it would be a little more readable to use 0x1_0000_0000L instead of 4294967296L.
(Just a thought, should Number be used instead of Long as the protocol numbers have unsigned int range?)

comment:7 in reply to:  5 ; Changed 3 years ago by iwakeh

Replying to karsten:

And there's now another commit for the second half of the changes above. Please review!

Tests and checks pass.

For a boolean return value I'd expect "is*" and not "get*", i.e., isSharedRandParticipate instead ofgetSharedRandParticipate.

===

One more comment on the previous commit:

The new private void parse* methods just set a global variable each; why not have the this.* = ParseHelper.parseProtocolVersions(line, line, parts); lines in the switch stmt w/o the method calls?

comment:8 in reply to:  6 ; Changed 3 years ago by karsten

Replying to iwakeh:

Replying to karsten:

Please review this commit for the first half of the changes above. Shared randomness follows later today, I hope.

Tests and checks pass.

Great!

For checking the upper limit it would be a little more readable to use 0x1_0000_0000L instead of 4294967296L.

Changed.

(Just a thought, should Number be used instead of Long as the protocol numbers have unsigned int range?)

Hmm, can you elaborate?

comment:9 in reply to:  7 ; Changed 3 years ago by karsten

Replying to iwakeh:

Replying to karsten:

And there's now another commit for the second half of the changes above. Please review!

Tests and checks pass.

Thanks for checking!

For a boolean return value I'd expect "is*" and not "get*", i.e., isSharedRandParticipate instead ofgetSharedRandParticipate.

Changed.

===

One more comment on the previous commit:

The new private void parse* methods just set a global variable each; why not have the this.* = ParseHelper.parseProtocolVersions(line, line, parts); lines in the switch stmt w/o the method calls?

I'm not sure. I think the switch statement is more readable if we just make one simple method call per line and move the parsing logic to separate methods. And what if this parsing logic ever gets more complicated and requires another line; would we have to move that logic to a new method then? Let's just stick to the current scheme if you don't mind.

comment:10 in reply to:  8 Changed 3 years ago by iwakeh

Replying to karsten:

...

(Just a thought, should Number be used instead of Long as the protocol numbers have unsigned int range?)

Hmm, can you elaborate?

Basically a version is just a positive number and that the spec chose an unsigned int range is probably due to the fact that integer is the usual default chosen in programming, but not that anyone envisions that many versions. To really have the concept of version number in java code a subclass of Number would be necessary that only accepts positive values, but that would be too much implementation for hardly any benefit. Still, using metrics-lib api I might feel better to call intValue() on Number rather than on Long (the highest version I saw so far was 4). But, that is a little 'esoteric' maybe. As said just a thought. I don't feel strongly about this.

comment:11 in reply to:  9 Changed 3 years ago by iwakeh

Replying to karsten:

...

The new private void parse* methods just set a global variable each; why not have the this.* = ParseHelper.parseProtocolVersions(line, line, parts); lines in the switch stmt w/o the method calls?

I'm not sure. I think the switch statement is more readable if we just make one simple method call per line and move the parsing logic to separate methods. And what if this parsing logic ever gets more complicated and requires another line; would we have to move that logic to a new method then? Let's just stick to the current scheme if you don't mind.

The switch is quite long and having the one line assignment there and method calls only when there is more to be done would save an unfamiliar reader from checking the method call to find that it just hides an assigment, but I don't feel strongly about this. And, you're right that the newly created methods just continue the existing scheme. Maybe, we should try to have a different approach in new classes :-)

comment:12 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

comment:13 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Okay, I see your point on both the version numbers topic and the one-line methods. But let's not overengineer/overthink this. Squashed and merged. Thanks for the review! Closing.

Note: See TracTickets for help on using tickets.