Add `ChannelReady` event #1743

tnull · 2022-09-26T12:17:39Z

Closes #1394.

This adds a ChannelReady event that will be emitted as soon as a new
channel becomes usable, i.e., after both sides have sent channel_ready.

tnull · 2022-09-26T12:18:02Z

~~Draft for now, since tests are still failing.~~

lightning/src/util/events.rs

lightning/src/ln/channelmanager.rs

tnull · 2022-10-26T12:39:55Z

Rebased on main and reworked the approach: a bool is now bubbled up from Channel::check_get_channel_ready along side the msgs::ChannelReady and used in ChannelManager to decide whether to emit_channel_ready!.

In the second commit I included the discussed renaming of ChannelState::ChannelFunded to ChannelState::ChannelReady, and since all three align nicely then, I now reconsidered and named the event Event::ChannelReady after all.

Finally, the third commit includes a minor fix for unused import warnings.

Tests currently still failing, looking into it.

lightning/src/ln/channelmanager.rs

tnull · 2022-10-28T10:53:18Z

Fixed tests and rebased on main.

codecov-commenter · 2022-10-28T11:55:47Z

Codecov Report

Base: 90.73% // Head: 90.78% // Increases project coverage by 0.04% 🎉

Coverage data is based on head (2ce2b15) compared to base (ad7ff0b).
Patch coverage: 81.19% of modified lines in pull request are covered.

❗ Current head 2ce2b15 differs from pull request most recent head 2f10adf. Consider uploading reports for the commit 2f10adf to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1743       /-   ##
==========================================
  Coverage   90.73%   90.78%    0.04%     
==========================================
  Files          87       87              
  Lines       47336    47608      272     
  Branches    47336    47608      272     
==========================================
  Hits        42950    43219      269     
- Misses       4386     4389        3

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`55.82% <ø> (ø)`
lightning/src/util/events.rs	`37.93% <34.78%> (-0.20%)`	⬇️
lightning-background-processor/src/lib.rs	`95.96% <71.42%> (-0.28%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.04% <80.00%> ( 0.09%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.20% <91.66%> ( 0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`93.43% <92.85%> (-0.01%)`	⬇️
lightning-invoice/src/utils.rs	`95.20% <100.00%> ( 0.02%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.67% <100.00%> ( <0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <100.00%> ( 0.02%)`	⬆️
lightning/src/ln/payment_tests.rs	`98.81% <100.00%> (ø)`
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

wpaulino

Feel free to squash.

lightning/src/util/events.rs

lightning/src/ln/channel.rs

tnull · 2022-11-01T09:09:44Z

I now reverted the 'bubble-up' approach after all since all required checks are conducted in the should_emit_channel_ready_event and send_channel_ready!, which makes this much less intrusive.

Squashed changes.

dunxen

Looks good. Just noticed we use "0conf" and "0-conf" in comments throughout the project. The hyphenated one looks "more correct". No biggie.

lightning-background-processor/src/lib.rs

lightning/src/ln/channel.rs

lightning-invoice/src/utils.rs

lightning/src/ln/channel.rs

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/functional_tests.rs

TheBlueMatt · 2022-11-01T19:41:09Z

lightning/src/ln/payment_tests.rs

@@ -499,7  500,6 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 	let bs_htlc_claim_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(bs_htlc_claim_txn.len(), 1);
 	check_spends!(bs_htlc_claim_txn[0], as_commitment_tx);
-	expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], None, false, false);


I'm not sure I understand why we have to move this up?

Ah, we want to move this because the PaymentForwarded event is emitted by the handle_update_fulfill_htlc already and then the expect_channel_ready_event call in create_announced_chan_between_nodes fails due to 2 events being queued.

lightning/src/util/events.rs

lightning/src/ln/peer_handler.rs

lightning/src/util/events.rs

TheBlueMatt · 2022-11-02T20:30:15Z

I think this is good - can you squash the fixups into the appropriate commits? Its somewhat hard to re-review when the fixups are all at the end rather than with the commit they'll eventually get squashed into.

tnull · 2022-11-02T20:41:23Z

I think this is good - can you squash the fixups into the appropriate commits? Its somewhat hard to re-review when the fixups are all at the end rather than with the commit they'll eventually get squashed into.

Think all fixups belonged to the first commit => Done.

TheBlueMatt · 2022-11-02T21:03:22Z

Is it worth having a release notes entry for this that says that "no ChannelReady events will be generated for existing channels, including those which become ready on 0.0.113"? I don't think its worth having complicated logic for whether we default channel_ready_event_emitted to true or false based on state, but I also think it may be worth calling out.

TheBlueMatt · 2022-11-02T21:23:56Z

Either way, current code LGTM, probably fine to squash given the reviewers currently. You can leave a diff-tree of the fixups if you want.

This adds a `ChannelReady` event that is emitted as soon as a new channel becomes usable, i.e., after both sides have sent `channel_ready`.

We rename `ChannelState::ChannelFunded` to `ChannelState::ChannelReady` as we'll be in this state when both sides sent the `ChannelReady` messages, which may also be before funding in the 0conf case.

Previously introduced during release commit.

tnull · 2022-11-03T10:47:36Z

Squashed fixups and included the pending changelog message

> git diff-tree -U2 a8720b2 49dfcb6
diff --git a/pending_changelog/1743.txt b/pending_changelog/1743.txt
new file mode 100644
index 00000000..93e41202
--- /dev/null
    b/pending_changelog/1743.txt
@@ -0,0  1,7 @@
 ## API Updates
 - A new `ChannelReady` event is generated whenever a channel becomes ready to
   be used, i.e., after both sides sent the `channel_ready` message.
 
 ## Backwards Compatibilty
 - No `ChannelReady` events will be generated for previously existing channels, including
   those which become ready after upgrading 0.0.113.

tnull marked this pull request as draft September 26, 2022 12:17

tnull commented Sep 26, 2022

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

tnull commented Sep 26, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

tnull changed the title ~~Add ChannelEstablished event~~ Add ChannelEstablished event Sep 26, 2022

tnull changed the title ~~Add ChannelEstablished event~~ Add ChannelOpened event Sep 28, 2022

TheBlueMatt reviewed Sep 29, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.113 milestone Oct 21, 2022

tnull mentioned this pull request Oct 25, 2022

Migration to rust-lightning 0.0.113 lightningdevkit/ldk-node#28

Closed

3 tasks

tnull force-pushed the 2022-09-channel-events branch from 33dbc0c to 405c589 Compare October 26, 2022 12:30

tnull changed the title ~~Add ChannelOpened event~~ Add ChannelReady event Oct 26, 2022

tnull added the Seeking Code Review label Oct 26, 2022

tnull marked this pull request as ready for review October 26, 2022 12:31

tnull force-pushed the 2022-09-channel-events branch 2 times, most recently from b7135dd to 2526975 Compare October 26, 2022 13:15

TheBlueMatt reviewed Oct 26, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-09-channel-events branch 2 times, most recently from f7d8c41 to dabe755 Compare October 28, 2022 10:52

tnull force-pushed the 2022-09-channel-events branch 4 times, most recently from 7fde0f7 to f969b45 Compare October 28, 2022 11:54

tnull force-pushed the 2022-09-channel-events branch 5 times, most recently from 75861b2 to 735fa78 Compare October 28, 2022 13:43

tnull force-pushed the 2022-09-channel-events branch 3 times, most recently from ae803b8 to 85ee73a Compare October 28, 2022 15:32

wpaulino reviewed Oct 31, 2022

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

lightning/src/util/events.rs Outdated Show resolved Hide resolved

lightning/src/util/events.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-09-channel-events branch 2 times, most recently from 43e5538 to 71a999b Compare November 1, 2022 09:06

dunxen reviewed Nov 1, 2022

View reviewed changes

lightning-background-processor/src/lib.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

tnull force-pushed the 2022-09-channel-events branch 2 times, most recently from 9033c30 to f3c23a8 Compare November 1, 2022 09:25

TheBlueMatt reviewed Nov 1, 2022

View reviewed changes

TheBlueMatt reviewed Nov 2, 2022

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-09-channel-events branch from 2ce2b15 to 2f10adf Compare November 2, 2022 18:06

TheBlueMatt removed the Seeking Code Review label Nov 2, 2022

tnull force-pushed the 2022-09-channel-events branch from 2f10adf to a8720b2 Compare November 2, 2022 20:39

tnull added 3 commits November 3, 2022 11:45

Add ChannelReady event

f4c2d40

This adds a `ChannelReady` event that is emitted as soon as a new channel becomes usable, i.e., after both sides have sent `channel_ready`.

Rename chan state ChannelFunded to ChannelReady

0911723

We rename `ChannelState::ChannelFunded` to `ChannelState::ChannelReady` as we'll be in this state when both sides sent the `ChannelReady` messages, which may also be before funding in the 0conf case.

Fix warnings for ununsed anchor imports

49dfcb6

Previously introduced during release commit.

tnull force-pushed the 2022-09-channel-events branch from a8720b2 to 49dfcb6 Compare November 3, 2022 10:47

dunxen approved these changes Nov 3, 2022

View reviewed changes

TheBlueMatt approved these changes Nov 3, 2022

View reviewed changes

TheBlueMatt merged commit 3ba91ce into lightningdevkit:main Nov 3, 2022

tnull mentioned this pull request Nov 3, 2022

Add ChannelReady event lightningdevkit/ldk-node#20

Closed

TheBlueMatt mentioned this pull request Nov 3, 2022

Handle events asynchronously in the BackgroundProcessor's async variant #1787

Merged

tnull mentioned this pull request Nov 8, 2022

Randomize user_channel_id for inbound channels #1790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ChannelReady` event #1743

Add `ChannelReady` event #1743

tnull commented Sep 26, 2022 •

edited

Loading

tnull commented Sep 26, 2022 •

edited

Loading

tnull commented Oct 26, 2022

tnull commented Oct 28, 2022

codecov-commenter commented Oct 28, 2022 •

edited

Loading

wpaulino left a comment

tnull commented Nov 1, 2022

dunxen left a comment

TheBlueMatt Nov 1, 2022

tnull Nov 2, 2022

TheBlueMatt commented Nov 2, 2022

tnull commented Nov 2, 2022

TheBlueMatt commented Nov 2, 2022 •

edited

Loading

TheBlueMatt commented Nov 2, 2022

tnull commented Nov 3, 2022 •

edited

Loading

Add ChannelReady event #1743

Add ChannelReady event #1743

Conversation

tnull commented Sep 26, 2022 • edited Loading

tnull commented Sep 26, 2022 • edited Loading

tnull commented Oct 26, 2022

tnull commented Oct 28, 2022

codecov-commenter commented Oct 28, 2022 • edited Loading

Codecov Report

wpaulino left a comment

Choose a reason for hiding this comment

tnull commented Nov 1, 2022

dunxen left a comment

Choose a reason for hiding this comment

TheBlueMatt Nov 1, 2022

Choose a reason for hiding this comment

tnull Nov 2, 2022

Choose a reason for hiding this comment

TheBlueMatt commented Nov 2, 2022

tnull commented Nov 2, 2022

TheBlueMatt commented Nov 2, 2022 • edited Loading

TheBlueMatt commented Nov 2, 2022

tnull commented Nov 3, 2022 • edited Loading

Add `ChannelReady` event #1743

Add `ChannelReady` event #1743

tnull commented Sep 26, 2022 •

edited

Loading

tnull commented Sep 26, 2022 •

edited

Loading

codecov-commenter commented Oct 28, 2022 •

edited

Loading

TheBlueMatt commented Nov 2, 2022 •

edited

Loading

tnull commented Nov 3, 2022 •

edited

Loading