Clean up and modularize chain syncing logic #598

tnull · 2025-07-24T14:38:06Z

We intended to modularize and clean up our chain sync since quite a while. Here, we finally do it by introducing a new ChainSourceKind type and moving the respective syncing logic to new EsploraChainSource, ElectrumChainSource, and BitcoindChainSource objects in the respective modules.

This is a rather large refactor, but 99% of the changes are code moves.

Additional cleanup steps / minor fixes will be coming up, but given the PR size I'll probably open a follow-up for them.

ldk-reviews-bot · 2025-07-24T14:38:09Z

👋 Thanks for assigning @joostjager 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.

joostjager · 2025-07-24T14:54:10Z

src/chain/mod.rs

@@ -697,118 +650,8 @@ impl ChainSource {
 	// etc.)
 	pub(crate) async fn sync_onchain_wallet(&self) -> Result<(), Error> {
 		match &self.kind {
-			ChainSourceKind::Esplora {


I really don't know how to review a diff like this. But I think it is possible to improve by keeping the moved code close to where it was removed in a first commit, and then doing a clean move in a second.

I think it is too much to ask from a reviewer to see changes like this in the diff as it is.

Hmm, will play around with that a bit. Maybe it makes sense to first move everything into individual impl blocks beneath the original method before joining all in the new location.

I now went ahead and introduced intermittent impl blocks, and split the commits introducing the new chain source types and moving them to the dedicated modules. All of this should make for a considerable improvement, especially when reviewed via git diff --color-moved --ignore-all-space. I think there are some areas where git diff is still tripping, but I honestly don't know how to improve things further there.

Now pushed this new rewrite, which amounted to an net diff that is almost 0 (just moved a single helper method to a more reasonable location in the process).

We introduce a new `ChainSourceKind` that is held as a field by `ChainSource`, which better encapsulates the chain syncing logic, and in future commits allows us to move some common fields to `ChainSource`.

.. in the hopes of making the git diff more readable going forward, we break up the `ChainSource` impl block.

We refactor our `ChainSource` logic and move out the Esplora code into a new object.

We refactor our `ChainSource` logic and move out the Electrum code into a new object.

We refactor our `ChainSource` logic and move out the Bitcoind code into a new object.

joostjager

This PR shows a really nice decomposition / clean up. I think it was worth it.

The only real comment/question that I have is whether ChainSource is still needed? It is now mainly a pass through and I was wondering if that could be replaced by a trait?

joostjager · 2025-07-29T11:44:43Z

src/chain/mod.rs

-										logger,
-										"Successfully broadcast transaction {}",
-										txid
+										self.logger,


Glad you did this somewhat artificial commit struct. Very easy to ascertain that it's still the same.

joostjager · 2025-07-29T11:46:32Z

src/chain/electrum.rs

@@ -42,7 +42,83 @@ const BDK_ELECTRUM_CLIENT_BATCH_SIZE: usize = 5;
 const ELECTRUM_CLIENT_NUM_RETRIES: u8 = 3;
 const ELECTRUM_CLIENT_TIMEOUT_SECS: u8 = 10;

-pub(crate) struct ElectrumRuntimeClient {
+pub(super) enum ElectrumRuntimeStatus {


Maybe the diff would improve a bit if you left this at roughly the same location as where it was.

I'm not sure I moved this much, but rather moved other things around that have git conclude that this was moved. Not sure it's worth experimenting further, I already did a few iterations until I reached the current state^^

Yes, fair. Current state is already 90% of the win, if not 100% given diff limits.

src/chain/bitcoind.rs

tnull · 2025-07-29T12:10:09Z

The only real comment/question that I have is whether ChainSource is still needed? It is now mainly a pass through and I was wondering if that could be replaced by a trait?

Ah, I don't think we can due to how different the transaction vs. block-polling-based chain sources are. If we'd introduce two traits for them, we'd still need an adaptor that would forward to the right implementation.

.. now that we don't need them anymore for review, we drop the extra `impl` blocks again.

tnull · 2025-07-29T12:14:10Z

I think I'm gonna land this, will see to get a PR with follow-ups up soon.

joostjager · 2025-07-29T12:19:27Z

The only real comment/question that I have is whether ChainSource is still needed? It is now mainly a pass through and I was wondering if that could be replaced by a trait?

Ah, I don't think we can due to how different the transaction vs. block-polling-based chain sources are. If we'd introduce two traits for them, we'd still need an adaptor that would forward to the right implementation.

I would think that something like process_broadcast_queue could just as well be a method of a shared trait?

But the actual logic is now separated, so goal achieved.

tnull requested a review from joostjager July 24, 2025 14:38

joostjager reviewed Jul 24, 2025

View reviewed changes

tnull added this to Weekly Goals Jul 24, 2025

tnull self-assigned this Jul 24, 2025

tnull moved this to Goal: Merge in Weekly Goals Jul 24, 2025

tnull added 2 commits July 29, 2025 09:12

Introduce ChainSourceKind type

7e93a8b

We introduce a new `ChainSourceKind` that is held as a field by `ChainSource`, which better encapsulates the chain syncing logic, and in future commits allows us to move some common fields to `ChainSource`.

Intermittently introduce additional impl blocks

5afe490

.. in the hopes of making the git diff more readable going forward, we break up the `ChainSource` impl block.

tnull force-pushed the 2025-07-chainsource-refactor branch from e966cc9 to d39869a Compare July 29, 2025 07:16

tnull added 2 commits July 29, 2025 09:23

Move Esplora sync logic to a EsploraChainSource type

56167ce

We refactor our `ChainSource` logic and move out the Esplora code into a new object.

Move EsploraChainSource to a new module chain::esplora

8a01e17

tnull force-pushed the 2025-07-chainsource-refactor branch from d39869a to c6a45c1 Compare July 29, 2025 07:26

tnull added 3 commits July 29, 2025 09:40

Move Electrum sync logic to a ElectrumChainSource type

fd517ed

We refactor our `ChainSource` logic and move out the Electrum code into a new object.

Move ElectrumChainSource type to chain::electrum module

40d6440

Move Bitcoind sync logic to a BitcoindChainSource type

4541a0e

We refactor our `ChainSource` logic and move out the Bitcoind code into a new object.

tnull force-pushed the 2025-07-chainsource-refactor branch from c6a45c1 to f82eec4 Compare July 29, 2025 08:08

tnull requested a review from joostjager July 29, 2025 08:12

joostjager approved these changes Jul 29, 2025

View reviewed changes

tnull added 2 commits July 29, 2025 14:10

Move BitcoindChainSource type to chain::bitcoind module

a7e54d8

Drop intermittent impl blocks on ChainSource

7cb1d34

.. now that we don't need them anymore for review, we drop the extra `impl` blocks again.

tnull force-pushed the 2025-07-chainsource-refactor branch from f82eec4 to 7cb1d34 Compare July 29, 2025 12:10

tnull merged commit 0935401 into lightningdevkit:main Jul 29, 2025
10 of 15 checks passed

github-project-automation bot moved this from Goal: Merge to Done in Weekly Goals Jul 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clean up and modularize chain syncing logic #598

Clean up and modularize chain syncing logic #598

Uh oh!

tnull commented Jul 24, 2025

Uh oh!

ldk-reviews-bot commented Jul 24, 2025 •

edited

Loading

Uh oh!

joostjager Jul 24, 2025

Uh oh!

tnull Jul 24, 2025

Uh oh!

tnull Jul 29, 2025 •

edited

Loading

Uh oh!

joostjager left a comment

Uh oh!

joostjager Jul 29, 2025

Uh oh!

joostjager Jul 29, 2025

Uh oh!

tnull Jul 29, 2025

Uh oh!

joostjager Jul 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

tnull commented Jul 29, 2025

Uh oh!

tnull commented Jul 29, 2025

Uh oh!

Uh oh!

joostjager commented Jul 29, 2025

Uh oh!

Uh oh!

Clean up and modularize chain syncing logic #598

Clean up and modularize chain syncing logic #598

Uh oh!

Conversation

tnull commented Jul 24, 2025

Uh oh!

ldk-reviews-bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull commented Jul 29, 2025

Uh oh!

tnull commented Jul 29, 2025

Uh oh!

Uh oh!

joostjager commented Jul 29, 2025

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 24, 2025 •

edited

Loading

tnull Jul 29, 2025 •

edited

Loading

joostjager Jul 29, 2025 •

edited

Loading