[chain] Change tx_last_seen to `Option<u64>` #1416

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

fixes #1446
fixes #1396

Notes to the reviewers

Changelog notice

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

Bugfixes:

This pull request breaks the existing API
I'm linking the issue being fixed by this PR

ValuedMammal · 2024-05-08T00:44:00Z

crates/chain/tests/test_tx_graph.rs

@@ -959,10 +959,10 @@ fn test_chain_spends() {
        }))
    );

-    // Even if unconfirmed tx has a last_seen of 0, it can still be part of a chain spend.
+    // Even if unconfirmed tx has a last_seen of None, it can still be part of a chain spend.


Is this a valid assumption?

If instead we were to filter unseen txs in try_get_chain_position, it has the same desired effect on canonical txs, but then this assertion no longer holds. In other words, the above comment will say

// An unconfirmed and unseen tx cannot be part of a chain spend.

Experimenting with that approach in 03d0515. This might be preferable in that it applies the same reasoning (that an unseen tx does not have a chain position) to list_chain_spends and filter_chain_txouts.

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

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

evanlinjin · 2024-05-16T02:36:34Z

crates/chain/src/tx_graph.rs

+            Some(val) => {
+                let (_, anchors, last_seen) = val;
+                if anchors.is_empty() && last_seen.is_none() {
+                    // We don't consider unanchored, unseen transactions
+                    // to have a chain position.
+                    return Ok(None);
+                }
+                val
+            }


I like the idea of moving this logic into try_get_chain_position. However, for transactions with last_seen as None, but with an anchor that anchors the transaction in the best chain, wouldn't the transaction be in the best chain?

My understanding of last_seen is that a value of None signals that it has not been seen in the mempool. However, this fact shouldn't invalidate the fact that the transaction can be confirmed in the best chain?

Should we rename this method to try_get_canonical_position?

I think having a chain position should imply the tx is canonical, so I think these are synonymous.

evanlinjin · 2024-05-16T07:31:43Z

crates/chain/src/chain_data.rs

@@ -10,7 +10,7 @@ pub enum ChainPosition<A> {
    /// The chain data is seen as confirmed, and in anchored by `A`.
    Confirmed(A),
    /// The chain data is not confirmed and last seen in the mempool at this timestamp.
-    Unconfirmed(u64),
+    Unconfirmed(Option<u64>),


Would it be fair to say that unconfirmed transactions without a last-seen will not have a ChainPosition? Would it make sense to rename ChainPosition to CanonicalPosition?

evanlinjin · 2024-05-16T08:42:24Z

crates/chain/src/chain_data.rs

@@ -58,14 +58,16 @@ pub enum ConfirmationTime {
    /// The transaction is unconfirmed
    Unconfirmed {
        /// The last-seen timestamp in unix seconds.
-        last_seen: u64,
+        last_seen: Option<u64>,


Maybe it's not worth it to put an option here either? Assuming that we agree that unconfirmed txs without a last-seen are not worth returning? The ConfirmationTime type is only used in bdk_wallet as a field in LocalOutput (which is a UTXO). There is no point showing a UTXO that is not part of the canonical chain right?

evanlinjin · 2024-05-16T09:12:15Z

crates/chain/src/tx_graph.rs

-    pub fn try_list_chain_txs<'a, C: ChainOracle + 'a>(
+    /// [`full_txs`]: Self::full_txs
+    /// [`canonical_transactions`]: Self::canonical_transactions
+    pub fn try_canonical_transactions<'a, C: ChainOracle + 'a>(


Would try_list_canonical_transactions be a better name?

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 the test test_tx_conflict_handling. 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.

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

crates/wallet/src/wallet/coin_selection.rs

@@ -784,13 +784,21 @@ mod test {

    fn get_test_utxos() -> Vec<WeightedUtxo> {


These aren't really useful UTXOs when they are not seen at all right?

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.

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.

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 transactions with unknown anchors were erroneously being given a `ChainPosition`. Note this commit changes the way `Balance` is calculated 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`, and hence only these `TxOut`s will influence the `Balance` calculation.

and add a disclaimer about safety for `Wallet::tx_graph_mut`

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

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

evanlinjin reviewed May 16, 2024

View reviewed changes

evanlinjin mentioned this pull request May 16, 2024

Wallet::insert_tx should not have position input #1446

Open

ValuedMammal force-pushed the fix/lastseen-none branch from 6fbd167 to 7b00dca Compare May 21, 2024 11:28

ValuedMammal marked this pull request as draft May 21, 2024 11:29

ValuedMammal added 3 commits May 23, 2024 15:33

doc(wallet): Fixup docs for insert_tx

c547aa4

and add a disclaimer about safety for `Wallet::tx_graph_mut`

ValuedMammal force-pushed the fix/lastseen-none branch from c37abfa to c547aa4 Compare May 24, 2024 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chain] Change tx_last_seen to `Option<u64>` #1416

[chain] Change tx_last_seen to `Option<u64>` #1416

ValuedMammal commented Apr 30, 2024 •

edited

ValuedMammal May 8, 2024 •

edited

storopoli commented May 8, 2024

evanlinjin May 16, 2024

evanlinjin May 16, 2024

ValuedMammal May 24, 2024

evanlinjin May 16, 2024

evanlinjin May 16, 2024

evanlinjin May 16, 2024

evanlinjin May 16, 2024

ValuedMammal May 24, 2024

evanlinjin May 16, 2024

evanlinjin commented May 16, 2024

		@@ -784,13 +784,21 @@ mod test {

		fn get_test_utxos() -> Vec<WeightedUtxo> {

[chain] Change tx_last_seen to Option<u64> #1416

Are you sure you want to change the base?

[chain] Change tx_last_seen to Option<u64> #1416

Conversation

ValuedMammal commented Apr 30, 2024 • edited

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

ValuedMammal May 8, 2024 • edited

Choose a reason for hiding this comment

storopoli commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlinjin commented May 16, 2024

[chain] Change tx_last_seen to `Option<u64>` #1416

[chain] Change tx_last_seen to `Option<u64>` #1416

ValuedMammal commented Apr 30, 2024 •

edited

ValuedMammal May 8, 2024 •

edited