Process updates before archiving monitors. #3276
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3276 +/- ##
==========================================
+ Coverage 89.82% 90.69% +0.86%
==========================================
Files 125 126 +1
Lines 102830 110223 +7393
Branches 102830 110223 +7393
==========================================
+ Hits 92368 99965 +7597
+ Misses 7752 7671 -81
+ Partials 2710 2587 -123 ☔ View full report in Codecov by Sentry. |
|
LGTM, though could split it into more than one commit. |
|
Technically both commits are refactors that work in isolation, so you could de-fixup the other one, unless you envision a different splt. But on the other hand, it's a ±50-line-change. |
|
Probably removing the parameters from the existing functions should be a separate commit because it does something that is logically quite different and the change itself is an important bugfix. |
|
Feel free to squash, change commits, and please write commit messages. |
arik-so
force-pushed
the
arik/2024/08/apply_monitor_updates_on_archive
branch
from
e0560ac to
7162cd3
Compare
`MonitorUpdatingPersister` does not currently correctly archive monitors because it neglects any unapplied updates. In order to start applying these updates, the archiving methods will require access to instances of `BroadcasterInterface` and `FeeEstimator`. This commit requires that the `MonitorUpdatingPersister` be instantiated with those instances, obviating the need for passing them around, and laying the foundation for the following commit.
Previously, `MonitorUpdatingPersister` was disregarding any unapplied monitor updates when archiving them. This commit ensures that upon reading monitors, their corresponding updates are also read and applied prior to archiving.
arik-so
force-pushed
the
arik/2024/08/apply_monitor_updates_on_archive
branch
from
7162cd3 to
47c8aa5
Compare
| @@ -762,17 +762,18 @@ where | |||
|
|
|||
| fn archive_persisted_channel(&self, funding_txo: OutPoint) { | |||
| let monitor_name = MonitorName::from(funding_txo); | |||
| let monitor = match self.read_monitor(&monitor_name) { | |||
| let monitor_key = monitor_name.as_str().to_string(); | |||
| let monitor = match self.read_channel_monitor_with_updates(monitor_key) { | |||
| Ok((_block_hash, monitor)) => monitor, | |||
There was a problem hiding this comment.
Pre-existing, but I think the read_monitor method could use a rename and maybe some docs since it basically reads an old monitor, which seems worth a call-out. No need to do it in this PR though
There was a problem hiding this comment.
Good point. Separately, there's also the issue of applying monitor updates requiring a fee estimator and a broadcaster in the first place, which should be avoidable. I'll create issues for both.
| @@ -762,17 +762,18 @@ where | |||
|
|
|||
| fn archive_persisted_channel(&self, funding_txo: OutPoint) { | |||
| let monitor_name = MonitorName::from(funding_txo); | |||
| let monitor = match self.read_monitor(&monitor_name) { | |||
| let monitor_key = monitor_name.as_str().to_string(); | |||
| let monitor = match self.read_channel_monitor_with_updates(monitor_key) { | |||
There was a problem hiding this comment.
FWIW, I'm not sure this bug is reachable at the moment because I think LDK will persist the full monitor on each block connection, and we don't archive until ~4k blocks have passed post-channel close. Still seems like a reasonable fix though in case things change.
There was a problem hiding this comment.
Yeah, that caveat is addressed in the issue this is fixing. I think it primarily affects mobile clients.
Document monitor archival idempotency requirement (#3276 followup)
Fixes #3228.