Refactor chain sourcing logic to make result propagation more robust #599
Conversation
Previously, we might have overlooked some cases that would exit the method and bubble up an error via `?` instead of propagating to all subscribers. Here, we split out the code to an inner method to ensure we always propagate.
Previously, we might have overlooked some cases that would exit the method and bubble up an error via `?` instead of propagating to all subscribers. Here, we split out the code to an inner method to ensure we always propagate.
|
👋 Thanks for assigning @joostjager as a reviewer! |
| Err(e) => { | ||
| log_error!(self.logger, "Failed to poll for chain data: {:?}", e); | ||
| let res = Err(Error::TxSyncFailed); | ||
| self.wallet_polling_status.lock().unwrap().propagate_result_to_subscribers(res); |
There was a problem hiding this comment.
The cleanup here, does that mean that the previous commit would send redundant updates?
There was a problem hiding this comment.
Not quite sure what you mean? We previously propagated manually whenever erroring out. But now we can just bubble up the error as we'll propagate to subscribers in the wrapping method.
There was a problem hiding this comment.
Yes, but before the change in this commit (this specific commit), was the propagate method being called twice? I am wondering if all commits in the stack are leaving the code in a correct state.
There was a problem hiding this comment.
Ah, yes, that might be true. Happy to squash the cleanup if you prefer.
There was a problem hiding this comment.
Now went ahead and did so
| ); | ||
| *self.latest_chain_tip.write().unwrap() = Some(tip); | ||
|
|
||
| periodically_archive_fully_resolved_monitors( |
There was a problem hiding this comment.
Context question: channel manager already knows the best block and I assume it is updated somehow, but still we need an external trigger here to archive? It seems responsibility is now with two units instead of one.
There was a problem hiding this comment.
So far we left when/how to archive to the users in LDK. I guess we could consider making it part of the background processor eventually, but I don't think ChannelManager could do it (in absence of KVStore access, etc)
There was a problem hiding this comment.
Ok. I was confused a bit about the two sources of block information being used (here the connection event, and then the height in cm). But if this is how it is done for now, it's ok.
tnull
force-pushed
the
2025-07-fix-res-propagation-sync
branch
from
cf990f8 to
aae6e5d
Compare
|
Addressed pending comments. |
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
LGTM. One comment outstanding about the stability of an intermediate commit, but non blocking.
Previously, we might have overlooked some cases that would exit the method and bubble up an error via `?` instead of propagating to all subscribers. Here, we split out the code to an inner method to ensure we always propagate.
Previously, we might have overlooked some cases that would exit the method and bubble up an error via `?` instead of propagating to all subscribers. Here, we split out the code to an inner method to ensure we always propagate.
Previously, we might have overlooked some cases that would exit the method and bubble up an error via `?` instead of propagating to all subscribers. Here, we split out the code to an inner method to ensure we always propagate.
Which we previously overlooked
tnull
force-pushed
the
2025-07-fix-res-propagation-sync
branch
from
aae6e5d to
a6349a4
Compare
3 participants
Previously, we might have overlooked some cases that would exit the syncing methods and bubble up an error via
?instead of propagating to all subscribers.Here, we split out the code to inner methods to ensure we always propagate.
Should be easy to review via
git diff --color-moved --ignore-all-space