Opened 10 years ago

Last modified 4 weeks ago

#1136 new defect (None)

When Tor is offline, it doesn't quite run out of relays, so doesn't realize it's offline — at Version 6

Reported by: arma Owned by: mikeperry
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, shutdown, bootstrap, sponsor8-maybe
Cc: arma, nickm, Sebastian, mikeperry, teor Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by mikeperry)

If your Tor client goes offline, it will keep trying to establish circuits,
and as each TLS connection fails, it will mark that relay down.

In update_router_have_minimum_dir_info() Tor checks whether (num_present < 2)
but we never actually mark down the last few relays, either because we don't
have enough left to make a circuit so we don't ever try another TLS connection,
or because none of the remaining relays are suitable exit nodes so we can't
pick a path that would be a useful circuit so we don't try.

I think we need to catch the case where we failed to pick a path because we
don't have enough circuits, and if it case occurs and many of our relays are
marked down, we should mark them up.

That will cause us to attempt circuits for a lot longer than currently, but on
the other hand Tor will actually work when you come back to the network and
try to make a new application request.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (6)

comment:1 Changed 9 years ago by mikeperry

I feel like this means that we should create a different flag variable for clients. The variable is_running in
routerinfo_t for example is marked as local only, yet we still appear to updating the routerstatus_t version when
we fail to reach a relay as a client. The problem seems to be that router_set_status() sets both versions
of the flag.

If we differentiate, we can then safely re-copy all of the consensus versions of is_running over our local versions,
as opposed to marking *every* router as now up.

comment:2 Changed 9 years ago by nickm

That sounds great. Let's talk about how best to do this. Immediately, just adding a new flag might do. But
in the meantime, I'm planning a related change for microdescriptors, where we stop using routerinfo_t as a
repository of local per-router knowledge. What we make a new shiny router_t that tracks the local Tor's
knowledge and opinion of each router, and turn all the routerstatus_t in a consensus, and every routerinfo_t,
into a nice immutable thing?

comment:3 Changed 9 years ago by mikeperry

Ok. This bug is blocking some of my circuit build times and bandwidth weight tests. Can I start out by just
moving the mutable version of this flag into routerinfo_t and implementing this reset by copy method in the
meantime? I can try to look for any other instances of modification of either routerstatus_t or routerinfo_t
and mark them with comments in the branch I do this in.

comment:4 Changed 9 years ago by nickm

There are a _lot_ of things that touch flags in routerstatus_t and routerinfo_t. If you're going to do this,
we should think closely about what the new status is, and how you're going to make sure that everything that
currently sets or examines the old flag is changed as needed. Then we should write it in or-dev so that we
know what design we're going for.

comment:5 Changed 9 years ago by mikeperry

Ok, I meant to simply stop the modification of is_running in routerstatus_t and move those modifications to
routerinfo_t. As for when to read which version, I think that Tor should primarily use the routerinfo_t version,
except when it detects that it runs out of routers, in which case it should retry every so often by resetting
all of the is_running flags to their routerstatus_t consensus versions. I can describe this in more depth on
or-dev when I start looking through the code again.

As far as commenting, I was planning on only marking weird modifications of routerstatus_t that I happen to
see while making this change. I feel like routerstatus_t is the first thing we should try to make readonly.
I don't intend on being exhaustive here, just a 'while I'm at it' kind of thing.

comment:6 Changed 9 years ago by mikeperry

Description: modified (diff)
Owner: set to mikeperry
Status: newassigned
Note: See TracTickets for help on using tickets.