Add transaction sync crate

[tnull]

This crate provides utilities for syncing LDK via the transaction-based Confirm interface. The initial implementation facilitates synchronization with an Esplora backend server.

Upstreamed from lightningdevkit/ldk-node#9.

[tnull]

I now included this essentially as a hotfix for the latest feedback from lightningdevkit/ldk-node#9. However, I'm still not sure if we shouldn't rather go either with the "simply always confirm everything" or the "re-register outputs via load_outputs_to_watch upon start of the sync round" approach.

[benthecarman]

putting this here causes it so confirmed txs are never added to watched_transactions and thus never confirmed

[tnull]

Huh, I'm not sure why they never would be confirmed?

Generally all transactions will be added to locked_watched_transactions when the Filter queues are processed via process_queues(). Any transaction returned by get_relevant_txids() only needs to be monitored for re- and unconfirmations, in which case we call transaction_confirmed and re-add them to the list to monitor for confirmation, i.e., watched_transactions. Am I missing something?

[benthecarman]

Haven't debugged it yet but we're using a copy-paste of this in mutiny and transactions would never get confirmed unless we added txs that were confirmed here

https://github.com/BitcoinDevShop/mutiny-web-poc/blob/master/node-manager/src/chain.rs

This was our bug fix

MutinyWallet/mutiny-node@9104a7d

[tnull]

Mh, I'm still not clear why that would happen when only re-adding the unconfirmed transactions.

I now cleaned up the approach a bit with 5d94cb6. Could you check whether this mitigates what you saw before?

Btw. I now also migrated to future::lock::Mutex. Does this allow you to re-use the create instead of copy-pasting?

[tnull]

Seems due to the dependencies of rust-esplora-client this will only pass CI for stable and above. Are we fine with this for a new crate? We'd need to upgrade the linting job to have it pass also it seems...

[TheBlueMatt]

Cool! A few assorted notes:

• I think ideally we wouldn't take on a dependency with a big HTTP client for this, but I assume we kinda need TLS, so we can't really avoid it. It may be worth looking into contributing an HTTP client upstream so that we can avoid the HTTP library and just take on a RustTLS (or openssl-sys or whatever) dependency, but it doesn't have to happen immediately. In the mean time, we can have a different MSRV for different crates, though ideally BDK starts supporting versions of rustc people have, rather than just latest.
• I believe this cannot land until we have some way to be much more confident in a server-side reorg not being possible without detection. I believe we need upstream changes for that, but that should be relatively easy, I think? Is there something I'm missing here?

[tnull]

• I believe this cannot land until we have some way to be much more confident in a server-side reorg not being possible without detection. I believe we need upstream changes for that, but that should be relatively easy, I think? Is there something I'm missing here?

No, I think while I covered a lot of cases, some edge cases still remain.

I'm currently considering whether adding a confirmed_since/in_best_chain_since timestamp field to Esplora's /block/:hash/status endpoint might be enough and a very unobtrusive way to get what we need. I.e., we could remember that timestamp when we check the tip initially and compare it to the one in the last check to figure out if there had been reorgs in-between. I imagine this is much more likely to be accepted than breaking the API by adding a as_of_tip_hash field to every call.

[TheBlueMatt]

I don't think adding a new field to calls "breaks the API" - isn't the point of JSON that we can add new fields and old clients will ignore them? Have you chatted with upstream yet?

[tnull]

I don't think adding a new field to calls "breaks the API" - isn't the point of JSON that we can add new fields and old clients will ignore them? Have you chatted with upstream yet?

Fair enough. Will look into opening corresponding issue/PR upstream. If there's much push-back, we might have above mentioned variant as a fallback option.

[tnull]

Now opened Blockstream/electrs#52, which adds the best_tip_hash to the /block/:hash/status endpoint. I think this should be sufficient to assert consistency.

Unfortunately it's not as straight forward to add a similar field to the merkleblock-proof endpoint as this returns just the raw hex-encoded data in Core's format. This means we'll still need to collect the corresponding block hashes during a sync round and check their status and the tip invariance at the end once. But I think that should be no big deal.

[TheBlueMatt]

I'm confused how that's sufficient here - can't the server do a reorg while we're making other queries and then reorg back to the original chain before we get to check it?

[tnull]

I'm confused how that's sufficient here - can't the server do a reorg while we're making other queries and then reorg back to the original chain before we get to check it?

All other calls (only get_merkle_block and get_output_status are relevant really, I think) give us block hashes in which the confirmations happened. We currently already call get_block_status for essentially all of them to retrieve the block height. With the upstream change we can now also assert that the tip hasn't changed since we started syncing, which ensures that there is no inconsistency, i.e., block status is confirmed and under best_tip_hash == cur_tip_hash.

We may be able to save some of these calls if upstream also added a best_tip_hash field to the transaction status returned from get_output_status()'s spending transaction. I may add this as follow-up PR upstream, but would like to keep it separate as it is less trivial than the one for the block status.

[tnull]

@TheBlueMatt Rebased on main and now uses the recent release of esplora-client, which includes all calls we need.

Also, I think the observation @andrei-21 recently made is correct: even in a reorg-then-reorg-back scenario the tip would change (as it only would reorg-back if there is a longer chain featuring a new tip). As we check tip consistency just before handing over any changes to LDK, we would detect any reorg that happened in the meantime. I therefore think the currently taken fail-restart approach should be safe as is.

Note that we'd currently also restart if we're still on the same chain but a new tip has been appended. If upstream added the tip hash field to the block and tx statuses, we may even be able to recover and continue the sync in this case, but this would mostly be a performance improvement I think.

[TheBlueMatt]

Also, I think the observation @andrei-21 recently made is correct: even in a reorg-then-reorg-back scenario the tip would change (as it only would reorg-back if there is a longer chain featuring a new tip).

No, this is not correct. Bitcoin Core will expose the intermediary state. So while, yes, eventually we'll get to a new tip, the tip may jump back to the previous tip, the Bitcoin Core will drop the cs_main lock, allowing it to expose that tip to clients (in this case esplora), which can be exposed ultimately to us.

[jkczyz]

Looks like CI is unhappy for 1.57. Otherwise, looks good. Feel free to rebase and leave any comments needing large changes to a follow-up.

[tnull]

Looks like CI is unhappy for 1.57. Otherwise, looks good. Feel free to rebase and leave any comments needing large changes to a follow-up.

Squashed the fixups.

[tnull]

Squashed fixups without further changes.

[tnull]

Squashed again removing superfluous whitespace:

git diff-tree -U2 45d41467 d4580117
diff --git a/lightning-transaction-sync/src/esplora.rs b/lightning-transaction-sync/src/esplora.rs
index 9f109cb1..807ef807 100644
--- a/lightning-transaction-sync/src/esplora.rs
+++ b/lightning-transaction-sync/src/esplora.rs
@@ -213,5 +213,5 @@ where

                        sync_state.watched_transactions.remove(&ctx.tx.txid());
-
+
                        for input in &ctx.tx.input {
                                sync_state.watched_outputs.remove(&input.previous_output);

[jkczyz]

Discussed offline last week. I wasn't able to get the integration test to run on my corp machine. Not sure why but seems like the connection to bitcoind gets disconnected when waiting for blocks. Shouldn't be a blocker, though.