authdir_mode_any_nonhidserv() is an obsolete concept

/** Return true if we believe ourselves to be any kind of
 * authoritative directory beyond just a hidserv authority. */
int
authdir_mode_any_nonhidserv(const or_options_t *options)

This phrase "hidserv authority" is from back in the days of v1 directory authorities, where there were 3 central authorities that collected and served all the hidden service descriptors.

Those days are gone, so references to a hidserv authority no longer make sense.

In particular, I think there are now only two types of directory authorities -- bridge authorities and main authorities. So this function could be replaced with just authdir_mode(). I'm marking it as 'easy' since it's a good one for a newbie to grab.

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor easy milestone::Tor: 0.3.2.x-final priority::medium resolution::implemented review-group-18 reviewer::arma severity::normal status::closed tor-hs type::defect labels

As a bonus, any call to authdir_mode_handles_descs(options, -1) can also be converted to just a check to authdir_mode().

Then if we like, authdir_mode_handles_descs(options, ROUTER_PURPOSE_BRIDGE) could become just authdir_mode_bridge(options).

And authdir_mode_handles_descs(options, ROUTER_PURPOSE_GENERAL) could become just authdir_mode_v3(options).

And authdir_mode_any_main() could disappear because we don't need it anymore.

Maybe the other wrapper functions are now overly complexified too. Worth exploring!

Trac:
Milestone: N/A to Tor: 0.3.2.x-final
Keywords: N/A deleted, tor-hs added

Trac:
22311.patch

I added a patch for this, PTAL.

Trac:
22311.2.patch

Updated patch: also remove authdir_mode_any_main()

Trac:
Status: new to needs_review
Cc: N/A to huyvq

Trac:
Reviewer: N/A to arma

Hi huyvq! Thanks for the patches.

Are you comfortable using git? If so, the best thing to do here would be to separate these into a series of commits, where each commit resolves one of the functions that doesn't need to exist anymore. That way we don't have a bigger diff where we're removing several functions at once and replacing several functions at once and it's harder to audit.

It's possible that none of these functions are interconnected, that is, you could remove them in any order. It's also possible that there are some orderings of removal that are cleaner to do and some that are messier.

I've also been looking through authdir_mode_publishes_statuses(), which looks like it doesn't have to exist anymore and could just be replaced with a call to authdir_mode()? It looks like there's an old comment in list_server_status_v1() about including v2 dir auths, which is not accurate anymore -- in fact, I just opened #22473 (moved) so we can get rid of that whole function, but that's not this ticket. :)

And the same with authdir_mode_tests_reachability() -- now that all types of auths test reachability, I don't think we need a separate wrapper function.

Tackling one function per commit will make auditing the commits much easier, and also it is the right sort of best practice for development. :)

(If you don't want to deal with this, that's fine too, let us know and we'll try to refactor things into separate commits.)

Trac:
trac22311.patch

Replying to arma:

I've also been looking through authdir_mode_publishes_statuses(), which looks like it doesn't have to exist anymore and could just be replaced with a call to authdir_mode()?

As I understand, authdir_mode_publishes_statuses() cannot be replaced with authdir_mode() since it requires BridgeAuthoritativeDir == 0 && AuthoritativeDir != 0. Am I missing something?

And the same with authdir_mode_tests_reachability() -- now that all types of auths test reachability, I don't think we need a separate wrapper function.

Yes.

I separated the patch to a series of commits as your comment. Please let me know if I could do better :) Thanks.

Trac:
Keywords: N/A deleted, review-group-18 added

Thanks! I've pulled this into my public repository as bug22311. Here are the additional changes I suggest, which I've made on that branch:

• I've made a fix to use authdir_mode_v3() instead of looking at V3AuthoritativeDir directly.
• Since we no longer call authdir_mode_handles_descs() with purpose==-1, I am calling that behavior a bug.

I also think that we shouldn't take 99c49cae9f8a354660a53dc2744cd0cc14c2e5b6 ("Remove authdir_mode_tests_reachability()") -- at some point in the future, we are likely to severely change how reachability testing works, especially on the bridge authorities.

If this still looks okay, I'll rebase and squash and merge.

Trac:
Status: needs_review to merge_ready

Replying to nickm:

If this still looks okay, I'll rebase and squash and merge.

It's okay for me. Thanks for the fixup.

Nitpicking: my handle is huyvq (in a96fd1263dfb46af1c49d1e5788d7d7996cf812e message)

Fixed; squashed; merged! Thank you!

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

closed

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