Document monitor archival idempotency requirement (#3276 followup) #3329
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3329 +/- ##
==========================================
+ Coverage 89.66% 90.89% +1.23%
==========================================
Files 126 127 +1
Lines 102676 115304 +12628
Branches 102676 115304 +12628
==========================================
+ Hits 92062 104805 +12743
+ Misses 7894 7861 -33
+ Partials 2720 2638 -82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lightning/src/chain/chainmonitor.rs
Outdated
| @@ -162,6 +162,13 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> { | |||
| /// | |||
| /// Archiving the data in a backup location (rather than deleting it fully) is useful for | |||
| /// hedging against data loss in case of unexpected failure. | |||
| /// | |||
| /// Note that if a crash occurs during the archiving process, a state may emerge where the | |||
There was a problem hiding this comment.
The Persist trait doesn't mandate any specific I/O pattern/features, so talking about it being "copied" to a "path" or seeing the monitor in both places isn't really appropriate here. We should instead phrase this in terms of the requirements (it being idempotent and be explicit that this can be async/fail and LDK will just call it again on restart).
lightning/src/chain/chainmonitor.rs
Outdated
| @@ -162,6 +162,12 @@ pub trait Persist<ChannelSigner: EcdsaChannelSigner> { | |||
| /// | |||
| /// Archiving the data in a backup location (rather than deleting it fully) is useful for | |||
| /// hedging against data loss in case of unexpected failure. | |||
| /// | |||
| /// Note that if a crash occurs during the archiving process, a state may emerge with the | |||
| /// archival operation only being partially complete. In that scenario, the monitor may still be | |||
There was a problem hiding this comment.
I mean this is still only true if the Persist implementation allows for it. The Persist implementation may make this atomic, or it may not. We should phrase this around how it doesn't strictly need to be atomic, but then of course if its not the implementation has to handle it.
There was a problem hiding this comment.
It needs to be either atomic or idempotent since it is retried on startup.
There was a problem hiding this comment.
ok, I added that in the form of a caveat
|
Sure, feel free to squash. |
arik-so
force-pushed
the
monitor_archive_docs_followup
branch
from
8d0dda5
to
268675a
Compare
Followup to #3276, addressing #3122.