Bound sync loops in lightning-transaction-sync#4612
Conversation
|
I've assigned @tankyleo as a reviewer! |
Review SummaryIssues found: 1 (new)
Previously flagged issues (not repeated)
|
If we start syncing from an electrum or esplora server and find that the chain moved during our sync, we reset and start fresh. However, if that happens repeatedly, we probably shouldn't just spin forever. Here we give up after ten attempts and just hope we can sync properly later.
TheBlueMatt
force-pushed
the
2026-05-esplora-sync-timeout
branch
from
9db7e55 to
44c8162
Compare
| if i >= 10 { | ||
| log_debug!(self.logger, "Giving up trying to do transaction sync after 10 attempts."); | ||
| break; | ||
| } |
There was a problem hiding this comment.
After break-ing here, execution falls through to the "Finished transaction sync at tip …" log message (and Ok(())), which is misleading — sync didn't actually finish, it gave up. Same issue in esplora.rs.
Consider either:
- Adding an early
return Ok(())(orreturn Err(...)) right here so the "Finished" log is skipped, or - Guarding the "Finished" log with a check (e.g.,
if !sync_state.pending_sync).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4612 +/- ##
========================================
Coverage 86.11% 86.12%
========================================
Files 157 157
Lines 108841 108944 +103
Branches 108841 108944 +103
========================================
+ Hits 93725 93823 +98
- Misses 12497 12500 +3
- Partials 2619 2621 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| for i in 0..100 { | ||
| if i >= 10 { | ||
| log_debug!(self.logger, "Giving up trying to do transaction sync after 10 attempts."); | ||
| break; |
There was a problem hiding this comment.
Like claude I'm thinking we return Err early here instead of breaking ?
4 participants
If we start syncing from an electrum or esplora server and find that the chain moved during our sync, we reset and start fresh. However, if that happens repeatedly, we probably shouldn't just spin forever. Here we give up after ten attempts and just hope we can sync properly later.