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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.

[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

[chuksys]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

[tnull]

@enigbe Please let me know when this is ready for a second look.

[enigbe]

Thanks for the feedback @tnull. I revisited this last week and addressed concerns raised here.

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.

This has been removed and we just keep a config for REST sync.

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?

Agreed! This seems like a cleaner split. I've added a set_chain_source_bitcoind_rest variant accordingly. However, I think it'd be potentially confusing/awkward for callers to still pass RPC parameters to the REST function. I opted for set_chain_source_bitcoind_rest to take only REST parameters and clarified comments on usage and dependency on the RPC variant.

If you'd still prefer we pass RPC parameters as a required input for the REST variant, happy to adjust.

[enigbe]

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

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

[tnull]

Thanks for looking at this. I've rebased and have just the CLN integration tests failing. I tried reproducing the failing test locally but couldn't. I should give it another go today.

Yes, I think this is unrelated, so feel free to ignore the flaky CI in this PR. Fixing it is really an unrelated task.

[enigbe]

Looking into this PR has helped me gain a good understanding of how chain sourcing works. I think introducing BitcoindSyncClient is a good idea as the user can still carry out syncing operations using the REST API while the RPC Client is still maintained to carry out operations not supported by the REST API. I have just a few suggestions with regards to error handling.

Thanks for the review. Added the suggestions recommended for error handling.

[enigbe]

@enigbe Please let me know when this is ready for a second look.

This is ready @tnull.

[tnull]

Hmm, tbh. I still dislike the current refactorings as they make the codepaths unnecessarily confusing and duplicative. Why keep a BitcoindRpcClient around side-by-side with the new BitcoindSyncClient that could also be an RPC client? If you maintain that merging in the REST codepaths with the RPC client is the way to go, why not extend the enum BitcoindSyncClient variants so that one would only hold the RPC client and the other the RPC and REST clients, which would allow us to add an fn rpc_client accessor that we can use everywhere, i.e., hence would allow us to drop the bitcoind_rpc_client field?

[tnull]

Mhh, let's name this rest_client_config then.

[tnull]

I'm not sure why this was added here vs bitcoind_rpc.rs.

[enigbe]

Please excuse the delay on this. I have addressed concerns from the last review. Investigating a flaky test but it's ready for another look.

Let me know what you think when you can review.

[tnull]

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit? Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

Thank you!

[tnull]

nit: Seems this was first added here, then dropped in a later commit?

[tnull]

Can we please do the code moves in a prefactor commit? This would make it immediately clear that nothing changed when reviewing via git diff --color-moved.

And to be honest I'm not entirely sure why we move all that code at all, if we just end up with the implementation for both rpc and rest in mod.rs?

[tnull]

Why not just BitcoindClient?

[enigbe]

This has been renamed.

[enigbe]

Thanks for the feedback.

Excuse the considerable delay here, I have to admit this fell off my radar for a bit. Sorry! (feel free to re-request review if a PR is ready or it's been pending for a while)

Noted. I'll re-request going forward.

Generally I like the updated approach much better from what I see. However, due to the huge code move and some commits that revert prior commits it's almost impossible to just review the parts of the code that actually changed (i.e., neither going commit-by-commit nor full-diff really works). Could you do me favor and clean up the commit history a bit?

I'll clean up the commit history to make review much easier. Apologies.

Also, I'm not conviced we gain much by the huge code move to common.rs/mod.rs if we don't actually split out REST and RPC types. Can we maybe revert that and keep everything in one file for now? Or, do the code move as an entirely separate commit in the beginning or end.

You're right wrt common.rs. It is a leftover from when I separated REST and RPC types. Since we're not doing that split, I'll revert back to keeping everything in a single file to keep things simpler.

I'll have this cleaned up and ready shortly.

[tnull]

This also needs a rebase now.

[enigbe]

Hi @tnull

I've cleaned up the history and rebased. It is ready for another look.

[tnull]

Excuse the delay here.

As far as I can tell the changes look mostly good, but the commit scheme still is very confusing to me and makes this very hard to review. Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code.

[enigbe]

Hi @tnull,

Why are we first adding a _rest variant with a lot of copy/paste code, only to then merge them, whilst at the same time adding everything to a new file?

I thought this might be helpful for other reviewers to see the justification in unifying the clients. Nonetheless, I've removed that commit so it should be as you'd prefer.

If you think this should all end up living in bitcoind.rs, please first make a commit moving the file to its new location (so that git understands it's the same file), and then make the changes in following commits. This would allow to see what actually was added and what changed in reference to the original code

I have added a prefactor commit to address the file change/rename.

Apologies for the confusion here. This should be much cleaner now.

[tnull]

Thanks for the commit history cleanup, this is indeed now more reviewable. It still includes a bunch of unnecessary changes/refactorings that bloat the current changeset. It would be preferable not to include these (such as moving code blocks, switching up which types to import and which not, etc).

Moreover, I have to say I'm not super stoked about putting all the logic behind macros, as they are almost inaccessible to tooling (compiler checks, rustfmt, code completion, etc). Can we try to avoid them by prefactors that move the code to methods/closures or to helper methods that take whatever data these methods are operating on? If we do this, would be great to keep a sane git history.

All that said, functionally the code looks good to me.

[enigbe]

Hi @tnull,

This is ready for another look. As recommended, I have added:

• prefector commit where the previously introduced macros are refactored into helper methods in preparation for REST support.
• couple of fix-ups to address unwanted code move and type imports.

[tnull]

Cool, thank you, much better without the macros!

Feel free to squash the fixups!

[enigbe]

Squashed the fixups.