aboutsummaryrefslogtreecommitdiff
path: root/crates/tor-guardmgr/src
Commit message (Collapse)AuthorAgeFilesLines
* Merge branch 'clippy-allow-arc-clone' into 'main'Nick Mathewson2022-03-011-1/+0
|\ | | | | | | | | Disable clippy::clone_on_ref_ptr See merge request tpo/core/arti!352
| * Disable clippy::clone_on_ref_ptrIan Jackson2022-02-241-1/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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.
* | impl Debug for various internal typesIan Jackson2022-02-251-0/+11
|/ | | | | | | | I wanted this while debugging something. The ad-hoc impl Debug with f.debug_struct is getting repetitive and I've already perpetrated one copy-paste mistake. We should consider using something like the `educe` crate's Clone.
* Remove clippy::needless_borrow exception in CI.Nick Mathewson2022-02-201-1/+0
| | | | | This exception is no longer necessary now that the underlying CI bug is fixed.
* Change deny(clippy::all) to warn(clippy::all).Nick Mathewson2022-02-141-1/+1
| | | | Closes #338.
* Add TODOs on uncertain points about time_since_last_trafficNick Mathewson2022-02-091-0/+1
| | | | | | This edge-case was there even before the migration of 595fe1ab881b94106649, but now it's more explicit and ought to be revisited.
* Remove the use of Mutex in channel unused_since timestampYuan Lyu2022-02-081-5/+10
|
* Make SpawnError wrappers contain a 'spawning' stringNick Mathewson2022-02-041-11/+25
| | | | | (By our convention, these errors should say what we were trying to spawn when the error occurred.)
* errors: impl HasKind for GuardMgrErrorIan Jackson2022-02-041-0/+12
|
* spawn errors: tor-guardmgr: Use formulaic patternIan Jackson2022-02-041-2/+2
| | | | This makes this like all the others, and is marginally shorter
* tor_persist::Error: impl HasKind and adjust commentsIan Jackson2022-02-041-1/+2
| | | | | And change the comments to slightly reinterpret these errors, to relate to the circumstances rather than error generation site.
* Temporarily disable some clippy lints on nightlyIan Jackson2022-02-021-0/+1
|
* Merge branch 'ticket_176_v2' into 'main'Nick Mathewson2022-01-112-63/+144
|\ | | | | | | | | | | | | guardmgr: Use a better persistent data format Closes #176 See merge request tpo/core/arti!233
| * Remove now-unused GuardSet::new().Nick Mathewson2022-01-111-14/+8
| |
| * guardmgr: Use a better persistent data formatNick Mathewson2022-01-112-50/+137
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 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.
* | guardmgr::..::sample_test: Fix intermittent failure.Nick Mathewson2022-01-111-3/+37
|/ | | | | | | | | | | | | | This test should only fail very rarely (around 1/2.4e8) when guards are chosen from a list of 20 with uniform probability. But that wasn't what we were doing on the mock test network: we were choosing from a list of 10 viable guards, with nonuniform probability. As a fix, we change the test network probabilities so that the guards _are_ chosen with a uniform probability for this test, and we use a modified version of the test network where there are indeed 20 Guard-flagged relays with the required DirCache=2 protocol. Closes #276.
* Tests for new guardmgr functionality.Nick Mathewson2022-01-062-0/+83
|
* Add API to check if primary MDs are missing.Nick Mathewson2022-01-063-2/+34
| | | | | | | 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.
* guardmgr: Don't use no-md guards for data circs.Nick Mathewson2022-01-061-5/+31
| | | | | | | If we don't know a current microdescriptor for a guard, we can't use it for multihop circuits, since we don't know its onion keys. This is part of a fix for #178.
* extend lints to include 'clippy::all'Daniel Eades2021-12-281-0/+1
|
* Remove unused started_at PendingRequestNeel Chauhan2021-12-142-12/+2
|
* Make TlsConnector wrap TCP connections, not create its owneta2021-12-071-1/+1
| | | | | | | | | | | | | | | | | | | | `tor-rtcompat`'s `TlsConnector` trait previously included a method to create a TLS-over-TCP connection, which implied creating a TCP stream inside that method. This commit changes that, and makes the function wrap a TCP stream, as returned from the runtime's `TcpProvider` trait implementation, instead. This means you can actually override `TcpProvider` and have it apply to *all* connections Arti makes, which is useful for issues like arti#235 and other cases where you want to have a custom TCP stream implementation. This required updating the mock TCP/TLS types in `tor-rtmock` slightly; due to the change in API, we now store whether a `LocalStream` should actually be a TLS stream inside the stream itself, and check this property on reads/writes in order to detect misuse. The fake TLS wrapper checks this property and removes it in order to "wrap" the stream, making reads and writes work again.
* Merge branch 'bug183a_redux' into 'main'eta2021-12-072-15/+49
|\ | | | | | | | | | | | | 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
| * Tests for new family-related functions.Nick Mathewson2021-12-061-0/+23
| |
| * Use hashset _inside_ GuardRestriction.Nick Mathewson2021-12-062-1/+4
| | | | | | | | This approach saves us from a linear search when picking guards.
| * Change GuardUsage to have Vec of restrictions.Nick Mathewson2021-12-062-32/+26
| | | | | | | | | | | | | | | | 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.
| * Implement guard family restriction codeNeel Chauhan2021-12-062-9/+23
| |
* | Resolve roughly half of the XXXXs.Nick Mathewson2021-12-061-1/+3
|/ | | | | | | | 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.
* add semicolons if nothing returnedDaniel Eades2021-11-252-4/+5
|
* deglob some enums, use concise iteration syntaxDaniel Eades2021-11-251-6/+6
|
* More typo fixes that I forgot to save :(Nick Mathewson2021-11-241-3/+3
|
* Fix a clippy issue on nightlyNick Mathewson2021-11-241-0/+1
|
* Fix a few typos.Nick Mathewson2021-11-242-5/+5
| | | | Also fix some commonwealth spellings that had slipped in.
* Avoid a warning about retain_mut() in nightly.Nick Mathewson2021-11-231-2/+2
| | | | | | | 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.
* Merge remote-tracking branch 'origin/mr/140'Nick Mathewson2021-11-231-1/+13
|\
| * Use guard-extreme-restriction-percentNeel Chauhan2021-11-231-3/+8
| |
| * In guard filtering code, warn if the filter is too small according to guard ↵Neel Chauhan2021-11-221-1/+8
| | | | | | | | params
* | Fix typo in tor-guardmgr comment related to suspicious guardsNeel Chauhan2021-11-221-1/+1
|/
* Move top-level configuration downwards from `arti` to `arti-config`.Nick Mathewson2021-11-181-0/+1
| | | | | | | | 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.
* Fix typosDimitris Apostolou2021-11-121-1/+1
|
* Remove all remaining dbg! instances.Nick Mathewson2021-11-041-2/+0
|
* Merge branch 'bug219'Nick Mathewson2021-11-023-63/+28
|\
| * Refactor tor-guardmgr's inter-task communication.Nick Mathewson2021-11-023-63/+28
| | | | | | | | | | | | | | | | | | 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.
* | tor-circmgr: test ExitPathBuilder with guards.Nick Mathewson2021-11-021-0/+7
| |
* | tor-circmgr: test DirPathBuilder with GuardMgr.Nick Mathewson2021-11-021-1/+2
| |
* | Add a comment to explain the computation of net_has_been_down.Nick Mathewson2021-11-021-0/+5
| |
* | tor-guardmgr: Add tests for a few functions.Nick Mathewson2021-11-022-0/+57
| |
* | Mark primary guards as retriable when we come back online.Nick Mathewson2021-11-023-47/+63
|/ | | | | | | | | | | | 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.
* Improve some documentation linksNick Mathewson2021-10-292-6/+6
| | | | | | | | | Instead of putting a fully qualified name in the text, in most cases we should just use the short name of the type or function we're referring to. In other words, instead of saying [`crate::module::Foo`], we should typically say [`Foo`](crate::module::Foo).
* Update our disclaimers and limitations sections.Nick Mathewson2021-10-271-0/+1
|