Opened 5 months ago

Closed 4 months ago

#22311 closed defect (implemented)

authdir_mode_any_nonhidserv() is an obsolete concept

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, tor-hs, review-group-18
Cc: huyvq Actual Points:
Parent ID: Points:
Reviewer: arma Sponsor:

Description

/** 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.

Child Tickets

Attachments (3)

22311.patch (5.5 KB) - added by huyvq 5 months ago.
22311.2.patch (6.3 KB) - added by huyvq 5 months ago.
trac22311.patch (13.8 KB) - added by huyvq 5 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 months ago by arma

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!

comment:2 Changed 5 months ago by asn

Keywords: tor-hs added
Milestone: Tor: 0.3.2.x-final

Changed 5 months ago by huyvq

Attachment: 22311.patch added

comment:3 Changed 5 months ago by huyvq

I added a patch for this, PTAL.

Changed 5 months ago by huyvq

Attachment: 22311.2.patch added

comment:4 Changed 5 months ago by huyvq

Updated patch: also remove authdir_mode_any_main()

comment:5 Changed 5 months ago by huyvq

Cc: huyvq added
Status: newneeds_review

comment:6 Changed 5 months ago by huyvq

Reviewer: arma

comment:7 Changed 5 months ago by 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 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.)

Changed 5 months ago by huyvq

Attachment: trac22311.patch added

comment:8 in reply to:  7 Changed 5 months ago by huyvq

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.

comment:9 Changed 4 months ago by nickm

Keywords: review-group-18 added

comment:10 Changed 4 months ago by nickm

Status: needs_reviewmerge_ready

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.

comment:11 in reply to:  10 Changed 4 months ago by huyvq

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)

comment:12 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Fixed; squashed; merged! Thank you!

Note: See TracTickets for help on using tickets.