Merge branch 'proto-choose-hop' into 'main'

tor-proto: Make start_conversation_last_hop() use a given hop, not the last.

Closes #959

See merge request tpo/core/arti!1469

crates/tor-hsclient/src/connect.rs

@@ -1009,7 +1009,7 @@ impl<'c, R: Runtime, M: MocksForConnect<R>> Context<'c, R, M> {
             .await
             .map_err(handle_proto_error)?;
 
-        // `start_conversation_last_hop` returns as soon as the control message has been sent.
+        // `start_conversation` returns as soon as the control message has been sent.
         // We need to obtain the RENDEZVOUS_ESTABLISHED message, which is "returned" via the oneshot.
         let _: RendezvousEstablished = rend_established_rx.recv(handle_proto_error).await?;
 
@@ -1337,7 +1337,7 @@ trait MocksForConnect<R>: Clone {
 /// Mock for `HsCircPool`
 ///
 /// Methods start with `m_` to avoid the following problem:
-/// `ClientCirc::start_conversation_last_hop` (say) means
+/// `ClientCirc::start_conversation` (say) means
 /// to use the inherent method if one exists,
 /// but will use a trait method if there isn't an inherent method.
 ///
@@ -1435,7 +1435,8 @@ impl MockableClientCirc for ClientCirc {
         msg: Option<AnyRelayMsg>,
         reply_handler: impl MsgHandler + Send + 'static,
     ) -> tor_proto::Result<Self::Conversation<'_>> {
-        ClientCirc::start_conversation_last_hop(self, msg, reply_handler).await
+        let last_hop = self.last_hop_num()?;
+        ClientCirc::start_conversation(self, msg, reply_handler, last_hop).await
     }
     type Conversation<'r> = tor_proto::circuit::Conversation<'r>;
 

crates/tor-proto/Cargo.toml

@@ -42,7 +42,7 @@ hs-client = ["hs-common"]
 hs-service = ["hs-common", "__is_experimental"]
 hs-common = ["tor-hscrypto"]
 experimental-api = ["__is_experimental"]
-# start_conversation_last_hop etc.; TODO HS should be renamed
+# start_conversation etc.; TODO HS should be renamed
 send-control-msg = ["visibility"]
 stream-ctrl = ["__is_experimental"]
 # Enable testing-only APIs.  APIs under this feature are not

crates/tor-proto/semver.md

@@ -6,3 +6,8 @@ BREAKING: `IncomingStreamRequest::reject` is now async, takes `&mut self`,
 and returns a `Result`
 BREAKING: `ClientCirc::allow_stream_requests` now expects `self` to be
 `&Arc<ClientCirc>`
+ADDED: `HopNum` is now public
+ADDED: `ClientCirc::last_hop_num`
+DEPRECATED: `ClientCirc::start_conversation_last_hop()`
+ADDED: `ClientCirc::start_conversation()` to eventually replace
+`ClientCirc::start_conversation_last_hop()`

crates/tor-proto/src/circuit.rs

@@ -283,6 +283,20 @@ impl ClientCirc {
         }
     }
 
+    /// Return the [`HopNum`](crate::HopNum) of the last hop of this circuit.
+    ///
+    /// Returns an error if there is no last hop.  (This should be impossible outside of the
+    /// tor-proto crate, but within the crate it's possible to have a circuit with no hops.)
+    pub fn last_hop_num(&self) -> Result<HopNum> {
+        Ok(self
+            .mutable
+            .lock()
+            .expect("poisoned lock")
+            .path
+            .last_hop_num()
+            .ok_or_else(|| internal!("no last hop index"))?)
+    }
+
     /// Return a description of all the hops in this circuit.
     ///
     /// This method is **deprecated** for several reasons:
@@ -325,7 +339,7 @@ impl ClientCirc {
         &self.channel
     }
 
-    /// Start an ad-hoc protocol exchange to the final hop on this circuit
+    /// Start an ad-hoc protocol exchange to the specified hop on this circuit
     ///
     /// To use this:
     ///
@@ -333,7 +347,7 @@ impl ClientCirc {
     ///     the outcome of your conversation,
     ///     and bundle it into a [`MsgHandler`].
     ///
-    ///  1. Call `start_conversation_last_hop`.
+    ///  1. Call `start_conversation`.
     ///     This will install a your handler, for incoming messages,
     ///     and send the outgoing message (if you provided one).
     ///     After that, each message on the circuit
@@ -341,7 +355,7 @@ impl ClientCirc {
     ///     is passed to your provided `reply_handler`.
     ///
     ///  2. Possibly call `send_msg` on the [`Conversation`],
-    ///     from the call site of `start_conversation_last_hop`,
+    ///     from the call site of `start_conversation`,
     ///     possibly multiple times, from time to time,
     ///     to send further desired messages to the peer.
     ///
@@ -376,7 +390,7 @@ impl ClientCirc {
     /// while the conversation is in progress.
     ///
     /// After the conversation has finished, the circuit may be extended.
-    /// Or, `start_conversation_last_hop` may be called again;
+    /// Or, `start_conversation` may be called again;
     /// but, in that case there will be a gap between the two conversations,
     /// during which no `MsgHandler` is installed,
     /// and unexpected incoming messages would close the circuit.
@@ -385,7 +399,7 @@ impl ClientCirc {
     ///
     /// ## Precise definition of the lifetime of a conversation
     ///
-    /// A conversation is in progress from entry to `start_conversation_last_hop`,
+    /// A conversation is in progress from entry to `start_conversation`,
     /// until entry to the body of the [`MsgHandler::handle_msg`]
     /// call which returns [`ConversationFinished`](MetaCellDisposition::ConversationFinished).
     /// (*Entry* since `handle_msg` is synchronously embedded
@@ -401,11 +415,24 @@ impl ClientCirc {
     //
     // TODO hs: it might be nice to avoid exposing tor-cell APIs in the
     //   tor-proto interface.
-    //
-    // TODO hs: Possibly this function should use
-    // HopNum or similar to indicate which hop we're talking to, rather than
-    // just doing "the last hop".
     #[cfg(feature = "send-control-msg")]
+    pub async fn start_conversation(
+        &self,
+        msg: Option<tor_cell::relaycell::msg::AnyRelayMsg>,
+        reply_handler: impl MsgHandler + Send + 'static,
+        hop_num: HopNum,
+    ) -> Result<Conversation<'_>> {
+        let handler = Box::new(msghandler::UserMsgHandler::new(hop_num, reply_handler));
+        let conversation = Conversation(self);
+        conversation.send_internal(msg, Some(handler)).await?;
+        Ok(conversation)
+    }
+
+    /// Start an ad-hoc protocol exchange to the final hop on this circuit
+    ///
+    /// See the [`ClientCirc::start_conversation`] docs for more information.
+    #[cfg(feature = "send-control-msg")]
+    #[deprecated(since = "0.13.0", note = "Use start_conversation instead.")]
     pub async fn start_conversation_last_hop(
         &self,
         msg: Option<tor_cell::relaycell::msg::AnyRelayMsg>,
@@ -418,10 +445,8 @@ impl ClientCirc {
             .path
             .last_hop_num()
             .ok_or_else(|| internal!("no last hop index"))?;
-        let handler = Box::new(msghandler::UserMsgHandler::new(last_hop, reply_handler));
-        let conversation = Conversation(self);
-        conversation.send_internal(msg, Some(handler)).await?;
-        Ok(conversation)
+
+        self.start_conversation(msg, reply_handler, last_hop).await
     }
 
     /// Tell this circuit to begin allowing the final hop of the circuit to try
@@ -781,7 +806,7 @@ impl ClientCirc {
 
 /// Handle to use during an ongoing protocol exchange with a circuit's last hop
 ///
-/// This is obtained from [`ClientCirc::start_conversation_last_hop`],
+/// This is obtained from [`ClientCirc::start_conversation`],
 /// and used to send messages to the last hop relay.
 ///
 /// See also [`ConversationInHandler`], which is a type used for the same purpose
@@ -803,7 +828,7 @@ impl Conversation<'_> {
 
     /// Send a `SendMsgAndInstallHandler` to the reactor and wait for the outcome
     ///
-    /// The guts of `start_conversation_last_hop` and `Conversation::send_msg`
+    /// The guts of `start_conversation` and `Conversation::send_msg`
     async fn send_internal(
         &self,
         msg: Option<tor_cell::relaycell::msg::AnyRelayMsg>,

crates/tor-proto/src/circuit/msghandler.rs

@@ -1,5 +1,5 @@
 //! A message handler trait for use with
-//! [`ClientCirc::start_conversation_last_hop`](super::ClientCirc::start_conversation_last_hop).
+//! [`ClientCirc::start_conversation`](super::ClientCirc::start_conversation).
 //!
 //! Although this is similar to `stream::cmdcheck`, I am deliberately leaving
 //! them separate. Conceivably they should be unified at some point down the
@@ -18,7 +18,7 @@ use super::{ConversationInHandler, MetaCellDisposition};
 /// circuit, and delivers them to a client if so.
 ///
 /// The handler is supplied to
-/// [`ClientCirc::start_conversation_last_hop`](super::ClientCirc::start_conversation_last_hop).  It
+/// [`ClientCirc::start_conversation`](super::ClientCirc::start_conversation).  It
 /// is used to check any incoming message whose stream ID is 0, and which would
 /// otherwise not be accepted on a given circuit.
 ///

crates/tor-proto/src/circuit/reactor.rs

@@ -287,7 +287,7 @@ impl CircHop {
 /// This is passed to `MsgHandler::handle_msg`.
 ///
 /// See also [`ConversationInHandler`], which is a type used for the same purpose
-/// but available to the caller of `start_conversation_last_hop`
+/// but available to the caller of `start_conversation`
 //
 // This is the subset of the arguments to MetaCellHandler::handle_msg
 // which are needed to be able to call send_relay_cell.

crates/tor-proto/src/crypto/cell.rs

@@ -116,7 +116,7 @@ pub(crate) trait InboundClientLayer {
 ///
 /// Hop indices are zero-based: "0" denotes the first hop on the circuit.
 #[derive(Copy, Clone, Eq, PartialEq, Debug)]
-pub(crate) struct HopNum(u8);
+pub struct HopNum(u8);
 
 impl From<HopNum> for u8 {
     fn from(hop: HopNum) -> u8 {

crates/tor-proto/src/lib.rs

@@ -50,6 +50,7 @@ pub use util::err::{Error, ResolveError};
 pub use util::skew::ClockSkew;
 
 pub use channel::params::ChannelPaddingInstructions;
+pub use crypto::cell::HopNum;
 
 /// A Result type for this crate.
 pub type Result<T> = std::result::Result<T, Error>;