From 80105beb73e3921813b87338e69ea0a4d9d1e7cc Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 8 May 2024 09:54:05 -0700 Subject: [PATCH] Better threshold UX (#4352) ## Describe your changes Improves UX for the DKG ceremony based on user feedback, so we can actually tell people to use it. This is a PR on top of #4343 to make it as easy as possible to merge. ## Issue ticket number and link #4335 ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > client-side key generation logic only --- crates/bin/pcli/src/terminal.rs | 35 +++++-- crates/custody/src/terminal.rs | 48 +++++++-- crates/custody/src/threshold.rs | 175 ++++++++++++++------------------ 3 files changed, 142 insertions(+), 116 deletions(-) diff --git a/crates/bin/pcli/src/terminal.rs b/crates/bin/pcli/src/terminal.rs index dd826ceea4..9dc1837622 100644 --- a/crates/bin/pcli/src/terminal.rs +++ b/crates/bin/pcli/src/terminal.rs @@ -2,7 +2,7 @@ use std::io::{IsTerminal, Read, Write}; use anyhow::Result; use penumbra_custody::threshold::{SigningRequest, Terminal}; -use termion::input::TermRead; +use termion::{color, input::TermRead}; use tonic::async_trait; async fn read_password(prompt: &str) -> Result { @@ -48,26 +48,37 @@ impl Terminal for ActualTerminal { println!("Do you approve this {description}?"); println!("{json}"); println!("Press enter to continue"); - self.next_response().await?; + self.read_line_raw().await?; Ok(true) } - async fn explain(&self, msg: &str) -> Result<()> { - println!("{}", msg); + fn explain(&self, msg: &str) -> Result<()> { + println!( + "{}{}{}", + color::Fg(color::Blue), + msg, + color::Fg(color::Reset) + ); Ok(()) } async fn broadcast(&self, data: &str) -> Result<()> { - println!("{}", data); + println!( + "\n{}{}{}\n", + color::Fg(color::Yellow), + data, + color::Fg(color::Reset) + ); Ok(()) } - async fn next_response(&self) -> Result> { + async fn read_line_raw(&self) -> Result { // Use raw mode to allow reading more than 1KB/4KB of data at a time // See https://unix.stackexchange.com/questions/204815/terminal-does-not-accept-pasted-or-typed-lines-of-more-than-1024-characters use termion::raw::IntoRawMode; tracing::debug!("about to enter raw mode for long pasted input"); + print!("{}", color::Fg(color::Red)); // In raw mode, the input is not mirrored into the terminal, so we need // to read char-by-char and echo it back. let mut stdout = std::io::stdout().into_raw_mode()?; @@ -75,10 +86,16 @@ impl Terminal for ActualTerminal { let mut bytes = Vec::with_capacity(8192); for b in std::io::stdin().bytes() { let b = b?; + // In raw mode, we need to handle control characters ourselves + if b == 3 || b == 4 { + // Ctrl-C or Ctrl-D + return Err(anyhow::anyhow!("aborted")); + } // In raw mode, the enter key might generate \r or \n, check either. if b == b'\n' || b == b'\r' { break; } + // Store the byte we read and print it back to the terminal. bytes.push(b); stdout.write(&[b]).unwrap(); // Flushing may not be the most efficient but performance isn't critical here. @@ -89,16 +106,14 @@ impl Terminal for ActualTerminal { // We consumed a newline of some kind but didn't echo it, now print // one out so subsequent output is guaranteed to be on a new line. println!(""); + print!("{}", color::Fg(color::Reset)); tracing::debug!("exited raw mode and returned to cooked mode"); let line = String::from_utf8(bytes)?; tracing::debug!(?line, "read response line"); - if line.is_empty() { - return Ok(None); - } - Ok(Some(line)) + Ok(line) } async fn get_password(&self) -> Result { diff --git a/crates/custody/src/terminal.rs b/crates/custody/src/terminal.rs index b3b313af55..7f9e00f0f8 100644 --- a/crates/custody/src/terminal.rs +++ b/crates/custody/src/terminal.rs @@ -1,7 +1,9 @@ use anyhow::Result; use penumbra_governance::ValidatorVoteBody; +use penumbra_proto::DomainType; use penumbra_stake::validator::Validator; use penumbra_transaction::TransactionPlan; +use serde::de::DeserializeOwned; use tonic::async_trait; #[derive(Debug, Clone)] @@ -15,7 +17,7 @@ pub enum SigningRequest { /// This is mainly used to accommodate the kind of interaction we have with the CLI /// interface, but it can also be plugged in with more general backends. #[async_trait] -pub trait Terminal { +pub trait Terminal: Sync { /// Have a user confirm that they want to sign this transaction or other data (e.g. validator /// definition, validator vote) /// @@ -29,16 +31,48 @@ pub trait Terminal { /// what subsequent data means, and what the user needs to do. /// /// Backends can replace this with a no-op. - async fn explain(&self, msg: &str) -> Result<()>; + fn explain(&self, msg: &str) -> Result<()>; /// Broadcast a message to other users. async fn broadcast(&self, data: &str) -> Result<()>; - /// Wait for a response from *some* other user, it doesn't matter which. - /// - /// This function should not return None spuriously, when it does, - /// it should continue to return None until a message is broadcast. - async fn next_response(&self) -> Result>; + /// Try to read a typed message from the terminal, retrying until + /// the message parses successfully or the user interrupts the program. + async fn next_response(&self) -> Result + where + D: DomainType, + anyhow::Error: From<::Proto>>::Error>, + ::Proto: DeserializeOwned, + { + loop { + // Read a line or bubble up an error if we couldn't. + let line = self.read_line_raw().await?; + + let proto = match serde_json::from_str::<::Proto>(&line) { + Ok(proto) => proto, + Err(e) => { + self.explain(&format!("Error parsing response: {:#}", e))?; + self.explain("Please try again:")?; + continue; + } + }; + + let message = match D::try_from(proto) { + Ok(message) => message, + Err(e) => { + let e: anyhow::Error = e.into(); + self.explain(&format!("Error parsing response: {:#}", e))?; + self.explain("Please try again:")?; + continue; + } + }; + + return Ok(message); + } + } + + /// Read a single line from the terminal. + async fn read_line_raw(&self) -> Result; /// Wait for the user to supply a password. async fn get_password(&self) -> Result; diff --git a/crates/custody/src/threshold.rs b/crates/custody/src/threshold.rs index 17ded7e7be..dd5521674f 100644 --- a/crates/custody/src/threshold.rs +++ b/crates/custody/src/threshold.rs @@ -1,7 +1,7 @@ use anyhow::{anyhow, Result}; use penumbra_transaction::AuthorizationData; use rand_core::OsRng; -use serde::{Deserialize, Serialize}; +use serde::Serialize; use tonic::{async_trait, Request, Response, Status}; use penumbra_keys::{keys::AddressIndex, Address, FullViewingKey}; @@ -44,15 +44,6 @@ where Ok(serde_json::to_string(&data.to_proto())?) } -fn from_json<'a, T: DomainType>(data: &'a str) -> Result -where - T: DomainType, - anyhow::Error: From<::Proto>>::Error>, - ::Proto: Deserialize<'a>, -{ - Ok(serde_json::from_str::<::Proto>(data)?.try_into()?) -} - /// Act as a follower in the signing protocol. /// /// All this function does is produce side effects on the terminal, potentially returning @@ -63,16 +54,8 @@ pub async fn follow( terminal: &impl Terminal, ) -> Result<()> { // Round 1 - terminal - .explain("Paste the coordinator's first message:") - .await?; - let round1_message: sign::CoordinatorRound1 = { - let string = terminal - .next_response() - .await? - .ok_or(anyhow!("expected message from coordinator"))?; - from_json(&string)? - }; + terminal.explain("Paste the coordinator's first message:")?; + let round1_message = terminal.next_response::().await?; // Pick the right config based on the message let config = match round1_message.signing_request() { SigningRequest::TransactionPlan(_) => config.ok_or(anyhow!( @@ -92,25 +75,13 @@ pub async fn follow( return Ok(()); } let (round1_reply, round1_state) = sign::follower_round1(&mut OsRng, config, round1_message)?; - terminal - .explain("Send this message to the coordinator:") - .await?; + terminal.explain("Send this message to the coordinator:")?; terminal.broadcast(&to_json(&round1_reply)?).await?; // Round 2 - terminal - .explain("Paste the coordinator's second message:") - .await?; - let round2_message: sign::CoordinatorRound2 = { - let string = terminal - .next_response() - .await? - .ok_or(anyhow!("expected message from coordinator"))?; - from_json(&string)? - }; + terminal.explain("Paste the coordinator's second message:")?; + let round2_message = terminal.next_response::().await?; let round2_reply = sign::follower_round2(config, round1_state, round2_message)?; - terminal - .explain("Send this message to the coordinator:") - .await?; + terminal.explain("Send this message to the coordinator:")?; terminal.broadcast(&to_json(&round2_reply)?).await?; Ok(()) @@ -127,48 +98,72 @@ pub async fn dkg(t: u16, n: u16, terminal: &impl Terminal) -> Result { let expected_responses = n.saturating_sub(1) as usize; // Round 1 top let (round1_message, state) = dkg::round1(&mut OsRng, t, n)?; - terminal - .explain("Round 1/2: Send this message to all other participants:") - .await?; + terminal.explain("Round 1/2: Send this message to all other participants:")?; terminal.broadcast(&to_json(&round1_message)?).await?; // Round 1 bottom - terminal - .explain(&format!( - "Round 1/2: Gather {expected_responses} messages from the other participants:" - )) - .await?; + terminal.explain(&format!( + "Round 1/2: Gather {expected_responses} messages from the other participants:" + ))?; let round1_replies = { let mut acc: Vec = Vec::new(); while acc.len() < expected_responses { - let string = terminal - .next_response() - .await? - .ok_or(anyhow!("expected message from another participant"))?; - acc.push(from_json(&string)?); + let rsp = terminal.next_response::().await?; + // Before we accept, check that the user hasn't double-pasted the same message. + if acc + .iter() + // Inefficient but good enough. + .any(|existing| existing.encode_to_vec() == rsp.encode_to_vec()) + { + terminal.explain("Received a duplicate message, ignoring")?; + continue; + } + // Before we accept, check that the user hasn't pasted their own message. + if round1_message.encode_to_vec() == rsp.encode_to_vec() { + terminal.explain("Received our own outbound message by mistake, ignoring")?; + continue; + } + acc.push(rsp); + terminal.explain(&format!( + "Received {}/{} responses...", + acc.len(), + expected_responses + ))?; } acc }; // Round 2 top let (round2_message, state) = dkg::round2(&mut OsRng, state, round1_replies)?; - terminal - .explain("Round 2/2: Send this message to all other participants:") - .await?; + terminal.explain("Round 2/2: Send this message to all other participants:")?; terminal.broadcast(&to_json(&round2_message)?).await?; // Round 2 bottom - terminal - .explain(&format!( - "Round 2/2: Gather {expected_responses} messages from the other participants:" - )) - .await?; + terminal.explain(&format!( + "Round 2/2: Gather {expected_responses} messages from the other participants:" + ))?; let round2_replies = { let mut acc: Vec = Vec::new(); while acc.len() < expected_responses { - let string = terminal - .next_response() - .await? - .ok_or(anyhow!("expected message from another participant"))?; - acc.push(from_json(&string)?); + let rsp = terminal.next_response::().await?; + // Before we accept, check that the user hasn't double-pasted the same message. + if acc + .iter() + // Inefficient but good enough. + .any(|existing| existing.encode_to_vec() == rsp.encode_to_vec()) + { + terminal.explain("Received a duplicate message, ignoring")?; + continue; + } + // Before we accept, check that the user hasn't pasted their own message. + if round2_message.encode_to_vec() == rsp.encode_to_vec() { + terminal.explain("Received our own outbound message by mistake, ignoring")?; + continue; + } + acc.push(rsp); + terminal.explain(&format!( + "Received {}/{} responses...", + acc.len(), + expected_responses + ))?; } acc }; @@ -203,26 +198,17 @@ impl Threshold { // Round 1 let (round1_message, state1) = sign::coordinator_round1(&mut OsRng, &self.config, request)?; self.terminal - .explain("Send this message to the other signers:") - .await?; + .explain("Send this message to the other signers:")?; self.terminal.broadcast(&to_json(&round1_message)?).await?; - self.terminal - .explain(&format!( - "Now, gather at least {} replies from the other signers, and paste them below:", - self.config.threshold() - 1 - )) - .await?; + self.terminal.explain(&format!( + "Now, gather at least {} replies from the other signers, and paste them below:", + self.config.threshold() - 1 + ))?; let round1_replies = { - let mut acc = Vec::new(); + let mut acc = Vec::::new(); // We need 1 less, since we've already included ourselves. for _ in 1..self.config.threshold() { - let reply_str = self - .terminal - .next_response() - .await? - .ok_or(anyhow!("expected round1 reply"))?; - let reply = from_json::(&reply_str)?; - acc.push(reply); + acc.push(self.terminal.next_response().await?); } acc }; @@ -230,25 +216,16 @@ impl Threshold { let (round2_message, state2) = sign::coordinator_round2(&self.config, state1, &round1_replies)?; self.terminal - .explain("Send this message to the other signers:") - .await?; + .explain("Send this message to the other signers:")?; self.terminal.broadcast(&to_json(&round2_message)?).await?; - self.terminal - .explain( - "Now, gather the replies from the *same* signers as Round 1, and paste them below:", - ) - .await?; + self.terminal.explain( + "Now, gather the replies from the *same* signers as Round 1, and paste them below:", + )?; let round2_replies = { - let mut acc = Vec::new(); + let mut acc = Vec::::new(); // We need 1 less, since we've already included ourselves. for _ in 1..self.config.threshold() { - let reply_str = self - .terminal - .next_response() - .await? - .ok_or(anyhow!("expected round2 reply"))?; - let reply = from_json::(&reply_str)?; - acc.push(reply); + acc.push(self.terminal.next_response().await?); } acc }; @@ -402,7 +379,7 @@ mod test { Ok(true) } - async fn explain(&self, _msg: &str) -> Result<()> { + fn explain(&self, _msg: &str) -> Result<()> { Ok(()) } @@ -411,8 +388,8 @@ mod test { Ok(()) } - async fn next_response(&self) -> Result> { - Ok(self.incoming.lock().await.recv().await) + async fn read_line_raw(&self) -> Result { + Ok(self.incoming.lock().await.recv().await.unwrap_or_default()) } async fn get_password(&self) -> Result { @@ -444,7 +421,7 @@ mod test { Ok(true) } - async fn explain(&self, _msg: &str) -> Result<()> { + fn explain(&self, _msg: &str) -> Result<()> { Ok(()) } @@ -455,8 +432,8 @@ mod test { Ok(()) } - async fn next_response(&self) -> Result> { - Ok(self.incoming.lock().await.recv().await) + async fn read_line_raw(&self) -> Result { + Ok(self.incoming.lock().await.recv().await.unwrap_or_default()) } async fn get_password(&self) -> Result {