Skip to content

Process updates before archiving monitors. #3276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Process updates before archiving monitors. #3276

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6119739
Store Broadcaster and FeeEstimator on MonitorUpdatingPersister.
arik-so Aug 28, 2024
47c8aa5
Process updates before archiving monitors.
arik-so Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lightning/src/util/persist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that caveat is addressed in the issue this is fixing. I think it primarily affects mobile clients.

Ok((_block_hash, monitor)) => monitor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Err(_) => return
};
match self.kv_store.write(
ARCHIVED_CHANNEL_MONITOR_PERSISTENCE_PRIMARY_NAMESPACE,
ARCHIVED_CHANNEL_MONITOR_PERSISTENCE_SECONDARY_NAMESPACE,
monitor_name.as_str(),
&monitor.encode()
&monitor.encode(),
) {
Ok(()) => {},
Ok(()) => {}
Err(_e) => return,
};
let _ = self.kv_store.remove(
Expand Down