fix(wallet_esplora): missing_heights uses the graph update

[danielabrozzoni]

Fixes #1151.

When wallet_esplora_* was used to sync a wallet containing a transaction confirmed some time ago (more than 10-15 blocks ago), the transaction would be stuck in an "unconfirmed" state forever.

At the first scan time, update_local_chain would just fetch the last 10 to 15 blocks (depending on the server used), and tx_graph.missing_heights wouldn't return the tx's confirmation block as it was called on the original, non-updated tx_graph.
So, after the first scan, we would have a transaction in memory with an anchor that doesn't exist in our local_chain, and try_get_chain_position would return unconfirmed.

When scanning again, missing_heights would find the missing anchor, but update_local_chain wouldn't include it as it's older than ASSUME_FINAL_DEPTH.

The missing block would be downloaded every time, but never included in the local_chain, and the transaction would remain unconfirmed forever.

Here we call missing_heights on the updated graph, so that it can correctly return the anchor height, and update_local_chain can fetch it and include it in the chain.

Notes to the reviewers

I'm not sure if this is the right approach, so I'm opening this PR to gather feedback. I still need to add tests, I'll do so once we decide if this is the right way to go or not.

Checklists

All Submissions:

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

[danielabrozzoni]

Cherry-picked cdfb91f from #1041 for ci :(

[realeinherjar]

Good one.
Another MSRV break in CI...
tACK cbf0ad9 (with mempool.space not blockstream.info)

[evanlinjin]

This is clearly my oversight. Thanks for identifying and fixing it.

Can you rearrange the commits to that the MSRV fix comes before the wallet_esplora.. commit? Thanks.

Will give this a test, ACK and merge tomorrow.

[danielabrozzoni]

I updated the code to match @LLFourn suggestion; other than being way cleaner, I later found out that my original solution was broken as well.

FYI, doing

    let update = Update {
        last_active_indices,
        graph: update_graph,
        chain: Some(chain_update),
    };
    wallet.apply_update(update)?;

yields a different result than

    let update = Update {
        graph: update_graph,
        ..Default::default()
    };
    wallet.apply_update(update)?;
    
    let update = Update {
        last_active_indices,
        chain: Some(chain_update),
        ..Default::default()
    };
    wallet.apply_update(update)?;

I haven't looked much into it, but when the graph update is added separately from the rest of the update, list_unspent gives a wrong result. I'm not sure if this is expected (it might be, but I certainly didn't expect it!), or a bug.

[danielabrozzoni]

Regarding tests, we have an issue open for integration tests: #1094. Should I start to tackle it in this PR, or can we merge and think about it in the next milestone?

[evanlinjin]

I haven't looked much into it, but when the graph update is added separately from the rest of the update, list_unspent gives a wrong result. I'm not sure if this is expected (it might be, but I certainly didn't expect it!), or a bug.

We should always apply the index update first. When the index update and graph update is in the same wallet::Update, the logic of Wallet::apply_update applies the index update first. If you don't apply the index update first, the transactions in the graph may not be indexed properly (some outputs will not end up being tracked, even though it's spk is part of the keychain).

Calling missing_heights on the unapplied TxGraph should get the same result as calling it on the wallet's TxGraph. The difference is that the prior is more efficient (so the new changes are welcome).

[evanlinjin]

As a side note, I realized there is no way to set the keychain index's lookahead in Wallet. This will pose a problem for block-by-block chain sources.

[evanlinjin]

ACK b1461f0