Add `ChannelReady` event by tnull · Pull Request #1743 · lightningdevkit/rust-lightning

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

 	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

Conversation

tnull commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Oct 26, 2022

Uh oh!

Uh oh!

tnull commented Oct 28, 2022

Uh oh!

codecov-commenter commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Nov 1, 2022

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

tnull Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Nov 2, 2022

Uh oh!

tnull commented Nov 2, 2022

Uh oh!

TheBlueMatt commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Nov 2, 2022

Uh oh!

tnull commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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