Don't disable dirport when accounting limit can't be reached due to bwrate

A lot of people set accounting options just for the statistics, not to actually enforce a bandwidth cap (they do that via bandwidthrate). We shouldn't disable their dirports when they do that.

(Of course, the real bug is that you don't get the statistics without setting accounting options, but that's unrelated IMO.)

changed milestone to %Tor: 0.2.3.x-final

added component::core tor/tor milestone::Tor: 0.2.3.x-final priority::medium resolution::fixed small-feature status::closed tor-relay type::defect labels

(I should point people at bug #871 (moved) here, too. A few things are discussed there. I think this proposal is a bit different tho)

Is this still relevant with the implementation of #2345 (moved)? The accounting is sometimes set to provide the traffic usage, which that option provides. Cheers! -Damian

I think it might still be relevant, not sure what other features accountingmax provides, especially since you need a controller for #2345 (moved).

Implementation of this in branch bug2434 in my repository.

Trac:
Status: new to needs_review
Milestone: N/A to Tor: 0.2.3.x-final

Looks reasonable.

My first minor suggestion would be to put a comment in the code as to why we have that extra case, and a short comment in the changes file saying what that lets you do.

Second minor suggestion would be to switch the accounting_set_wakeup_time() function to use your new accounting_get_interval_length() abstraction rather than its time_in_interval variable.

The broader suggestion would be, if we want an alternate more explicit way to do this (rather than relying on a hack), to have another torrc option (e.g. AccountingTrack) which explicitly says to do all the accounting stuff but not for hibernating. I don't think this change is a big enough deal to matter either way though.

Trac:
Points: N/A to N/A

Looks okay to me too; I concur with roger's first two recommendations, and suggest that we defer the third "till later".

I am not totally comfortable with the new calculation in decide_to_advertise_dirport: get_effective_bwrate() returns uint32_t, and accounting_get_interval_length returns an int. On platforms where int is 32-bit, their product can easily overflow the result type.

What are our options here? First option is to use a 64-bit type. If the longest interval we support is a month, then we'll be okay until somebody has get_effective_bwrate() returning more than about 6.8 terabytes per second. Second option is to use division, and check whether get_effective_bwrate(options) >= options->AccountingMax / accounting_get_interval_length().

Trac:
Actualpoints: N/A to N/A

I'm calling this 0.2.4.x, unless you think the revisions will happen by Friday. It's worth doing, but it can certainly wait.

Trac:
Milestone: Tor: 0.2.3.x-final to Tor: 0.2.4.x-final

I rebased the branch onto master because it didn't merge cleanly anymore, and added a fixup commit to change the identified problems. I don't think a new torrc option is warranted, especially because we have way too many already, and I think this feature only means that a few people will not be confused where they'd otherwise have been confused.

Oh, changing the milestone also. Please change it back if you'd rather wait with the merge.

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.3.x-final

I'm a little worried about a case where accounting_get_interval_time() returns 0 and we divide by it. Can we add a defensive programming check for that? Also, let's maybe log the values here at log_info severity, so we can track down any infelicities in the equation.

Trac:
Keywords: N/A deleted, small-feature added

added another fixup commit. Did you mean something like that?

Yes, just so. Squashing & merging & closing.

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

Trac:
Keywords: small-feature deleted, small-feature tor-relay added

Trac:
Component: Tor Relay to Tor

closed

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