Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix last seen unconfirmed #1385

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

ValuedMammal
Copy link
Contributor

@ValuedMammal ValuedMammal commented Mar 22, 2024

A method update_last_seen_unconfirmed is added for TxGraph that allows inserting a seen_at time for all unanchored transactions.

fixes #1336

Changelog notice

Changed:

  • Methods into_tx_graph and into_confirmation_time_tx_graphfor RelevantTxids are changed to no longer accept a seen_at parameter.

Added:

  • Added method update_last_seen_unconfirmed for TxGraph.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@notmandatory
Copy link
Member

This looks fine to me, but my understanding is that a time value always should be provided, even if it's made up as long the the values are increasing. If we make this optional we should at least explain what the downside is of using None, which I believe was unneeded DB updates.

@ValuedMammal
Copy link
Contributor Author

This looks fine to me, but my understanding is that a time value always should be provided, even if it's made up as long the the values are increasing. If we make this optional we should at least explain what the downside is of using None, which I believe was unneeded DB updates.

Ok, I think I agree it should be required.

@rustaceanrob
Copy link
Contributor

I agree on required. It should also make for a better UX as the end user "refreshes" their wallet.

@evanlinjin
Copy link
Member

Sorry about the conflicts! We have merged #1171

@ValuedMammal
Copy link
Contributor Author

electrum: into_tx_graph for RelevantTxids takes an optional seen_at, that could be why I was undecided before whether it should always be provided.

@evanlinjin Can we assume in electrum_ext if a relevant txid has no associated anchors, it must be unconfirmed and therefore be an appropriate time to insert_seen_at?

@evanlinjin
Copy link
Member

@ValuedMammal that sounds right to me! Let's add a couple of tests and I'll be happy to ACK and merge.

My proposed tests for both ElectrumExt and EsploraExt: When syncing, ensure that transactions that stay unconfirmed between rounds have their last_seen increased and transactions that stay confirmed have unchanging last_seens.

@ValuedMammal
Copy link
Contributor Author

ValuedMammal commented Mar 25, 2024

Update on test coverage:

For electrum, repeated syncing doesn't update the last seen timestamp, because if a tx is already present in graph, TxGraph won't report it as missing.

For esplora I did successfully check that timestamps increase for repeat syncs as long as a tx is unconfirmed. When the tx confirms, the timestamp is unchanged.

Note: I don't love the fact that every test needs to mine 100 regtest blocks, as that slows down CI. We should perhaps think about consolidating various assertions into more monolithic tests.

@ValuedMammal ValuedMammal changed the title [esplora] Change sync, full_scan to take a timestamp parameter [electrum,esplora] Fix last seen unconfirmed Mar 25, 2024
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here look exactly how I imagined they should. Thanks! But now that I see it, isn't it simpler just to have method like update_last_seen_unconfirmed on TxGraph which sets the last seen of all unanchored transactions? We can just call this on the update TxGraph and set it to now before applying it. This means we wouldn't need to pass it in, we can just mutate the TxGraph afterwards.

At least presently it doesn't look like we lose anything by doing this since the backends are both just setting whatever you tell them when you pass the time in. I was thinking that maybe esplora or electrum would tell you the last time it was seen in the API but I guess that's not the case. Mempool.space does have this api: https://mempool.space/docs/api/rest#get-transaction-times but that's "first seen" rather than last seen.

crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
@ValuedMammal
Copy link
Contributor Author

The changes here look exactly how I imagined they should. Thanks! But now that I see it, isn't it simpler just to have method like update_last_seen_unconfirmed on TxGraph which sets the last seen of all unanchored transactions? We can just call this on the update TxGraph and set it to now before applying it. This means we wouldn't need to pass it in, we can just mutate the TxGraph afterwards.

I'll have another look at this.

@ValuedMammal ValuedMammal changed the title [electrum,esplora] Fix last seen unconfirmed Fix last seen unconfirmed Mar 30, 2024
@ValuedMammal
Copy link
Contributor Author

  • Updated the PR description to reflect the approach using TxGraph, so the above comments are mostly outdated. This way has the advantage of not breaking the current API for electrum/esplora.

if let Some(seen_at) = seen_at {
let _ = graph.insert_seen_at(txid, seen_at);
if anchors.is_empty() {
if let Some(seen_at) = seen_at {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ I know it breaks the current API but shouldn't seen_at be required and non-optional here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether it makes more sense to make this seen_at parameter required, or to remove the param and use the new update_last_seen_unconfirmed method on the update TxGraph.

Copy link
Contributor

@LLFourn LLFourn Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine as it is. Will likely be radically changed soon.

[EDIT] but when you did:

 l    let mut graph_update =
        relevant_txids.into_confirmation_time_tx_graph(&client, None, missing)?;
    let now = std::time::UNIX_EPOCH.elapsed().unwrap().as_secs();
    let _ = graph_update.update_last_seen_unconfirmed(now);

You are choosing to pass in None but then manually update it. If this is what you want to do in the examples then just remove the option from these functions. It's confusing to use both in the examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I made seen_at a required parameter for into_tx_graph.

Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just need to make the electrum examples consistent (decide whether to pass it into the into_confirmation_time_tx_graph or just remove the Option and do it manually outside).

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! However, there may be a situation where the chain-source returns anchors that do not point to a block in the best chain. Those unconfirmed transactions will therefore have no "timestamp". Would it be better if TxGraph::update_last_seen_unconfirmed takes in a checkpoint tip so unconfirmed transactions are transactions that do not have an anchor that points to a checkpoint?

@ValuedMammal
Copy link
Contributor Author

This looks great! However, there may be a situation where the chain-source returns anchors that do not point to a block in the best chain. Those unconfirmed transactions will therefore have no "timestamp". Would it be better if TxGraph::update_last_seen_unconfirmed takes in a checkpoint tip so unconfirmed transactions are transactions that do not have an anchor that points to a checkpoint?

I had not thought of that

@LLFourn
Copy link
Contributor

LLFourn commented Apr 3, 2024

This looks great! However, there may be a situation where the chain-source returns anchors that do not point to a block in the best chain. Those unconfirmed transactions will therefore have no "timestamp". Would it be better if TxGraph::update_last_seen_unconfirmed takes in a checkpoint tip so unconfirmed transactions are transactions that do not have an anchor that points to a checkpoint?

last_seen_unconfirmed is for transactions that we've actually seen as unconfirmed in a fully validating mempool. Why would we set it for transactions that we've only seen as confirmed? Or are you saying esplora will allow us to determine that a tx is both in the mempool but was also confirmed in some non-canonical block? If that's the case then we have to pass in the timestamp into the sync/scan function so the logic can set both. The suggestion that we add and use TxGraph::update_last_seen_unconfirmed was based on the assumption that esplora and electrum would not allow us to distinguish this case.

@evanlinjin
Copy link
Member

@LLFourn the problem is we can't get blocks and transactions atomically. We either get blocks before getting transactions, or get transactions before getting blocks.

There might be a situation when a reorg happens between fetching txs and blocks. The tx might be sent back to mempool because it's not included in replacement block.

@LLFourn
Copy link
Contributor

LLFourn commented Apr 3, 2024

@LLFourn the problem is we can't get blocks and transactions atomically. We either get blocks before getting transactions, or get transactions before getting blocks.

There might be a situation when a reorg happens between fetching txs and blocks. The tx might be sent back to mempool because it's not included in replacement block.

...or it might not be if the re-org confirms a double spend. It might be in the mempool it might not be so we don't set last_seen because we don't know. Where's the problem?

Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b21ea0f

From looking at it, the changes in electrum_ext.rs seem to do exactly the same thing that would be achieved if you just called update_last_seen_unconfirmed so it would make sense to just do that instead of passing in seen_at. I'm pretty sure @evanlinjin will change this soon so I don't think it matters that much.

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 449dd9a

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the quality work. Let's drop the last_seen parameter from .into_tx_graph and I'll ACK/merge.

That accepts a `u64` as param representing the latest timestamp
and internally calls `insert_seen_at` for all transactions in
graph that aren't yet anchored in a confirmed block.
and `into_confirmation_time_tx_graph`, since now it makes
sense to use `TxGraph::update_last_seen_unconfirmed`.

Also, use `update_last_seen_unconfirmed` in examples for
electrum/esplora. We show how to update the last seen
time for transactions by calling `update_last_seen_unconfirmed`
on the graph update returned from a blockchain source, passing
in the current time, before applying it to another `TxGraph`.
@ValuedMammal
Copy link
Contributor Author

LGTM! Thanks for the quality work. Let's drop the last_seen parameter from .into_tx_graph and I'll ACK/merge.

Done. Thank you to the reviewers @evanlinjin @LLFourn @notmandatory @rustaceanrob

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a2a64ff

@evanlinjin evanlinjin merged commit 62619d3 into bitcoindevkit:master Apr 11, 2024
12 checks passed
@notmandatory notmandatory mentioned this pull request Apr 12, 2024
25 tasks
@ValuedMammal ValuedMammal deleted the fix/esplora-lastseen branch April 12, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-blockchain
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

last_seen is wrong on both electrum and esplora
5 participants