Support filtered blocks in lightning-block-sync

[jkczyz]

Expand the BlockSource trait to allow filtered blocks now that chain::Listen supports them. This makes it possible to use BIP 157/158 compact block filters with lightning-block-sync.

[codecov-commenter]

Codecov Report

Base: 90.84% // Head: 90.83% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (3675d29) compared to base (15a5966).
Patch coverage: 85.91% of modified lines in pull request are covered.

❗ Current head 3675d29 differs from pull request most recent head c1938e8. Consider uploading reports for the commit c1938e8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1706      +/-   ##
==========================================
- Coverage   90.84%   90.83%   -0.02%     
==========================================
  Files          86       86              
  Lines       46448    46503      +55     
  Branches    46448    46503      +55     
==========================================
+ Hits        42198    42243      +45     
- Misses       4250     4260      +10     

Impacted Files	Coverage Δ
lightning-block-sync/src/rest.rs	62.06% <0.00%> (ø)
lightning-block-sync/src/rpc.rs	74.80% <0.00%> (ø)
lightning-block-sync/src/poll.rs	87.77% <80.00%> (+0.27%)	⬆️
lightning-block-sync/src/test_utils.rs	93.22% <89.28%> (-0.25%)	⬇️
lightning-block-sync/src/lib.rs	93.37% <95.83%> (+0.13%)	⬆️
lightning-block-sync/src/init.rs	90.90% <100.00%> (-2.66%)	⬇️
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/ln/monitor_tests.rs	99.44% <0.00%> (-0.12%)	⬇️
lightning/src/ln/channelmanager.rs	84.97% <0.00%> (-0.03%)	⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Nice! Sorry, needs rebase now cause the import fix got merged separately.

[TheBlueMatt]

Still needs rebase to lose the first two commits.

[wpaulino]

LGTM once the outstanding feedback is addressed.

[wpaulino]

Feel free to squash, looks like 66e863e can be dropped entirely?

[TheBlueMatt]

Code itself LGTM, I'm a bit unsure of how practical this is to use when we have to rescan, though? The API won't, AFAICT, allow the user to pass in dependent transaction data when they need to rescan. If I'm missing something feel free to merge, but for a really easy to use API here I feel like the poller needs to actually be in the filter path somehow and decide to rescan.

[jkczyz]

Code itself LGTM, I'm a bit unsure of how practical this is to use when we have to rescan, though? The API won't, AFAICT, allow the user to pass in dependent transaction data when they need to rescan. If I'm missing something feel free to merge, but for a really easy to use API here I feel like the poller needs to actually be in the filter path somehow and decide to rescan.

IIUC, for BIP 157/158 use case, the BlockSource should return either a full block or an empty block. So there's no need to rescan at least for that use case.

[TheBlueMatt]

Ah, okay, right. So this probably should mention that in the docs, no?

[jkczyz]

Yeah, good call.

Just so I understand the more general use case, this is not Electrum but some custom server written to give back pre-filtered blocks with transactions matching chain::Filter (and descendants, as should be documented)? And the reason for doing this is largely to save on bandwidth?

[TheBlueMatt]

Hmm? I read your comment there to be that the use-case for this is for clients wishing to download BIP 157/158 filters, then if they get a match, download the full block (eg via REST or RPC). This is not for electrum or a super-filtering server, more for the BIP 157/158 clients.

[jkczyz]

Hmm? I read your comment there to be that the use-case for this is for clients wishing to download BIP 157/158 filters, then if they get a match, download the full block (eg via REST or RPC). This is not for electrum or a super-filtering server, more for the BIP 157/158 clients.

What I meant was, besides BIP 157/158, is there a more general "pre-filtered blocks" use case that we wish to support with chain::Listen? (i.e., not via chain::Confirm, which is used for Electrum)

Also, it occurs to me that with chain::Listen we can't support re-scans since, at least as the trait is currently implemented, we assert that successive calls are always for the next block. So any pre-filtered blocks must contain all dependent transactions.

[TheBlueMatt]

What I meant was, besides BIP 157/158, is there a more general "pre-filtered blocks" use case that we wish to support with chain::Listen? (i.e., not via chain::Confirm, which is used for Electrum)

Ahhh, probably not? I see your point about some proprietary protocol, and I guess we should support that, but...

Also, it occurs to me that with chain::Listen we can't support re-scans since, at least as the trait is currently implemented, we assert that successive calls are always for the next block. So any pre-filtered blocks must contain all dependent transactions.

Hmm, right, we'd have to adapt this to make it work. I'm not really sure it's worth it, though - most folks wanting to rescan are probably using electrum and just want Confirm.

I do wonder if this has implications for the API here, though - we never actually want to return a filtered block cause that rescan-needed risk, we only want a "have block" and "don't need block" options.

[jkczyz]

I do wonder if this has implications for the API here, though - we never actually want to return a filtered block cause that rescan-needed risk, we only want a "have block" and "don't need block" options.

Yeah, I was going to suggest either making the docs clear that all filtered blocks must contain dependent transactions or changing the enum variant from FilteredBlock to EmptyBlock.

[TheBlueMatt]

Right, I suppose if someone had a bespoke filtering system that included all in-block dependents we could use that and keep the current API, but I'm not sure that exists? And it seems better to just require full-or-empty blocks than having the API be easy to accidentally screw up by failing to read docs.

[jkczyz]

Updated accordingly. Though note that any bespoke implementation wouldn't be able to use lightning-block-sync. Guess it would be better to wait until we have a feature request.

[TheBlueMatt]

LGTM, basically, feel free to squash.