From 6694e4f4e06b6cc4aa9e195287475fc25a6a42d5 Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Wed, 19 May 2021 16:21:22 +1000 Subject: [PATCH] Ensure that output of lock script is at tx-output index `0` We subscribe to transactions upon broadcast, where we use output index `0` for the subscription. In order to ensure that this subscription is guaranteed to be for the locking script (and not a change output) we now ensure that the locking script output is always at index `0` of the outputs of the transaction. We chose this solution because otherwise we would have to add more information to broadcasting a transaction. This solution is less intrusive, because the order of transaction outputs should not have any side effects and ensuring index `0` makes the whole behaviour more deterministic. --- swap/src/bitcoin/wallet.rs | 101 +++++++++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 15 deletions(-) diff --git a/swap/src/bitcoin/wallet.rs b/swap/src/bitcoin/wallet.rs index 59fac072..6268d64e 100644 --- a/swap/src/bitcoin/wallet.rs +++ b/swap/src/bitcoin/wallet.rs @@ -16,6 +16,7 @@ use reqwest::Url; use rust_decimal::prelude::*; use rust_decimal::Decimal; use rust_decimal_macros::dec; +use std::cmp::Ordering; use std::collections::{BTreeMap, HashMap}; use std::convert::TryFrom; use std::fmt; @@ -108,18 +109,6 @@ impl Wallet { Ok((txid, subscription)) } - pub async fn sign_and_finalize(&self, psbt: PartiallySignedTransaction) -> Result { - let (signed_psbt, finalized) = self.wallet.lock().await.sign(psbt, None)?; - - if !finalized { - bail!("PSBT is not finalized") - } - - let tx = signed_psbt.extract_tx(); - - Ok(tx) - } - pub async fn get_raw_transaction(&self, txid: Txid) -> Result { self.get_tx(txid) .await? @@ -257,6 +246,18 @@ where C: EstimateFeeRate, D: BatchDatabase, { + pub async fn sign_and_finalize(&self, psbt: PartiallySignedTransaction) -> Result { + let (signed_psbt, finalized) = self.wallet.lock().await.sign(psbt, None)?; + + if !finalized { + bail!("PSBT is not finalized") + } + + let tx = signed_psbt.extract_tx(); + + Ok(tx) + } + pub async fn balance(&self) -> Result { let balance = self .wallet @@ -293,6 +294,10 @@ where Ok(Amount::from_sat(fees)) } + /// Builds a partially signed transaction + /// + /// Ensures that the address script is at output index `0` + /// for the partially signed transaction. pub async fn send_to_address( &self, address: Address, @@ -301,11 +306,30 @@ where let wallet = self.wallet.lock().await; let client = self.client.lock().await; let fee_rate = client.estimate_feerate(self.target_block)?; + let script = address.script_pubkey(); let mut tx_builder = wallet.build_tx(); - tx_builder.add_recipient(address.script_pubkey(), amount.as_sat()); + tx_builder.add_recipient(script.clone(), amount.as_sat()); tx_builder.fee_rate(fee_rate); let (psbt, _details) = tx_builder.finish()?; + let mut psbt: PartiallySignedTransaction = psbt; + + // When subscribing to transactions we depend on the relevant script being at + // output index 0, thus we ensure the relevant output to be at index `0`. + psbt.outputs.sort_by(|a, _| { + if a.witness_script.as_ref() == Some(&script) { + Ordering::Less + } else { + Ordering::Greater + } + }); + psbt.global.unsigned_tx.output.sort_by(|a, _| { + if a.script_pubkey == script { + Ordering::Less + } else { + Ordering::Greater + } + }); Ok(psbt) } @@ -480,7 +504,7 @@ where use bitcoin::OutPoint; use testutils::testutils; - let descriptors = testutils!(@descriptors ("wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)")); + let descriptors = testutils!(@descriptors ("wpkh(tprv8ZgxMBicQKsPeZRHk4rTG6orPS2CRNFX3njhUXx5vj9qGog5ZMH4uGReDWN5kCkY3jmWEtWause41CDvBRXD1shKknAMKxT99o9qUTRVC6m/*)")); let mut database = MemoryDatabase::new(); bdk::populate_test_db!( @@ -760,7 +784,7 @@ impl fmt::Display for ScriptStatus { #[cfg(test)] mod tests { use super::*; - use crate::bitcoin::TxLock; + use crate::bitcoin::{PublicKey, TxLock}; use proptest::prelude::*; #[test] @@ -990,4 +1014,51 @@ mod tests { assert!(amount.as_sat() > 0); } + + /// This test ensures that the relevant script output of the transaction + /// created out of the PSBT is at index 0. This is important because + /// subscriptions to the transaction are on index `0` when broadcasting the + /// transaction. + #[tokio::test] + async fn given_amounts_with_change_outputs_when_signing_tx_then_output_index_0_is_ensured_for_script( + ) { + // We don't care about fees in this test, thus use a zero fee rate + struct NoFeeRate(); + impl EstimateFeeRate for NoFeeRate { + fn estimate_feerate(&self, _target_block: usize) -> Result { + Ok(FeeRate::from_sat_per_vb(0.0)) + } + + fn min_relay_fee(&self) -> Result { + Ok(bitcoin::Amount::from_sat(0)) + } + } + + // This value is somewhat arbitrary but the indexation problem usually occurred + // on the first or second value (i.e. 547, 548) We keep the test + // iterations relatively low because these tests are expensive. + let above_dust = 547; + let balance = 2000; + + let wallet = Wallet::new_funded(balance, NoFeeRate()); + + // sorting is only relevant for amounts that have a change output + // if the change output is below dust it will be dropped by the BDK + for amount in above_dust..(balance - (above_dust - 1)) { + let (A, B) = (PublicKey::random(), PublicKey::random()); + let txlock = TxLock::new(&wallet, bitcoin::Amount::from_sat(amount), A, B) + .await + .unwrap(); + let txlock_output = txlock.script_pubkey(); + + let tx = wallet.sign_and_finalize(txlock.into()).await.unwrap(); + let tx_output = tx.output[0].script_pubkey.clone(); + + assert_eq!( + tx_output, txlock_output, + "Output {:?} index mismatch for amount {} and balance {}", + tx.output, amount, balance + ); + } + } }