bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Fix `last_seen` in Esplora and Electrum chains [WIP]

Open rustaceanrob opened this issue 1 year ago • 5 comments

Description

The last seen unix time should be set for mempool transactions (#1336). I started working on this in the blocking Esplora client.

Notes to the reviewers

I decided to check-in with a WIP to get some clarifications from maintainers. There is a method on tx_graph that allows an insertion into the map for storing last_seen . This method is insert_seen_at, which I think could be called insert_last_seen without any ambiguity. When a transaction has been confirmed, there is no method to remove the Txid from the map so we don't continually store last_seen when a TX has been confirmed.

Should I add a remove_seen_at in tx_graph? Or if my naming alternative makes sense, add a remove_last_seen?

Thanks (:

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

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

rustaceanrob avatar Feb 10 '24 03:02 rustaceanrob

Concept ACK, approach NACK.

This will be a great feature to have! However, I think we are limited by the Esplora/Electrum APIs (let me know if I have missed something).

Note that your approach here is incorrect. Please check the Esplora API docs: https://github.com/Blockstream/esplora/blob/master/API.md#transaction-format

status.block_time is only available for confirmed transactions (so will always be None).

evanlinjin avatar Feb 11 '24 03:02 evanlinjin

Oh got it! That is trickier to implement than I thought. Am I correct in that the last_seen field is only intended for mempool transactions, and would be appropriate to remove once a transaction has been confirmed? In that case a remove_seen_at on tx_graph could be beneficial later.

rustaceanrob avatar Feb 11 '24 03:02 rustaceanrob

Oh got it! That is trickier to implement than I thought. Am I correct in that the last_seen field is only intended for mempool transactions, and would be appropriate to remove once a transaction has been confirmed? In that case a remove_seen_at on tx_graph could be beneficial later.

last_seen just means "last seen in the mempool". It is used to resolve conflicts between unconfirmed transactions (not necessarily mempool transactions - BDK would not know, it just approximates). The current assumption is that for two conflicting unconfirmed transactions, the transaction with the more recent last_seen takes precedence (however, this approximation can be improved - see #1321).

Currently, the nature of TxGraph is fully monotone. Nothing can be added or removed. TxGraph is combined with a ChainOracle implementation to know whether a transaction is confirmed or not. When a transaction gets reorged out (the block which the tx is anchored to is no longer in the best chain), BDK considered it unconfirmed. If a conflicting transaction with a more recent last_seen gets introduced, the prior transaction is considered evicted.

evanlinjin avatar Feb 11 '24 08:02 evanlinjin

Currently, the nature of TxGraph is fully monotone. Nothing can be added or removed.

I think things can be added!

LLFourn avatar Feb 13 '24 07:02 LLFourn

Currently, the nature of TxGraph is fully monotone. Nothing can be added or removed.

I think things can be added!

Sorry *nothing can be removed 😅

evanlinjin avatar Feb 13 '24 10:02 evanlinjin