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

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT

[tnull]

Previously, the ChainListenerSetListen implementation wouldn't forward to the listeners block_connected implementation outside of tests. This would result in the default implementation of Listen::block_connected being used and the listeners implementation never being called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a952d2d) to head (f4fe450).
Report is 13 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3354 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01% 
==========================================
Files 127 127 Lines 103531 103531 Branches 103531 103531 ==========================================
- Hits 92782 92779 -3 + Misses 8052 8048 -4 - Partials 2697 2704 +7 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jkczyz]

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.

[tnull]

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).

[vincenzopalazzo]

LGMT