Opened 7 years ago

Last modified 6 hours ago

#3723 needs_revision enhancement

Report version of bwscanners in votes

Reported by: mikeperry Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-bwauth, 035-triaged-in-20180711
Cc: juga, karsten, aagbsn Actual Points:
Parent ID: #25925 Points:
Reviewer: teor Sponsor:

Description

Each directory authority should list the git version of the bandwidth scanners in their votes. This ticket has two parts:

  1. Edit aggregate.py to output a line with the git commit that it is from.
  1. Alter the tor voting to add an "opt" line or other ignored keyword that lists the version from the bwscan file.

There should be no consensus process for this. We just want a comment in the vote documents, basically.

Child Tickets

TicketTypeStatusOwnerSummary
#25869enhancementclosedBandwidth List Format
#26222enhancementclosedjugaAdd a bandwidth-file line to votes in dir-spec.txt
#26541defectclosedjugaFix minor mistakes in the bandwidth-file dir-spec entry
#26799defectclosedteordir-spec: specify failure modes for the bandwidth-file-headers vote line

Change History (38)

comment:1 Changed 7 years ago by mikeperry

Cc: karsten aagbsn added

comment:2 Changed 7 years ago by mikeperry

Priority: normalmajor

comment:3 Changed 6 years ago by mikeperry

Owner: changed from mikeperry to aagbsn
Status: newassigned

comment:4 Changed 8 months ago by teor

Parent ID: #13630
Severity: Blocker

This is a feature that belongs in the new bwauth replacement project, see #13630.

comment:5 Changed 8 months ago by teor

Priority: HighMedium
Severity: BlockerNormal

Priorities and Severities in torflow are meaningless, setting them all to Medium/Normal.

comment:6 Changed 8 months ago by teor

Owner: aagbsn deleted

aagbsn was the default owner, unassigning

comment:7 Changed 7 months ago by teor

Status: assignednew

Mark all tickets that are assigned to nobody as "new".

comment:8 Changed 3 months ago by teor

Cc: juga added
Component: Core Tor/TorflowCore Tor/Tor
Milestone: Tor: unspecified

Hi juga, this is the ticket for putting the bandwidth authority version in votes.

We are working on the file format in #25869 and https://github.com/torproject/torspec/pull/4

I think we should:

  1. add a "timestamp=" to the timestamp
  2. turn the entire bwfile header into a single line
  3. put it all in the vote as a line with a new keyword, "bandwidth-file"

For bandwidth file format 1.0.0, the line would be:
bandwidth-file timestamp=1234567890

For bandwidth file format 1.1.0, the line would be:
bandwidth-file timestamp=1234567890 version=1.1.0 software=sbws software_version=0.1.0

If we implement the feature in this way, then Tor will automatically add new fields to the bandwidth-file line.

comment:9 in reply to:  8 Changed 3 months ago by teor

This is about the format of the *vote* line, not the format of the v3bwfile header, which is multiple lines.

Replying to teor:

Hi juga, this is the ticket for putting the bandwidth authority version in votes.

We are working on the file format in #25869 and https://github.com/torproject/torspec/pull/4

I think we should:

  1. add a "timestamp=" to the timestamp
  2. turn the entire bwfile header into a single line
  3. put it all in the vote as a line with a new keyword, "bandwidth-file"

For bandwidth file format 1.0.0, the line would be:
bandwidth-file timestamp=1234567890

For bandwidth file format 1.1.0, the line would be:
bandwidth-file timestamp=1234567890 version=1.1.0 software=sbws software_version=0.1.0

If we implement the feature in this way, then Tor will automatically add new fields to the bandwidth-file line.

comment:10 Changed 3 months ago by juga

Parent ID: #1363025925

comment:11 Changed 3 months ago by juga

Parent ID: 25925#25925

comment:12 Changed 8 weeks ago by juga

To which Tor version should we backport this?, 0.3.2 too since it's the ealier version dirauths are running?

Would be the place to put the one i'm using in this commit?: https://github.com/juga0/tor/commit/dd19df3a042e9b9bbf29fdb73d717a78f7dc5e57#diff-141d60fb101694336aa8683a8e2a1541R241

In which structure should the timestamp (and version) be stored so that dirvote.c can read it?.

comment:13 in reply to:  12 Changed 8 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

Replying to juga:

To which Tor version should we backport this?, 0.3.2 too since it's the ealier version dirauths are running?

New features do not get backported, they get merged to master.

Since there is an 0.3.4 alpha release, it is closed to new features.
So the earliest release this feature will be in is 0.3.5.

Would be the place to put the one i'm using in this commit?: https://github.com/juga0/tor/commit/dd19df3a042e9b9bbf29fdb73d717a78f7dc5e57#diff-141d60fb101694336aa8683a8e2a1541R241

That is almost the right place.
Please add the new "bandwidth-file" line to the end of the authority header, after the shared random information.
We add new lines to the end of the section, to avoid merge conflicts.

In which structure should the timestamp (and version) be stored so that dirvote.c can read it?.

Please add every header key_value to the bandwidth-file line, not just timestamp and version.
This means that new header lines are automatically added to the vote.

You can add these key_values to a space-separated string.

I suggest that you modify dirserv_read_measured_bandwidths() to store the headers in a string, add a function that gets that string, and include the string in the vote.

  1. Set the bandwidth-file string to NULL at the start of the function, freeing it if was not NULL:

https://github.com/torproject/tor/blob/1eede00a4bd9a7de2acf77393f2fc57aa3196d08/src/or/dirserv.c#L2584

  1. Once you know the time is a number, read the rest of the header lines, and add them to the bandwidth-file string:

https://github.com/torproject/tor/blob/1eede00a4bd9a7de2acf77393f2fc57aa3196d08/src/or/dirserv.c#L2615

  1. When voting, add a bandwidth-file line if the bandwidth-file string is not NULL:

https://github.com/juga0/tor/commit/dd19df3a042e9b9bbf29fdb73d717a78f7dc5e57#diff-141d60fb101694336aa8683a8e2a1541R210

You will also need to change the dir-spec. I will open a child ticket.

comment:14 Changed 7 weeks ago by juga

Keywords: tor-dirauth added

Since it is not being backported i based it on master, now in my branch ticket3723

After looking more to dirvote.c i thought it was better to use smartlist_t instead of string, and get it converted to string in format_networkstatus_vote.

Since dirserv_read_measured_bandwidths is call from dirserv_generate_networkstatus_vote_obj, i don't need a different function to obtain the headers.

Since i didn't found specific tests for dirserv_generate_networkstatus_vote_obj and format_networkstatus_vote, i'm not sure how to add tests for the vote document.

This code break other tests, hints?.

comment:15 Changed 7 weeks ago by teor

Status: newneeds_review

comment:16 in reply to:  14 Changed 7 weeks ago by juga

Replying to juga:

This code break other tests, hints?.

solved.

comment:17 Changed 6 weeks ago by juga

Keywords: tor-bwauth added; tor-dirauth removed

comment:18 Changed 6 weeks ago by dgoulet

Reviewer: teor

comment:19 Changed 5 weeks ago by teor

I won't have time to review this ticket until next week, I'm still catching up.

comment:20 Changed 3 weeks ago by teor

The GitHub pull request is:
https://github.com/torproject/tor/pull/126

comment:21 Changed 3 weeks ago by teor

Status: needs_reviewneeds_revision

Please see my comments on the GitHub pull request.

comment:22 Changed 3 weeks ago by teor

Are there any unit tests for reading measured bandwidths with non-NULL routerststuses?
If there aren't, please open a separate ticket, we should write some eventually.

comment:23 in reply to:  21 Changed 3 weeks ago by juga

Status: needs_revisionneeds_review

Replying to teor:

Please see my comments on the GitHub pull request.

Rebased to master and continuing on https://github.com/torproject/tor/pull/194. Also left comments there.

comment:24 Changed 3 weeks ago by teor

Status: needs_reviewneeds_revision

Please see my comments on the patch.

comment:25 Changed 3 weeks ago by teor

You'll need to click "see outdated" for the last few comments, because I made them on the commit, not the final code.

comment:26 Changed 3 weeks ago by juga

Status: needs_revisionneeds_review

Rebased to master and continuing on ​https://github.com/torproject/tor/pull/195.

comment:27 in reply to:  26 Changed 3 weeks ago by teor

Replying to juga:

Rebased to master and continuing on ​https://github.com/torproject/tor/pull/195.

When I'm reviewing, I find rebases and new pull requests really confusing. I lose all my past GitHub comments, and I have to switch between the old and new pull requests. (Some people like new pull requests, I've just found out that I don't..)

So can we leave any more rebases until the patch is complete?

comment:28 Changed 11 days ago by juga

Sorry, there were new commits on master touching parts of that code and i thought that you prefered that workflow. Ok, no more rebases on master until is finished.

comment:29 Changed 9 days ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:30 in reply to:  22 Changed 5 days ago by teor

Replying to teor:

Are there any unit tests for reading measured bandwidths with non-NULL routerststuses?
If there aren't, please open a separate ticket, we should write some eventually.

I think this ticket is #25947.

comment:31 Changed 5 days ago by juga

Need to check, but i think that even after all the new tests, there was not any with non-NULL routerstatuses in #25947.

comment:32 in reply to:  31 Changed 5 days ago by teor

Replying to juga:

Need to check, but i think that even after all the new tests, there was not any with non-NULL routerstatuses in #25947.

Ok, I opened #26798 for these tests. They are a low priority. Please do all the 0.3.5 work first.

comment:33 in reply to:  26 Changed 5 days ago by teor

Replying to juga:

Rebased to master and continuing on ​https://github.com/torproject/tor/pull/195.

Hi Juga,

Please see my branch ticket3723_03 on https://github.com/teor2345/tor.git

I did some small fixups:

  • removed a redundant header,
  • added a missing cast on Windows, and
  • fixed some code that was in the wrong place.

I also wrote a spec for parsing failures in #26799, and implemented it.

Please review https://github.com/torproject/tor/pull/231

If you're happy with the code, please:

  • run it against a test bandwidth file, and check the vote contents
  • squash all your fixups (and my fixups, if you want)
  • rebase to the latest master

comment:34 Changed 4 days ago by teor

It looks like the squashed and rebased code is in juga's ticket3723_03_squashed_rebased branch.

comment:35 Changed 3 days ago by juga

It looks like the squashed and rebased code is in juga's ticket3723_03_squashed_rebased branch.

It is, i didn't notify here because i wanted first to:

run it against a test bandwidth file, and check the vote contents

i did it, example output:

bandwidth-file-headers timestamp=1531683804 version=1.1.0 earliest_bandwidth=2018-07-13T12:49:16 file_created=2018-07-17T13:13:03 generator_started=2018-07-14T15:14:35 latest_bandwidth=2018-07-15T19:43:24 software=sbws software_version=0.6.1-dev

comment:36 Changed 3 days ago by teor

Status: needs_reviewmerge_ready

Linux CI passed on this branch.
I opened ​https://github.com/torproject/tor/pull/241 so that Windows CI runs on this branch.
You can set up Windows CI on your GitHub by going to https://ci.appveyor.com/

After it's successful, let's merge the branch.

comment:37 Changed 3 days ago by teor

Appveyor CI was successful. Let's merge!

comment:38 Changed 6 hours ago by nickm

Status: merge_readyneeds_revision

Questions, not necessarily requring changes:

  • Do we envision ever having a private field in the bwauth file?
  • And is this "terminator" a change to the existing bandwidth file format?
  • Does anything use MAX_BW_FILE_HEADERS_LINE_LEN?

Necessary changes:

  • This branch needs a changes file.
Note: See TracTickets for help on using tickets.