Skip to content

Commit

Permalink
Merge #1373: Wrap transactions as Arc<Transaction> in TxGraph
Browse files Browse the repository at this point in the history
8ab58af feat(chain)!: wrap `TxGraph` txs with `Arc` (志宇)

Pull request description:

  ### Description

  This PR makes `TxGraph` store transactions as `Arc<Transaction>`.

  `Arc<Transaction>` can be shared between the chain-source and receiving structures. This allows the chain-source to keep the already-fetched transactions (save bandwith) and have a shared pointer to the transaction (save memory).

  Our current logic to avoid re-fetching transactions is to refer back to the receiving structures. However, this means an additional round of locking our receiving structures.

  ### Notes to the reviewers

  This will make more sense once I update the esplora/electrum chain sources to make use of both #1369 and this PR. The result would be chain sources which would only require locking the receiving structures twice (once for initiating the update, and once for applying the update).

  ### Changelog notice

  * Changed `TxGraph` to store transactions as `Arc<Transaction>`. This allows chain-sources to cheaply keep a copy of already-fetched transactions.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [ ] I've added tests for the new feature
  * [x] I've added docs for the new feature

ACKs for top commit:
  LLFourn:
    ACK 8ab58af
  notmandatory:
    ACK 8ab58af

Tree-SHA512: 81d7ad35fed7f253a3b902f09a41aaef2877f6201d4f6e78318741bf00e4b898a8000d878ffcbfe75701094ce3e9021bd718b190a655331a9e11f0ad66bdb02f
  • Loading branch information
evanlinjin committed Mar 31, 2024
2 parents b5557dc + 8ab58af commit 19304c1
Show file tree
Hide file tree
Showing 8 changed files with 150 additions and 114 deletions.
22 changes: 12 additions & 10 deletions crates/bdk/src/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,7 @@ impl<D> Wallet<D> {
/// # let mut wallet: Wallet<()> = todo!();
/// # let txid:Txid = todo!();
/// let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx;
/// let fee = wallet.calculate_fee(tx).expect("fee");
/// let fee = wallet.calculate_fee(&tx).expect("fee");
/// ```
///
/// ```rust, no_run
Expand Down Expand Up @@ -973,16 +973,16 @@ impl<D> Wallet<D> {
/// # let mut wallet: Wallet<()> = todo!();
/// # let txid:Txid = todo!();
/// let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx;
/// let fee_rate = wallet.calculate_fee_rate(tx).expect("fee rate");
/// let fee_rate = wallet.calculate_fee_rate(&tx).expect("fee rate");
/// ```
///
/// ```rust, no_run
/// # use bitcoin::psbt::PartiallySignedTransaction;
/// # use bdk::Wallet;
/// # let mut wallet: Wallet<()> = todo!();
/// # let mut psbt: PartiallySignedTransaction = todo!();
/// let tx = &psbt.clone().extract_tx();
/// let fee_rate = wallet.calculate_fee_rate(tx).expect("fee rate");
/// let tx = psbt.clone().extract_tx();
/// let fee_rate = wallet.calculate_fee_rate(&tx).expect("fee rate");
/// ```
/// [`insert_txout`]: Self::insert_txout
pub fn calculate_fee_rate(&self, tx: &Transaction) -> Result<FeeRate, CalculateFeeError> {
Expand All @@ -1003,8 +1003,8 @@ impl<D> Wallet<D> {
/// # use bdk::Wallet;
/// # let mut wallet: Wallet<()> = todo!();
/// # let txid:Txid = todo!();
/// let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx;
/// let (sent, received) = wallet.sent_and_received(tx);
/// let tx = wallet.get_tx(txid).expect("tx exists").tx_node.tx;
/// let (sent, received) = wallet.sent_and_received(&tx);
/// ```
///
/// ```rust, no_run
Expand Down Expand Up @@ -1065,7 +1065,7 @@ impl<D> Wallet<D> {
pub fn get_tx(
&self,
txid: Txid,
) -> Option<CanonicalTx<'_, Transaction, ConfirmationTimeHeightAnchor>> {
) -> Option<CanonicalTx<'_, Arc<Transaction>, ConfirmationTimeHeightAnchor>> {
let graph = self.indexed_graph.graph();

Some(CanonicalTx {
Expand Down Expand Up @@ -1167,7 +1167,8 @@ impl<D> Wallet<D> {
/// Iterate over the transactions in the wallet.
pub fn transactions(
&self,
) -> impl Iterator<Item = CanonicalTx<'_, Transaction, ConfirmationTimeHeightAnchor>> + '_ {
) -> impl Iterator<Item = CanonicalTx<'_, Arc<Transaction>, ConfirmationTimeHeightAnchor>> + '_
{
self.indexed_graph
.graph()
.list_chain_txs(&self.chain, self.chain.tip().block_id())
Expand Down Expand Up @@ -1670,6 +1671,7 @@ impl<D> Wallet<D> {
let mut tx = graph
.get_tx(txid)
.ok_or(BuildFeeBumpError::TransactionNotFound(txid))?
.as_ref()
.clone();

let pos = graph
Expand Down Expand Up @@ -1739,7 +1741,7 @@ impl<D> Wallet<D> {
sequence: Some(txin.sequence),
psbt_input: Box::new(psbt::Input {
witness_utxo: Some(txout.clone()),
non_witness_utxo: Some(prev_tx.clone()),
non_witness_utxo: Some(prev_tx.as_ref().clone()),
..Default::default()
}),
},
Expand Down Expand Up @@ -2295,7 +2297,7 @@ impl<D> Wallet<D> {
psbt_input.witness_utxo = Some(prev_tx.output[prev_output.vout as usize].clone());
}
if !desc.is_taproot() && (!desc.is_witness() || !only_witness_utxo) {
psbt_input.non_witness_utxo = Some(prev_tx.clone());
psbt_input.non_witness_utxo = Some(prev_tx.as_ref().clone());
}
}
Ok(psbt_input)
Expand Down
19 changes: 11 additions & 8 deletions crates/bdk/tests/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,12 @@ fn test_get_funded_wallet_sent_and_received() {

let mut tx_amounts: Vec<(Txid, (u64, u64))> = wallet
.transactions()
.map(|ct| (ct.tx_node.txid, wallet.sent_and_received(ct.tx_node.tx)))
.map(|ct| (ct.tx_node.txid, wallet.sent_and_received(&ct.tx_node)))
.collect();
tx_amounts.sort_by(|a1, a2| a1.0.cmp(&a2.0));

let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx;
let (sent, received) = wallet.sent_and_received(tx);
let (sent, received) = wallet.sent_and_received(&tx);

// The funded wallet contains a tx with a 76_000 sats input and two outputs, one spending 25_000
// to a foreign address and one returning 50_000 back to the wallet as change. The remaining 1000
Expand All @@ -227,7 +227,7 @@ fn test_get_funded_wallet_tx_fees() {
let (wallet, txid) = get_funded_wallet(get_test_wpkh());

let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx;
let tx_fee = wallet.calculate_fee(tx).expect("transaction fee");
let tx_fee = wallet.calculate_fee(&tx).expect("transaction fee");

// The funded wallet contains a tx with a 76_000 sats input and two outputs, one spending 25_000
// to a foreign address and one returning 50_000 back to the wallet as change. The remaining 1000
Expand All @@ -240,7 +240,9 @@ fn test_get_funded_wallet_tx_fee_rate() {
let (wallet, txid) = get_funded_wallet(get_test_wpkh());

let tx = wallet.get_tx(txid).expect("transaction").tx_node.tx;
let tx_fee_rate = wallet.calculate_fee_rate(tx).expect("transaction fee rate");
let tx_fee_rate = wallet
.calculate_fee_rate(&tx)
.expect("transaction fee rate");

// The funded wallet contains a tx with a 76_000 sats input and two outputs, one spending 25_000
// to a foreign address and one returning 50_000 back to the wallet as change. The remaining 1000
Expand Down Expand Up @@ -1307,7 +1309,7 @@ fn test_add_foreign_utxo_where_outpoint_doesnt_match_psbt_input() {
.add_foreign_utxo(
utxo2.outpoint,
psbt::Input {
non_witness_utxo: Some(tx1),
non_witness_utxo: Some(tx1.as_ref().clone()),
..Default::default()
},
satisfaction_weight
Expand All @@ -1320,7 +1322,7 @@ fn test_add_foreign_utxo_where_outpoint_doesnt_match_psbt_input() {
.add_foreign_utxo(
utxo2.outpoint,
psbt::Input {
non_witness_utxo: Some(tx2),
non_witness_utxo: Some(tx2.as_ref().clone()),
..Default::default()
},
satisfaction_weight
Expand Down Expand Up @@ -1384,7 +1386,7 @@ fn test_add_foreign_utxo_only_witness_utxo() {
let mut builder = builder.clone();
let tx2 = wallet2.get_tx(txid2).unwrap().tx_node.tx;
let psbt_input = psbt::Input {
non_witness_utxo: Some(tx2.clone()),
non_witness_utxo: Some(tx2.as_ref().clone()),
..Default::default()
};
builder
Expand Down Expand Up @@ -3050,7 +3052,8 @@ fn test_taproot_sign_using_non_witness_utxo() {
let mut psbt = builder.finish().unwrap();

psbt.inputs[0].witness_utxo = None;
psbt.inputs[0].non_witness_utxo = Some(wallet.get_tx(prev_txid).unwrap().tx_node.tx.clone());
psbt.inputs[0].non_witness_utxo =
Some(wallet.get_tx(prev_txid).unwrap().tx_node.as_ref().clone());
assert!(
psbt.inputs[0].non_witness_utxo.is_some(),
"Previous tx should be present in the database"
Expand Down
2 changes: 1 addition & 1 deletion crates/chain/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ readme = "README.md"
[dependencies]
# For no-std, remember to enable the bitcoin/no-std feature
bitcoin = { version = "0.30.0", default-features = false }
serde_crate = { package = "serde", version = "1", optional = true, features = ["derive"] }
serde_crate = { package = "serde", version = "1", optional = true, features = ["derive", "rc"] }

# Use hashbrown as a feature flag to have HashSet and HashMap from it.
hashbrown = { version = "0.9.1", optional = true, features = ["serde"] }
Expand Down
Loading

0 comments on commit 19304c1

Please sign in to comment.