bdk icon indicating copy to clipboard operation
bdk copied to clipboard

[chain] last_seen_unconfirmed should have a default of `None` not 0

Open LLFourn opened this issue 1 year ago • 4 comments

Current implementation of TxGraph insert_tx is:

    /// Inserts the given transaction into [`TxGraph`].
    ///
    /// The [`ChangeSet`] returned will be empty if `tx` already exists.
    pub fn insert_tx(&mut self, tx: Transaction) -> ChangeSet<A> {
        let mut update = Self::default();
        update.txs.insert(
            tx.txid(),
            (TxNodeInternal::Whole(tx.into()), BTreeSet::new(), 0),
        );
        self.apply_update(update)
    }

The 0 in the tuple represents the last time the transaction has been seen unconfirmed. I think it is wrong to set 0 here because you should be able to insert transactions into the graph (or wallet) that have never been broadcast and have never been seen. When you list canonical transactions[^1] you don't want to list transactions that have been inserted but never actually seen. In other words the field should be an Option which is still monotonically increasing with None being the smallest value.

I don't think this change affects the changeset struct, it just changes how you interpret a missing last_seen.

[^1]: Incidentally we should rename try_list_chain_txs to canonincal_transactions

LLFourn avatar Apr 03 '24 23:04 LLFourn

What is the exact distinction between None and 0? The assumption that the tx has been seen by the outside world? I understand this as making the fuzzy line fiction between "the mempool" and "my mempool" a concrete distinction.

DanGould avatar Apr 04 '24 02:04 DanGould

None would be in no ones mempool (not even yours). You can have a transaction that is valid but has never left application memory. But I realized after writing this we don't have a way of inserting a transaction into Wallet that would have a last_seen of None. The api forces you to specify either confirmed or unconfirmed with last_seen.

Maybe the reason we didn't do an Option in the first place is that we didn't have motivation for inserting these kinds of transactions into the graph. I think the motivation is to be able to store and analyze a transaction chain in the context of the graph without yet broadcasting it.

LLFourn avatar Apr 04 '24 05:04 LLFourn

Assuming we treat a last_seen of 0 as effectively None, could this be fixed by just filtering list_chain_txs to exclude those that have no anchors and a last_seen of 0, or is there an extra advantage to making last_seen an Option ?

ValuedMammal avatar May 03 '24 00:05 ValuedMammal

Indeed an alternative proposal is to treat 0 as a magic value. But this means that two states have the same semantics (when the last_seen is 0 and when it doesn't exist). It's better not to have this ambiguity and just have explicit None for no last seen.

LLFourn avatar May 06 '24 07:05 LLFourn