Adapt to lightning-block-sync API changes

[jkczyz]

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]

I've assigned @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]

@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]

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]

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]

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.