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

[ValuedMammal]

fixes #1446 fixes #1396

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

• [x] I've signed all my commits
• [x] I followed the contribution guidelines
• [x] 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

[storopoli]

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

[evanlinjin]

@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]

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.

[evanlinjin]

Another priority review for next week.

[notmandatory]

@ValuedMammal if this is already rebased and ready to go please change the status to "ready to review".

[evanlinjin]

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?

[ValuedMammal]

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?

[evanlinjin]

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.

[ValuedMammal]

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