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

[LLFourn]

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

Footnotes

1. Incidentally we should rename try_list_chain_txs to canonincal_transactions ↩

[DanGould]

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.

[LLFourn]

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.

[ValuedMammal]

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 ?

[LLFourn]

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.