-
Notifications
You must be signed in to change notification settings - Fork 461
Only log if we actually persisted LiquidityManager data (0.2 backport)
#4662
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.
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
base: 0.2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -244,15 +244,17 @@ where | |||||||
| }) | ||||||||
| } | ||||||||
|
|
||||||||
| pub(crate) async fn persist(&self) -> Result<(), lightning::io::Error> { | ||||||||
| pub(crate) async fn persist(&self) -> Result<bool, lightning::io::Error> { | ||||||||
| // TODO: We should eventually persist in parallel, however, when we do, we probably want to | ||||||||
| // introduce some batching to upper-bound the number of requests inflight at any given | ||||||||
| // time. | ||||||||
|
|
||||||||
| let mut did_persist = false; | ||||||||
|
|
||||||||
| if self.persistence_in_flight.fetch_add(1, Ordering::AcqRel) > 0 { | ||||||||
| // If we're not the first event processor to get here, just return early, the increment | ||||||||
| // we just did will be treated as "go around again" at the end. | ||||||||
| return Ok(()); | ||||||||
| return Ok(did_persist); | ||||||||
| } | ||||||||
|
|
||||||||
| loop { | ||||||||
|
|
@@ -277,6 +279,7 @@ where | |||||||
| for client_id in need_persist.into_iter() { | ||||||||
| debug_assert!(!need_remove.contains(&client_id)); | ||||||||
| self.persist_peer_state(client_id).await?; | ||||||||
| did_persist = true; | ||||||||
| } | ||||||||
|
|
||||||||
| for client_id in need_remove { | ||||||||
|
|
@@ -311,6 +314,7 @@ where | |||||||
| } | ||||||||
| if let Some(future) = future_opt { | ||||||||
| future.await?; | ||||||||
| did_persist = true; | ||||||||
| } else { | ||||||||
| self.persist_peer_state(client_id).await?; | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same bug as in lsps2:
Suggested change
|
||||||||
| } | ||||||||
|
|
@@ -325,7 +329,7 @@ where | |||||||
| break; | ||||||||
| } | ||||||||
|
|
||||||||
| Ok(()) | ||||||||
| Ok(did_persist) | ||||||||
| } | ||||||||
|
|
||||||||
| fn check_prune_stale_webhooks<'a>( | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -670,23 +670,27 @@ where | |
| self.pending_events.get_and_clear_pending_events() | ||
| } | ||
|
|
||
| /// Persists the state of the service handlers towards the given [`KVStore`] implementation. | ||
| /// Persists the state of the service handlers towards the given [`KVStore`] implementation if | ||
| /// needed. | ||
| /// | ||
| /// Returns `true` if it persisted sevice handler data. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: "sevice" → "service" |
||
| /// | ||
| /// This will be regularly called by LDK's background processor if necessary and only needs to | ||
| /// be called manually if it's not utilized. | ||
| pub async fn persist(&self) -> Result<(), lightning::io::Error> { | ||
| pub async fn persist(&self) -> Result<bool, lightning::io::Error> { | ||
| // TODO: We should eventually persist in parallel. | ||
| self.pending_events.persist().await?; | ||
| let mut did_persist = false; | ||
| did_persist |= self.pending_events.persist().await?; | ||
|
|
||
| if let Some(lsps2_service_handler) = self.lsps2_service_handler.as_ref() { | ||
| lsps2_service_handler.persist().await?; | ||
| did_persist |= lsps2_service_handler.persist().await?; | ||
| } | ||
|
|
||
| if let Some(lsps5_service_handler) = self.lsps5_service_handler.as_ref() { | ||
| lsps5_service_handler.persist().await?; | ||
| did_persist |= lsps5_service_handler.persist().await?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| Ok(did_persist) | ||
| } | ||
|
|
||
| fn handle_lsps_message( | ||
|
|
@@ -1285,8 +1289,10 @@ where | |
|
|
||
| /// Persists the state of the service handlers towards the given [`KVStoreSync`] implementation. | ||
| /// | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same typo: "sevice" → "service" |
||
| /// Returns `true` if it persisted sevice handler data. | ||
| /// | ||
| /// Wraps [`LiquidityManager::persist`]. | ||
| pub fn persist(&self) -> Result<(), lightning::io::Error> { | ||
| pub fn persist(&self) -> Result<bool, lightning::io::Error> { | ||
| let mut waker = dummy_waker(); | ||
| let mut ctx = task::Context::from_waker(&mut waker); | ||
| match Box::pin(self.inner.persist()).as_mut().poll(&mut ctx) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
did_persistis not set totruehere. When a peer was initially flagged for removal but is no longer prunable (it got new state while we were persisting),needs_persistis forced totrueon line 1845, andpersist_peer_statehere will actually write to the KV store (it checksneeds_persist, finds it true, clears it, and persists). So real persistence happens, but the return value won't reflect it.Low severity since it only affects logging, but the return value is inaccurate.