Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#3886 closed defect (fixed)

testingtornetwork with all new relays, none get hsdir flag

Reported by: arma Owned by:
Priority: Very Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If you set TestingTorNetwork to 1, it sets MinUptimeHidServDirectoryV2 to 0. So far so good.

But then in dirserv_thinks_router_is_hs_dir() we check

  return (router->wants_to_be_hs_dir && router->dir_port &&
          uptime > get_options()->MinUptimeHidServDirectoryV2 &&
          router->is_running);

Should that middle '>' be a '>=' ?

We want to give relays with 0 uptime the hsdir flag in testing tor networks.

(Leaving milestone unchosen -- should this go in 0.2.3, or into 0.2.2 on the theory that it's still in 'soft' stable status?

Child Tickets

Change History (10)

comment:1 in reply to:  description Changed 6 years ago by rransom

Milestone: Tor: 0.2.2.x-final

Replying to arma:

If you set TestingTorNetwork to 1, it sets MinUptimeHidServDirectoryV2 to 0. So far so good.

But then in dirserv_thinks_router_is_hs_dir() we check

  return (router->wants_to_be_hs_dir && router->dir_port &&
          uptime > get_options()->MinUptimeHidServDirectoryV2 &&
          router->is_running);

Should that middle '>' be a '>=' ?

Yes. That's what 'Min' means.

(Leaving milestone unchosen -- should this go in 0.2.3, or into 0.2.2 on the theory that it's still in 'soft' stable status?

It's a bug that prevents us from testing 0.2.2.x in private networks.

comment:2 Changed 6 years ago by karsten

Yup, '>=' makes more sense here.

But does this bug really prevent us from testing 0.2.2.x in private networks? Isn't an uptime of 1 second enough to get the HSDir flag? (Not arguing that this shouldn't be fixed, but does it have to be in 0.2.2.x?)

comment:3 Changed 6 years ago by arma

See also #2088 where we fixed most of the issue but missed this piece of it.

comment:4 Changed 6 years ago by arma

See bug3886 in my arma (based on maint-0.2.2)

On further looking, 0.2.1 lacked the patch in f87c6f100d, so it could be that f87c6f100d is sufficient for all practical situations. Can't hurt much to handle the impractical situations too I guess.

comment:5 Changed 6 years ago by karsten

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final

Patch looks correct.

Regarding your commit message, this bug isn't specific to the TestingTorNetwork config, it's only most relevant there. But if the config option says Min*, we shouldn't test for '>', even for 86400 vs. 86401. This code was wrong when someone wrote it in 2007. Not sure if we care about that detail, just saying.

Also, setting milestone to 0.2.3.x, because this bug is mostly harmless, IMHO. Doesn't it take a few minutes for a private network to stabilize before being useful anyway?

comment:6 Changed 6 years ago by nickm

Actually, it looks to me like this could be worth backporting. One point of testing networks is for interoperability testing, and if we're going to hack hidden service stuff on 0.2.3.x, it would be neat to test it on a mixed network of noes.

I'm going to suggest that we merge this into 0.2.3.x, let it percolate for a release or two, and then see how we feel about applying it to 0.2.2.x. Objections?

comment:7 Changed 6 years ago by Sebastian

This bug is completely harmless in practice, even in 0.2.2.x testing networks. Changing the > into a >= is just as harmless. We should just do it in 0.2.2.x and 0.2.3.x and forget about it, imo. What are we trying to learn from 0.2.3.x? We can't even trigger the bug except possibly with micro-precision timing, haven't checked the codepaths in detail

comment:8 Changed 5 years ago by nickm

Priority: normaltrivial
Resolution: fixed
Status: newclosed

Harmless, but obviously more correct than the previous behavior. This slipped off my radar due to not being in needs_review. Merging into 0.2.3 and closing.

comment:9 Changed 5 years ago by nickm

Keywords: tor-auth added

comment:10 Changed 5 years ago by nickm

Component: Tor Directory AuthorityTor
Note: See TracTickets for help on using tickets.