Skip to content
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

Handle remaining panic on persistence failure #498

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elnosh
Copy link
Contributor

@elnosh elnosh commented Mar 19, 2025

This is the only other panic! I could find related to a persistence failure. Could've used ? there to return the Err but left the match to maintain the log_error! message.

There were other panic!s during event handling such as this one:

ldk-node/src/event.rs

Lines 542 to 555 in f0338d1

Err(err) => {
log_error!(self.logger, "Failed to create funding transaction: {}", err);
self.channel_manager
.force_close_without_broadcasting_txn(
&temporary_channel_id,
&counterparty_node_id,
"Failed to create funding transaction".to_string(),
)
.unwrap_or_else(|e| {
log_error!(self.logger, "Failed to force close channel after funding generation failed: {:?}", e);
panic!(
"Failed to force close channel after funding generation failed"
);
});
but not related to a persistence failure so I left as is.

Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

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

Overall, the diff looks good but not sure if we can mark 381 closed just with this.

@G8XSU G8XSU requested a review from tnull March 20, 2025 19:54
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Overall, the diff looks good but not sure if we can mark 381 closed just with this.

Yes, I agree. Generally LGTM (mod some nits), but #381 is about much more that only this case, and unfortunately we're still requiring considerable refactorings upstream on the LDK end before we can resolve all of the panic-on-persistence-failure here.

src/lib.rs Outdated
panic!("Couldn't mark event handled due to persistence failure");
});
pub fn event_handled(&self) -> Result<(), Error> {
match self.event_queue.event_handled() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Given this just returns the same error, replacing the unwrap_or_else with map_err instead of making this a match might be cleaner here.

@@ -53,7 +53,7 @@ macro_rules! expect_event {
match $node.wait_next_event() {
ref e @ Event::$event_type { .. } => {
println!("{} got event {:?}", $node.node_id(), e);
$node.event_handled();
assert!($node.event_handled().is_ok());
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Let's just use unwrap or expect everywhere instead of assert!(..is_ok())?

@elnosh elnosh force-pushed the handle-persistance-failures branch from e73d808 to 4d8accf Compare March 21, 2025 17:37
@elnosh elnosh force-pushed the handle-persistance-failures branch from 4d8accf to 171a358 Compare March 21, 2025 17:39
@elnosh
Copy link
Contributor Author

elnosh commented Mar 21, 2025

ok sorry about that and thank you for the clarification. Pushed other changes addressing comments from review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants