refactor(chain)!: Change tx_last_seen to `Option<u64>` by ValuedMammal · Pull Request #1416 · bitcoindevkit/bdk

ValuedMammal · 2024-04-30T20:42:56Z

The PR changes the type of last_seen to Option<u64> for txs member of TxGraph.

This fixes an issue where unbroadcast and otherwise non-canonical transactions were returned from methods list_chain_txs and Wallet::transactions because every new tx inserted had a last_seen of 0 making it appear unconfirmed.

fixes #1446
fixes #1396

Notes to the reviewers

Changelog notice

Changed

Member last_seen_unconfirmed of TxNode is changed to Option<u64>
Renamed TxGraph method list_chain_txs to list_canonical_txs
Changed Wallet::insert_tx to take a single tx: Transaction as parameter

Added

Add method txs_with_no_anchor_or_last_seen for TxGraph
Add method unbroadcast_transactions for Wallet

Checklists

All Submissions:

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

crates/chain/tests/test_tx_graph.rs

storopoli · 2024-05-08T16:02:30Z

Isn't Default for Option<T> None? If yes, maybe we can simplify with ..Default::default()?

crates/chain/src/tx_graph.rs

crates/chain/src/chain_data.rs

crates/chain/src/tx_graph.rs

evanlinjin · 2024-05-16T09:20:54Z

crates/chain/tests/common/tx_template.rs

-        if let Some(seen_at) = tx_tmp.last_seen {
-            let _ = graph.insert_seen_at(tx.txid(), seen_at);
-        }
+        let _ = graph.insert_seen_at(tx.txid(), tx_tmp.last_seen.unwrap_or(0));


Why are we force-inserting a last_seen when the template last_seen is None?

This was a fix for test_tx_graph_conflicts. Because we're interested in testing tx conflicts, we have to control for the fact that txs without a last_seen would now be filtered out, so we just assume all txs are canonical.

edit: An alternative is to patch up some of the scenarios in test_tx_graph_conflicts to ensure every tx template is given some value of last_seen. That might make things clearer.

crates/wallet/src/wallet/coin_selection.rs

evanlinjin · 2024-05-16T14:35:04Z

@LLFourn's comment...

None would be in no ones mempool (not even yours). You can have a transaction that is valid but has never left application memory.

summarizes the main usecase of these changes.

With this context in mind, this PR description needs to highlight how we should handle non-broadcasted transactions in TxGraph. How should these non-broadcasted transactions influence methods filter_chain_txouts/filter_chain_unspents-esc methods.

How can the caller list these non-broadcasted transactions? What if the caller would like to include UTXOs of non-broadcasted transactions when listing their UTXO set? What if the caller would NOT like to do so? What if the caller would like to replace non-broadcasted transactions? Currently, we do transaction-precedence based on last_seen. Wouldn't we need something like this for non-broadcasted transactions too?

It seems that many things have not be thought through clearly. Let's have some more concrete discussion about how everything should look after these changes.

ValuedMammal · 2024-06-11T14:06:41Z

While it's possible to store a transaction in TxGraph before broadcasting it, I hesitate to advertise this as a feature, because there's not an easy way to make any owned txouts available for coin selection. The design of TxGraph requires all txs have a canonical chain position in order to count toward the total balance. You can manipulate the graph into considering a transaction canonical, but without a more robust way to abandon a tx and restore an input's unspent status, the only way to revert it is to do a full wallet rescan.

As an aside, if you're trying to build a chain of dependent transactions without broadcasting them, it should be possible to bypass coin selection by adding any owned txouts as a foreign utxo to a new TxBuilder.

ValuedMammal · 2024-06-21T11:51:27Z

This fixes an issue where unbroadcast and otherwise non-canonical transactions were returned from methods

How would a non-canonical tx be returned by the methods?

For example in test_list_owned_txouts tx3 is registered as spending tx2 at block 1 when tx3 doesn't confirm until block 2 and is never explicitly given a last_seen in the test. Does that seem unexpected?

since we'd be lacking context that should normally occur during sync with a chain source. The logic for inserting a graph anchor from a `ConfirmationTime` is moved to the wallet common test module in order to simulate receiving new txs and confirming them.

crates/wallet/src/wallet/mod.rs

crates/chain/src/tx_graph.rs

evanlinjin · 2024-06-25T09:44:53Z

I feel like we should include a method for returning "unseen and unanchored" transactions. Not sure what to name it, maybe unseen_txs is enough? I guess this would be on TxGraph and re-exposed on Wallet. The intention is to have a method that can be helpful for determining which txs still need to be broadcasted.

notmandatory

ACK 42fab2b

This looks good and all comments addressed.

LLFourn

Thanks. I've made a few suggestions about how to clean up the internals and tests. API looks good.

LLFourn · 2024-06-27T01:58:18Z

crates/chain/src/tx_graph.rs

 pub struct TxGraph<A = ()> {
    // all transactions that the graph is aware of in format: `(tx_node, tx_anchors, tx_last_seen)`
-    txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, u64)>,
+    txs: HashMap<Txid, (TxNodeInternal, BTreeSet<A>, Option<u64>)>,


This should not be in this tuple I think. last_seen should just been its own HashMap<Txid, u64> if the value would be None here it would just not have an entry in the map. This is more idomatic and easier to reason about imo.

crates/chain/src/tx_graph.rs

crates/chain/tests/test_tx_graph.rs

crates/wallet/src/wallet/mod.rs

crates/chain/src/tx_graph.rs

ValuedMammal · 2024-06-27T14:11:01Z

I'm interested in implementing the suggestion to make last-seen a new field in TxGraph that is HashMap<Txid, u64> if it can be done in this PR cleanly

Also fixup `test_list_owned_txouts` to check that the right outputs, utxos, and balance are returned at different local chain heights. This fixes an issue where unbroadcast and otherwise non-canonical transactions were returned from methods `list_chain_txs` and `Wallet::transactions` because every tx inserted had a last_seen of 0 making it appear unconfirmed. Note this commit changes the way `Balance` is represented due to new logic in `try_get_chain_position` that no longer considers txs with non-canonical anchors. Before this change, a tx anchored to a block that is reorged out had a permanent effect on the pending balance, and now only txs with a last_seen time or an anchor confirmed in the best chain will return a `ChainPosition`.

notmandatory

Re ACK af75817

@LLFourn (or anyone else) any other suggestions before we merge this one?

Review comment was addressed.

ValuedMammal force-pushed the fix/lastseen-none branch from 23ffb41 to fc090cb Compare May 3, 2024 00:14

nondiremanuel added this to the 1.0.0-alpha milestone May 7, 2024

ValuedMammal force-pushed the fix/lastseen-none branch from fc090cb to 59315f7 Compare May 8, 2024 00:22

ValuedMammal changed the title ~~tx_graph: change last_seen to Option<u64>~~ [chain] Change tx_last_seen to Option<u64> May 8, 2024

ValuedMammal marked this pull request as ready for review May 8, 2024 00:29

ValuedMammal force-pushed the fix/lastseen-none branch from 59315f7 to 15d9579 Compare May 8, 2024 00:30

notmandatory assigned ValuedMammal May 8, 2024

notmandatory added the module-blockchain label May 8, 2024

ValuedMammal force-pushed the fix/lastseen-none branch from 15d9579 to 007beaf Compare May 8, 2024 00:41

ValuedMammal commented May 8, 2024

View reviewed changes

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

ValuedMammal force-pushed the fix/lastseen-none branch from 03d0515 to 6fbd167 Compare May 13, 2024 21:04