Missing relay keys in bandwidth file spec

Not in spec or examples:

• relay_in_recent_consensus_count
• consensus_bandwidth
• consensus_bandwidth_is_unmeasured
• desc_bw_avg
• desc_bw_bur
• desc_bw_obs_last
• desc_bw_obs_mean

Not in spec:

• relay_recent_measurement_attempt_count
• relay_recent_priority_list_count

We need a specification for these keys so we know what they mean.

changed milestone to %sbws: 1.1.x-final

Trac:
Parent Ticket: #33121 (moved)

added component::core tor/tor milestone::sbws: 1.1.x-final parent::33121 points::6 priority::high resolution::fixed reviewer::ahf reviewer::gk sbws-roadmap sbws-spec severity::major status::closed type::defect labels

Trac:
Points: N/A to 6
Keywords: sbws-spec deleted, sbws-spec sbws-roadmap-september added

Changing keyword of roadmapped open sbws tickets to a general sbws-roadmap one.

Trac:
Keywords: sbws-spec sbws-roadmap-september deleted, sbws-roadmap, sbws-spec added

The goal is to deploy sbws in all bw authorities. We need to fix critical bugs to do this.

Trac:
Parent: N/A to #33121 (moved)

putting this ticket to needs review because i've some questions.

i realized that there are keys in the spec like desc_bw_average, that was never named like that in sbws, it was named desc_bw_avg, that's why the last one is missing in the spec, it was a mistake when writing the spec.

Should we leave them as they are for version 1.2 and add the correct names in version 1.5? ,and remove desc_bw_average from the sbws examples in the Annexes?

and the keys that were missing, should we also say that we add them in version 1.5 even if they were in sbws time ago?

Trac:
Status: new to needs_review
Reviewer: N/A to ahf, gk

This is about the spec, so component is Tor

Trac:
Component: Core Tor/sbws to Core Tor/Tor

Trac:
Cc: juga to juga, gk

Replying to juga:

putting this ticket to needs review because i've some questions.

i realized that there are keys in the spec like desc_bw_average, that was never named like that in sbws, it was named desc_bw_avg, that's why the last one is missing in the spec, it was a mistake when writing the spec.

Should we leave them as they are for version 1.2 and add the correct names in version 1.5? ,and remove desc_bw_average from the sbws examples in the Annexes?

and the keys that were missing, should we also say that we add them in version 1.5 even if they were in sbws time ago?

So, all of those examples look to me like spec bugs: the right thing happened in the code, but the spec does not reflect that and thus needs to get updated. If that's the case we should just fix the spec without the need for bumping the version. That goes both for missing as well as wrongly spelled keys. The missing ones, too, should get added to the spec with the first version in which they appeared (as all the other ones that we did not forget).

Re: the examples. Yes, if we change desc_bw_average to desc_bw_avg (and do not say the bug is actually desc_bw_avg in the code which should be desc_bw_average as it is written in the spec) then we should fix up the examples as well in the Annexes (I don't know where those are but even without looking at them in detail it makes sense to fix the examples :)).

Let's change this to revision until i create a PR

Trac:
Status: needs_review to needs_revision

Replying to gk:

So, all of those examples look to me like spec bugs: the right thing happened in the code, but the spec does not reflect that and thus needs to get updated.

Yes. And i think the source of true for the version and when the KeyValues appeared should be the bandwidth files published by the bwauths, because with sbws happens that:

• we change KeyValues
• we change the specification version in sbws
• we make a new sbws release (and usually bwauths have run only releases, except longclaw a couple of times)
• the new version spec and the KeyValues start to appear in the bandwidth files
• we change some more KeyValues (if there's no bwauth running a non-released version, they're still publishing the last specification version
• new specification change, new sbws release, bwauths update sbws version (and therefore KeyValues and specification version)...

So i should check the collector to be sure about the which keys were in which versions

Replying to juga:

Replying to gk:

So, all of those examples look to me like spec bugs: the right thing happened in the code, but the spec does not reflect that and thus needs to get updated.

Yes. And i think the source of true for the version and when the KeyValues appeared should be the bandwidth files published by the bwauths, because with sbws happens that:

• we change KeyValues
• we change the specification version in sbws
• we make a new sbws release (and usually bwauths have run only releases, except longclaw a couple of times)
• the new version spec and the KeyValues start to appear in the bandwidth files
• we change some more KeyValues (if there's no bwauth running a non-released version, they're still publishing the last specification version
• new specification change, new sbws release, bwauths update sbws version (and therefore KeyValues and specification version)...

So i should check the collector to be sure about the which keys were in which versions

Sounds good.

Replying to gk:

So i should check the collector to be sure about the which keys were in which versions

Sounds good.

I checked and all the collected bandwidth files started with the specification version 1.4.0, so in versions earlier than that it does not really matter the KeyValues are not correct.

Still, i fixed them in their respective versions, including the version 1.3.0 that was never generated by sbws (it should have been other ticket, but the change is small).

https://github.com/torproject/torspec/pull/110

What i should still check is whether we need to open tickets to metrics and stem because of the change of version in the spec. I don't think so because there were no bandwidth files published earlier than 1.4.0.

Trac:
Status: needs_revision to needs_review

Replying to juga:

Replying to gk:

So i should check the collector to be sure about the which keys were in which versions

Sounds good.

I checked and all the collected bandwidth files started with the specification version 1.4.0, so in versions earlier than that it does not really matter the KeyValues are not correct.

Still, i fixed them in their respective versions, including the version 1.3.0 that was never generated by sbws (it should have been other ticket, but the change is small).

https://github.com/torproject/torspec/pull/110

Looks mostly good to me. I have some comments/nits mainly for 0b6e97ca37b60fbfbd628306ec4b0551b34142cc:

4ec93f2d8b2add1da598d927785ed7d8e60f0ec3: looks good

0b6e97ca37b60fbfbd628306ec4b0551b34142cc:

• desc_bw_bur, consensus_bandwidth, and consensus_bandwidth_is_unmeasured are all in v1.0.3 released which is using SPEC_VERSION 1.2.0. Thus, those KeyValues are already in that spec version available
• in the commit message s/They appeared in in/They appeared in/
• s/the consensus bandwdith/the consensus bandwidth/
• In the new "in the last data_period days" parts the default value for "data_period days" is not mentioned anymore as it is in previous KeyValue explanations. Maybe add a hint to the default one, here too
• In relay_recent_priority_list_count there is no "in the last data_period days" but there should be, shouldn't it (at least that's what v3bwfile.py is indicating)?
• "Add more KeyValues": I think it might make sense to use "Adds more KeyValues" in the sense that a particular spec version adds those things. I am a bit confused though, as the explanations to previous spec versions alternate between "Add" and "Adds" without some pattern. You could fix that part up in a follow-up commit if you think we should do that.

fb36a2bae2bc0aa1f0dfbfa68fbb903a5ee6d193: looks good (modulo the previously mentioned "Adds"/"Add" inconsistency; could be part of a follow-up fix as well)

What i should still check is whether we need to open tickets to metrics and stem because of the change of version in the spec. I don't think so because there were no bandwidth files published earlier than 1.4.0.

I agree that it should not be needed from my understanding but, yes, could be worth double-checking, though.

Trac:
Status: needs_review to needs_revision

Thanks, think i addressed what you comment, except the commit message cause i pushed fixups. I should remember to change that when squashing.

Trac:
Status: needs_revision to needs_review

Replying to gk:

What i should still check is whether we need to open tickets to metrics and stem because of the change of version in the spec. I don't think so because there were no bandwidth files published earlier than 1.4.0.

I agree that it should not be needed from my understanding but, yes, could be worth double-checking, though.

stem parsers only the headers and the not matching version is only a comment (https://gitweb.torproject.org/stem.git/tree/stem/descriptor/bandwidth_file.py#n127).

Replying to juga:

Thanks, think i addressed what you comment, except the commit message cause i pushed fixups. I should remember to change that when squashing.

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Replying to juga:

Replying to juga:

Thanks, think i addressed what you comment, except the commit message cause i pushed fixups. I should remember to change that when squashing.

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Thanks. Just one final nit: s/days.(unless/days. (Unless/

Trac:
Status: needs_review to needs_revision

Replying to gk:

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Thanks. Just one final nit: s/days.(unless/days. (Unless/

Done, squashed now is PR 113

Trac:
Status: needs_revision to needs_review

Replying to juga:

Replying to gk:

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Thanks. Just one final nit: s/days.(unless/days. (Unless/

Done, squashed now is PR 113

Looks good now, thanks!

Trac:
Status: needs_review to merge_ready

Merged!

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

closed

changed time estimate to 48h

mentioned in issue #33121 (moved)

moved to tpo/core/tor#30726 (closed)