Opened 8 years ago

Closed 6 years ago

#2286 closed enhancement (implemented)

We still use self-published relay bandwidth sometimes

Reported by: arma Owned by: andrea
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: arma-cares needs-proposal tor-auth
Cc: mikeperry, aagbsn, ln5 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There's a gap between when a new relay shows up and when there are enough Measured votes from the directory authorities that we use the measured bandwidth in the consensus. So all the various papers that talk about "just run a tiny relay and advertise it as a huge relay" are not actually solved yet -- their algorithm should just be modified to add "for a few days".

(Actually, this bug isn't quite as bad as it could be, since you need to be around for 3 or 5 days in order to get the Guard flag. I wonder how quickly measurements are ready in general, i.e. who wins the race here.)

I suggest three fixes:

A) There should be a quite low cap (e.g. 50KB) for bandwidth weightings in the consensus if there aren't enough Measured votes. I wonder if the 50KB should be a fixed number (in which case we could just have the directory authorities vote it, and not need to change the consensus method), or a function of the overall numbers in the consensus (which would require a new consensus method, but could pick a smarter cap given that some relays have really bloated bandwidth weights).

A') Somebody should evaluate how much of our overall capacity we'd be cutting, and what effect this cutting has on our entropy.

B) To reduce the harm to the network (since new relays would be contributing much less), we should teach the bwauth scripts to measure new relays more aggressively, to shorten this window.

C) The longer term solution is that we need to integrate Robin and Nikita's secure bandwidth estimation stuff -- right now Mike's bwauth scripts believe your self-advertised number and then tweak it based on what they see, so you'll still get a much higher number if you self-advertise a much higher number. Open research question how to improve security here without sacrificing too much accuracy and without adding too much load to the network.

Added with 0.2.3.x as the milestone, since we should do it pretty soon but we can make the change in the directory authorities so timing isn't critical.

Child Tickets

TicketTypeStatusOwnerSummary
#6133enhancementclosedaagbsnbwauths need to let us know when they've been running long enough

Attachments (1)

consensus-weights-not-measured.png (105.9 KB) - added by karsten 7 years ago.
Fraction of consensus bandwidth weights that are NOT based on measured bandwidths (updated)

Download all attachments as: .zip

Change History (67)

comment:1 in reply to:  description Changed 8 years ago by rransom

Replying to arma:

C) The longer term solution is that we need to integrate Robin and Nikita's secure bandwidth estimation stuff

2010-12-12 18:25 <armadev> http://hatswitch.org/~nikita/papers/tuneup-tdsc.pdf
2010-12-12 18:26 <armadev> please add that url to the trac

comment:2 Changed 8 years ago by mikeperry

Actually the distributed bandwidth estimation is called EigenSpeed:
http://www.usenix.org/event/iptps09/tech/full_papers/snader/snader.pdf

We also need to be careful about both not using this cap when there are *no* measured values, and make sure that the bandwidth authorities do not have special code to deal with unmeasured relays already.

comment:3 Changed 8 years ago by arma

Component: Tor RelayTor Directory Authority

comment:4 in reply to:  description Changed 7 years ago by karsten

Replying to arma:

(Actually, this bug isn't quite as bad as it could be, since you need to be around for 3 or 5 days in order to get the Guard flag. I wonder how quickly measurements are ready in general, i.e. who wins the race here.)

I think the time for a new relay to get the Guard flag is more like 16 days (or 8 days Weighted Time). Here are the revised requirements from dir-spec.txt:

"A router is a possible ‘Guard’ [if it is ‘familiar’,] if its Weighted Fractional Uptime is at least the median for ‘familiar’ active routers [or its Weighted Fractional Uptime is at least 98 %], and if its [advertised] bandwidth is at least median [of all active routers] or at least 250KB/s. [...] A node is ‘familiar’ if 1/8 of all active nodes have appeared more recently than it, or it has been around for [a weighted time of 8 days]."

See pages 2 and 3 of An Analysis of Tor Relay Stability.

Of course, a relay could show up for a few hours without being measured, disappear for 16 days, and come back with a huge advertised bandwidth that we'll have to believe, because we don't have measurements. Not sure how the bandwidth scanners would handle that case.

comment:5 Changed 7 years ago by arma

Keywords: arma-cares added

comment:6 in reply to:  description Changed 7 years ago by arma

Replying to arma:

B) To reduce the harm to the network (since new relays would be contributing much less), we should teach the bwauth scripts to measure new relays more aggressively, to shorten this window.

Opened this one as #4359.

comment:7 Changed 7 years ago by mikeperry

Cc: mikeperry added

My vote is on option A here. As I explain in https://trac.torproject.org/projects/tor/ticket/4359#comment:1, I think it's not useful to add excessive complexity to the bandwidth auths for measurement security.. We already know that's not a winnable game for centralized measurements.

However, we will need some complexity in them even to support option A.. The bandwidth auths will need to know not to treat the gimped consensus bw as a real value for purposes of feedback... So we'll either need some sort of magic number or some other way to signify it..

comment:8 Changed 7 years ago by mikeperry

Type: defectproject

Changing to project because trac is hating on #4359 being a child of a defect. That might be a hate crime, trac.

comment:9 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Type: projectenhancement

Didn't make the 0.2.3.x feature cutoff. :(

comment:10 Changed 7 years ago by arma

Type: enhancementdefect

To be clear, this is a security vulnerability, not an enhancement.

comment:11 Changed 7 years ago by arma

I would even go so far as to say it's a major security vulnerability.

comment:12 Changed 7 years ago by nickm

Could you summarize which of the approaches you actually favor then, and what the remaining work is?

comment:13 Changed 7 years ago by arma

I favor all of the approaches. It is not a menu.

That said, we can get some good mileage just by doing A and A'.

Step zero is A': write a little tool to look through the consensus and the votes, distinguish between relays whose weight is a result of bwauth votes and those whose weight is just using the number from the descriptor, and see what fraction of the guard / exit / total weights we're talking about. My guess is it's a tiny fraction, and then we can proceed with A.

comment:14 Changed 7 years ago by nickm

So, the idea for A would need a quick spec IIUC; are you suggesting that:

  • 1: we should cap the consensus bandwidth if it comes down to a vote between non-measuring authorities. (New consensus method needed.)
  • 2: we should cap the amount of bandwidth an authority will vote that a router has, if it's not measuring. (No new consensus method needed.)
  • 3: something else?

I can implement either one. I would really like somebody else to do A'. I have enough crazy stuff to do.

comment:15 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final
Status: newneeds_review

To be more constructive: i'm leaning slightly towards 1, with a new consensus method.

To describe it in full, see branch "bug2286_part_a" in my public torspec repo. To see an (untested!) implementation, please review branch "bug2286_part_a" in my public tor repo.

comment:16 Changed 7 years ago by karsten

Here's a quick analysis of consensus bandwidth weights that are not based on measured bandwidths. A relay falls into this category if it didn't have 3 or more Measured values in the votes that the consensus was based on. When looking at the relay count, this fraction is at 0.03%, the fraction of total consensus weights is at 1.4%.

comment:17 Changed 7 years ago by nickm

Sounds like my branch might be a good idea then; please review?

comment:18 in reply to:  16 ; Changed 7 years ago by arma

Replying to karsten:

Here's a quick analysis of consensus bandwidth weights that are not based on measured bandwidths. A relay falls into this category if it didn't have 3 or more Measured values in the votes that the consensus was based on. When looking at the relay count, this fraction is at 0.03%, the fraction of total consensus weights is at 1.4%.

.03% of the relays is 1 in 3333. Are you sure you don't have "relays" and "weights" swapped in your graph?

comment:19 in reply to:  15 Changed 7 years ago by arma

Replying to nickm:

To describe it in full, see branch "bug2286_part_a" in my public torspec repo.

Am I reading your branch wrong, or is there no relevant commit here?

To see an (untested!) implementation, please review branch "bug2286_part_a" in my public tor repo.

I'm afraid we'll need a bit more complexity here -- for starters, we don't want to cap everybody when there aren't three measuring authorities. I wonder if it would be simpler just to have bwauths vote at-most-50KB on relays they don't have an opinion about. That would also make it really easy for us to adapt it to be vote-at-most-.1% in the future. The downside is that it would now be harder to recognize whether the vote is an actual vote or a cap vote. We could have them say something else on the w line if it's a cap vote, to distinguish. Starts to get a bit klunky though.

comment:20 Changed 7 years ago by arma

See branch bug2286_parta_v2 in arma's arma for what I meant by the alternate approach.

comment:21 Changed 7 years ago by arma

I just force-pushed a revised version.

comment:22 Changed 7 years ago by arma

Here are some statistics, with moria1 running my patch:

It's voting Running about 2920 relays.

It votes Measured lines about 2541 of those 2920. So ~13% of its Running relays are unmeasured.

Total weight of capped relays was 29098, compared to total overall weight of 2392006. Or ~1.2%.

comment:23 Changed 7 years ago by arma

Now, my patch has another side effect: it drags down the voted weights when some of the bwauths haven't measured a relay yet but some have. I'm not sure how big a deal that is.

comment:24 Changed 7 years ago by mikeperry

Cc: aagbsn added

Also, the way the bandwidth authorites work is to group nodes with similar values for the ratio (consensus_bw / descriptor_bw) for measurement together in 2 hop paths. So if we set this to like 50 for unmeasured nodes, they will currently get grouped with the slowest measured nodes on the network. That might be a hard sinkhole to get out of.

We could just hack the bw auths to treat this "50" value special, or something... But then how do we tell real 50 from "you're not measured" 50?

comment:25 in reply to:  24 ; Changed 7 years ago by arma

Replying to mikeperry:

Also, the way the bandwidth authorites work is to group nodes with similar values for the ratio (consensus_bw / descriptor_bw) for measurement together in 2 hop paths. So if we set this to like 50 for unmeasured nodes, they will currently get grouped with the slowest measured nodes on the network. That might be a hard sinkhole to get out of.

Are you talking about the present case where we use the descriptor bandwidth (in which case this 50 doesn't matter), or the future case where you're hoping to feed back the consensus weight into the new weight?

Even in that future case, maybe we should have a bwauth use the descriptor bandwidth if it has no measurement of its own yet?

comment:26 in reply to:  25 ; Changed 7 years ago by mikeperry

Replying to arma:

Replying to mikeperry:

Also, the way the bandwidth authorites work is to group nodes with similar values for the ratio (consensus_bw / descriptor_bw) for measurement together in 2 hop paths. So if we set this to like 50 for unmeasured nodes, they will currently get grouped with the slowest measured nodes on the network. That might be a hard sinkhole to get out of.

Are you talking about the present case where we use the descriptor bandwidth (in which case this 50 doesn't matter), or the future case where you're hoping to feed back the consensus weight into the new weight?

Present case. Even without feedback, the pairings of nodes during measurement is decided based on the ratio of consensus to descriptor bandwidth... So the 50 will matter now, until we find an alternate way to handle it for grouping.

Changed 7 years ago by karsten

Fraction of consensus bandwidth weights that are NOT based on measured bandwidths (updated)

comment:27 in reply to:  18 Changed 7 years ago by karsten

Replying to arma:

.03% of the relays is 1 in 3333. Are you sure you don't have "relays" and "weights" swapped in your graph?

You're right, I messed things up. I just fixed the script and re-ran it. See the updated graph.

comment:28 in reply to:  26 Changed 7 years ago by arma

Replying to mikeperry:

Present case. Even without feedback, the pairings of nodes during measurement is decided based on the ratio of consensus to descriptor bandwidth... So the 50 will matter now, until we find an alternate way to handle it for grouping.

Oh. So both my patch and Nick's patch will make this a problem for you.

Can we make it so the bwauth ignores the consensus weight (i.e. calls the ratio 1) for relays that it has no local opinion about yet? That won't be perfect, since a given bwauth could form an opinion even though the other bwauths are voting 50, and then that bwauth will believe the consensus weight next time.

My patch makes moria1 vote Capped=1 on the w line when it capped. But that doesn't make it into the consensus. But it could if we wanted to make a new consensus method.

comment:29 Changed 7 years ago by nickm

Roger, I don't get at this point why you prefer your alternative approach (vote differently) vs the one I started to implement (consense differently). I kind of prefer the latter, since it doesn't change the meaning of any vote, and it looks like we'll probably need a new consensus method anyway.

Mike, assuming that we're going to do _something_ so that the output of the consensus changes here such that nodes can't get high bandwidth in the consensus when they're not measured -- what *would* work to keep measurements happy?

comment:30 in reply to:  29 Changed 7 years ago by mikeperry

Replying to nickm:

Mike, assuming that we're going to do _something_ so that the output of the consensus changes here such that nodes can't get high bandwidth in the consensus when they're not measured -- what *would* work to keep measurements happy?

I think some way to explicitly say that a relay has not been measured is the best plan. I think a new consensus method with "Capped=1" could be workable. Patching the bandwidth authorities for that shouldn't be terribly hard, I think (though I am speaking from memory here).

To bikeshed a little: Maybe Capped is the wrong keyword. We have other bandwidth caps we used to do, and we may want to cap in the other direction for other reasons.. Perhaps just Unmeasured=1 instead?

comment:31 in reply to:  29 ; Changed 7 years ago by arma

Replying to nickm:

Roger, I don't get at this point why you prefer your alternative approach (vote differently) vs the one I started to implement (consense differently). I kind of prefer the latter, since it doesn't change the meaning of any vote, and it looks like we'll probably need a new consensus method anyway.

I'm open to either approach. We'll want to make sure we don't cap all nodes to 50 in the case where there aren't enough bwauths putting in Measured votes. We may also find that we want to only cap weights when a certain threshold of the relays have a Measured vote. I'm not sure if we'll want to do that threshold, or what we'll want it to be, so I had figured leaving the flexibility to each authority might make it easier. For example, I worry about a case where all the bwauths restart afresh and have just a few votes each -- currently it makes sense for them to include the votes, but now they'd better think about not putting in any votes until they have quite a few.

That said, we also want to avoid an attack where the adversary floods the network with new nodes, and suddenly the bwauths take away their caps because they haven't covered the required threshold of the nodes yet.

*That* said, now that it looks like we're going to be making a new consensus method anyway, there's not as much reason left to avoid doing it in the consensus.

comment:32 in reply to:  31 Changed 7 years ago by nickm

Replying to arma:

Replying to nickm:

Roger, I don't get at this point why you prefer your alternative approach (vote differently) vs the one I started to implement (consense differently). I kind of prefer the latter, since it doesn't change the meaning of any vote, and it looks like we'll probably need a new consensus method anyway.

I'm open to either approach. We'll want to make sure we don't cap all nodes to 50 in the case where there aren't enough bwauths putting in Measured votes.

We may also find that we want to only cap weights when a certain threshold of the relays have a Measured vote. I'm not sure if we'll want to do that threshold, or what we'll want it to be, so I had figured leaving the flexibility to each authority might make it easier. For example, I worry about a case where all the bwauths restart afresh and have just a few votes each -- currently it makes sense for them to include the votes, but now they'd better think about not putting in any votes until they have quite a few.

I think that approach (Don't say "measured" until you have measured a bunch of routers) is smarter and way easier to implement than the approach where we don't cap until a threshold of routers has measured values.

To avoid the first problem you talk about, I'd suggest a simple rule of "implement the cap approach when at least 3 authorities are publishing measurements." It's not perfect, and has corner cases, but it should be basically ok.

That said, we also want to avoid an attack where the adversary floods the network with new nodes, and suddenly the bwauths take away their caps because they haven't covered the required threshold of the nodes yet.

*That* said, now that it looks like we're going to be making a new consensus method anyway, there's not as much reason left to avoid doing it in the consensus.

I've updated the branches "bug2286_part_a" in my public tor repo and in my public torspec repo to try to implement the behavior we've been discussing. Sensible now? Correctly implemented?

comment:33 Changed 6 years ago by arma

Looks like it should work. Three thoughts:

A) I notice that this plan doesn't affect flags at all. So we could have a relay with the Fast flag but a weight of 20 (which would ordinarily not warrant the Fast flag). That's a separate topic (whether authorities should vote flags based on the descriptor bandwidth, the vote bandwidth, or now the consensus bandwidth), but I'd be more comfortable if we raise our default cap to e.g. 100.

B) It would seem this change requires a corresponding change in the torflow bwauth code, to not admit opinions about any relays until it has 'enough' opinions. Or maybe torflow is the wrong place to put that logic, and it should go in the Tor dir auth logic, either in dirserv_read_measured_bandwidths() or probably better in routerstatus_format_entry() when we're deciding

    if (format == NS_V3_VOTE && rs->has_measured_bw) {

C)

-    "w" SP "Bandwidth=" INT [SP "Measured=" INT] NL
+    "w" SP "Bandwidth=" INT [SP "Capped=1"] [SP "Measured=" INT] NL

Is the order mandatory here? Sure looks like it is. I guess in our current implementation it doesn't matter, since we never say Capped and Measured together.

Smaller thoughts:

A) run make check-spaces on it

B) the spec change says "maxunmappedbw" which is probably not the right word.

comment:34 in reply to:  33 ; Changed 6 years ago by arma

Replying to arma:

Or maybe torflow is the wrong place to put that logic, and it should go in the Tor dir auth logic,

Wherever we put it, what should the logic be? We could say "don't ever say Measured= unless you have opinions on >=50% of the Running relays you'll be voting about". That should be fine for normal operation, but is vulnerable to the "adversary floods network with new nodes to make bwauths stop saying Measured about anybody" attack.

comment:35 in reply to:  34 Changed 6 years ago by arma

Replying to arma:

but is vulnerable to the "adversary floods network with new nodes to make bwauths stop saying Measured about anybody" attack.

Seems to me that we don't want to ask what fraction of relays we have an opinion about. We want to ask if our bwauth has been running for the past n days, where n is enough to get a good handle on the relays during normal operation.

comment:36 in reply to:  33 ; Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Replying to arma:

Looks like it should work. Three thoughts:

A) I notice that this plan doesn't affect flags at all. So we could have a relay with the Fast flag but a weight of 20 (which would ordinarily not warrant the Fast flag). That's a separate topic (whether authorities should vote flags based on the descriptor bandwidth, the vote bandwidth, or now the consensus bandwidth), but I'd be more comfortable if we raise our default cap to e.g. 100.

Okay, I should do this.

[...]

C)

-    "w" SP "Bandwidth=" INT [SP "Measured=" INT] NL
+    "w" SP "Bandwidth=" INT [SP "Capped=1"] [SP "Measured=" INT] NL

Is the order mandatory here? Sure looks like it is. I guess in our current implementation it doesn't matter, since we never say Capped and Measured together.

Order doesn't have to be mandatory in votes, I guess. Might as well leave it that way though.

Smaller thoughts:

A) run make check-spaces on it

will do

B) the spec change says "maxunmappedbw" which is probably not the right word.

should probably say "uncapped".

WRT the other matters -- arma says that there need to be corresponding changes to bwauth before we can merge this, and that he'll amke the tickets for that.

comment:37 in reply to:  36 Changed 6 years ago by arma

Replying to nickm:

WRT the other matters -- arma says that there need to be corresponding changes to bwauth before we can merge this, and that he'll amke the tickets for that.

#6131 and #6133.

comment:38 Changed 6 years ago by nickm

(FWIW, this could IMO go into 0.2.4.1-alpha as easily as 0.2.3.x-rc, since authorities tend to go bleeding-edge. Since we can't merge it until the bwauths support it, I don't want to defer 0.2.3.x until that's ready.)

comment:39 in reply to:  38 Changed 6 years ago by arma

Replying to nickm:

(FWIW, this could IMO go into 0.2.4.1-alpha as easily as 0.2.3.x-rc, since authorities tend to go bleeding-edge. Since we can't merge it until the bwauths support it, I don't want to defer 0.2.3.x until that's ready.)

True, but let's not be too eager to go that route -- right now you *can't* usefully run an authority on 0.2.2, since you won't support the consensus method that the threshold of authorities pick. I don't want to make it so you can't usefully run an authority on 0.2.3 either, at least not quite yet.

comment:40 Changed 6 years ago by nickm

I'm not "eager" to go that route. But as I see it the alternatives are:

  • Add this to 0.2.3.x without the bwauth changes, even though it will break things.
  • Delay 0.2.3.x until bwauth changes are completed on an unknown timeframe.
  • Add this tricky new feature into 0.2.3.x while 0.2.3 is stable.
  • Do not add this to 0.2.3.x; put it in 0.2.4.x.

I think that the first two options are not okay. That leaves us with the last two options, which are not great.

We'll see when this can go in. I will not block 0.2.3.x for it, and I will not promise that it will merge into 0.2.3.x if it isn't ready until 0.2.3.x has been stable for some time.

comment:41 in reply to:  40 Changed 6 years ago by nickm

Replying to nickm:

We'll see when this can go in. I will not block 0.2.3.x for it, and I will not promise that it will merge into 0.2.3.x if it isn't ready until 0.2.3.x has been stable for some time.

So I suppose I'm implying that if it's ready early in 0.2.3.x's stable lifecycle, it can go in.

comment:42 Changed 6 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

Okay, this is now out-of-scope for 0.2.3, and vital for 0.2.4.

comment:43 Changed 6 years ago by arma

We're making progress on the subtickets.

Does this patch need reworking to be compatible with the other consensus methods that got invented in the meantime? Perhaps we should combine the next consensus method number with the "require a minimum number of votes about "a" lines before believing them" plan that ln5 and sebastian raised re consensus method 14?

comment:44 Changed 6 years ago by ln5

Cc: ln5 added

comment:45 Changed 6 years ago by mikeperry

We really need to use the keyword "Unmeasured" for this branch + the spec + Torflow, so that when we talk amongst ourselves and to researchers about this type of capping, there is no ambiguity.

I'm now pretty convinced this is not merely a bikeshed concern. We already have a hell of a time talking about all the different types of "weights" with researchers and others who are new to tor. We shouldn't compound this by also overloading the term "capping", too.

Similarly, the cap itself should be called "maxunmeasuredbw" rather than "maxuncappedbw" or some other word involving capping.

comment:46 Changed 6 years ago by nickm

Keywords: needs-proposal added

comment:47 Changed 6 years ago by nickm

Keywords: tor-auth added

comment:48 Changed 6 years ago by nickm

Component: Tor Directory AuthorityTor

comment:49 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

Ugh. We have GOT to get this done in 0.2.5.

Mike, where do we stand on getting this working with the bandwidth authority?

comment:50 Changed 6 years ago by mikeperry

Aarron is the person to ask. I think his latest patch on the bw auth side for this is sane.

Again, note he used 'Unmeasured' there, not 'Capped'. Please do not use 'Capped'.

comment:51 Changed 6 years ago by nickm

So, the format this outputs needs to be tweaked? Can I have a patch to the proposal or something that tells me what changes to make to the 2286 branch before merging it in 0.2.5?

comment:52 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

comment:53 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

I've put a lightly rebased version at "bug2286_redux". The field now does something different and is called "Unmeasured". Is this what folks had in mind?

comment:54 Changed 6 years ago by mikeperry

Yes. This looks good to me. It should work fine with #6131 as far as I can tell.

comment:55 Changed 6 years ago by nickm

Owner: set to andrea
Status: needs_reviewassigned

Asking andrea to write a quick unit test for this before we merge.

comment:56 Changed 6 years ago by nickm

Status: assignedneeds_revision

comment:57 Changed 6 years ago by nickm

Type: defectenhancement

comment:58 Changed 6 years ago by nickm

I see some commits from yesterday; is this ready?

comment:59 in reply to:  58 Changed 6 years ago by andrea

Replying to nickm:

I see some commits from yesterday; is this ready?

Not just yet; the refactor is done but I need to actually use it for generating a bandwidth-clipping consensus still, and it could use a squash.

comment:60 Changed 6 years ago by nickm

okay. Let me know when it's done -- hopefully in the next day or two or so, if you think that's doable.

comment:61 Changed 6 years ago by andrea

Possible bug found! Hurrah for unit tests!

When we do networkstatus_compute_consensus() with unmeasured bandwidth clipping, we emit a Bandwidth=<value> and optionally an "Unmeasured=1" flag to indicate we do not have a measured bandwidth. See dirvote.c line 2057. However, when we parse the router status lines in the consensus in routerstatus_parse_entry_from_string(), we read the Bandwidth=<value> and the Measured=1 flags older consensus versions can emit, but not the Unmeasured=1 flag. The has_measured_bw field of routerstatus_t does not survive a round trip through the parser. I'm not sure this actually breaks anything (do the consumers of the consensus care?) but it was unexpected enough that I wrote my unit tests in a way that flunked it. Should we change this behavior or should I change my test?

comment:62 Changed 6 years ago by andrea

My bug2286_unit_test branch has a refactoring of the v3_networkstatus test and a new unmeasured bandwidth clipping unit test. Per my preceding comment, this unit test currently fails. If you comment out the tests on measured_bw and has_measured_bw on lines 1992, 1994, 2024 and 2026 of src/test/test_dir.c, it passes. I've verified that this test distinguishes the new behavior (i.e., the same test fails if you run it with consensus method 16 instead).

We still need to decide about the Unmeasured flag, and I should add a variant of this to check overriding the default clipping value in the consensus, and squash the branch, then we're good to go on this.

comment:63 Changed 6 years ago by andrea

The Unmeasured=1 issue is now fixed in my bug2286_unit_test branch.

comment:64 Changed 6 years ago by nickm

I've added #8273 to nuke another case of "using advertised bandwidth in the voting process", so we can close this once we merge the branch above. (Andrea wants to write one more test here.)

comment:65 Changed 6 years ago by andrea

The test for overriding maxunmeasuredbw in params is in now too; squashed version in my bug2286_squashed branch is ready for merging.

comment:66 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Merged, thanks! (I cherry-picked your last commit onto a squash branch I'd started earlier. I also added 804be10b096de to appease GCC 4.7.2)

Note: See TracTickets for help on using tickets.