Add channel_id to SpendableOutputs event #2511

jbesraa · 2023-08-21T19:45:58Z

resolves #2480

jbesraa · 2023-08-21T19:47:21Z

lightning/src/ln/monitor_tests.rs

 			assert_eq!(outputs.len(), 1);
+			// let funding_outpoint = OutPoint { txid: chan_a.3.txid(), index: 0 };
+			// assert_eq!(channel_id, &funding_outpoint.to_channel_id());


@tnull this bit here is failing, whats the best way to construct the channel_id on the right side of the assertion?

There's one event for each channel chan_a and chan_b.

thank you!
changed the test to use .contains as the events are not ordered so the test was flaky

wpaulino · 2023-08-21T21:57:59Z

lightning/src/ln/monitor_tests.rs

 			assert_eq!(outputs.len(), 1);
+			// let funding_outpoint = OutPoint { txid: chan_a.3.txid(), index: 0 };
+			// assert_eq!(channel_id, &funding_outpoint.to_channel_id());


There's one event for each channel chan_a and chan_b.

wpaulino · 2023-08-21T21:59:03Z

lightning/src/events/mod.rs

 					read_tlv_fields!(reader, {
 						(0, outputs, required),
+						(1, channel_id, required),


We need to write this as option to not break upgrades.

To give a little more context on our serialization logic: we generally use Lightning's TLV (type-length-value) format and apply the corresponding "it's okay to be odd" rule, i.e., when reading TLV fields we ignore any unknown fields if they are odd numbered and would refuse to continue if they were even.

Moreover, we would refuse to continue if any required field would not be present at the time of reading. As we aim for forwards- and backwards-compatiblity, we usually need to make new fields odd options. Otherwise users upgrading from a prior version might get a panic when LDK refuses to read a SpendableOutputs event in which the required channel_id is not present.

wpaulino · 2023-08-21T21:59:46Z

lightning/src/events/mod.rs

@@ -673,6 +673,8 @@ pub enum Event {
 	SpendableOutputs {
 		/// The outputs which you should store as spendable by you.
 		outputs: Vec<SpendableOutputDescriptor>,
+		/// Channel id


Nit: comments seems incomplete

We should also add a compatibility notice here: as mentioned below this should be Option<[u8; 32]> to not break backwards compatibility and we should specify starting from which version users can expect this to be Some.

codecov-commenter · 2023-08-21T22:17:34Z

Codecov Report

Patch coverage: 90.90% and project coverage change: +0.18% 🎉

Comparison is base (d4ad826) 90.40% compared to head (14b7612) 90.59%.
Report is 10 commits behind head on main.

❗ 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    #2511      +/-   ##
==========================================
+ Coverage   90.40%   90.59%   +0.18%     
==========================================
  Files         106      106              
  Lines       56268    56531     +263     
  Branches    56268    56531     +263     
==========================================
+ Hits        50868    51213     +345     
+ Misses       5400     5318      -82

Files Changed	Coverage Δ
lightning/src/ln/functional_tests.rs	`98.16% <ø> (+0.01%)`	⬆️
lightning/src/events/mod.rs	`46.75% <50.00%> (+4.24%)`	⬆️
lightning/src/chain/channelmonitor.rs	`94.72% <100.00%> (-0.05%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.52% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`

... and 11 files with indirect coverage changes

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

tnull

Thanks for picking this up!

A few notes regarding commits and commit messages: please try to break down the changes into logical steps of which each should live in in its own commit. Note that with each commit (except for maybe fixups that will be squashed before merging) the code should still be in a buildable and testable state, i.e., in this PR probably all changes need to live in one commit. Please clearly mark fixups to make them distinguishable from commits that should be present when merging.
In general I'd recommend getting familiar with git rebase -i as it's a very powerful tool when managing commits, and you might want to refer to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits.

Commit messages should consist of a short summary (no longer than 50 chars wide) and a body that summarizes not only the changes themselves but also gives more context on the how and why. This guide gives a good summary of best practices: https://cbea.ms/git-commit/

tnull · 2023-08-22T06:47:49Z

lightning/src/events/mod.rs

@@ -673,6 +673,8 @@ pub enum Event {
 	SpendableOutputs {
 		/// The outputs which you should store as spendable by you.
 		outputs: Vec<SpendableOutputDescriptor>,
+		/// Channel id


We should also add a compatibility notice here: as mentioned below this should be Option<[u8; 32]> to not break backwards compatibility and we should specify starting from which version users can expect this to be Some.

tnull · 2023-08-22T06:56:22Z

lightning/src/events/mod.rs

 					read_tlv_fields!(reader, {
 						(0, outputs, required),
+						(1, channel_id, required),


To give a little more context on our serialization logic: we generally use Lightning's TLV (type-length-value) format and apply the corresponding "it's okay to be odd" rule, i.e., when reading TLV fields we ignore any unknown fields if they are odd numbered and would refuse to continue if they were even.

Moreover, we would refuse to continue if any required field would not be present at the time of reading. As we aim for forwards- and backwards-compatiblity, we usually need to make new fields odd options. Otherwise users upgrading from a prior version might get a panic when LDK refuses to read a SpendableOutputs event in which the required channel_id is not present.

tnull · 2023-08-22T07:05:15Z

lightning/src/ln/monitor_tests.rs

@@ -95,7 +95,7 @@ fn chanmon_fail_from_stale_commitment() {
 fn test_spendable_output<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, spendable_tx: &Transaction) {
 	let mut spendable = node.chain_monitor.chain_monitor.get_and_clear_pending_events();
 	assert_eq!(spendable.len(), 1);
-	if let Event::SpendableOutputs { outputs } = spendable.pop().unwrap() {
+	if let Event::SpendableOutputs { outputs, channel_id } = spendable.pop().unwrap() {


We should either check the channel_id here, or avoid the unused variable:

Suggested change

if let Event::SpendableOutputs { outputs, channel_id } = spendable.pop().unwrap() {

if let Event::SpendableOutputs { outputs, .. } = spendable.pop().unwrap() {

jbesraa · 2023-08-22T12:29:09Z

thank you @tnull @wpaulino
Made the request changes.

Notice that I left ldk version in docs above channel_id def in the struct as X. will amend once we decide which version this will be included in!

tnull

LGTM, besides some doc nits.

lightning/src/events/mod.rs

tnull · 2023-08-22T12:54:14Z

lightning/src/events/mod.rs

@@ -673,6 +673,10 @@ pub enum Event {
 	SpendableOutputs {
 		/// The outputs which you should store as spendable by you.
 		outputs: Vec<SpendableOutputDescriptor>,
+		/// Channel id to help you locate the corresponding `ChannelMonitor`,
+		/// We define this as an option so we dont break backwards compatibility.
+		/// This will always be `Some` for events generated by LDK versions 0.0.XX and above.


I think it's safe to assume this will land with 0.0.117 :)

This will make it possible to link between SpendableOuts and ChannelMonitor - change channel_id to option so we dont break upgrade - remove unused channel_id - document channel_id - extract channel id dynamically to pass test - use contains to check channel_id in test as the events are not ordered - update docs framing - specify ldk version channel_id will be introduced in Co-authored-by: Elias Rohrer <[email protected]> Update lightning/src/events/mod.rs Co-authored-by: Elias Rohrer <[email protected]>

jbesraa · 2023-08-22T14:09:51Z

added docs suggestion(s), updated ldk version to 0.0.117 and squashed 🎈

tnull

LGTM

TheBlueMatt

For future reference, commit messages must start with a single line (the title) and then be followed by an empty line. In this case you missed the blank line after the title.

jbesraa commented Aug 21, 2023

View reviewed changes

jbesraa changed the title ~~Add channel_id~~ Add channel_id to SpendableOutputs event Aug 21, 2023

wpaulino reviewed Aug 21, 2023

View reviewed changes

tnull reviewed Aug 22, 2023

View reviewed changes

jbesraa force-pushed the add-channel-id-to-spendableoutputs-event branch 3 times, most recently from 87abb91 to ead7332 Compare August 22, 2023 12:19

tnull reviewed Aug 22, 2023

View reviewed changes

jbesraa force-pushed the add-channel-id-to-spendableoutputs-event branch from 4f71cf5 to 14b7612 Compare August 22, 2023 14:08

tnull approved these changes Aug 22, 2023

View reviewed changes

wpaulino approved these changes Aug 22, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 22, 2023

View reviewed changes

TheBlueMatt merged commit e9be7e2 into lightningdevkit:main Aug 22, 2023

jbesraa mentioned this pull request Aug 23, 2023

Print contents of our own channel updates #2516

Closed

jbesraa mentioned this pull request Oct 1, 2023

Improve block connection logging #2559

Closed

bonomat mentioned this pull request Oct 26, 2023

Closing channel does not give you feedback get10101/10101#1533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add channel_id to SpendableOutputs event #2511

Add channel_id to SpendableOutputs event #2511

jbesraa commented Aug 21, 2023

jbesraa Aug 21, 2023

wpaulino Aug 21, 2023

jbesraa Aug 22, 2023

wpaulino Aug 21, 2023

wpaulino Aug 21, 2023

tnull Aug 22, 2023

wpaulino Aug 21, 2023

tnull Aug 22, 2023

codecov-commenter commented Aug 21, 2023 •

edited

Loading

tnull left a comment •

edited

Loading

tnull Aug 22, 2023

tnull Aug 22, 2023

tnull Aug 22, 2023

jbesraa commented Aug 22, 2023

tnull left a comment

tnull Aug 22, 2023

jbesraa commented Aug 22, 2023

tnull left a comment

TheBlueMatt left a comment

	if let Event::SpendableOutputs { outputs, channel_id } = spendable.pop().unwrap() {
	if let Event::SpendableOutputs { outputs, .. } = spendable.pop().unwrap() {

Add channel_id to SpendableOutputs event #2511

Add channel_id to SpendableOutputs event #2511

Conversation

jbesraa commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 21, 2023 • edited Loading

Codecov Report

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Aug 22, 2023

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Aug 22, 2023

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 21, 2023 •

edited

Loading

tnull left a comment •

edited

Loading