Refactor syncing and introduce ChainSource

[tnull]

Based on #358
Closes #362

We refactor our chain syncing and introduce a new ChainSource enum, which is a prerequisite for adding new chain data sources in a follow-up.

To this end, we first move the syncing logic to ChainSource and then introduce incremental syncing for Esplora which is always used except for initial syncs (now permanently persisted via a new NodeMetrics struct).

TODO:

• Move Esplora-specific config options to fields in a tba. EsploraConfig object, whose defaults can be overridden in the Builder::set_chain_source_esplora method.

[tnull]

Now addressed the open TODO mentioned above: moved Esplora-specific sync options to a new EsploraSyncConfig.

[tnull]

Rebased after #366 landed.

[tnull]

Rebased after #358 landed.

[jkczyz]

Will these helpers be called more than once? Just wondering what the rationale for splitting the logic across files. If the only difference later is how convert_fee_rate is called, seems that update_fee_estimates could be parameterized by a trait instead.

Otherwise, all the fee estimation logic is in the chain module instead of here, which seems wrong. A similar argument can be made for other code moved to chain, IMO (e.g., broadcasting).

[tnull]

Will these helpers be called more than once? Just wondering what the rationale for splitting the logic across files. If the only difference later is how convert_fee_rate is called, seems that update_fee_estimates could be parameterized by a trait instead.

Yes, they will be used by the chain-source specific fee estimation implementations. Unfortunately convert_fee_rate isn't the only difference, each of the implementations will work out quite differently (e.g., bitcoind will deliver the estimates individually instead of all together, and in a different unit. Additionally we'll want to use the mempoolminfee value from the getmempoolinfo call to determine the mempool minimum, rather than a far out block-based target, etc. (cf. 47beb0f)).

Otherwise, all the fee estimation logic is in the chain module instead of here, which seems wrong.

I agree that it's not super intuitive, but unfortunately the logic for fee estimation and even broadcasting will work out pretty differently for Esplora, bitcoind RPC, and Electrum. So if we left them in fee_estimator and tx_broadcaster we will in the contrary need to pull in a lot of ChainSource-specifics in there, not sure whether that would be much preferable? When I wrote it there were also some concerns around introducing circular object dependencies, although I have to admit that I'm failing to pin point / remember them right now.

So for now it may be fine to implement the ChainSource-specific parts (which is most of the logic) in ChainSource? We should however do another round of refactoring/DRYing up where we can once it's all said an done.

[jkczyz]

Hmm... right, but it seems doable. For instance, the trait's method could take a list of confirmation targets and return an iterator over fee rates. This would keep the code fairly DRY, avoiding repeating much of the logic and keeping these functions internal to the fee_estimation module, IIUC.

[tnull]

the trait's method

Do I understand correctly that you referring to a new internal trait here, not LDK's FeeEstimator? Then I would agree that this would likely be a reasonable refactor if we end up with separating out individual ChainSource objects (rather than the single enum), which would lend themselves to implement this trait. We'll see, as a first step I want to get everything working as expected and then we can clean up the internal APIs afterwards. This prefactor PR is really mainly meant to lay the groundwork to get started with other chain sources in the first place.

[jkczyz]

Yeah, an internal trait but I believe it could be implemented on EsploraAsyncClient. You wouldn't need individual ChainSource objects.

[tnull]

Hmm... why couldn't the trait be internal to LDK Node?

Ah, nevermind, indeed it could live in LDK Node. We might then need to take a dependency on async_trait for it though, so not sure if going the trait route would be worth it, but that's tbd.

[jkczyz]

Hmm... or possibly returning some sort of async iterator? Alternatively, OnchainFeeEstimator::update_fee_estimates can be made into a macro and instantiated for each.

[tnull]

To be clear, all I'm suggesting is parameterizing OnchainFeeEstimator::update_fee_estimates by a trait that can be implemented for both EsploraAsyncClient and BitcoindRpcClient. Then we wouldn't need to move all the code just to move it back later.

Right, but that trait method would at the very least be async (meaning our <1.75 MSRV we'd need to take a dependency on async_trait) and given the difference in logic probably would need to be update_fee_estimates itself, minus the set_fee_rate_cache part, which still would need to be called afterwards based on the return value of said trait method? Then again, BDK currently doesn't offer an async version of their electrum client, so at least for the initial implementation we will need to fit in a blocking client in the otherwise async internal GossipSource APIs. This might be prohibitive for going the async trait route, or at least for implementing it directly on the clients themselves, as we'll need to hand in our runtime object and use spawn_blocking for every call we make.

[jkczyz]

Right, I was thinking something like this:

pub(crate) async fn update_fee_estimates<F: FeeSource>(&self, fee_source: f) -> Result<(), Error> {
    let mut new_fee_rate_cache = HashMap::with_capacity(10);
    let confirmation_targets = vec![/* .. */];
    for (target, fee_rate) in fee_source.fee_rates(confirmation_targets) {
        let adjusted_fee_rate = /* .. */;
        new_fee_rate_cache.insert(target, adjusted_fee_rate);
    }

    if fee_estimator.set_fee_rate_cache(new_fee_rate_cache) { /* .. */ }
}

Where FeeSource::fee_rates returns an iterator. I'm handwaving the async part -- so fair enough if our MSRV prevents this -- but I'd think the different logic could be encapsulated in FeeSource::fee_rates implementations since it is given all targets. For esplora, it can fetch all at once using get_fee_estimates -- so the trait method would need to be async as you said -- and return an iterator callinh convert_fee_rate on each item. For bitcoind, it can return an iterator that fetches them one-by-one.

Maybe there is a way to do it with a Fn trait parameter instead if using aync_trait is a problem? Something like: https://forum.dfinity.org/t/passing-async-function-or-closure-as-parameter/21785/9

Anyhow, happy to leave at is for now given the difficulties with async / blocking.

[tnull]

Yeah, as said above I don't disagree that we should explore something like this, but for now (and in this PR) I'd like to keep the focus on actually shipping the features. We'll then follow-up with more refactoring once we have a better understanding how to accommodate all the different requirements, i.e., after we're done with the bitcoind and Electrum integration.

I understand that we'd usually want to avoid touching the same code multiple times, but in this instance (and given the time constraints) I'd prefer to get something functional merged soon rather than spending more time on figuring out the best refactoring pattern (which would require to think through the details of the Electrum integration and accoommodate the anticipated changes).

TLDR: I agree we should (and will) do this, but let's do one step at a time to keep making progress.

[jkczyz]

LGTM. Please squash.

[tnull]

LGTM. Please squash.

Squashed the fixup and rebased on main to fix the build post-yanking of the v0.0.124 lightning-* crates.