Adapt to `lightning-block-sync` API changes by jkczyz · Pull Request #874 · lightningdevkit/ldk-node

jkczyz · 2026-04-14T03:50:04Z

Update to use the new HeaderCache type instead of implementing the Cache trait, pass BestBlock instead of BlockHash to synchronize_listeners, and pass HeaderCache by value to SpvClient::new.

Also adapt to BestBlock gaining a previous_blocks field and ChannelManager deserialization returning BestBlock instead of BlockHash.

These changes are from lightningdevkit/rust-lightning#4266.

ldk-reviews-bot · 2026-04-14T03:50:07Z

👋 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, unfortunately bindings are currently broken.

Hmm, so, it seems lightningdevkit/rust-lightning#4266 fundamentally broke what BestBlock used to mean, i.e., being a simple data type for the chain tip. Really not loving the change, but I probably should have spoken up on that PR.

@TheBlueMatt any thoughts on still creating a new type for the 'best-sub-chain' that BestBlock now represents and switching the LDK APIs to that in a follow-up to lightningdevkit/rust-lightning#4266, rather than completely changing BestBlock semantics?

In any case, we need to figure something out as the changes currently broke our bindings, and the changes in BestBlock simply can't be exposed via uniffi as-is. I guess we could duplicate the old BestBlock code in this repo, but I'd really prefer if we could revert BestBlock and just use a new type for the semantics.

TheBlueMatt · 2026-04-14T13:34:41Z

@TheBlueMatt any thoughts on still creating a new type for the 'best-sub-chain' that BestBlock now represents and switching the LDK APIs to that in a follow-up to lightningdevkit/rust-lightning#4266, rather than completely changing BestBlock semantics?

I don't understand this? A BestBlock is the chain tip info, its used (pretty consistently) to indicate where we last synced to and that can/should absolutely include a few recent headers. At least in lightning we don't really have a use for a "best block hash and height" type, if LDK Node has a use for such a type it could of course have an internal type for it, but its not clear to me what the use for that would be?

tnull · 2026-04-14T16:12:44Z

I don't understand this? A BestBlock is the chain tip info, its used (pretty consistently) to indicate where we last synced to

For one it's a misnomer / unexpected API now. If something is called BestBlock I expect it to represent the information of a (single) block. Something like bdk_chain's CheckPoint that is made up of BlockIds (the equivalent of BestBlock) would be a much more intuitive/expected API and naming scheme, IMO. The previous_blocks field seems like information specific to how our chain syncing logic works, which preferably wouldn't leak into the public API?

It's also unnecessarily inefficient for many cases, as we for example now clone a bunch of BlockHashess when simply checking {ChannelManager, OutputSweeper, etc}::current_best_block.

and that can/should absolutely include a few recent headers.

Thankfully it's just a few hashes, not the full Headers.

At least in lightning we don't really have a use for a "best block hash and height" type, if LDK Node has a use for such a type it could of course have an internal type for it, but its not clear to me what the use for that would be?

Yeah, fair if we don't deem the inefficiency the additional clones fixing, but from my point of view a more intuitive name would still be worth adopting (before the 0.3 release), even if we just move the old BestBlock to LDK Node.

If we think previous_blocks is worth to always have as part of this 'chain check point', it would be nice if LDK could make it somewhat bindings compatible, so that we don't have to introduce yet another awkward From implementation and always have to re-allocate etc.

jkczyz · 2026-04-23T23:06:07Z

Discussed offline. We'll @TheBlueMatt will rename upstream's BestBlock to BlockLocator. Here, I've added a BestBlock that is like BlockLocator but without previous_blocks. Also rebased.

jkczyz · 2026-04-23T23:08:22Z

BTW, we'll probably want to merge this rather than wait on the upstream change since another breaking change (updated in #878) is already upstream.

tnull

Looks good, but CI is unhappy:

error: use of deprecated associated function `bitcoin::FeeRate::from_sat_per_vb_unchecked`: use from_sat_per_vb_u32 instead
   --> /home/runner/work/ldk-node/ldk-node/target/debug/build/ldk-node-8a4d944ca9f27956/out/ldk_node.uniffi.rs:757:12
    |
757 |     pub fn r#from_sat_per_vb_unchecked(
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(deprecated)]`

error: could not compile `ldk-node` (lib) due to 1 previous error

tnull · 2026-04-27T08:54:17Z

CI failure is fixed by #891, would be good to rebase this PR after this lands to have uniffi CI pass before we merge.

tnull

LGTM, one question

tnull · 2026-04-28T08:03:55Z

+		let mut current_block = Some(checkpoint.clone());
+		let previous_blocks = std::array::from_fn(|_| {
+			let child = current_block.take()?;
+			let parent = child.prev().filter(|cp| cp.height() + 1 == child.height())?;


Do I understand correctly that this filter is mostly defensive as we expect prev already to return the right parent and we shouldn't have gaps? Maybe it's worth leaving a comment here? (not sure if a debug_assert would be warrented?)

It can actually have gaps as CheckPoint::from_block_ids doesn't care if they are contiguous. But I was only able to reproduce the behavior with an Esplora backend, which doesn't use current_best_block. Left a comment.

It can actually have gaps as CheckPoint::from_block_ids doesn't care if they are contiguous. But I was only able to reproduce the behavior with an Esplora backend, which doesn't use current_best_block. Left a comment.

Right, that's what I thought. However, one of the main motivations for the lightning-block-sync refactor was that wanted to enable safe switching between chain sources. Are we positive that users can switch back-and-forth between Esplora / Electrum and bitcoind now, without risking that they don't have the necessary data available to handle reorgs? Or maybe it's fine because BDK doesn't have the same consistency guarantees in general, i.e., don't require to manually unconfirm blocks?

Right, that's what I thought. However, one of the main motivations for the lightning-block-sync refactor was that wanted to enable safe switching between chain sources. Are we positive that users can switch back-and-forth between Esplora / Electrum and bitcoind now, without risking that they don't have the necessary data available to handle reorgs?

AFAICT, Esplora uses 10 or 15 depending on the backend.

https://github.com/bitcoindevkit/rust-esplora-client/blob/603ba6bfb811e379d855413c03bf420f589c2abe/src/blocking.rs#L495-L502

And Electrum uses 8:

https://github.com/bitcoindevkit/bdk/blob/646661fa3d19ac6f2bd7bef58b321372b795433c/crates/electrum/src/bdk_electrum_client.rs#L14

After that they jump back to the genesis block.

So probably enough given it covers ANTI_REORG_DELAY at least. A deeper re-org could be a problem, though.

Or maybe it's fine because BDK doesn't have the same consistency guarantees in general, i.e., don't require to manually unconfirm blocks?

Hmm... isn't the concern more about LDK? We're using Wallet::current_best_block to pass a BlockLocator to lightning_block_sync::init::synchronize_listeners.

So probably enough given it covers ANTI_REORG_DELAY at least. A deeper re-org could be a problem, though.

Ah, alright, that should be fine then, given we haven't seen a deeper reorg since the early days and we do already assume ANTI_REORG_DELAY in a few other places?

Hmm... isn't the concern more about LDK? We're using Wallet::current_best_block to pass a BlockLocator to lightning_block_sync::init::synchronize_listeners.

Right, but it's mostly about LDK objects, not lightning_block_sync logic overall, no?

Update to use the new HeaderCache type instead of implementing the Cache trait, pass BestBlock instead of BlockHash to synchronize_listeners, and pass HeaderCache by value to SpvClient::new. Also adapt to BestBlock gaining a previous_blocks field and ChannelManager deserialization returning BestBlock instead of BlockHash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

UniFFI cannot represent the fixed-size array that upstream's BestBlock carries via `previous_blocks`, so NodeStatus.current_best_block was unusable from Swift, Kotlin, and Python once upstream added that field. Introduce a small ldk-node BestBlock with just hash and height — the pieces bindings can handle — and expose it in place of the upstream type on the public API. Generated with assistance from Claude Code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

tnull

LGTM

jkczyz changed the title ~~Adapt to lightning-block-sync API changes~~ Adapt to lightning-block-sync API changes Apr 14, 2026

ldk-reviews-bot requested a review from tnull April 14, 2026 04:00

tnull reviewed Apr 14, 2026

View reviewed changes

jkczyz mentioned this pull request Apr 17, 2026

Drop max_inbound_htlc_value_in_flight_percent_of_channel #878

Merged

tnull added this to the 0.8 milestone Apr 20, 2026

jkczyz force-pushed the 2026-04-block-sync-api branch 2 times, most recently from 3841a65 to 1f99308 Compare April 23, 2026 23:03

tnull reviewed Apr 27, 2026

View reviewed changes

jkczyz force-pushed the 2026-04-block-sync-api branch 2 times, most recently from ea17859 to 9450bdd Compare April 27, 2026 17:51

jkczyz requested a review from tnull April 27, 2026 20:20

tnull reviewed Apr 28, 2026

View reviewed changes

jkczyz and others added 2 commits April 28, 2026 11:11

jkczyz force-pushed the 2026-04-block-sync-api branch from 9450bdd to 2ab93c1 Compare April 28, 2026 16:13

jkczyz requested a review from tnull April 28, 2026 16:13

tnull approved these changes Apr 29, 2026

View reviewed changes

tnull merged commit 147247a into lightningdevkit:main Apr 29, 2026
21 checks passed

Conversation

jkczyz commented Apr 14, 2026

Uh oh!

ldk-reviews-bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment • 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!

TheBlueMatt commented Apr 14, 2026

Uh oh!

tnull commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz commented Apr 23, 2026

Uh oh!

jkczyz commented Apr 23, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Apr 27, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

jkczyz Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

tnull Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Apr 14, 2026 •

edited

Loading

tnull left a comment •

edited

Loading

tnull commented Apr 14, 2026 •

edited

Loading