Wrap transactions as `Arc<Transaction>` in `TxGraph` #1373

evanlinjin · 2024-03-14T06:39:17Z

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:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

evanlinjin · 2024-03-14T16:28:28Z

TODO: Make sure all the examples still work.

LLFourn · 2024-03-15T04:20:37Z

Main concern: I don't think we need T here. That seem premature. We can just concretely have an Arc<Transaction>.

crates/bdk/src/wallet/mod.rs

LLFourn · 2024-03-17T23:00:22Z

crates/bdk/src/wallet/mod.rs

@@ -1006,7 +1006,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 (sent, received) = wallet.sent_and_received(tx);
+    /// let (sent, received) = wallet.sent_and_received(&tx);


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?

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.

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.

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).

LLFourn · 2024-03-17T23:04:44Z

Can this PR not be done on top of #1369. I think it can go before and is pretty unrelated.

evanlinjin · 2024-03-18T14:32:46Z

@LLFourn removed the dependency on #1369

crates/chain/src/tx_graph.rs

crates/chain/tests/test_tx_graph.rs

Wrapping transactions as `Arc<Transaction>` allows us to share transactions cheaply between the chain-source and receiving structures. Therefore the chain-source can keep already-fetched transactions (save bandwidth) and have a shared pointer to the transactions (save memory). This is better than the current way we do things, which is to refer back to the receiving structures mid-sync. Documentation for `TxGraph` is also updated.

LLFourn

ACK 8ab58af

notmandatory

ACK 8ab58af

May need a rebase.

notmandatory · 2024-03-29T20:02:02Z

crates/chain/src/tx_graph.rs

@@ -430,7 +444,7 @@ impl<A> TxGraph<A> {
    /// Note that this only returns directly conflicting txids and won't include:
    /// - descendants of conflicting transactions (which are technically also conflicting)
    /// - transactions conflicting with the given transaction's ancestors
-    pub fn direct_conflitcs<'g>(
+    pub fn direct_conflicts<'g>(


🏕️ nice cleanup!

96a9aa6 feat(chain): refactor `merge_chains` (志宇) 2f22987 chore(chain): fix comment (志宇) daf588f feat(chain): optimize `merge_chains` (志宇) 77d3595 feat(chain)!: rm `local_chain::Update` (志宇) 1269b06 test(chain): fix incorrect test case (志宇) 72fe65b feat(esplora)!: simplify chain update logic (志宇) eded1a7 feat(chain): introduce `CheckPoint::insert` (志宇) 519cd75 test(esplora): move esplora tests into src files (志宇) a6e613e test(esplora): add `test_finalize_chain_update` (志宇) 494d253 feat(testenv): add `genesis_hash` method (志宇) 886d72e chore(chain)!: rm `missing_heights` and `missing_heights_from` methods (志宇) bd62aa0 feat(esplora)!: remove `EsploraExt::update_local_chain` (志宇) 1e99793 feat(testenv): add `make_checkpoint_tip` (志宇) Pull request description: Fixes #1354 ### Description Built on top of both #1369 and #1373, we simplify the `EsploraExt` API by removing the `update_local_chain` method and having `full_scan` and `sync` update the local chain in the same call. The `full_scan` and `sync` methods now takes in an additional input (`local_tip`) which provides us with the view of the `LocalChain` before the update. These methods now return structs `FullScanUpdate` and `SyncUpdate`. The examples are updated to use this new API. `TxGraph::missing_heights` and `tx_graph::ChangeSet::missing_heights_from` are no longer needed, therefore they are removed. Additionally, we used this opportunity to simplify the logic which updates `LocalChain`. We got rid of the `local_chain::Update` struct (which contained the update `CheckPoint` tip and a `bool` which signaled whether we want to introduce blocks below point of agreement). It turns out we can use something like `CheckPoint::insert` so the chain source can craft an update based on the old tip. This way, we can make better use of `merge_chains`' optimization that compares the `Arc` pointers of the local and update chain (before we were crafting the update chain NOT based on top of the previous local chain). With this, we no longer need the `Update::introduce_older_block` field since the logic will naturally break when we reach a matching `Arc` pointer. ### Notes to the reviewers * Obtaining the `LocalChain`'s update now happens within `EsploraExt::full_scan` and `EsploraExt::sync`. Creating the `LocalChain` update is now split into two methods (`fetch_latest_blocks` and `chain_update`) that are called before and after fetching transactions and anchors. * We need to duplicate code for `bdk_esplora`. One for blocking and one for async. ### Changelog notice * Changed `EsploraExt` API so that sync only requires one round of fetching data. The `local_chain_update` method is removed and the `local_tip` parameter is added to the `full_scan` and `sync` methods. * Removed `TxGraph::missing_heights` and `tx_graph::ChangeSet::missing_heights_from` methods. * Introduced `CheckPoint::insert` which allows convenient checkpoint-insertion. This is intended for use by chain-sources when crafting an update. * Refactored `merge_chains` to also return the resultant `CheckPoint` tip. * Optimized the update `LocalChain` logic - use the update `CheckPoint` as the new `CheckPoint` tip when possible. ### 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: * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: LLFourn: ACK 96a9aa6 Tree-SHA512: 3d4f2eab08a1fe94eb578c594126e99679f72e231680b2edd4bfb018ba1d998ca123b07acb2d19c644d5887fc36b8e42badba91cd09853df421ded04de45bf69

evanlinjin force-pushed the fix/1354-arc-tx branch 10 times, most recently from b5fdfaa to a65b935 Compare March 14, 2024 16:26

evanlinjin marked this pull request as ready for review March 14, 2024 16:28

evanlinjin changed the title ~~Make transactions a generic~~ Make transactions generic in TxGraph. Mar 14, 2024

evanlinjin force-pushed the fix/1354-arc-tx branch from a65b935 to ee66da1 Compare March 15, 2024 04:27

evanlinjin changed the title ~~Make transactions generic in TxGraph.~~ Wrap transactions as Arc<Transaction> in TxGraph Mar 15, 2024

evanlinjin force-pushed the fix/1354-arc-tx branch 2 times, most recently from 5e24ad6 to 6786915 Compare March 15, 2024 05:46

notmandatory added module-blockchain api A breaking API change labels Mar 16, 2024

notmandatory added this to the 1.0.0-alpha milestone Mar 16, 2024

LLFourn reviewed Mar 17, 2024

View reviewed changes

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved

LLFourn reviewed Mar 17, 2024

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

LLFourn reviewed Mar 17, 2024

View reviewed changes

notmandatory assigned evanlinjin Mar 18, 2024

evanlinjin force-pushed the fix/1354-arc-tx branch 2 times, most recently from cc6c8ad to ba5b5d6 Compare March 18, 2024 14:30

evanlinjin requested a review from LLFourn March 18, 2024 14:33

ValuedMammal reviewed Mar 20, 2024

View reviewed changes

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

ValuedMammal reviewed Mar 20, 2024

View reviewed changes

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved

evanlinjin mentioned this pull request Mar 21, 2024

Simplified EsploraExt API #1380

Merged

5 tasks

evanlinjin force-pushed the fix/1354-arc-tx branch from ba5b5d6 to 211a4aa Compare March 25, 2024 04:46

evanlinjin force-pushed the fix/1354-arc-tx branch from 211a4aa to 8ab58af Compare March 25, 2024 04:57

evanlinjin requested a review from ValuedMammal March 25, 2024 05:09

LLFourn approved these changes Mar 27, 2024

View reviewed changes

ValuedMammal approved these changes Mar 27, 2024

View reviewed changes

notmandatory approved these changes Mar 29, 2024

View reviewed changes

evanlinjin merged commit 19304c1 into bitcoindevkit:master Mar 31, 2024
12 checks passed

notmandatory mentioned this pull request Apr 12, 2024

Release 1.0.0-alpha.9 #1399

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap transactions as `Arc<Transaction>` in `TxGraph` #1373

Wrap transactions as `Arc<Transaction>` in `TxGraph` #1373

evanlinjin commented Mar 14, 2024 •

edited

evanlinjin commented Mar 14, 2024

LLFourn commented Mar 15, 2024

LLFourn Mar 17, 2024

evanlinjin Mar 18, 2024

ValuedMammal Mar 20, 2024 •

edited

evanlinjin Mar 25, 2024

LLFourn commented Mar 17, 2024

evanlinjin commented Mar 18, 2024

LLFourn left a comment

notmandatory left a comment

notmandatory Mar 29, 2024

Wrap transactions as Arc<Transaction> in TxGraph #1373

Wrap transactions as Arc<Transaction> in TxGraph #1373

Conversation

evanlinjin commented Mar 14, 2024 • edited

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

evanlinjin commented Mar 14, 2024

LLFourn commented Mar 15, 2024

LLFourn Mar 17, 2024

Choose a reason for hiding this comment

evanlinjin Mar 18, 2024

Choose a reason for hiding this comment

ValuedMammal Mar 20, 2024 • edited

Choose a reason for hiding this comment

evanlinjin Mar 25, 2024

Choose a reason for hiding this comment

LLFourn commented Mar 17, 2024

evanlinjin commented Mar 18, 2024

LLFourn left a comment

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory Mar 29, 2024

Choose a reason for hiding this comment

Wrap transactions as `Arc<Transaction>` in `TxGraph` #1373

Wrap transactions as `Arc<Transaction>` in `TxGraph` #1373

evanlinjin commented Mar 14, 2024 •

edited

ValuedMammal Mar 20, 2024 •

edited