feat: chain source rest sync

[enigbe]

What this PR does

This PR introduces chain source synchronization with Bitcoin Core's REST API. To do so, it explicitly distinguishes between the bitcoind_rpc_client and the newly introduced bitcoind_sync_client that can sync either via the RPC or REST APIs.

BitcoindRpcClient is maintained and the distinction created because the REST interface exposed by Bitcoin Core is not as robust as the RPC interface. This makes it impossible to send POST request to broadcast a transaction for example.

Related Issues

Fixes #496

cc @tnull @joostjager

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tnull]

Thanks for looking into this! Before I get into more detailed review, I want to note two things on the chosen approach that stood out to me from a first look:

1. I'm not quite convinced we should rename the BitcoindRpc client. Wouldn't it be cleaner to just add a (granted, rather duplicative) BitcoindRest variant side-by-side, and then in a second step maybe see how we can DRY up the code. Mushing both variants together seems to result in a rather confusing API and code structure.
2. I'm a bit confused why we'd now still have RPC credentials on set_chain_source_bitcoind (and elsewhere in downstream from that), even if we want to use the REST backend? Is there any operation that still requires RPC access? Can't we just rely on REST alone?

[enigbe]

Thanks for the early feedback.

1. I'm not quite convinced we should rename the BitcoindRpc client. Wouldn't it be cleaner to just add a (granted, rather duplicative) BitcoindRest variant side-by-side, and then in a second step maybe see how we can DRY up the code. Mushing both variants together seems to result in a rather confusing API and code structure.
2. I'm a bit confused why we'd now still have RPC credentials on set_chain_source_bitcoind (and elsewhere in downstream from that), even if we want to use the REST backend? Is there any operation that still requires RPC access? Can't we just rely on REST alone?

I understand your concerns about the name and API changes I have introduced but here is a bit of context.

Adding a ChainSource::BitcondRest variant was my initial approach too because I had erroneously thought Bitcoin Core's REST interface exposed (more or less) the same APIs as the RPC interface. However, the REST interface exposes a handful of APIs only for GET requests. It is not possible to broadcast transactions with a pure REST variant as we do with the BitcoindRpcClient, There is no REST API to sendrawtransaction.
For that particular use case, given that we'd need a handle on an RPC client regardless, I thought it best to:

• Keep the BitcoindRpcClient as is.
• Introduce a configurable REST/RPC sync client. With this, users can decide if syncing should happen either via the REST or RPC APIs.
• Rename ChainSource::BitcoindRpc to ChainSource::Bitcoind as the latter leaves no ambiguity about the possibility of a REST variant.

[tnull]

I understand your concerns about the name and API changes I have introduced but here is a bit of context.

Adding a ChainSource::BitcondRest variant was my initial approach too because I had erroneously thought Bitcoin Core's REST interface exposed (more or less) the same APIs as the RPC interface. However, the REST interface exposes a handful of APIs only for GET requests. It is not possible to broadcast transactions with a pure REST variant as we do with the BitcoindRpcClient, There is no REST API to sendrawtransaction. For that particular use case, given that we'd need a handle on an RPC client regardless, I thought it best to:

* Keep the `BitcoindRpcClient` as is.

* Introduce a configurable REST/RPC sync client. With this, users can decide if syncing should happen either via the REST or RPC APIs.

* Rename `ChainSource::BitcoindRpc` to `ChainSource::Bitcoind` as the latter leaves no ambiguity about the possibility of a REST variant.

Aaah, okay, then the proposed approach makes more sense. I think it's still a bit awkward to have the mandatory RPC credentials on the set_chain_source_bitcoind method and addtionally have the BitcoindSyncConfig::Rpc variant.

While it then makes sense to reuse the client internally, it might be simpler to just keep the original set_chain_source_bitcoind_rpc method and just add a set_chain_source_bitcoind_rest variant, that also features the mandatory RPC credentials?

[Camillarhi]

Hi @enigbe,

I noticed your CI is failing. I had a similar issue, but it was resolved after rebasing onto the latest dev branch (which includes changes from #524).

Could you try rebasing and see if that fixes the problem?

Thanks