| Commit message (Collapse) | Author | Age | Files | Lines |
| | |
|
| |
|
|
|
|
|
|
|
|
|
| |
The FirstHopId type now records an enum that stores whether the hop
is a guard or a fallback. This change addresses concerns about
remembering to check the type or source of an Id before passing it
down to the FallbackState or GuardSet.
Making this change required an API change, so that dirmgr can
report success/failure status without actually knowing whether it's
using a fallback or a guard.
|
| |
|
|
|
| |
This is preparation for having separate GuardId and FirstHopId types
that distinguish which back-end they index.
|
| | |
|
| | |
|
| | |
|
| |
|
|
|
|
| |
The code here uses a new iterator type, since I couldn't find one of
these on crates.io. I tried writing the code without it, but it was
harder to follow and test.
|
| |
|
|
|
|
|
|
|
|
|
| |
We do this by creating a new FallbackSet type that includes status
information, and updating the GuardMgr APIs to record success and
failure about it when appropriate. We can use this to mark
FallbackDirs retriable (or not).
With this change, FallbackDir is now stored internally as a Guard in
the GuardMgr crate. That's fine: the FallbackDir type really only
matters for configuration.
|
| |
|
|
|
|
|
|
|
|
|
| |
We only do this when we fail to get a regular guard (e.g., because
they're all down), and when we have been asked for a guard for a
one-hop directory.
Most of the change in this commit is plumbing to make all of the
types match up.
As before, compilation may still be broken.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
| |
The guard manager is responsible for handing out the first hops of
tor circuits, keeping track of their successes and failures, and
remembering their states. Given that, it makes sense to store this
information here. It is not yet used; I'll be fixing that in
upcoming commits.
Arguably, this information no longer belongs in the directory
manager: I've added a todo about moving it.
This commit will break compilation on its own in a couple of places;
subsequent commits will fix it up.
|
| |
|
|
| |
This is more in keeping with the rest of our code.
|
| |
|
|
|
|
|
| |
This is the logical place for it, I think: the GuardMgr's job is to
pick the first hop for a circuit depending on remembered status for
possible first hops. Making this change will let us streamline the
code that interacts with these objects.
|
| | |
|
| |
|
|
|
| |
We'll need this so that we can say "This guard behaved bogusly
as a directory cache; try somebody else."
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When all guards are down, we would previously mark them all as up,
and retry aggressively. But that's far too aggressive: if there's
something wrong with our ability to connect to guards, it makes us
hammer the network over and over, ignoring all the guard retry
timeouts in practice.
Instead,
* We now allow the `pick_guard()` function to fail without
automatically retrying.
* We give different errors in the cases when all our guards are
down, and when all of the guards selected by our active usage
are down.
* Our "guards are down" error includes the time at which a guard
will next be retriable.
This is part of #407.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This lint is IMO inherently ill-conceived.
I have looked for the reasons why this might be thought to be a good
idea and there were basically two (and they are sort of contradictory):
I. "Calling ‘.clone()` on an Rc, Arc, or Weak can obscure the fact
that only the pointer is being cloned, not the underlying data."
This is the wording from
https://rust-lang.github.io/rust-clippy/v0.0.212/#clone_on_ref_ptr
It is a bit terse; we are left to infer why it is a bad idea to
obscure this fact. It seems to me that if it is bad to obscure some
fact, that must be because the fact is a hazard. But why would it be
a hazard to not copy the underlying data ?
In other languages, faliing to copy the underlying data is a serious
correctness hazard. There is a whose class of bugs where things were
not copied, and then mutated and/or reused in multiple places in ways
that were not what the programmer intended. In my experience, this is
a very common bug when writing Python and Javascript. I'm told it's
common in golang too.
But in Rust this bug is much much harder to write. The data inside an
Arc is immutable. To have this bug you'd have use interior mutability
- ie mess around with Mutex or RefCell. That provides a good barrier
to these kind of accidents.
II. "The reason for writing Rc::clone and Arc::clone [is] to make it
clear that only the pointer is being cloned, as opposed to the
underlying data. The former is always fast, while the latter can
be very expensive depending on what is being cloned."
This is the reasoning found here
https://github.com/rust-lang/rust-clippy/issues/2048
This is saying that *not* using Arc::clone is hazardous.
Specifically, that a deep clone is a performance hazard.
But for this argument, the lint is precisely backwards. It's linting
the "good" case and asking for it to be written in a more explicit
way; while the supposedly bad case can be written conveniently.
Also, many objects (in our codebase, and in all the libraries we use)
that are Clone are in fact simply handles. They contain Arc(s) (or
similar) and are cheap to clone. Indeed, that is the usual case.
It does not make sense to distinguish in the syntax we use to clone
such a handle, whether the handle is a transparent Arc, or an opaque
struct containing one or more other handles.
Forcing Arc::clone to be written as such makes for code churn when a
type is changed from Arc<Something> to Something: Clone, or vice
versa.
|
| |
|
|
|
| |
This exception is no longer necessary now that the underlying CI bug
is fixed.
|
| |
|
|
| |
Closes #338.
|
| |
|
|
|
|
| |
This edge-case was there even before the migration of
595fe1ab881b94106649, but now it's more explicit and ought to be
revisited.
|
| | |
|
| |
|
|
|
| |
(By our convention, these errors should say what we were trying to
spawn when the error occurred.)
|
| | |
|
| |
|
|
| |
This makes this like all the others, and is marginally shorter
|
| |
|
|
|
| |
And change the comments to slightly reinterpret these errors, to
relate to the circumstances rather than error generation site.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Previously we stored only one guard sample, in a state file called
"default_guards". That's not future-proof, since we want to have
multiple samples in the future. (`guard-spec.txt` specifies
separate samples for highly restrictive filters, and for bridge
usage.)
This patch changes our behavior so that we can store multiple
samples in a new "guards" file.
I had thought about automatically migrating from the previous file
format and location, but I don't think that's necessary given our
current (lack of) stability guarantees.
Closes #176.
|
| |
|
|
|
|
|
| |
We need this information to know if it's okay to migrate to a new
NetDir, or if we need to download more information first.
Part of #178.
|
| | |
|
| | |
|
| |\
| |
| |
| |
| |
| |
| | |
Squash, refactor, and test !139 (Don't use same family as exit when picking a guard)
Closes #183
See merge request tpo/core/arti!173
|
| | |
| |
| |
| | |
This approach saves us from a linear search when picking guards.
|
| | |
| |
| |
| |
| |
| |
| |
| | |
There's not much reason to use a HashSet here, since we're just
going over the whole list.
This reverts commit 16e8489abbea1581b8e2 and does a little more
refactoring.
|
| | | |
|
| |/
|
|
|
|
|
|
| |
We want to only use TODO in the codebase for non-blockers, and open
tickets for anything that is a bigger blocker than a TODO. These
XXXXs seem like definite non-blockers to me.
Part of arti#231.
|
| | |
|
| | |
|
| |
|
|
|
|
|
| |
Rust nightly claims that Vec might get its own retain_mut method,
which would potentially conflict with the extension method we've
grabbed from the retain_mut crate. To solve this, we're calling the
method explicitly.
|
| | |
|
| |
|
|
| |
params
|
| |
|
|
|
|
|
|
| |
To do this at all neatly, I had to split out `tor-config` from
`arti-config` again, and putting the lower level stuff (paths,
builder errors) into tor-config. I also changed our use of
derive_builder to always use a common error type, to avoid
error type proliferation.
|
| | |
|
| |\ |
|
| | |
| |
| |
| |
| |
| |
| |
| |
| | |
This is based on @eta's patches for !118 and !119: Since we already
have an unbounded channel, we don't need to use an elaborate mess of
one-shot senders. We can just use the unbounded_send() method,
which also lets us enqueue a message without having to await.
Closes #219.
|
| | | |
|
| | | |
|
| |/
|
|
|
|
|
|
|
|
|
|
| |
We define "coming back online" as happening when a guard attempt
succeeds, if that attempt that was launched when we seemed to be
offline.
We define "seeming to be offline" as having all of our primary
guards marked unreachable, and having received no incoming network
traffic in a while.
Closes #216.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
We now track, for every guard: the total number of successful
circuits we've built through it, along with the total number of
"indeterminate" circuits.
Recall that a circuit's status is "indeterminate" if it has failed
for a reason that _might_ be the guard's fault, or might not be the
guard's fault. For example, if extending to the second hop of the
circuit fails, we have no way to know whether the guard deliberately
refused to connect there, or whether the second hop is just offline.
But we don't want to forgive all indeterminate circuit failures: if
we did, then a malicious guard could simply reject any second hops
that it didn't like, thereby filtering the client into a chosen
set of circuits.
As a stopgap solution, this patch now makes guards become
permanently disabled if the fraction of their circuit failures
becomes too high.
See also general-purpose path bias selection (arti#65), and Mike's
idea for changing the guard reachability definition (torspec#67).
This patch doesn't do either of those.
Closes #185.
|