lnwatcher: introduce loop to trigger callbacks#10656
Conversation
Deduplicate the now() helper function declared in 3 different modules into electrum.util. Use it consistently in submarine_swaps.py. Cleanup imports of lnchannel.py.
f321x
force-pushed
the
lnwatcher_heartbeat
branch
from
11aa565 to
3254bcd
Compare
SomberNight
left a comment
SomberNight
left a comment
There was a problem hiding this comment.
ACK 3254bcd. Just some comments.
| def now() -> float: | ||
| return time.time() | ||
|
|
||
| assert type(now()) == float, "OnionMessageManager requires float timestamps" |
There was a problem hiding this comment.
Right... that's part of why so far there has not been a global "now" function. Different use cases might want different behaviour.
For example, in several cases it would probably be more correct to use time.monotonic().
but ok, sure, just noting all that. The diff is fine.
There was a problem hiding this comment.
yeah, thats a little footgun, but also, using now() seems cleaner than using int(time.time()) everywhere and it felt a bit wrong to define yet another now().
| async def _callback_loop(self): | ||
| """ | ||
| Triggers the callbacks if no event has triggered them within the | ||
| last MAX_CALLBACK_TRIGGER_DELAY_SEC | ||
| (e.g. during a prolonged time without new blocks) | ||
| """ | ||
| while True: | ||
| time_since_last_cb_trigger = now() - self._last_callback_trigger_ts | ||
| if time_since_last_cb_trigger > self.MAX_CALLBACK_TRIGGER_DELAY_SEC: | ||
| await self.trigger_callbacks() | ||
| await asyncio.sleep(self.CALLBACK_LOOP_POLL_INTERVAL_SEC) |
There was a problem hiding this comment.
Not entirely set about the 10 min delay, might as well be 5/2/1 min?
Esp considering #10654, where it could happen that there are expired htlcs, but should_be_closed_due_to_expiring_htlcs decides to wait 30 secs, but then there is no new block for a long time and trigger_callbacks would only get called after 10 mins, note that the user might close their wallet without waiting 10 mins. In fact it is perfectly believable that some users ~never leave the wallet open for 10 mins; and blocks would be processed within the first few seconds of wallet-open.
What do you think about something like this?
$ git diff
diff --git a/electrum/lnwatcher.py b/electrum/lnwatcher.py
index cb3cb9dbf..55bad7778 100644
--- a/electrum/lnwatcher.py
+++ b/electrum/lnwatcher.py
@@ -63,9 +63,13 @@ class LNWatcher(Logger, EventListener):
last MAX_CALLBACK_TRIGGER_DELAY_SEC
(e.g. during a prolonged time without new blocks)
"""
+ t0 = now()
while True:
time_since_last_cb_trigger = now() - self._last_callback_trigger_ts
- if time_since_last_cb_trigger > self.MAX_CALLBACK_TRIGGER_DELAY_SEC:
+ max_delay = self.MAX_CALLBACK_TRIGGER_DELAY_SEC
+ if now() - t0 < max_delay: # if wallet just recently opened, be much more eager
+ max_delay /= 10
+ if time_since_last_cb_trigger > max_delay:
await self.trigger_callbacks()
await asyncio.sleep(self.CALLBACK_LOOP_POLL_INTERVAL_SEC)
There was a problem hiding this comment.
Yes this seems like a good enhancement, added in https://github.com/spesmilo/electrum/compare/3254bcdf7527b3b6523d9035efee2d04f49548a5..2460b030ff109cf40bb309273335f415ad577722
Introduce a taskgroup and polling loop to LNWatcher to guarantee the callbacks get called at least once every LNWatcher.MAX_CALLBACK_TRIGGER_DELAY_SEC (10 min). This should prevent callbacks that operate on time instead of blockheight from becoming (very) stale if there are no blockchain events triggering the callbacks for a longer time. Not entirely set about the 10 min delay, might as well be 5/2/1 min?
f321x
force-pushed
the
lnwatcher_heartbeat
branch
from
3254bcd to
2460b03
Compare
2 participants
Introduce a taskgroup and polling loop to LNWatcher to guarantee
the callbacks get called at least once every
LNWatcher.MAX_CALLBACK_TRIGGER_DELAY_SEC (10 min).
This should prevent callbacks that operate on time instead of
blockheight from becoming (very) stale if there are no blockchain
events triggering the callbacks for a longer time.
Not entirely set about the 10 min delay, might as well be 5/2/1 min?
Related to #10654