Skip to content

Commit

Permalink
pcli: improve UX for DKG ceremony
Browse files Browse the repository at this point in the history 
I implemented the suggestion in #4335 and then tested running a ceremony in
three split-pane terminal windows for maximum confusion about which messages go
where.  The resulting code is robust to mistaken entry, ensuring:

- messages are validly parseable (so cannot mix round 1 and round 2, and resilient to typos)
- messages are not duplicated (so pasting twice does not kill the ceremony)
- messages are not self-broadcast (so pasting our own message is ignored)
  • Loading branch information
@hdevalence
hdevalence committed May 8, 2024
1 parent b21800a commit 4381941
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 114 deletions.
13 changes: 5 additions & 8 deletions crates/bin/pcli/src/terminal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,21 @@ 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<()> {
fn explain(&self, msg: &str) -> Result<()> {
println!("{}", msg);
Ok(())
}

async fn broadcast(&self, data: &str) -> Result<()> {
println!("{}", data);
println!("\n{}\n", data);
Ok(())
}

async fn next_response(&self) -> Result<Option<String>> {
async fn read_line_raw(&self) -> Result<String> {
// 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;
Expand Down Expand Up @@ -101,10 +101,7 @@ impl Terminal for ActualTerminal {
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<String> {
Expand Down
48 changes: 41 additions & 7 deletions crates/custody/src/terminal.rs
View file
Original file line number Diff line number Diff line change
@@ -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)]
Expand All @@ -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)
///
Expand All @@ -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<Option<String>>;
/// 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<D>(&self) -> Result<D>
where
D: DomainType,
anyhow::Error: From<<D as TryFrom<<D as DomainType>::Proto>>::Error>,
<D as DomainType>::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::<<D as DomainType>::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<String>;

/// Wait for the user to supply a password.
async fn get_password(&self) -> Result<String>;
Expand Down
175 changes: 76 additions & 99 deletions crates/custody/src/threshold.rs
View file
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -44,15 +44,6 @@ where
Ok(serde_json::to_string(&data.to_proto())?)
}

fn from_json<'a, T: DomainType>(data: &'a str) -> Result<T>
where
T: DomainType,
anyhow::Error: From<<T as TryFrom<<T as DomainType>::Proto>>::Error>,
<T as DomainType>::Proto: Deserialize<'a>,
{
Ok(serde_json::from_str::<<T as DomainType>::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
Expand All @@ -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::<sign::CoordinatorRound1>().await?;
// Pick the right config based on the message
let config = match round1_message.signing_request() {
SigningRequest::TransactionPlan(_) => config.ok_or(anyhow!(
Expand All @@ -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::<sign::CoordinatorRound2>().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(())
Expand All @@ -127,48 +98,72 @@ pub async fn dkg(t: u16, n: u16, terminal: &impl Terminal) -> Result<Config> {
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<dkg::Round1> = 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::<dkg::Round1>().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<dkg::Round2> = 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::<dkg::Round2>().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
};
Expand Down Expand Up @@ -203,52 +198,34 @@ impl<T: Terminal> Threshold<T> {
// 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::<sign::FollowerRound1>::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::<sign::FollowerRound1>(&reply_str)?;
acc.push(reply);
acc.push(self.terminal.next_response().await?);
}
acc
};
// Round 2
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::<sign::FollowerRound2>::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::<sign::FollowerRound2>(&reply_str)?;
acc.push(reply);
acc.push(self.terminal.next_response().await?);
}
acc
};
Expand Down Expand Up @@ -402,7 +379,7 @@ mod test {
Ok(true)
}

async fn explain(&self, _msg: &str) -> Result<()> {
fn explain(&self, _msg: &str) -> Result<()> {
Ok(())
}

Expand All @@ -411,8 +388,8 @@ mod test {
Ok(())
}

async fn next_response(&self) -> Result<Option<String>> {
Ok(self.incoming.lock().await.recv().await)
async fn read_line_raw(&self) -> Result<String> {
Ok(self.incoming.lock().await.recv().await.unwrap_or_default())
}

async fn get_password(&self) -> Result<String> {
Expand Down Expand Up @@ -444,7 +421,7 @@ mod test {
Ok(true)
}

async fn explain(&self, _msg: &str) -> Result<()> {
fn explain(&self, _msg: &str) -> Result<()> {
Ok(())
}

Expand All @@ -455,8 +432,8 @@ mod test {
Ok(())
}

async fn next_response(&self) -> Result<Option<String>> {
Ok(self.incoming.lock().await.recv().await)
async fn read_line_raw(&self) -> Result<String> {
Ok(self.incoming.lock().await.recv().await.unwrap_or_default())
}

async fn get_password(&self) -> Result<String> {
Expand Down

0 comments on commit 4381941

Please sign in to comment.