Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#8435 closed enhancement (fixed)

Ignore advertised bandwidths for flags once we have enough measured bandwidths

Reported by: andrea Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Once a dirauth sees a large enough fraction of nodes with measured bandwidths, it should ignore advertised bandwidths for purposes of assigning flags. That is, nodes without measured bandwidths should never get the Fast, Guard, HSDir flags, etc.

This is a follow-on to 8273.

Child Tickets

Change History (13)

comment:1 Changed 6 years ago by rransom

Component: - Select a componentTor

comment:2 Changed 6 years ago by nickm

Keywords: tor-auth added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

Let's try this for 0.2.4. We've gone through so much effort to get bandwidth measurements that it's a shame to have ways to get around them.

comment:3 Changed 6 years ago by andrea

Notes on implementation:

The flags are set in set_routerstatus_from_routerinfo(), which calls dirserv_get_bandwidth_for_router() directly and through dirserv_thinks_router_is_unreliable(). We should modify that function to check if it should trust advertised bandwidths, or perhaps call a new dirserv_get_trusted_bandwidth_for_router(). When in constructing a vote does set_routerstatus_from_routerinfo() get called? We may need a prepass to count how many nodes have measured bandwidth and decide whether to require it first.

comment:4 Changed 6 years ago by andrea

There's also a dirserv_compute_performance_thresholds() which calls dirserv_get_bandwidth_for_router() directly and through router_counts_toward_thresholds(). The fix for 8273 altered the behavior here to use measured bandwidths rather than advertised when available. What possible attacks might be enabled by distorting the thresholds by spamming the dirauths with bogus bandwidth advertisements? We should contemplate this too once we have setting flags nailed down.

comment:5 Changed 6 years ago by andrea

There are three calls to set_routerstatus_from_routerinfo(); one is in a loop in dirserv_generate_networkstatus_vote_obj() after we dirserv_read_measured_bandwidths(), so we can insert whatever calculations are needed there. Another is in generate_v2_networkstatus_opinion(), which should never care about this, and the third is in networkstatus_getinfo_by_purpose(), which comes with a comment that explicitly says it should only be used for bridges, so measured bandwidths won't apply here.

I think we should just have an dirserv_update_bw_trust() that gets called from dirserv_generate_networkstatus_vote_obj() after dirserv_read_measured_bandwidths() but before the set_routerstatus_from_routerinfo() loop to calculate the proportion and decide if advertised bandwidths are acceptable, and then have set_routerstatus_from_routerinfo() use a new dirserv_get_trusted_bandwidth_for_router() that checks that and returns zero if the threshold is met and no measured bandwidth is available.

comment:6 Changed 6 years ago by nickm

then have set_routerstatus_from_routerinfo() use a new dirserv_get_trusted_bandwidth_for_router() that checks that and returns zero if the threshold is met and no measured bandwidth is available.

I like this idea, except for the name, since it'll sometimes return an untrusted bandwidth measure.

Let's see-- the behavior is "Tell me the measured bw. If we don't have a measured bandwidth for this, tell me 0 unless we don't have enough meaured bandwidth overall." I don't see a great way to name a function after that behavior, so instead perhaps we should name it after its application. The application is really "Tell me the bandwidth that I should act on when deciding things about this router." Maybe dirserv_get_{??}_bandwidth_for_router, where ?? is something like "operational" or "actionable"? I wonder if there's a less buzzwordy word we could use for that.

comment:7 in reply to:  6 ; Changed 6 years ago by andrea

Replying to nickm:

Let's see-- the behavior is "Tell me the measured bw. If we don't have a measured bandwidth for this, tell me 0 unless we don't have enough meaured bandwidth overall." I don't see a great way to name a function after that behavior, so instead perhaps we should name it after its application. The application is really "Tell me the bandwidth that I should act on when deciding things about this router." Maybe dirserv_get_{??}_bandwidth_for_router, where ?? is something like "operational" or "actionable"? I wonder if there's a less buzzwordy word we could use for that.

Hmmm - yeah, those are pretty buzzwordy. How about dirserv_get_bandwidth_for_router_flags()? (It wouldn't fit anymore if there were a future extension to use it for the thresholds too, though).

comment:8 in reply to:  7 Changed 6 years ago by nickm

Replying to andrea:

Hmmm - yeah, those are pretty buzzwordy. How about dirserv_get_bandwidth_for_router_flags()? (It wouldn't fit anymore if there were a future extension to use it for the thresholds too, though).

Maybe something like "dirsev_get_credible_bandwidth" ? Where "credible" is defined s.t. we become credulous when we don't know much.

comment:9 Changed 6 years ago by andrea

Status: newneeds_review

See my bug8435 branch.

comment:10 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Looks good!

Does routers_with_measured_bw really need to be static? Maybe dirserv_count_measured_bws could return a value. That could make things ugly, though. If it remains static, though, it should be documented what sets it when and what uses it when.

required_mbw should be documented in the doxygen comment for router_counts_toward_thresholds

dirserv_query_measured_bw_cache is used like a predicate but isn't named like one. Maybe we should add a dirserv_router_has_measured_bw() wrapper for clarity.

comment:11 in reply to:  10 Changed 6 years ago by andrea

Status: needs_revisionneeds_review

Replying to nickm:

Looks good!

Does routers_with_measured_bw really need to be static? Maybe dirserv_count_measured_bws could return a value. That could make things ugly, though. If it remains static, though, it should be documented what sets it when and what uses it when.

I think all the plumbing for making it a return value is too icky; I improved the comment though.

required_mbw should be documented in the doxygen comment for router_counts_toward_thresholds

dirserv_query_measured_bw_cache is used like a predicate but isn't named like one. Maybe we should add a dirserv_router_has_measured_bw() wrapper for clarity.

Done.

comment:12 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

MERGE THEM! MERGE THEM ALL AND KNUTH WILL KNOW HIS OWN!

comment:13 Changed 5 years ago by arma

It occurs to me that since only 3 or 4 authorities will have bwauths attached to them, then 3 or 4 auths will be voting this new way about flags. And that won't usually be enough to influence the flags in the final consensus.

So while we have indeed put effort into getting this going in 0.2.4, it won't really take effect until either a) we get bwauths attached to many dir authorities, or b) we make a new consensus method that strips flags from Unmeasured relays. 'b' would be pending on #8685 and it's not clear to me that it's a good idea (especially since we've already done #2286).

Note: See TracTickets for help on using tickets.