Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lightning-block-sync: Fix synchronize_listeners always calling default implementation #3354

Merged
merged 1 commit into from
Oct 14, 2024
Merged

lightning-block-sync: Fix synchronize_listeners always calling default implementation #3354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lightning-block-sync/src/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,6 @@ impl<'a, L: chain::Listen + ?Sized> chain::Listen for DynamicChainListener<'a, L
struct ChainListenerSet<'a, L: chain::Listen + ?Sized>(Vec<(u32, &'a L)>);

impl<'a, L: chain::Listen + ?Sized> chain::Listen for ChainListenerSet<'a, L> {
// Needed to differentiate test expectations.
#[cfg(test)]
Comment on lines -242 to -243
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it was calling each listener's filtered_block_connected method, though, instead of block_connected. Is that a problem? Not clear from the commit message why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As also discussed offline, the issue here is that the overridden Listen::block_connected implementation would never be called for the listeners. I.e., if they implement some behavior diverging from the default implementation, it would never be used which is clearly a bug.

If we don't expect users to override block_connected, it shouldn't be a trait method in the first place, IMO. Although I really think we should allow it (as we'll now also require it for LDK Node onchain wallet's listener).

fn block_connected(&self, block: &bitcoin::Block, height: u32) {
for (starting_height, chain_listener) in self.0.iter() {
if height > *starting_height {
Expand Down
Loading