Report version of bwscanners in votes

changed milestone to %Tor: 0.3.5.x-final

added 035-triaged-in-20180711 component::core tor/tor milestone::Tor: 0.3.5.x-final parent::25925 priority::medium resolution::implemented reviewer::teor severity::normal status::closed tor-bwauth type::enhancement labels

Trac:
Cc: N/A to karsten, aagbsn

Trac:
Priority: normal to major

Trac:
Owner: mikeperry to aagbsn
Status: new to assigned

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

Trac:
Reviewer: N/A to N/A
Severity: N/A to Blocker
Parent: N/A to #13630 (moved)
Sponsor: N/A to N/A

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

Trac:
Severity: Blocker to Normal
Priority: High to Medium

aagbsn was the default owner, unassigning

Trac:
Owner: aagbsn to N/A

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

Trac:
Status: assigned to new

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

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

I think we should:

add a "timestamp=" to the timestamp
turn the entire bwfile header into a single line
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.

Trac:
Cc: karsten, aagbsn to juga, karsten, aagbsn
Component: Core Tor/Torflow to Core Tor/Tor
Milestone: N/A to Tor: unspecified

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 (moved) and https://github.com/torproject/torspec/pull/4

I think we should:

add a "timestamp=" to the timestamp

turn the entire bwfile header into a single line

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.

Trac:
Parent: #13630 (moved) to 25925

Trac:
Parent: 25925 to #25925 (moved)

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?.

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.

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
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
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.

Trac:
Milestone: Tor: unspecified to Tor: 0.3.5.x-final

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?.

Trac:
Keywords: N/A deleted, tor-dirauth added

Trac:
Status: new to needs_review

Replying to juga:

This code break other tests, hints?. solved.

Trac:
Keywords: tor-dirauth deleted, tor-bwauth added

Trac:
Reviewer: N/A to teor

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

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

Please see my comments on the GitHub pull request.

Trac:
Status: needs_review to needs_revision

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.

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.

Trac:
Status: needs_revision to needs_review

Please see my comments on the patch.

Trac:
Status: needs_review to needs_revision

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

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

Trac:
Status: needs_revision to needs_review

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?

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.

Trac:
Keywords: N/A deleted, 035-triaged-in-20180711 added

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 (moved).

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

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 (moved).

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

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 (moved), 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

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

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

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.

Trac:
Status: needs_review to merge_ready

Appveyor CI was successful. Let's merge!

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.

Trac:
Status: merge_ready to needs_revision

Replying to nickm:

Questions, not necessarily requring changes:

Do we envision ever having a private field in the bwauth file?

AFAIK, we have not talk about it. What do you mean by "private"?, that would be for?

And is this "terminator" a change to the existing bandwidth file format?

No, are you asking that because the name is different?, we changed the name cause we thought it was less confussing: https://github.com/torproject/tor/pull/194#discussion_r199334483

Does anything use MAX_BW_FILE_HEADERS_LINE_LEN?

It is used in: https://github.com/torproject/tor/pull/241/files#diff-c93ae3ef7d4ec1024de706403d5ab13eR282 We thought that we should check not only the length of the headers smartlist but also the length of the headers string. Regarding the name, see discussion in https://github.com/torproject/tor/pull/194#discussion_r199345734

Necessary changes:

This branch needs a changes file.

ok, will add it

Replying to juga:

Replying to nickm:

Questions, not necessarily requring changes:

Do we envision ever having a private field in the bwauth file?

AFAIK, we have not talk about it. What do you mean by "private"?, that would be for?

I mean, would we ever want to have a field that would be part of the bandwidth file, but not copied into the vote?

And is this "terminator" a change to the existing bandwidth file format?

No, are you asking that because the name is different?, we changed the name cause we thought it was less confussing: https://github.com/torproject/tor/pull/194#discussion_r199334483

Okay, that makes sense.

Does anything use MAX_BW_FILE_HEADERS_LINE_LEN?

It is used in: https://github.com/torproject/tor/pull/241/files#diff-c93ae3ef7d4ec1024de706403d5ab13eR282 We thought that we should check not only the length of the headers smartlist but also the length of the headers string. Regarding the name, see discussion in https://github.com/torproject/tor/pull/194#discussion_r199345734

Necessary changes:

This branch needs a changes file.

ok, will add it

Thanks! Please make this merge_ready again you do.

Added changes file

Trac:
Status: needs_revision to merge_ready

Replying to nickm:

I mean, would we ever want to have a field that would be part of the bandwidth file, but not copied into the vote?

i can not find a reason for that right now. I guess if we need it at some point, we would need to change the spec and this code again.

Replying to juga:

Replying to nickm:

I mean, would we ever want to have a field that would be part of the bandwidth file, but not copied into the vote?

i can not find a reason for that right now. I guess if we need it at some point, we would need to change the spec and this code again.

The current headers don't contain any sensitive information: https://gitweb.torproject.org/torspec.git/tree/bandwidth-file-spec.txt#n105

Also, we want to make the entire bandwidth file public, see #21377 (moved) and: https://gitweb.torproject.org/torspec.git/tree/proposals/296-expose-bandwidth-files.txt

But before we make the entire file public, we need to work out if the individual relay bandwidths make it easier to run attacks, see #26904 (moved).

If you'd like, we could update the bandwidth file spec to say that the headers and the file content are public.