Refactor and document issues with modify_hop.

At the site of modify_hop, we now have a comment explaining the
internal-error issue.

To make the internal error less likely, we lower the modify_hop call
in lib.rs into GuardSet, where it can make sure it's looking at the
same filter as was used to select the guard.

The function name "pick_guard_ext" is not permanent; I'm going to
rename it in the next commit.

crates/tor-guardmgr/src/filter.rs

@@ -104,7 +104,11 @@ impl SingleFilter {
         }
     }
 
-    /// Modify `first_hop` so that it contains no elements not permitted by this filter.
+    /// Modify `first_hop` so that it contains no elements not permitted by this
+    /// filter.
+    ///
+    /// It is an internal error to call this function on a guard not already
+    /// passed by `self.permits()`.
     fn modify_hop(
         &self,
         mut first_hop: crate::FirstHop,
@@ -115,6 +119,18 @@ impl SingleFilter {
                     .orports
                     .retain(|addr| patterns.iter().any(|pat| pat.matches_sockaddr(addr)));
                 if first_hop.orports.is_empty() {
+                    // TODO(nickm): The fact that this check needs to be checked
+                    // happen indicates a likely problem in our code design.
+                    // Right now, we have `modify_hop` and `permits` as separate
+                    // methods because our GuardSet logic needs a way to check
+                    // whether a guard will be permitted by a filter without
+                    // actually altering that guard (since another filter might
+                    // be used in the future that would allow the same guard).
+                    //
+                    // To mitigate the risk of hitting this error, we try to
+                    // make sure that modify_hop is always called right after
+                    // (or at least soon after) the filter is checked, with the
+                    // same filter object.
                     return Err(tor_error::internal!(
                         "Tried to apply an address filter to an unsupported guard"
                     )

crates/tor-guardmgr/src/lib.rs

@@ -1179,19 +1179,9 @@ impl GuardMgrInner {
         usage: &GuardUsage,
         now: Instant,
     ) -> Result<(sample::ListKind, FirstHop), PickGuardError> {
-        let (source, id) = self
-            .guards
+        self.guards
             .active_guards()
-            .pick_guard(usage, &self.params, now)?;
-        let guard = self
-            .guards
-            .active_guards()
-            .get(&id)
-            .expect("Selected guard that wasn't in our sample!?")
-            .get_external_rep();
-        let guard = self.guards.active_guards().filter().modify_hop(guard)?;
-
-        Ok((source, guard))
+            .pick_guard_ext(usage, &self.params, now)
     }
 
     /// Helper: Select a fallback directory.

crates/tor-guardmgr/src/sample.rs

@@ -4,6 +4,7 @@
 use crate::filter::GuardFilter;
 use crate::guard::{Guard, NewlyConfirmed, Reachable};
 use crate::skew::SkewObservation;
+use crate::FirstHop;
 use crate::{
     ids::GuardId, ExternalActivity, GuardParams, GuardUsage, GuardUsageKind, PickGuardError,
 };
@@ -747,10 +748,30 @@ impl GuardSet {
         Some(false)
     }
 
+    /// Try to select a guard for a given `usage`.
+    ///
+    /// On success, returns the kind of guard that we got, and its filtered
+    /// representation in a form suitable for use as a first hop.
+    pub(crate) fn pick_guard_ext(
+        &self,
+        usage: &GuardUsage,
+        params: &GuardParams,
+        now: Instant,
+    ) -> Result<(ListKind, FirstHop), PickGuardError> {
+        let (list_kind, id) = self.pick_guard(usage, params, now)?;
+        let first_hop = self
+            .get(&id)
+            .expect("Somehow selected a guard we don't know!")
+            .get_external_rep();
+        let first_hop = self.active_filter.modify_hop(first_hop)?;
+
+        Ok((list_kind, first_hop))
+    }
+
     /// Try to select a guard for a given `usage`.
     ///
     /// On success, returns the kind of guard that we got, and its identity.
-    pub(crate) fn pick_guard(
+    fn pick_guard(
         &self,
         usage: &GuardUsage,
         params: &GuardParams,