Skip to content

Wrap transactions as Arc<Transaction> in TxGraph #1373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
evanlinjin merged 1 commit into from
Mar 31, 2024
Merged

Wrap transactions as Arc<Transaction> in TxGraph #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions crates/bdk/src/wallet/mod.rs
View file
Open in desktop
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");
Comment thread
evanlinjin marked this conversation as resolved.
/// ```
/// [`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);
Copy link
Copy Markdown
Collaborator

@LLFourn LLFourn Mar 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interested to understand why this & is needed now. Before tx was of type Transaction right, so shouldn't it needed to have been borrowed before?

Copy link
Copy Markdown
Member Author

@evanlinjin evanlinjin Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, TxNode's tx field is of a reference to T. I changed it in this PR to just be tx: T. Note that T is now Arc<Transaction>.

Because sent_and_received takes in a &Transaction, we expect to call .as_ref on Arc<Transaction>. However, we can make use of the auto-deref properties of AsRef implementations (such as Arc), so just doing &tx is enough.

Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can map to shared ref & internally and avoid returning Arc<Transaction> in get_tx{_node}

On second thought, the PR description implies that we specifically DO want to return an arc pointer.

Copy link
Copy Markdown
Member Author

@evanlinjin evanlinjin Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should return a concrete Arc for this API to be useful (cheaply share transactions between chain-source, receiving-structs and changesets).

/// ```
///
/// ```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
View file
Open in desktop
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
View file
Open in desktop
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