Add flag thresholds to the networkstatus created by a bridge authority

changed milestone to %Tor: 0.2.5.x-final

added component::core tor/tor milestone::Tor: 0.2.5.x-final priority::medium resolution::implemented status::closed tor-bridge trivial type::enhancement labels

The first question is: does the bridge authority compute values for the various variables used in dirserv_get_flag_thresholds_line() ?

I didn't see that a bridge auth took any code paths that called dirserv_compute_performance_thresholds(), so possibly not, which may actually be the cause of #1117 (moved), but I'll look into this further and confirm this is the case.

Please see bug9859_rebased in my public repo. This branch should actually solve #1117 (moved), also. The problem is that the bridge auth never computes the flag thresholds, so all bridges are at least as good as zero, unless they aren't good at all.

I successfully ran this overnight on a test bridge auth with a single test bridge uploading to it, but it can use more testing. Is this a good candidate for Chutney?

Trac:
Status: new to needs_review

Reviewing be044f0a8ff1927422f0f2fd17635bea6a0c6546 ...

It looks plausible, except that we're trying to avoid the "x = tor_malloc(n); tor_snprintf(x, n, ...)" pattern in new code. Instead, use "tor_asprintf(&x, ...)".

Is this a good candidate for Chutney?

It could be, but you'd need to make your own torrc templates to do so. Linus or I can walk you through that if it isn't apparent how from the source and the readme.

Trac:
Status: needs_review to needs_revision

Thanks for the comments! tor_asprintf is much better for this, I made that change. I also decided that the patch wasn't exactly what we want. It yielded the correct results, but it also changes the resulting output to the control port, which is less than ideal.

I pushed bug9859_rebased_2, which I think it a better patch.

Regarding Chutney, I hacked it a bit so it would do what I wanted, but overall it was a success.

stable-uptime=18391 stable-mtbf=19583 fast-speed=54000 guard-wfu=0.000% guard-tk=0 guard-bw-inc-exits=54000 guard-bw-exc-exits=54000 enough-mtbf=1 ignoring-advertised-bws=0
r test009br So4T1MobX5/KK/NHNZDAw7JDzWc hRdlnfQoOlWZKCITUZvamk9yunU 2013-10-11 10:31:45 127.0.0.1 5009 0
s Fast Guard Running Stable Valid
w Bandwidth=54
p reject 1-65535
r test010br UJkummsyfyPV+Tkv5xnoDBgTRG4 OhLHKRXMwoV1beeerm2bSTiMyYA 2013-10-11 10:41:45 127.0.0.1 5010 0
s Running Valid
w Bandwidth=3
p reject 1-65535
r test014br 0Si0953tzxo05XR+GK9JxRi9EfQ cAPmcu9q81ulTNRPUvqd239Qvsw 2013-10-11 10:33:45 127.0.0.1 5014 0
s Fast Guard Running Stable Valid
w Bandwidth=57
p reject 1-65535
r test015br 8akZGwVemVHPJv1Da95weuSsbD0 SIUENxUCVvG9AJc1zLyrPD2fW4Q 2013-10-11 10:35:45 127.0.0.1 5015 0
s Fast Guard Running Stable Valid
w Bandwidth=54
p reject 1-65535

Trac:
Status: needs_revision to needs_review

Looks mergeable to me.

Roger, Karsten: Does anything rely on this data not being at the start of the networkstatus-bridges file?

Trac:
Cc: N/A to arma, karsten

Actually, yes, parts of metrics might rely on that. Will fix that in the next hours and let you know here when it's fixed. Thanks for the heads-up!

I'm sorry, I'm running out of time before the weekend (7pm here, real life calling). I need to update metrics-db and test the changes a bit, or it might do unexpected things. I can do this on Monday. Please hold back this change until Tuesday or Wednesday. Will let you know here when metric-db is ready.

I had a quick look at bug9859_rebased_2 and it didn't look crazy.

We should encourage sysrqb to be sure that bridgedb can handle documents of this new form, but hopefully he's already thought of this. :)

The bridgedb parser will happily ignore this, so that won't be an issue.

Karsten, would it be better if we moved the threshold line to the end? There's no rush to merge this immediately (it looks like Tonga is running -rc, so we probably won't be able to start collecting this info for a few months).

Replying to sysrqb:

Karsten, would it be better if we moved the threshold line to the end?

No, at the start is fine.

There's no rush to merge this immediately (it looks like Tonga is running -rc, so we probably won't be able to start collecting this info for a few months).

Understood. Though I'd want to prepare metrics-db now, rather than being surprised by Tonga upgrading. But preparing metrics-db shouldn't be hard, I just need to sit down for a moment to think about any potential problems.

And here's one such problem: your new line should start with "flag-thresholds ", or we'll have a hard time parsing it once we decide to add more lines, or when we decide we want to remove the "stable-uptime" part. Can you add that keyword? Like:

-  tor_asprintf(&thresholds_and_status, "%s\n%s", thresholds, status);
+  tor_asprintf(&thresholds_and_status, "flag-thresholds %s\n%s", thresholds, status);

Oh, and just to be sure: there is nothing sensitive in that line, so that it can be copied as is into sanitized descriptors, right?

We should also ask atagar for his thoughts on the new line, because he'll have to extend stem to support this new line in sanitized bridge network statuses. atagar, do you see any problems with this new line, or do you have suggestions for making it easier to parse (in addition to my suggestion to prefix the line with a keyword)?

Trac:
Cc: arma, karsten to arma, karsten, atagar

This absolutely should have a prefix keyword, like everything else in consensus (otherwise this line will require special handling). Would you mind making the spec addition first? I'm not entirely clear from this backlog what precisely you're adding but it sounds akin to the 'params' keyword (but with string rather than numeric values). Parsers need a formal definition of what both the keys and values can include.

atagar, sorry, I might not have given you enough context. This is an addition to bridge network statuses, not consensuses. Also note that this line is very similar to the flag-thresholds line in votes which has a spec definition.

Can we make it "exactly the same as", rather than just "very similar to", so the spec can be the same?

I just wrote "very similar to", because I didn't check in detail before. Just checked that it's "exactly the same as" once the line starts with "flag-thresholds".

From the parser stem has for it I'm guessing...

bridge network status documents are not part of the spec
they're only available via the metrics site
their format was...

published [timestamp]
[list of v2 router status entries]

... and it's now becoming...

published [timestamp]
flag-thresholds [parameters]
[list of v2 router status entries]

Is that right? Apologies if I'm way off base. :)

They were intended to be the same as v2 networkstatus documents.

But it's possible they aren't.

Found the bit of the metrics site that describes them (see the bottom of this). V2 network status documents have quite a few more fields.

Bridge network status documents are not part of dir-spec.txt. They are like v2 network statuses (or even v3 consensuses?) but without header and footer. I added the "published" part to the start of sanitized bridge network statuses to make them easier to handle, but that part is not contained in the non-sanitized statuses.

They are like v2 network statuses (or even v3 consensuses?)

This actually seems like an important detail since it determines the type of router status entry. Presently stem treats them as v2.

I added the "published" part to the start of sanitized bridge network statuses to make them easier to handle, but that part is not contained in the non-sanitized statuses.

I treated 'published' as a mandatory field since the metrics page is the closest thing we have to a spec (and it says it's included). If this is a more generic descriptor type then it should probably be part of the dir-spec, otherwise my perception is that this is a metrics-specific type of descriptor.

Replying to atagar:

They are like v2 network statuses (or even v3 consensuses?)

This actually seems like an important detail since it determines the type of router status entry. Presently stem treats them as v2.

What's the difference between v2 and v3 router status entries?

I added the "published" part to the start of sanitized bridge network statuses to make them easier to handle, but that part is not contained in the non-sanitized statuses.

I treated 'published' as a mandatory field since the metrics page is the closest thing we have to a spec (and it says it's included). If this is a more generic descriptor type then it should probably be part of the dir-spec, otherwise my perception is that this is a metrics-specific type of descriptor.

Fine to perceive this as metrics-specific type of descriptor.

But regardless of dir-spec or not, it seems that we're on the safe side by adding the "flag-thresholds" keyword to make the new line exactly the same as in v3 votes.

sysrqb, can you add that keyword? And do you agree that this line can be copied as is into sanitized bridge network statuses?

Trac:
Status: needs_review to needs_revision

What's the difference between v2 and v3 router status entries?

V3 adds the or-address, bandwidth, measured, flag to indicate if unmeasured, microdescriptor hashes, and (most important) the relay's microdescriptor exit policy. Maybe none of those are especially important for bridges.

Replying to karsten:

But regardless of dir-spec or not, it seems that we're on the safe side by adding the "flag-thresholds" keyword to make the new line exactly the same as in v3 votes.

sysrqb, can you add that keyword?
Yeah, adding this sounds like a good plan. I admit I hadn't thought about this, but I'll add the prefix. And do you agree that this line can be copied as is into sanitized bridge network statuses? I'm hesitant to say this should be public, but I want arma's and nick's input on this. BridgeDB doesn't really take advantage of the status flags, except for supplying stable bridges, so it shouldn't affect bridge distribution now. I'm less certain this will be true in the future or if public availability could have negative consequences. otoh, this info will be good for metrics, so we should maintain an archive...so maybe they should remain unsanitized.

Replying to atagar:

What's the difference between v2 and v3 router status entries?

V3 adds the or-address, bandwidth, measured, flag to indicate if unmeasured, microdescriptor hashes, and (most important) the relay's microdescriptor exit policy. Maybe none of those are especially important for bridges.

Yeah, none of those are included in this Bridge networkstatus. Right now, it is exactly the same as the output returned from a NEWCONSENSUS event or NS event. This patch will simply prepend an additional line with the flag threshold values for bridges (but only when this networkstatus is created by the Bridge Auth).

Replying to sysrqb:

Replying to karsten:

And do you agree that this line can be copied as is into sanitized bridge network statuses? I'm hesitant to say this should be public, but I want arma's and nick's input on this. BridgeDB doesn't really take advantage of the status flags, except for supplying stable bridges, so it shouldn't affect bridge distribution now. I'm less certain this will be true in the future or if public availability could have negative consequences. otoh, this info will be good for metrics, so we should maintain an archive...so maybe they should remain unsanitized.

There's also the rule of thumb that we shouldn't use data that we don't make public. If something is too sensitive to publish, we shouldn't collect it.

But I don't see a reason for redacting these lines. The only thing that we're trying to hide from the public is bridge IP addresses. I don't see how flag thresholds would make it any easier to find bridge IP addresses.

Replying to karsten:

There's also the rule of thumb that we shouldn't use data that we don't make public. If something is too sensitive to publish, we shouldn't collect it.

Ok, I agree. That's a good rule and we should follow it for this.

But I don't see a reason for redacting these lines. The only thing that we're trying to hide from the public is bridge IP addresses. I don't see how flag thresholds would make it any easier to find bridge IP addresses. True, I only worried that, in the future, these threshold values will allow an operator to manipulate or game BridgeDB or one of the distributors, but I agree that preventing this via obscurity is not a way to solve this problem.

Pushed bug9859_5 based on master, which adds the keyword.

Any other thoughts?

Isis, sorry to bring you into this so late.

Trac:
Status: needs_revision to needs_review
Cc: arma, karsten, atagar to arma, karsten, atagar, isis

The code looks okay to me. Are there any pending objections from above that weren't addressed?

Replying to nickm:

The code looks okay to me. Are there any pending objections from above that weren't addressed?

I don't see any pending objections that aren't addresses by now. Fine to merge, I'd say.

Okay; merged into master. Thanks, all!

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

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

Add flag thresholds to the networkstatus created by a bridge authority

Child items ...

Activity