From 5e033f22a1d47b0b277322356d0b5b0ce9c581de Mon Sep 17 00:00:00 2001 From: valued mammal Date: Sat, 19 Apr 2025 07:44:33 -0400 Subject: [PATCH] Add docs + light review --- src/input.rs | 27 +++++++++++++++++---------- src/input_candidates.rs | 1 + src/output.rs | 8 ++++---- src/rbf.rs | 1 + src/selection.rs | 14 ++++++++------ src/selector.rs | 19 ++++++++----------- tests/synopsis.rs | 16 +++++++++------- 7 files changed, 48 insertions(+), 38 deletions(-) diff --git a/src/input.rs b/src/input.rs index 50abd2d..1d5b568 100644 --- a/src/input.rs +++ b/src/input.rs @@ -7,7 +7,7 @@ use miniscript::bitcoin; use miniscript::bitcoin::{OutPoint, Transaction, TxOut}; use miniscript::plan::Plan; -/// Confirmation status of a tx. +/// Confirmation status of a tx input. #[derive(Debug, Clone, Copy)] pub struct InputStatus { /// Confirmation block height. @@ -20,10 +20,11 @@ pub struct InputStatus { } impl InputStatus { - /// Helper method. + /// From consensus `height` and `time`. pub fn new(height: u32, time: u64) -> Result { Ok(Self { height: absolute::Height::from_consensus(height)?, + // TODO: handle `.try_into::()` time: absolute::Time::from_consensus(time as _)?, }) } @@ -46,7 +47,7 @@ impl PlanOrPsbtInput { /// TODO: Check whether satisfaction_weight calculations are correct. /// TODO: Return error type: invalid outpoint, no `witness_utxo` or `non_witness_utxo`, etc. /// - /// # WHy do we only support finalized psbt inputs? + /// # Why do we only support finalized psbt inputs? /// /// There is no mulit-party tx building protocol that requires choosing from foreign, /// non-finalized PSBT inputs. @@ -139,9 +140,12 @@ pub struct Input { } impl Input { - /// Create + /// Create [`Input`] from a previous transaction. /// - /// Returns `None` if `prev_output_index` does not exist in `prev_tx`. + /// # Errors + /// + /// Returns `OutputsIndexError` if the previous txout is not found in `prev_tx` + /// at `output_index`. pub fn from_prev_tx( plan: Plan, prev_tx: T, @@ -163,7 +167,7 @@ impl Input { }) } - /// Create + /// Create [`Input`] from a previous txout and plan. pub fn from_prev_txout( plan: Plan, prev_outpoint: OutPoint, @@ -181,9 +185,9 @@ impl Input { } } - /// Create + /// Create [`Input`] from a [`psbt::Input`]. /// - /// TODO: Return error type: out of bounds, etc. + // TODO: Return error type: out of bounds, etc. pub fn from_psbt_input( prev_outpoint: OutPoint, sequence: Sequence, @@ -207,6 +211,8 @@ impl Input { prev_tx, plan, status, + // TODO: maybe add `is_coinbase` parameter, or try to infer from the psbt + // input whether the input is coinbase. is_coinbase: false, }) } @@ -332,9 +338,10 @@ impl Input { self.plan.sequence() } - /// In weight units. + /// The weight in witness units needed for satisfying the [`Input`]. /// - /// TODO: Describe what fields are actually included in this calculation. + /// The satisfaction weight is the combined size of the fully satisfied input's witness + /// and scriptSig expressed in weight units. See . pub fn satisfaction_weight(&self) -> u64 { self.plan .satisfaction_weight() diff --git a/src/input_candidates.rs b/src/input_candidates.rs index 3d9b9b4..30636f4 100644 --- a/src/input_candidates.rs +++ b/src/input_candidates.rs @@ -274,6 +274,7 @@ impl Display for PolicyFailure { match self { PolicyFailure::MissingOutputs(missing_outputs) => Debug::fmt(missing_outputs, f), PolicyFailure::PolicyFailure(failure) => { + // Review: should not use fmt::Debug on Display write!(f, "policy failure: {:?}", failure) } } diff --git a/src/output.rs b/src/output.rs index adc991a..92e972a 100644 --- a/src/output.rs +++ b/src/output.rs @@ -3,12 +3,12 @@ use miniscript::bitcoin; use crate::DefiniteDescriptor; -/// Can get script pubkey from this. +/// Source of the output script pubkey #[derive(Debug, Clone)] pub enum ScriptSource { - /// From ScriptBuf. + /// bitcoin script Script(ScriptBuf), - /// From definite descriptor. + /// definite descriptor Descriptor(DefiniteDescriptor), } @@ -38,7 +38,7 @@ impl ScriptSource { /// To ScriptBuf pub fn script(&self) -> ScriptBuf { match self { - ScriptSource::Script(script_buf) => script_buf.clone(), + ScriptSource::Script(spk) => spk.clone(), ScriptSource::Descriptor(descriptor) => descriptor.script_pubkey(), } } diff --git a/src/rbf.rs b/src/rbf.rs index ae68498..f94526f 100644 --- a/src/rbf.rs +++ b/src/rbf.rs @@ -91,6 +91,7 @@ impl RbfSet { tx.input .iter() .map(|txin| txin.previous_output) + // Review: needless collect? .collect::>() }) .collect::>(); diff --git a/src/selection.rs b/src/selection.rs index 4196a9a..d0c8d1a 100644 --- a/src/selection.rs +++ b/src/selection.rs @@ -2,7 +2,7 @@ use core::fmt::{Debug, Display}; use std::vec::Vec; use bdk_coin_select::FeeRate; -use bitcoin::{absolute, transaction}; +use bitcoin::{absolute, transaction, Sequence}; use miniscript::bitcoin; use miniscript::psbt::PsbtExt; @@ -36,10 +36,13 @@ pub struct PsbtParams { /// It is best practive to set this to the latest block height to avoid fee sniping. pub fallback_locktime: absolute::LockTime, - /// Yes. - pub fallback_sequence: bitcoin::Sequence, + /// [`Sequence`] value to use by default if not provided by the input. + pub fallback_sequence: Sequence, - /// Recommended. + /// Whether to require the full tx, aka [`non_witness_utxo`] for segwit v0 inputs, + /// default is `true`. + /// + /// [`non_witness_utxo`]: bitcoin::psbt::Input::non_witness_utxo pub mandate_full_tx_for_segwit_v0: bool, } @@ -97,7 +100,7 @@ impl std::error::Error for CreatePsbtError {} impl Selection { /// Returns none if there is a mismatch of units in `locktimes`. /// - /// As according to BIP-64... + // TODO: As according to BIP-64... ? fn _accumulate_max_locktime( locktimes: impl IntoIterator, fallback: absolute::LockTime, @@ -138,7 +141,6 @@ impl Selection { .iter() .map(|input| bitcoin::TxIn { previous_output: input.prev_outpoint(), - // TODO: Custom fallback sequence? sequence: input.sequence().unwrap_or(params.fallback_sequence), ..Default::default() }) diff --git a/src/selector.rs b/src/selector.rs index 0e6b836..6e24491 100644 --- a/src/selector.rs +++ b/src/selector.rs @@ -8,7 +8,7 @@ use miniscript::bitcoin; use crate::{cs_feerate, DefiniteDescriptor, InputCandidates, InputGroup, Output, Selection}; use alloc::vec::Vec; -/// You seeing this? +/// A coin selector #[derive(Debug, Clone)] pub struct Selector<'c> { candidates: &'c InputCandidates, @@ -82,7 +82,8 @@ pub struct RbfParams { pub incremental_relay_feerate: FeeRate, } -/// TODO: Make this more flexible. +/// Change policy type +// TODO: Make this more flexible. #[derive(Debug, Clone, Copy)] pub enum ChangePolicyType { /// Avoid creating dust change output. @@ -95,13 +96,9 @@ pub enum ChangePolicyType { } impl OriginalTxStats { - /// Feerate. - /// - /// TODO: Make sure this is correct with the rounding. + /// Return the [`FeeRate`] of the original tx. pub fn feerate(&self) -> FeeRate { - FeeRate::from_sat_per_vb_unchecked( - ((self.fee.to_sat() as f32) / (self.weight.to_vbytes_ceil() as f32)) as _, - ) + self.fee / self.weight } } @@ -120,11 +117,10 @@ impl RbfParams { /// To coin select `Replace` params. pub fn to_cs_replace(&self) -> Replace { - let replace = Replace { + Replace { fee: self.original_txs.iter().map(|otx| otx.fee.to_sat()).sum(), incremental_relay_feerate: cs_feerate(self.incremental_relay_feerate), - }; - replace + } } /// Max feerate of all the original txs. @@ -134,6 +130,7 @@ impl RbfParams { self.original_txs .iter() .map(|otx| otx.feerate()) + // Review: should this be `max` instead? .min() .unwrap_or(FeeRate::ZERO) } diff --git a/tests/synopsis.rs b/tests/synopsis.rs index 4a100ed..cf98ac9 100644 --- a/tests/synopsis.rs +++ b/tests/synopsis.rs @@ -201,14 +201,16 @@ fn synopsis() -> anyhow::Result<()> { let addr = wallet.next_address().expect("must derive address"); - env.send(&addr, Amount::ONE_BTC)?; + let txid = env.send(&addr, Amount::ONE_BTC)?; env.mine_blocks(1, None)?; wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Received {}", txid); + println!("Balance (confirmed): {}", wallet.balance()); - env.send(&addr, Amount::ONE_BTC)?; + let txid = env.send(&addr, Amount::ONE_BTC)?; wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Received {txid}"); + println!("Balance (pending): {}", wallet.balance()); let (tip_height, tip_time) = wallet.tip_info(env.rpc_client())?; let longterm_feerate = FeeRate::from_sat_per_vb_unchecked(1); @@ -218,7 +220,7 @@ fn synopsis() -> anyhow::Result<()> { .get_new_address(None, None)? .assume_checked(); - // okay now create tx. + // Okay now create tx. let selection = wallet .all_candidates() .regroup(group_by_spk()) @@ -261,7 +263,7 @@ fn synopsis() -> anyhow::Result<()> { let txid = env.rpc_client().send_raw_transaction(&tx)?; println!("tx broadcasted: {}", txid); wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Balance (send tx): {}", wallet.balance()); // Try cancel a tx. // We follow all the rules as specified by @@ -345,7 +347,7 @@ fn synopsis() -> anyhow::Result<()> { let txid = env.rpc_client().send_raw_transaction(&tx)?; println!("tx broadcasted: {}", txid); wallet.sync(&env)?; - println!("balance: {}", wallet.balance()); + println!("Balance (RBF): {}", wallet.balance()); } Ok(())