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.

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

Jump to bottom

Updated ClosureReason::HolderForceClosed with whether txn was broadcasted. #3107

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Updated ClosureReason::HolderForceClosed with whether txn was broadcasted. #3107

merged 1 commit into from
Jun 11, 2024

Conversation

mhrheaume
Copy link
Contributor

@mhrheaume mhrheaume commented Jun 6, 2024 •
edited
Loading

This can be used to determine if the channel was force closed with the latest transaction broadcast, or if it was simply abandoned.

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.71%. Comparing base (2701bc5) to head (808d814).
Report is 46 commits behind head on main.

Files Patch % Lines
lightning/src/events/mod.rs 60.00% 1 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs 85.71% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3107      +/-   ##
==========================================
+ Coverage   89.88%   90.71%   +0.82%     
==========================================
  Files         119      119              
  Lines       97551   103899    +6348     
  Branches    97551   103899    +6348     
==========================================
+ Hits        87681    94247    +6566     
+ Misses       7304     7141     -163     
+ Partials     2566     2511      -55

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhrheaume mhrheaume marked this pull request as ready for review June 6, 2024 23:30
@TheBlueMatt
Copy link
Collaborator

Looking at this now I wonder if we shouldn't just add a bool to HolderForceClosed to indicate which force-closing method was called? It does seem a bit overkill to have a whole extra variant for this case.

@mhrheaume
Copy link
Contributor Author

Looking at this now I wonder if we shouldn't just add a bool to HolderForceClosed to indicate which force-closing method was called? It does seem a bit overkill to have a whole extra variant for this case.

Good call, I like that way better.

@mhrheaume mhrheaume changed the title Added ClosureReason::Abandoned for channel closes. Updated ClosureReason::HolderForceClosed with whether txn was broadcasted. Jun 7, 2024
TheBlueMatt
TheBlueMatt reviewed Jun 7, 2024
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, modulo docs

@mhrheaume mhrheaume force-pushed the mhr/closure_reason_abandoned branch 2 times, most recently from 1bc8529 to e9ab795 Compare June 7, 2024 23:39
@mhrheaume mhrheaume force-pushed the mhr/closure_reason_abandoned branch 2 times, most recently from 8299d04 to 099617f Compare June 10, 2024 17:58
TheBlueMatt
TheBlueMatt reviewed Jun 10, 2024
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Okay two more tiny nits, then we're good, I think.

@mhrheaume mhrheaume force-pushed the mhr/closure_reason_abandoned branch from 099617f to 808d814 Compare June 10, 2024 19:09
@mhrheaume
Copy link
Contributor Author

@TheBlueMatt is the windows-latest, stable failure a known issue here? Or something I should look into before we merge?

@TheBlueMatt
Copy link
Collaborator

Its I have not seen that one, but its clearly unrelated to this PR. Opened #3114

G8XSU
G8XSU approved these changes Jun 11, 2024
View reviewed changes
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.

Lgtm !
just 1 nit.

lightning/src/events/mod.rs
ClosureReason::HolderForceClosed { broadcasted_latest_txn } => {
f.write_str("user force-closed the channel")?;
if let Some(brodcasted) = broadcasted_latest_txn {
write!(f, " and {} the latest transaction", if *brodcasted { "broadcasted" } else { "did not broadcast" })
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: did not broadcast -> was not broadcasted.

"did not" implies we tried to broadcast it.

@TheBlueMatt
Copy link
Collaborator

Gonna land this, will just do the nit in a followup.

@TheBlueMatt TheBlueMatt merged commit b8cdde8 into lightningdevkit:main Jun 11, 2024
13 of 16 checks passed
@TheBlueMatt TheBlueMatt mentioned this pull request Jun 11, 2024
Merged
@mhrheaume mhrheaume deleted the mhr/closure_reason_abandoned branch June 11, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@G8XSU G8XSU G8XSU approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mhrheaume @codecov-commenter @TheBlueMatt @G8XSU