Opened 5 years ago

Closed 4 years ago

#6872 closed defect (fixed)

'bwweightscale' pram missing from spec

Reported by: atagar Owned by: nickm
Priority: Very Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The 'bandwidth-weights' attribute mentions that its values are divided by the 'bwweightscale' param which defaults to 10000 (I'm not sure what meaning "BW_WEIGHT_SCALE" has - guess it's the name of the constant in tor's codebase?)...

https://gitweb.torproject.org/torspec.git/blob/HEAD:/dir-spec.txt#l1482

The network status document's "params" section should include 'bwweightscale' and the default value.

https://gitweb.torproject.org/torspec.git/blob/HEAD:/dir-spec.txt#l1228

On a side note it's pointless for 'bandwidth-weights' to say that the entries are sorted. The above shows the order that entries must appear in.

I'm assuming that new "Wxx=" values are invalid since the spec explicitly lists what it contains and makes no allowances for new ones. If this is incorrect then we should fix that too.

Cheers! -Damian

Child Tickets

Change History (15)

comment:1 Changed 5 years ago by atagar

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by nickm

  • Cc mikeperry added

Seems right to me. Mike, please have a quick look?

comment:3 Changed 5 years ago by mikeperry

These changes look fine.

As for how we might add more Wxx values, I am not sure. IIRC, making them sorted made it easier to reuse most of the existing consensus parameter code for dealing with them? It certainly simplified something I was doing, at any rate.. If you have a new value in mind I can go digging to refresh my memory.

comment:4 Changed 5 years ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

merging

comment:5 Changed 5 years ago by atagar

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening with a patch as discussed in irc (again on my 6872 branch). Please see the commit description for more information...

https://gitweb.torproject.org/user/atagar/torspec.git/commitdiff/f6b3177f78549fcd4705849c79d83a41f3c2f441

Mike: Do you want to allow for negative weights? Are zero values ok? Any upper bound? If the answer to any of these isn't "any Int32 is ok" then now is the time to say so.

Nick: As a person parsing these documents I've liked how localized and well defined they are. For server descriptors we say how additional arguments should be handled...

In lines that take multiple arguments, extra arguments SHOULD be
accepted and ignored.

But extra-info descriptors and network status documents have no such note. This is probably an oversight. I'm happy for the above to be the default behavior for the dir-spec. My suggestion is to move the note from section 2.1 to section 0 (right below the 'The key words "MUST", "MUST NOT"...' note). This is an important parsing detail and putting it anywhere lower will cause it to be lost in the spec's unpleasantly large header.

I haven't given much about the control-spec. Iirc we defined how new parameters should be handled for events (that was the change that prompted the big TorCtl patch that was never merged...) but I'm not sure if this sort of default behavior would cause complications for other sections.

comment:6 Changed 5 years ago by nickm

  • Keywords tor-client added

comment:7 Changed 5 years ago by nickm

  • Component changed from Tor Client to Tor

comment:8 Changed 5 years ago by nickm

Hm. The 'params' change doesn't match what we generate today: we always generate that space. Removing it would require a new consensus method, which probably isn't worth it.

A weight should not ever be allowed to be negative. What would that mean?

comment:9 Changed 5 years ago by nickm

(Other than that, looks ok. Want to revise, or should I?)

comment:10 Changed 5 years ago by atagar

Hm. The 'params' change doesn't match what we generate today: we always generate that space. Removing it would require a new consensus method, which probably isn't worth it.

Gotcha. I was trying to make the params like the bandwidth-weights, but you're right. This would break backward compatibility when Parameters is excluded. Feel free to exclude that tweak.

A weight should not ever be allowed to be negative. What would that mean?

I don't know, but the spec presently doesn't disallow that. If we want to only allow positive values then we should say so.

comment:11 Changed 5 years ago by rransom

The currently defined bandwidth weights must be non-negative. Tor should be able to parse parameters with negative values if any are added in the future.

comment:12 Changed 5 years ago by nickm

  • Milestone set to Tor: 0.2.4.x-final

comment:13 Changed 4 years ago by nickm

  • Owner set to mikeperry
  • Status changed from reopened to assigned

comment:14 Changed 4 years ago by nickm

  • Owner changed from mikeperry to nickm

comment:15 Changed 4 years ago by nickm

  • Resolution set to fixed
  • Status changed from assigned to closed

Okay, cherrypicked and tweaked slightly. Anybody who wants it to be different, please submit a patch.

Note: See TracTickets for help on using tickets.