Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged

Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
vladimirfomene:add_extra_fields_to_ChannelClosed_event

Conversation

@vladimirfomene

@vladimirfomenevladimirfomene commented Jun 30, 2023

Copy link
Copy Markdown
Contributor

The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event. This helps users to have more context during a
channel close event. Solves #2343

wpaulino
wpaulino reviewed Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch from 04952ca to 01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace reviewed Jul 6, 2023
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.

Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
@vladimirfomene

vladimirfomene commented Jul 6, 2023

Copy link
Copy Markdown
ContributorAuthor

@valentinewallace, let me do that and open it up for reviews.

@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch 5 times, most recently from 9be7828 to ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter

codecov-commenter commented Jul 11, 2023•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 98.43% and project coverage change: -0.02%⚠️

Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more

... and 2 files with indirect coverage changes

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

@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch 2 times, most recently from a54a709 to ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene marked this pull request as ready for review July 12, 2023 12:34
@dunxen

dunxen commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

The fix for the CI breakage is merged now so this just needs a rebase :)

@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch from ee61e38 to 1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen reviewed Jul 13, 2023
View reviewed changes

@dunxendunxen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Just a few comments.

Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch from 1052ac6 to 63614a1CompareJuly 14, 2023 07:54
@benthecarman

benthecarman commented Jul 15, 2023

Copy link
Copy Markdown
Contributor

If possible getting the closing txid would be great here too

@vladimirfomene

vladimirfomene commented Jul 20, 2023

Copy link
Copy Markdown
ContributorAuthor

@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.

As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)

@dunxen
dunxen self-requested a review July 20, 2023 09:11
TheBlueMatt
TheBlueMatt reviewed Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
reason: ClosureReason::ProcessingError { err: err.err.clone() }
reason: ClosureReason::ProcessingError { err: err.err.clone() },
counterparty_node_id: Some($counterparty_node_id),
channel_capacity_sats: $channel_capacity_sats,

@TheBlueMattTheBlueMattJul 30, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.

@dunxendunxenAug 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?

@vladimirfomenevladimirfomeneAug 4, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object

Comment threadlightning/src/ln/channelmanager.rs Outdated
@vladimirfomene
vladimirfomene marked this pull request as draft July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch 2 times, most recently from 854b742 to ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene marked this pull request as ready for review August 4, 2023 14:04
dunxen
dunxen reviewed Aug 8, 2023
View reviewed changes

@dunxendunxen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.

Comment threadlightning/src/events/mod.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/functional_test_utils.rs Outdated
@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed the add_extra_fields_to_ChannelClosed_event branch from ca02769 to 7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen approved these changes Aug 9, 2023
View reviewed changes

@dunxendunxen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI failure unrelated.
LGTM

TheBlueMatt
TheBlueMatt approved these changes Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
/// The reason the channel was closed.
reason: ClosureReason
reason: ClosureReason,
/// Counterparty in the closed channel.

@TheBlueMattTheBlueMattAug 10, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.

Comment threadlightning/src/ln/monitor_tests.rs
check_added_monitors(&nodes[0], 1);
check_closed_broadcast(&nodes[0], 1, true);
check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,

@TheBlueMattTheBlueMattAug 10, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.

@TheBlueMatt
TheBlueMatt merged commit 131560e into lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino mentioned this pull request Aug 15, 2023
Closed
@benthecarmanbenthecarman mentioned this pull request Dec 4, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Add counterparty_node_id & channel_capacity to ChannelClosed event - #2387
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Aug 10, 2023
Merged
Add counterparty_node_id & channel_capacity to ChannelClosed event#2387
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
vladimirfomene:add_extra_fields_to_ChannelClosed_event
Conversation
@vladimirfomene
@vladimirfomenevladimirfomene
commented
Jun 30, 2023

Copy link


Copy Markdown

Contributor
The current ChannelClosed event does not let

you know the counterparty during a channel close

event. This change adds the counterparty_node_id

and the channel_capacity to the ChannelClosed event. This helps users to have more context during a

channel close event. Solves #2343
wpaulino
wpaulino
reviewed
Jun 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
04952ca to
01e3ef1CompareJuly 6, 2023 13:55
valentinewallace
valentinewallace
reviewed
Jul 6, 2023
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you add some test coverage? Otherwise LGTM.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

@vladimirfomene
vladimirfomene
commented
Jul 6, 2023

Copy link


Copy Markdown

ContributorAuthor
@valentinewallace, let me do that and open it up for reviews.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
5 times, most recently
from
9be7828 to
ef131e3CompareJuly 11, 2023 19:32
@codecov-commenter
codecov-commenter
commented
Jul 11, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 98.43% and project coverage change: -0.02%⚠️
Comparison is base (6f58072) 90.34% compared to head (ca02769) 90.32%.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2387 +/- ##
==========================================
- Coverage 90.34% 90.32% -0.02% 
==========================================
Files 106 106 Lines 55784 55850 +66 Branches 55784 55850 +66 ==========================================
+ Hits 50398 50447 +49 - Misses 5386 5403 +17 
Files ChangedCoverage Δ
lightning/src/events/mod.rs42.28% <80.00%> (+0.17%)⬆️
lightning/src/ln/channelmanager.rs85.64% <82.35%> (-0.08%)⬇️
lightning-persister/src/lib.rs89.16% <100.00%> (ø)
lightning/src/chain/chainmonitor.rs95.03% <100.00%> (+0.02%)⬆️
lightning/src/chain/channelmonitor.rs94.77% <100.00%> (+0.05%)⬆️
lightning/src/ln/chanmon_update_fail_tests.rs98.69% <100.00%> (+<0.01%)⬆️
lightning/src/ln/functional_test_utils.rs88.46% <100.00%> (-0.38%)⬇️
lightning/src/ln/functional_tests.rs98.26% <100.00%> (+<0.01%)⬆️
lightning/src/ln/monitor_tests.rs98.52% <100.00%> (+<0.01%)⬆️
lightning/src/ln/payment_tests.rs97.67% <100.00%> (+<0.01%)⬆️
... and 4 more
... and 2 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
a54a709 to
ee61e38CompareJuly 12, 2023 12:01
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
July 12, 2023 12:34
@dunxen
dunxen
commented
Jul 12, 2023

Copy link


Copy Markdown

Contributor
The fix for the CI breakage is merged now so this just needs a rebase :)
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ee61e38 to
1052ac6CompareJuly 13, 2023 03:22
dunxen
dunxen
reviewed
Jul 13, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks! Just a few comments.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
1052ac6 to
63614a1CompareJuly 14, 2023 07:54
@benthecarman
benthecarman
commented
Jul 15, 2023

Copy link


Copy Markdown

Contributor
If possible getting the closing txid would be great here too
@vladimirfomene
vladimirfomene
commented
Jul 20, 2023

Copy link


Copy Markdown

ContributorAuthor
@benthecarman in most cases when this event is fired, a closing tx has not yet been confirmed on chain so it is hard to figure out the closing txid at this point. I asked @TheBlueMatt on discord and here is what he had to say about it.
As you note, the issue is we don't know for sure what the closing txid will be when we generate ChannelClosed (because it hasn't confirmed and could change - even if we have a coop close txid we could still, or our counterparty could still, force-close and replace it.)
@dunxen
dunxen
self-requested a review
July 20, 2023 09:11
TheBlueMatt
TheBlueMatt
reviewed
Jul 30, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

 reason: ClosureReason::ProcessingError { err: err.err.clone() }
 reason: ClosureReason::ProcessingError { err: err.err.clone() },
 counterparty_node_id: Some($counterparty_node_id),
 channel_capacity_sats: $channel_capacity_sats,
@TheBlueMattTheBlueMattJul 30, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

If we think this may be set to None sometimes (due to dual funding?) then we should update the docs to indicate when the field can be None even for users of LDK 0.0.117, if not, let's make the macro take a non-option and wrap it in Some here so that its a compile error to pass None.
@dunxendunxenAug 1, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

When might it be considered None for the dual funding case? If we get a close event before accept_channel2 is sent / received?
@vladimirfomenevladimirfomeneAug 4, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Thought so as well. I don’t know if we need worry about that with the new fix. @TheBlueMatt thanks for the pointer to look at the internal object
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@vladimirfomene
vladimirfomene
marked this pull request as draft
July 31, 2023 06:50
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
2 times, most recently
from
854b742 to
ca02769CompareAugust 4, 2023 12:10
@vladimirfomene
vladimirfomene
marked this pull request as ready for review
August 4, 2023 14:04
dunxen
dunxen
reviewed
Aug 8, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looking good. I think you don't need to worry about addressing latest feedback with fixups commits. Can just edit the relevant commits and then we're ready for accept and merge unless any final things caught in review after.
Comment threadlightning/src/events/mod.rs
Outdated

Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/functional_test_utils.rs
Outdated

@vladimirfomene
Add counterparty_node_id & channel_capacity to ChannelClosed
bd6323e
The current ChannelClosed event does not let
you know the counterparty during a channel close
event. This change adds the counterparty_node_id
and the channel_capacity to the ChannelClosed event.
This helps users to have more context during a
channel close event. Solves lightningdevkit#2343
@vladimirfomene
Add test coverage ChannelClosed event fields
7cfafc9
@vladimirfomene
vladimirfomeneforce-pushed
the
add_extra_fields_to_ChannelClosed_event
branch
from
ca02769 to
7cfafc9CompareAugust 8, 2023 11:12
dunxen
dunxen
approved these changes
Aug 9, 2023
View reviewed changes
@dunxendunxen
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

CI failure unrelated.

LGTM
TheBlueMatt
TheBlueMatt
approved these changes
Aug 10, 2023
View reviewed changes
Comment threadlightning/src/events/mod.rs
 /// The reason the channel was closed.
 reason: ClosureReason
 reason: ClosureReason,
 /// Counterparty in the closed channel. 
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

You have quite a bit of whitespace at the ends of lines in your patch, a local git show should highlight them for you.
Comment threadlightning/src/ln/monitor_tests.rs
	check_added_monitors(&nodes[0], 1);
	check_closed_broadcast(&nodes[0], 1, true);
	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed, false);
	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
@TheBlueMattTheBlueMattAug 10, 2023

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We're trying to move away from the macros slowly towards the functions, please don't re-introduce macro calls where we previously had functions.
@TheBlueMatt
TheBlueMatt
merged commit 131560e
into
lightningdevkit:mainAug 10, 2023
@wpaulinowpaulino
mentioned this pull request
Aug 15, 2023

Closed

@benthecarmanbenthecarman
mentioned this pull request
Dec 4, 2023

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wpaulinowpaulinowpaulino left review comments
@valentinewallacevalentinewallacevalentinewallace left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen 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.

7 participants

@vladimirfomene@codecov-commenter@dunxen@benthecarman@TheBlueMatt@wpaulino@valentinewallace