From f5ff50157e3eb950ba98501ca7ed24703b5eebce Mon Sep 17 00:00:00 2001 From: Lucas Soriano del Pino Date: Fri, 6 Nov 2020 18:05:49 +1100 Subject: [PATCH] Some more fixes and comments after testing Alice's recovery --- swap/src/bob.rs | 8 +- swap/src/recover.rs | 77 ++++++++++++++--- swap/tests/e2e.rs | 203 ++++++++++++++++++++++---------------------- 3 files changed, 173 insertions(+), 115 deletions(-) diff --git a/swap/src/bob.rs b/swap/src/bob.rs index 39a380b5..3b5c5936 100644 --- a/swap/src/bob.rs +++ b/swap/src/bob.rs @@ -167,6 +167,9 @@ pub async fn swap( info!("Resumed execution of generator, got: {:?}", state); + // TODO: Protect against transient errors + // TODO: Ignore transaction-already-in-block-chain errors + match state { GeneratorState::Yielded(bob::Action::LockBtc(tx_lock)) => { let signed_tx_lock = bitcoin_wallet.sign_tx_lock(tx_lock).await?; @@ -184,8 +187,9 @@ pub async fn swap( guard.0.send_message3(alice.clone(), tx_redeem_encsig); info!("Sent Bitcoin redeem encsig"); - // TODO: Does Bob need to wait for Alice to send an empty response, or can we - // just continue? + // FIXME: Having to wait for Alice's response here is a big problem, because + // we're stuck if she doesn't send her response back. I believe this is + // currently necessary, so we may have to rework this and/or how we use libp2p match guard.0.next().shared().await { OutEvent::Message3 => { debug!("Got Message3 empty response"); diff --git a/swap/src/recover.rs b/swap/src/recover.rs index 21f34355..4cbfe54c 100644 --- a/swap/src/recover.rs +++ b/swap/src/recover.rs @@ -1,3 +1,15 @@ +//! This module is used to attempt to recover an unfinished swap. +//! +//! Recovery is only supported for certain states and the strategy followed is +//! to perform the simplest steps that require no further action from the +//! counterparty. +//! +//! The quality of this module is bad because there is a lot of code +//! duplication, both within the module and with respect to +//! `xmr_btc/src/{alice,bob}.rs`. In my opinion, a better approach to support +//! swap recovery would be through the `action_generator`s themselves, but this +//! was deemed too complicated for the time being. + use crate::{ monero::CreateWalletForOutput, state::{Alice, Bob, Swap}, @@ -257,7 +269,7 @@ pub async fn alice_recover( TxPunish::new(&tx_cancel, &state.punish_address, state.punish_timelock); let sig_a = state.a.sign(tx_punish.digest()); - let sig_b = state.tx_cancel_sig_bob.clone(); + let sig_b = state.tx_punish_sig_bob.clone(); let sig_tx_punish = tx_punish.add_signatures( &tx_cancel, @@ -282,22 +294,61 @@ pub async fn alice_recover( state.a.public(), state.B.clone(), ); + let tx_refund = TxRefund::new(&tx_cancel, &state.refund_address); - let tx_punish = TxPunish::new(&tx_cancel, &state.punish_address, state.punish_timelock); + info!("Checking if Bitcoin has already been refunded"); - let sig_a = state.a.sign(tx_punish.digest()); - let sig_b = state.tx_punish_sig_bob.clone(); + // TODO: Protect against transient errors so that we can correctly decide if the + // bitcoin has been refunded + match bitcoin_wallet.0.get_raw_transaction(tx_refund.txid()).await { + Ok(tx_refund_published) => { + info!("Bitcoin already refunded"); - let sig_tx_punish = tx_punish.add_signatures( - &tx_cancel, - (state.a.public(), sig_a), - (state.B.clone(), sig_b), - )?; + let s_a = monero::PrivateKey { + scalar: state.s_a.into_ed25519(), + }; - bitcoin_wallet - .broadcast_signed_transaction(sig_tx_punish) - .await?; - info!("Successfully punished Bob's inactivity by taking bitcoin"); + let tx_refund_sig = tx_refund + .extract_signature_by_key(tx_refund_published, state.a.public())?; + let tx_refund_encsig = state + .a + .encsign(state.S_b_bitcoin.clone(), tx_refund.digest()); + + let s_b = xmr_btc::bitcoin::recover( + state.S_b_bitcoin, + tx_refund_sig, + tx_refund_encsig, + )?; + let s_b = monero::PrivateKey::from_scalar( + xmr_btc::monero::Scalar::from_bytes_mod_order(s_b.to_bytes()), + ); + + monero_wallet + .create_and_load_wallet_for_output(s_a + s_b, state.v) + .await?; + info!("Successfully refunded monero"); + } + Err(_) => { + info!("Bitcoin not yet refunded"); + + let tx_punish = + TxPunish::new(&tx_cancel, &state.punish_address, state.punish_timelock); + + let sig_a = state.a.sign(tx_punish.digest()); + let sig_b = state.tx_punish_sig_bob.clone(); + + let sig_tx_punish = tx_punish.add_signatures( + &tx_cancel, + (state.a.public(), sig_a), + (state.B.clone(), sig_b), + )?; + + bitcoin_wallet + .broadcast_signed_transaction(sig_tx_punish) + .await?; + info!("Successfully punished Bob's inactivity by taking bitcoin"); + } + } } Alice::BtcRefunded { view_key, diff --git a/swap/tests/e2e.rs b/swap/tests/e2e.rs index bebb052b..d17ca0db 100644 --- a/swap/tests/e2e.rs +++ b/swap/tests/e2e.rs @@ -1,120 +1,123 @@ -#[cfg(not(feature = "tor"))] -mod e2e_test { - use bitcoin_harness::Bitcoind; - use futures::{channel::mpsc, future::try_join}; - use libp2p::Multiaddr; - use monero_harness::Monero; - use std::sync::Arc; - use swap::{alice, bob, network::transport::build, storage::Database}; - use tempfile::tempdir; - use testcontainers::clients::Cli; +use bitcoin_harness::Bitcoind; +use futures::{channel::mpsc, future::try_join}; +use libp2p::Multiaddr; +use monero_harness::Monero; +use std::sync::Arc; +use swap::{alice, bob, network::transport::build, storage::Database}; +use tempfile::tempdir; +use testcontainers::clients::Cli; - // NOTE: For some reason running these tests overflows the stack. In order to - // mitigate this run them with: - // - // RUST_MIN_STACK=100000000 cargo test +// NOTE: For some reason running these tests overflows the stack. In order to +// mitigate this run them with: +// +// RUST_MIN_STACK=100000000 cargo test - #[tokio::test] - async fn swap() { - let alice_multiaddr: Multiaddr = "/ip4/127.0.0.1/tcp/9876" - .parse() - .expect("failed to parse Alice's address"); +#[tokio::test] +async fn swap() { + use tracing_subscriber::util::SubscriberInitExt as _; + let _guard = tracing_subscriber::fmt() + .with_env_filter("swap=info,xmr_btc=info") + .with_ansi(false) + .set_default(); - let cli = Cli::default(); - let bitcoind = Bitcoind::new(&cli, "0.19.1").unwrap(); - let _ = bitcoind.init(5).await; + let alice_multiaddr: Multiaddr = "/ip4/127.0.0.1/tcp/9876" + .parse() + .expect("failed to parse Alice's address"); - let btc = bitcoin::Amount::from_sat(1_000_000); - let btc_alice = bitcoin::Amount::ZERO; - let btc_bob = btc * 10; + let cli = Cli::default(); + let bitcoind = Bitcoind::new(&cli, "0.19.1").unwrap(); + dbg!(&bitcoind.node_url); + let _ = bitcoind.init(5).await; - // this xmr value matches the logic of alice::calculate_amounts i.e. btc * - // 10_000 * 100 - let xmr = 1_000_000_000_000; - let xmr_alice = xmr * 10; - let xmr_bob = 0; + let btc = bitcoin::Amount::from_sat(1_000_000); + let btc_alice = bitcoin::Amount::ZERO; + let btc_bob = btc * 10; - let alice_btc_wallet = Arc::new( - swap::bitcoin::Wallet::new("alice", bitcoind.node_url.clone()) - .await - .unwrap(), - ); - let bob_btc_wallet = Arc::new( - swap::bitcoin::Wallet::new("bob", bitcoind.node_url.clone()) - .await - .unwrap(), - ); - bitcoind - .mint(bob_btc_wallet.0.new_address().await.unwrap(), btc_bob) + // this xmr value matches the logic of alice::calculate_amounts i.e. btc * + // 10_000 * 100 + let xmr = 1_000_000_000_000; + let xmr_alice = xmr * 10; + let xmr_bob = 0; + + let alice_btc_wallet = Arc::new( + swap::bitcoin::Wallet::new("alice", bitcoind.node_url.clone()) + .await + .unwrap(), + ); + let bob_btc_wallet = Arc::new( + swap::bitcoin::Wallet::new("bob", bitcoind.node_url.clone()) + .await + .unwrap(), + ); + bitcoind + .mint(bob_btc_wallet.0.new_address().await.unwrap(), btc_bob) + .await + .unwrap(); + + let (monero, _container) = + Monero::new(&cli, None, vec!["alice".to_string(), "bob".to_string()]) .await .unwrap(); + monero + .init(vec![("alice", xmr_alice), ("bob", xmr_bob)]) + .await + .unwrap(); - let (monero, _container) = - Monero::new(&cli, None, vec!["alice".to_string(), "bob".to_string()]) - .await - .unwrap(); - monero - .init(vec![("alice", xmr_alice), ("bob", xmr_bob)]) - .await - .unwrap(); + let alice_xmr_wallet = Arc::new(swap::monero::Wallet( + monero.wallet("alice").unwrap().client(), + )); + let bob_xmr_wallet = Arc::new(swap::monero::Wallet(monero.wallet("bob").unwrap().client())); - let alice_xmr_wallet = Arc::new(swap::monero::Wallet( - monero.wallet("alice").unwrap().client(), - )); - let bob_xmr_wallet = Arc::new(swap::monero::Wallet(monero.wallet("bob").unwrap().client())); + let alice_behaviour = alice::Alice::default(); + let alice_transport = build(alice_behaviour.identity()).unwrap(); - let alice_behaviour = alice::Alice::default(); - let alice_transport = build(alice_behaviour.identity()).unwrap(); + let db = Database::open(std::path::Path::new("../.swap-db/")).unwrap(); + let alice_swap = alice::swap( + alice_btc_wallet.clone(), + alice_xmr_wallet.clone(), + db, + alice_multiaddr.clone(), + alice_transport, + alice_behaviour, + ); - let db_dir = tempdir().unwrap(); - let db = Database::open(db_dir.path()).unwrap(); - let alice_swap = alice::swap( - alice_btc_wallet.clone(), - alice_xmr_wallet.clone(), - db, - alice_multiaddr.clone(), - alice_transport, - alice_behaviour, - ); + let db_dir = tempdir().unwrap(); + let db = Database::open(db_dir.path()).unwrap(); + let (cmd_tx, mut _cmd_rx) = mpsc::channel(1); + let (mut rsp_tx, rsp_rx) = mpsc::channel(1); + let bob_behaviour = bob::Bob::default(); + let bob_transport = build(bob_behaviour.identity()).unwrap(); + let bob_swap = bob::swap( + bob_btc_wallet.clone(), + bob_xmr_wallet.clone(), + db, + btc.as_sat(), + alice_multiaddr, + cmd_tx, + rsp_rx, + bob_transport, + bob_behaviour, + ); - let db_dir = tempdir().unwrap(); - let db = Database::open(db_dir.path()).unwrap(); - let (cmd_tx, mut _cmd_rx) = mpsc::channel(1); - let (mut rsp_tx, rsp_rx) = mpsc::channel(1); - let bob_behaviour = bob::Bob::default(); - let bob_transport = build(bob_behaviour.identity()).unwrap(); - let bob_swap = bob::swap( - bob_btc_wallet.clone(), - bob_xmr_wallet.clone(), - db, - btc.as_sat(), - alice_multiaddr, - cmd_tx, - rsp_rx, - bob_transport, - bob_behaviour, - ); + // automate the verification step by accepting any amounts sent over by Alice + rsp_tx.try_send(swap::Rsp::VerifiedAmounts).unwrap(); - // automate the verification step by accepting any amounts sent over by Alice - rsp_tx.try_send(swap::Rsp::VerifiedAmounts).unwrap(); + try_join(alice_swap, bob_swap).await.unwrap(); - try_join(alice_swap, bob_swap).await.unwrap(); + let btc_alice_final = alice_btc_wallet.as_ref().balance().await.unwrap(); + let btc_bob_final = bob_btc_wallet.as_ref().balance().await.unwrap(); - let btc_alice_final = alice_btc_wallet.as_ref().balance().await.unwrap(); - let btc_bob_final = bob_btc_wallet.as_ref().balance().await.unwrap(); + let xmr_alice_final = alice_xmr_wallet.as_ref().get_balance().await.unwrap(); - let xmr_alice_final = alice_xmr_wallet.as_ref().get_balance().await.unwrap(); + bob_xmr_wallet.as_ref().0.refresh().await.unwrap(); + let xmr_bob_final = bob_xmr_wallet.as_ref().get_balance().await.unwrap(); - bob_xmr_wallet.as_ref().0.refresh().await.unwrap(); - let xmr_bob_final = bob_xmr_wallet.as_ref().get_balance().await.unwrap(); + assert_eq!( + btc_alice_final, + btc_alice + btc - bitcoin::Amount::from_sat(xmr_btc::bitcoin::TX_FEE) + ); + assert!(btc_bob_final <= btc_bob - btc); - assert_eq!( - btc_alice_final, - btc_alice + btc - bitcoin::Amount::from_sat(xmr_btc::bitcoin::TX_FEE) - ); - assert!(btc_bob_final <= btc_bob - btc); - - assert!(xmr_alice_final.as_piconero() <= xmr_alice - xmr); - assert_eq!(xmr_bob_final.as_piconero(), xmr_bob + xmr); - } + assert!(xmr_alice_final.as_piconero() <= xmr_alice - xmr); + assert_eq!(xmr_bob_final.as_piconero(), xmr_bob + xmr); }