Opened 2 years ago

Last modified 19 months ago

#20284 needs_revision defect

consensus weight case 2b3 does not follow dir-spec

Reported by: pastly Owned by: pastly
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth weight-calculation spec-conformance
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

dir-spec says the following.

If M > T/3, then the Wmd weight above will become negative. Set it to 0
in this case:
   Wmd = 0
   Wgd = weight_scale - Wed

The code dutifully sets Wmd to 0, but neglects Wgd.

I assume the spec is correct and the intended behavior. Branch incoming once I get a ticket number.

Child Tickets

Change History (15)

comment:1 Changed 2 years ago by pastly

Status: newneeds_review

comment:2 Changed 2 years ago by teor

Cc: mikeperry added
Milestone: Tor: 0.3.0.x-final
Status: needs_reviewmerge_ready

This looks like a simple fix, and it seems to work, but I'm hesitant to apply it late in 0.2.9.

comment:3 Changed 2 years ago by nickm

Status: merge_readyneeds_review

This would be trouble if we merged it as-is: it changes networkstatus_compute_bw_weights_v10, which is invoked from networkstatus_compute_consensus. We need all the authorities who are using the same consensus_method to produce the same output for the consensus, or else it won't be a consensus.

As a fix, I would suggest having networkstatus_compute_bw_weights_v10() take an argument that says what the actual consensus_method is, and allocating a new consensus_method for doing this bandwidth calculation correctly. Also we'd need to document the old way and the new way in dir-spec.txt

Also, mikeperry did most of the work on these equations; I'm hoping we can get his feedback about whether he believes more in the code or in dir-spec.txt.

comment:4 Changed 2 years ago by nickm

Keywords: review-group-11 added

comment:5 Changed 2 years ago by nickm

Owner: set to pastly
Status: needs_reviewassigned

setting owner

comment:6 Changed 2 years ago by nickm

Status: assignedneeds_review

comment:7 Changed 2 years ago by pastly

I've made the changes nickm suggests on this new branch.

https://github.com/pastly/public-tor/tree/ticket20284-v2

Maybe it's just me, but my changes seem rather ugly. I think it's because I want to keep the structure of the code matching dir-spec so one can read along.

I'm wide open for suggestions if others feel this isn't merge-able.

We also haven't heard from mikeperry yet about his thoughts regarding code vs. dir-spec.

comment:8 Changed 2 years ago by pastly

Oh, and this doesn't help #20285, a relevant ticket but not necessarily dependent ticket.

comment:9 Changed 2 years ago by teor

Status: needs_reviewneeds_revision

I am concerned that we are turning off the unit tests for networkstatus_compute_bw_weights_v10 for consensus methods < 27, when we will continue to use consensus methods < 27 in the live network for some time to come.

I suggest that you run the unit test at least twice: once for a consensus method < 27, and another for >= 27. You could use the arg parameter to test_dir_networkstatus_compute_bw_weights_v10 to do this.

There is a passthrough setup method you can add to the unit test table at the end of the file. It will let you declare two tests that run the same function, and pass them different values for the argument.

comment:10 Changed 2 years ago by teor

Also, let's make sure we include a unit test case where the output is different before and after this change. We don't appear to have one at the moment.

comment:11 Changed 2 years ago by teor

I wonder if the name networkstatus_compute_bw_weights_v10 is confusing after this change.
What it used to mean was: 'compute bandwidth weights using the method introduced at consensus method 10'. Now it means: 'if the consensus_method parameter is < 27, use the method introduced at consensus method 10, otherwise, use the method introduced at consensus method 27'.

Maybe we could drop the v10 from the name, and document those semantics in the function header?

comment:12 Changed 2 years ago by nickm

Keywords: review-group-11 removed

comment:13 Changed 2 years ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: unspecified

Triaged out on December 2016 from 030 to unspecified

comment:14 Changed 20 months ago by nickm

Keywords: triage-out-030-201612 removed

comment:15 Changed 19 months ago by nickm

Keywords: tor-dirauth weight-calculation spec-conformance added
Note: See TracTickets for help on using tickets.