From 1d0961e32dec3d7d27f4233597157d1df45efb43 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 6 Feb 2025 18:20:22 -0500 Subject: [PATCH 1/6] Explicitly run payjoin-cli v1 tests in coverage.sh The v1 e2e test is feature-gated behind `not(feature = "v2")`, so it doesn't run when --all-features is enabled. This change ensures v1 coverage is reported. --- contrib/coverage.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/coverage.sh b/contrib/coverage.sh index 573ddfbe..ee19b940 100755 --- a/contrib/coverage.sh +++ b/contrib/coverage.sh @@ -4,4 +4,5 @@ set -e # https://github.com/taiki-e/cargo-llvm-cov?tab=readme-ov-file#merge-coverages-generated-under-different-test-conditions cargo llvm-cov clean --workspace # remove artifacts that may affect the coverage results cargo llvm-cov --no-report --all-features +cargo llvm-cov --no-report --package payjoin-cli --no-default-features --features=v1,_danger-local-https # Explicitly run payjoin-cli v1 e2e tests cargo llvm-cov report --lcov --output-path lcov.info # generate report without tests From 1610d003ab1d4c9a0dd6e0db96119594cb1df76b Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 5 Feb 2025 17:20:46 -0500 Subject: [PATCH 2/6] Handle SIGINT gracefully in payjoin-cli v1 Exiting cleanly allows other processes to complete (e.g. coverage reports). --- payjoin-cli/src/app/mod.rs | 9 +++++++++ payjoin-cli/src/app/v1.rs | 16 +++++++++++++--- payjoin-cli/src/app/v2.rs | 10 +--------- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/payjoin-cli/src/app/mod.rs b/payjoin-cli/src/app/mod.rs index 89690d65..4795999e 100644 --- a/payjoin-cli/src/app/mod.rs +++ b/payjoin-cli/src/app/mod.rs @@ -10,6 +10,8 @@ use payjoin::bitcoin::psbt::Psbt; use payjoin::bitcoin::FeeRate; use payjoin::receive::InputPair; use payjoin::{bitcoin, PjUri}; +use tokio::signal; +use tokio::sync::watch; pub mod config; use crate::app::config::AppConfig; @@ -136,3 +138,10 @@ pub fn input_pair_from_list_unspent( }; InputPair::new(txin, psbtin).expect("Input pair should be valid") } + +async fn handle_interrupt(tx: watch::Sender<()>) { + if let Err(e) = signal::ctrl_c().await { + eprintln!("Error setting up Ctrl-C handler: {}", e); + } + let _ = tx.send(()); +} diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index b8b10028..17e60c69 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -20,10 +20,11 @@ use payjoin::receive::Error; use payjoin::send::v1::SenderBuilder; use payjoin::{Uri, UriExt}; use tokio::net::TcpListener; +use tokio::sync::watch; use super::config::AppConfig; use super::App as AppTrait; -use crate::app::{http_agent, input_pair_from_list_unspent}; +use crate::app::{handle_interrupt, http_agent, input_pair_from_list_unspent}; use crate::db::Database; #[cfg(feature = "_danger-local-https")] pub const LOCAL_CERT_FILE: &str = "localhost.der"; @@ -39,13 +40,16 @@ impl payjoin::receive::v1::Headers for Headers<'_> { pub(crate) struct App { config: AppConfig, db: Arc, + interrupt: watch::Receiver<()>, } #[async_trait::async_trait] impl AppTrait for App { fn new(config: AppConfig) -> Result { let db = Arc::new(Database::create(&config.db_path)?); - let app = Self { config, db }; + let (interrupt_tx, interrupt_rx) = watch::channel(()); + tokio::spawn(handle_interrupt(interrupt_tx)); + let app = Self { config, db, interrupt: interrupt_rx }; app.bitcoind()? .get_blockchain_info() .context("Failed to connect to bitcoind. Check config RPC connection.")?; @@ -116,7 +120,13 @@ impl AppTrait for App { ); println!("{}", pj_uri_string); - self.start_http_server().await?; + let mut interrupt = self.interrupt.clone(); + tokio::select! { + res = self.start_http_server() => { res?; } + _ = interrupt.changed() => { + println!("Interrupted."); + } + } Ok(()) } } diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 2de0c085..4da63666 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -10,12 +10,11 @@ use payjoin::receive::v2::{Receiver, UncheckedProposal}; use payjoin::receive::Error; use payjoin::send::v2::{Sender, SenderBuilder}; use payjoin::{bitcoin, Uri}; -use tokio::signal; use tokio::sync::watch; use super::config::AppConfig; use super::App as AppTrait; -use crate::app::{http_agent, input_pair_from_list_unspent}; +use crate::app::{handle_interrupt, http_agent, input_pair_from_list_unspent}; use crate::db::Database; #[derive(Clone)] @@ -399,13 +398,6 @@ async fn unwrap_ohttp_keys_or_else_fetch(config: &AppConfig) -> Result) { - if let Err(e) = signal::ctrl_c().await { - eprintln!("Error setting up Ctrl-C handler: {}", e); - } - let _ = tx.send(()); -} - async fn post_request(req: payjoin::Request) -> Result { let http = http_agent()?; http.post(req.url) From af38c509575efa8fee4ed933c4d2cdca638476b5 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 6 Feb 2025 18:24:38 -0500 Subject: [PATCH 3/6] Have `sigint` wait until process exits cleanly Ensure that the child process completes its handling of the interrupt signal. This prevents the child process from panicking with `failed printing to stdout: Broken pipe (os error 32)` when it tries to print to stdout in the interrupt handler. --- payjoin-cli/tests/e2e.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/payjoin-cli/tests/e2e.rs b/payjoin-cli/tests/e2e.rs index 7c05420e..db55ddb8 100644 --- a/payjoin-cli/tests/e2e.rs +++ b/payjoin-cli/tests/e2e.rs @@ -1,7 +1,7 @@ #[cfg(feature = "_danger-local-https")] mod e2e { use std::env; - use std::process::Stdio; + use std::process::{ExitStatus, Stdio}; use nix::sys::signal::{kill, Signal}; use nix::unistd::Pid; @@ -10,9 +10,11 @@ mod e2e { use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::process::Command; - fn sigint(child: &tokio::process::Child) -> nix::Result<()> { + async fn sigint(mut child: tokio::process::Child) -> tokio::io::Result { let pid = child.id().expect("Failed to get child PID"); - kill(Pid::from_raw(pid as i32), Signal::SIGINT) + kill(Pid::from_raw(pid as i32), Signal::SIGINT)?; + // wait for child process to exit completely + child.wait().await } const RECEIVE_SATS: &str = "54321"; @@ -119,8 +121,9 @@ mod e2e { .unwrap_or(Some(false)) // timed out .expect("rx channel closed prematurely"); // recv() returned None - sigint(&cli_receiver).expect("Failed to kill payjoin-cli"); - sigint(&cli_sender).expect("Failed to kill payjoin-cli"); + sigint(cli_receiver).await.expect("Failed to kill payjoin-cli"); + sigint(cli_sender).await.expect("Failed to kill payjoin-cli"); + payjoin_sent }) .await?; @@ -286,7 +289,7 @@ mod e2e { } log::debug!("Got bip21 {}", &bip21); - sigint(&cli_receiver).expect("Failed to kill payjoin-cli"); + sigint(cli_receiver).await.expect("Failed to kill payjoin-cli"); bip21 } @@ -315,7 +318,7 @@ mod e2e { let timeout = tokio::time::Duration::from_secs(35); let fallback_sent = tokio::time::timeout(timeout, rx.recv()).await?; - sigint(&cli_sender).expect("Failed to kill payjoin-cli initial sender"); + sigint(cli_sender).await.expect("Failed to kill payjoin-cli initial sender"); assert!(fallback_sent.unwrap_or(false), "Fallback send was not detected"); Ok(()) @@ -347,7 +350,7 @@ mod e2e { let timeout = tokio::time::Duration::from_secs(10); let response_successful = tokio::time::timeout(timeout, rx.recv()).await?; - sigint(&cli_receive_resumer).expect("Failed to kill payjoin-cli"); + sigint(cli_receive_resumer).await.expect("Failed to kill payjoin-cli"); assert!(response_successful.unwrap_or(false), "Did not respond with Payjoin PSBT"); Ok(()) @@ -379,7 +382,7 @@ mod e2e { let timeout = tokio::time::Duration::from_secs(10); let payjoin_sent = tokio::time::timeout(timeout, rx.recv()).await?; - sigint(&cli_send_resumer).expect("Failed to kill payjoin-cli"); + sigint(cli_send_resumer).await.expect("Failed to kill payjoin-cli"); assert!(payjoin_sent.unwrap_or(false), "Payjoin send was not detected"); Ok(()) From 9ebf0274cd9fe75025bde761ef3dc1f478b73e94 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 6 Feb 2025 18:25:04 -0500 Subject: [PATCH 4/6] Remove unnecessary tokio spawns in e2e handlers There is no obvious reason to spawn new tasks only for the purpose of reading and writing from/to stdout. More importantly, this behavior previously caused the payjoin-cli interrupt handlers to panic with `failed printing to stdout: Broken pipe (os error 32)` due to `stdout` getting dropped by the time sigint is called. This ensures `stdout` lives long enough for the interrupt message to print. --- payjoin-cli/tests/e2e.rs | 75 ++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/payjoin-cli/tests/e2e.rs b/payjoin-cli/tests/e2e.rs index db55ddb8..b9cb7f0a 100644 --- a/payjoin-cli/tests/e2e.rs +++ b/payjoin-cli/tests/e2e.rs @@ -296,24 +296,21 @@ mod e2e { async fn send_until_request_timeout(mut cli_sender: Child) -> Result<()> { let stdout = cli_sender.stdout.take().expect("Failed to take stdout of child process"); let reader = BufReader::new(stdout); + let mut stdout = tokio::io::stdout(); let (tx, mut rx) = tokio::sync::mpsc::channel(1); let mut lines = reader.lines(); - tokio::spawn(async move { - let mut stdout = tokio::io::stdout(); - while let Some(line) = - lines.next_line().await.expect("Failed to read line from stdout") - { - stdout - .write_all(format!("{}\n", line).as_bytes()) - .await - .expect("Failed to write to stdout"); - if line.contains("No response yet.") { - let _ = tx.send(true).await; - break; - } + while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") + { + stdout + .write_all(format!("{}\n", line).as_bytes()) + .await + .expect("Failed to write to stdout"); + if line.contains("No response yet.") { + let _ = tx.send(true).await; + break; } - }); + } let timeout = tokio::time::Duration::from_secs(35); let fallback_sent = tokio::time::timeout(timeout, rx.recv()).await?; @@ -328,24 +325,21 @@ mod e2e { let stdout = cli_receive_resumer.stdout.take().expect("Failed to take stdout of child process"); let reader = BufReader::new(stdout); + let mut stdout = tokio::io::stdout(); let (tx, mut rx) = tokio::sync::mpsc::channel(1); let mut lines = reader.lines(); - tokio::spawn(async move { - let mut stdout = tokio::io::stdout(); - while let Some(line) = - lines.next_line().await.expect("Failed to read line from stdout") - { - stdout - .write_all(format!("{}\n", line).as_bytes()) - .await - .expect("Failed to write to stdout"); - if line.contains("Response successful") { - let _ = tx.send(true).await; - break; - } + while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") + { + stdout + .write_all(format!("{}\n", line).as_bytes()) + .await + .expect("Failed to write to stdout"); + if line.contains("Response successful") { + let _ = tx.send(true).await; + break; } - }); + } let timeout = tokio::time::Duration::from_secs(10); let response_successful = tokio::time::timeout(timeout, rx.recv()).await?; @@ -360,24 +354,21 @@ mod e2e { let stdout = cli_send_resumer.stdout.take().expect("Failed to take stdout of child process"); let reader = BufReader::new(stdout); + let mut stdout = tokio::io::stdout(); let (tx, mut rx) = tokio::sync::mpsc::channel(1); let mut lines = reader.lines(); - tokio::spawn(async move { - let mut stdout = tokio::io::stdout(); - while let Some(line) = - lines.next_line().await.expect("Failed to read line from stdout") - { - stdout - .write_all(format!("{}\n", line).as_bytes()) - .await - .expect("Failed to write to stdout"); - if line.contains("Payjoin sent") { - let _ = tx.send(true).await; - break; - } + while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") + { + stdout + .write_all(format!("{}\n", line).as_bytes()) + .await + .expect("Failed to write to stdout"); + if line.contains("Payjoin sent") { + let _ = tx.send(true).await; + break; } - }); + } let timeout = tokio::time::Duration::from_secs(10); let payjoin_sent = tokio::time::timeout(timeout, rx.recv()).await?; From 4f4432b4a65b8942705cfc4b350923049623cf32 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 6 Feb 2025 20:14:14 -0500 Subject: [PATCH 5/6] Extract shared logic for matching on stdout line Avoid repetition by introducing `wait_for_stdout_match` which reads from stdout until a match is found against the given pattern. Note: it's important that `child_stdout` lives long enough, until `sigint` is called on the child process. Otherwise the interrupt handler would try to `println!` to a dropped stdout and panic. --- payjoin-cli/tests/e2e.rs | 131 ++++++++++++++++----------------------- 1 file changed, 54 insertions(+), 77 deletions(-) diff --git a/payjoin-cli/tests/e2e.rs b/payjoin-cli/tests/e2e.rs index b9cb7f0a..bc572152 100644 --- a/payjoin-cli/tests/e2e.rs +++ b/payjoin-cli/tests/e2e.rs @@ -146,7 +146,7 @@ mod e2e { use std::path::PathBuf; use payjoin_test_utils::{init_tracing, TestServices}; - use tokio::process::Child; + use tokio::process::{Child, ChildStdout}; type Result = std::result::Result; @@ -266,27 +266,13 @@ mod e2e { } async fn get_bip21_from_receiver(mut cli_receiver: Child) -> String { - let stdout = - cli_receiver.stdout.take().expect("Failed to take stdout of child process"); - let reader = BufReader::new(stdout); - let mut stdout = tokio::io::stdout(); - let mut bip21 = String::new(); - - let mut lines = reader.lines(); - - while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") - { - // Write to stdout regardless - stdout - .write_all(format!("{}\n", line).as_bytes()) - .await - .expect("Failed to write to stdout"); - - if line.to_ascii_uppercase().starts_with("BITCOIN") { - bip21 = line; - break; - } - } + let mut stdout = + cli_receiver.stdout.take().expect("failed to take stdout of child process"); + let bip21 = wait_for_stdout_match(&mut stdout, |line| { + line.to_ascii_uppercase().starts_with("BITCOIN") + }) + .await + .expect("payjoin-cli receiver should output a bitcoin URI"); log::debug!("Got bip21 {}", &bip21); sigint(cli_receiver).await.expect("Failed to kill payjoin-cli"); @@ -294,89 +280,80 @@ mod e2e { } async fn send_until_request_timeout(mut cli_sender: Child) -> Result<()> { - let stdout = cli_sender.stdout.take().expect("Failed to take stdout of child process"); - let reader = BufReader::new(stdout); - let mut stdout = tokio::io::stdout(); - let (tx, mut rx) = tokio::sync::mpsc::channel(1); - - let mut lines = reader.lines(); - while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") - { - stdout - .write_all(format!("{}\n", line).as_bytes()) - .await - .expect("Failed to write to stdout"); - if line.contains("No response yet.") { - let _ = tx.send(true).await; - break; - } - } - + let mut stdout = + cli_sender.stdout.take().expect("failed to take stdout of child process"); let timeout = tokio::time::Duration::from_secs(35); - let fallback_sent = tokio::time::timeout(timeout, rx.recv()).await?; + let res = tokio::time::timeout( + timeout, + wait_for_stdout_match(&mut stdout, |line| line.contains("No response yet.")), + ) + .await?; sigint(cli_sender).await.expect("Failed to kill payjoin-cli initial sender"); - - assert!(fallback_sent.unwrap_or(false), "Fallback send was not detected"); + assert!(res.is_some(), "Fallback send was not detected"); Ok(()) } async fn respond_with_payjoin(mut cli_receive_resumer: Child) -> Result<()> { - let stdout = + let mut stdout = cli_receive_resumer.stdout.take().expect("Failed to take stdout of child process"); - let reader = BufReader::new(stdout); - let mut stdout = tokio::io::stdout(); - let (tx, mut rx) = tokio::sync::mpsc::channel(1); - - let mut lines = reader.lines(); - while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") - { - stdout - .write_all(format!("{}\n", line).as_bytes()) - .await - .expect("Failed to write to stdout"); - if line.contains("Response successful") { - let _ = tx.send(true).await; - break; - } - } - let timeout = tokio::time::Duration::from_secs(10); - let response_successful = tokio::time::timeout(timeout, rx.recv()).await?; + let res = tokio::time::timeout( + timeout, + wait_for_stdout_match(&mut stdout, |line| line.contains("Response successful")), + ) + .await?; sigint(cli_receive_resumer).await.expect("Failed to kill payjoin-cli"); - - assert!(response_successful.unwrap_or(false), "Did not respond with Payjoin PSBT"); + assert!(res.is_some(), "Did not respond with Payjoin PSBT"); Ok(()) } async fn check_payjoin_sent(mut cli_send_resumer: Child) -> Result<()> { - let stdout = + let mut stdout = cli_send_resumer.stdout.take().expect("Failed to take stdout of child process"); - let reader = BufReader::new(stdout); - let mut stdout = tokio::io::stdout(); - let (tx, mut rx) = tokio::sync::mpsc::channel(1); + let timeout = tokio::time::Duration::from_secs(10); + let res = tokio::time::timeout( + timeout, + wait_for_stdout_match(&mut stdout, |line| line.contains("Payjoin sent")), + ) + .await?; + + sigint(cli_send_resumer).await.expect("Failed to kill payjoin-cli"); + assert!(res.is_some(), "Payjoin send was not detected"); + Ok(()) + } + /// Read lines from `child_stdout` until `match_pattern` is found and the corresponding + /// line is returned. + /// Also writes every read line to tokio::io::stdout(); + async fn wait_for_stdout_match( + child_stdout: &mut ChildStdout, + match_pattern: F, + ) -> Option + where + F: Fn(&str) -> bool, + { + let reader = BufReader::new(child_stdout); let mut lines = reader.lines(); + let mut res = None; + + let mut stdout = tokio::io::stdout(); while let Some(line) = lines.next_line().await.expect("Failed to read line from stdout") { + // Write all output to tests stdout stdout .write_all(format!("{}\n", line).as_bytes()) .await .expect("Failed to write to stdout"); - if line.contains("Payjoin sent") { - let _ = tx.send(true).await; + + if match_pattern(&line) { + res = Some(line); break; } } - let timeout = tokio::time::Duration::from_secs(10); - let payjoin_sent = tokio::time::timeout(timeout, rx.recv()).await?; - - sigint(cli_send_resumer).await.expect("Failed to kill payjoin-cli"); - - assert!(payjoin_sent.unwrap_or(false), "Payjoin send was not detected"); - Ok(()) + res } Ok(()) From 1a56abb6c295e33ca99c8c524c1f8332e3301e36 Mon Sep 17 00:00:00 2001 From: spacebear Date: Fri, 7 Feb 2025 10:39:19 -0500 Subject: [PATCH 6/6] Rename `sigint` to `terminate` It no longer just sends a sigint, so rename it to something more descriptive. --- payjoin-cli/tests/e2e.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/payjoin-cli/tests/e2e.rs b/payjoin-cli/tests/e2e.rs index bc572152..095d4982 100644 --- a/payjoin-cli/tests/e2e.rs +++ b/payjoin-cli/tests/e2e.rs @@ -10,7 +10,7 @@ mod e2e { use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::process::Command; - async fn sigint(mut child: tokio::process::Child) -> tokio::io::Result { + async fn terminate(mut child: tokio::process::Child) -> tokio::io::Result { let pid = child.id().expect("Failed to get child PID"); kill(Pid::from_raw(pid as i32), Signal::SIGINT)?; // wait for child process to exit completely @@ -121,8 +121,8 @@ mod e2e { .unwrap_or(Some(false)) // timed out .expect("rx channel closed prematurely"); // recv() returned None - sigint(cli_receiver).await.expect("Failed to kill payjoin-cli"); - sigint(cli_sender).await.expect("Failed to kill payjoin-cli"); + terminate(cli_receiver).await.expect("Failed to kill payjoin-cli"); + terminate(cli_sender).await.expect("Failed to kill payjoin-cli"); payjoin_sent }) @@ -275,7 +275,7 @@ mod e2e { .expect("payjoin-cli receiver should output a bitcoin URI"); log::debug!("Got bip21 {}", &bip21); - sigint(cli_receiver).await.expect("Failed to kill payjoin-cli"); + terminate(cli_receiver).await.expect("Failed to kill payjoin-cli"); bip21 } @@ -289,7 +289,7 @@ mod e2e { ) .await?; - sigint(cli_sender).await.expect("Failed to kill payjoin-cli initial sender"); + terminate(cli_sender).await.expect("Failed to kill payjoin-cli initial sender"); assert!(res.is_some(), "Fallback send was not detected"); Ok(()) } @@ -304,7 +304,7 @@ mod e2e { ) .await?; - sigint(cli_receive_resumer).await.expect("Failed to kill payjoin-cli"); + terminate(cli_receive_resumer).await.expect("Failed to kill payjoin-cli"); assert!(res.is_some(), "Did not respond with Payjoin PSBT"); Ok(()) } @@ -319,7 +319,7 @@ mod e2e { ) .await?; - sigint(cli_send_resumer).await.expect("Failed to kill payjoin-cli"); + terminate(cli_send_resumer).await.expect("Failed to kill payjoin-cli"); assert!(res.is_some(), "Payjoin send was not detected"); Ok(()) }