Add support for sourcing chain data from Electrum

[tnull]

Closes#196.

So far, we support Esplora and Bitcoind RPC chain sources. In this PR we add Electrum support based on the blocking rust-electrum-client, and it's bdk_electrum and lightning-transaction-sync counterparts.

Due to the blocking nature of rust-electrum-client and as it directly connects to the server upon Client::new (see bitcoindevkit/rust-electrum-client#166), we ended up wrapping the runtime-specific behavior in an ElectrumRuntimeClient object that is initialized and dropped in Node::start and stop, respectively.

One thing missing that we still need to consider is how we'd reestablish connections to the remote after they have been lost for one reason or another. IMO, that behavior should live in rust-electrum-client to avoid all users having to duplicate it, so it's pending resolution of bitcoindevkit/rust-electrum-client#165

As we did with bitcoind-RPC, Electrum support is tested by adding another full_cycle integration test.

[elnosh]

I'm familiarizing myself with the codebase but I tried running a node locally with the changes here pointing to an electrum server and basic operations are working. I just have a couple of questions, if I may, to get a better understanding.

Not strictly related to the PR but maybe I can sneak it in, why is it that for bitcoin core rpc and electrum they are in their separate bitcoind_rpc.rs and electrum.rs files but for esplora it is all in the mod.rs? I see that that the EsploraAsyncClient is directly used without a wrapper but wanted to see what were the reasons for it (:

[tnull]

Not strictly related to the PR but maybe I can sneak it in, why is it that for bitcoin core rpc and electrum they are in their separate bitcoind_rpc.rs and electrum.rs files but for esplora it is all in the mod.rs?

Hmm, for one it has historic reasons, but also bitcoind RPC and electrum both ended up needing additional helper objects/types that made sense to split out to dedicated sub-modules. While we could eventually refactor the Esplora part so more code lives in a dedicated esplora.rs file, there is no strong necessity for it right now I think.

I see that that the EsploraAsyncClient is directly used without a wrapper but wanted to see what were the reasons for it (:

Right, that's essentially the main reason why it still lives in mod.rs. For electrum we ended up needing a wrapper that is created on runtime, and for bitcoind RPC we implemented a specific wrapper around RpcClient that handles the JSON parsing, essentially.

[tnull]

Blocked on rust-bitcoin/corepc#111 for now.

[tnull]

Rebased on main after #508 landed.

[joostjager]

The question that I asked myself most during review is whether the way this PR deals with the runtime and start/stop is really the best option for now.

[tnull]

Squashed fixups without further changes.

[joostjager]

Nearly there. Just the one question about the double timeouts.

[joostjager]

I see you also set retries now. I saw that it was 1 previously (the default). Did the earlier test with the wifi disconnect pass because that one retry happened after you already reconnected wifi?

If I understand this correctly, setting the timeout on the electrum client itself is essential to not end up with a potential graveyard of stuck threads? This might be important to add as a comment.

Also curious to hear why you think that retrying over and over again when a previous request is still stuck is a good idea? It will eventually exhaust the thread pool, right? And then stop independent processes within LDK too?

The original reason for the double timeout is because esplora-client turned out to be not trustworthy. But, as suggested above, isn't electrum different because it isn't an async library and it is unlikely for the timeout to fail? Looking at the sources, it seems that the timeout is set on a pretty low level for all calls.

[tnull]

The corerpc-node just shipped the needed fix as part of the 0.7 minor release, now we just need RCasatta/electrsd#100 to land as part of an electrsd release to go forward with this.

[enigbe]

I have taken another look at this PR and now understand why you apply pending filter entries to the pending transaction and output vectors. The refactor that introduced ElectrumRuntimeStatus made it easier to reason about it as a state machine with the attendant events, actions, and state or internal transitions.

From the snippet below, when we start the Node with the tokio runtime, it indicates that other background tasks do not get started before the ChainSource has started:

pubfn start_with_runtime(&self,runtime:Arc<tokio::runtime::Runtime>) -> Result<(),Error>{
... // Start up any runtime-dependant chain sources (e.g. Electrum)self.chain_source.start(Arc::clone(&runtime)).map_err(|e| {log_error!(self.logger,"Failed to start chain syncing: {}", e);
e
})?;
...
// Other background tasks

This suggests that it is unlikely that Confirmables would register transactions and outputs prior the chain source starting and, in extension, prior to the runtime client being available (ElectrumRuntimeStatus::Started). Are there scenarios I am missing where this could happen? I'd appreciate any further clarification.

Otherwise, the PR looks good to me. Reviewing this has been quite educational. Thank you!

[tnull]

I have taken another look at this PR and now understand why you apply pending filter entries to the pending transaction and output vectors.

Thank you for the review!

This suggests that it is unlikely that Confirmables would register transactions and outputs prior the chain source starting and, in extension, prior to the runtime client being available (ElectrumRuntimeStatus::Started). Are there scenarios I am missing where this could happen? I'd appreciate any further clarification.

Unfortunately, the ChannelMonitors register the transactions/outputs they're interested in when ChainMonitor::watch_channel is called. See:
https://github.com/lightningdevkit/rust-lightning/blob/85185d863871fefa5243fa9e38f3037b5db3eac6/lightning/src/chain/chainmonitor.rs#L790

and

https://github.com/lightningdevkit/rust-lightning/blob/85185d863871fefa5243fa9e38f3037b5db3eac6/lightning/src/chain/channelmonitor.rs#L1776

In turn we currently call this upon initialization:

ldk-node/src/builder.rs

Line 1235 in 5abb42f

chain_monitor.watch_channel(funding_outpoint, channel_monitor).map_err(|e| {

not at runtime/during Node::start. This is one of the reasons we decided to soon refactor this so that we'd intialize most LDK objects only at start, not in Builder::build. Another (actually even more important) reason to do this is that we'd then be able to first retrieve the current best block before initializing the ChannelManager and other objects for the first time, avoiding the currently necessary resyncing from genesis (cf. #415).

[tnull]

Squashed fixups without further changes.

[joostjager]

LGTM!

Still interested to see if conversion to a more object-oriented style for the backends is straight-forward or not.