Handle if funding output is in a coinbase transaction #1924

benthecarman · 2022-12-19T08:03:04Z

codecov-commenter · 2022-12-19T08:31:40Z

Codecov Report

Patch coverage: 92.18% and project coverage change: +0.01% 🎉

Comparison is base (e9d9711) 90.58% compared to head (418b0dc) 90.59%.

❗ Current head 418b0dc differs from pull request most recent head b28bf0b. Consider uploading reports for the commit b28bf0b to get more accurate results

❗ 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    #1924      +/-   ##
==========================================
+ Coverage   90.58%   90.59%   +0.01%     
==========================================
  Files         110      110              
  Lines       57526    57573      +47     
  Branches    57526    57573      +47     
==========================================
+ Hits        52112    52161      +49     
+ Misses       5414     5412       -2

Files Changed	Coverage Δ
lightning/src/ln/channelmanager.rs	`87.17% <50.00%> (-0.02%)`	⬇️
lightning/src/ln/channel.rs	`89.86% <83.33%> (+0.02%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`89.53% <92.85%> (+0.54%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.17% <100.00%> (-0.01%)`	⬇️

... and 2 files with indirect coverage changes

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

tnull

Thanks for looking into this!

A few assorted notes:

this def. needs testing
we should introduce a constant for the maturation delay
the extended number of required confirmations need to be reflected in other places, e.g., in ChannelDetails::confirmations_required, otherwise users might be confused why the channel isn't ready after this value has been reached. I therefore wonder if the check should be moved to accept_channel, i.e., if we could simply set Channel::minimum_depth to min(100, minimum_depth) for coinbase-funded channels there and call it a day?

benthecarman · 2022-12-20T08:53:37Z

if we could simply set Channel::minimum_depth to min(100, minimum_depth) for coinbase-funded channels there and call it a day?

I don't think this would work in accept_channel because it would only be for channels we open, not inbound channels, also we don't know if it is a coinbase transaction yet.

tnull · 2022-12-20T10:00:57Z

I don't think this would work in accept_channel because it would only be for channels we open, not inbound channels, also we don't know if it is a coinbase transaction yet.

Ah, you're of course right, I think I meant funding_signed/funding_created, which however basically is what you're doing already since check_get_channel_ready is used there. Still, the point stands that we need to consider the increased maturation period in other places, such as giving the right values in ChannelDetails::confirmations_required. Also note that we still need to consider the 0conf case, i.e., 0conf channel funded by a coinbase transaction.

benthecarman · 2023-02-28T11:54:00Z

Haven't started on tests just yet but made it so it should handle the zero-conf case AFIAK and made it so it updates the minimum_depth so I assume that'll propogate to everything like ChannelDetails::confirmations_require.

Does this look like I am on the right track?

7d1645a Add constant for coinbase maturity (benthecarman) Pull request description: Not sure if this is the best place to put this but it is nice to have a constant for this instead of having other libraries make their own (ie lightningdevkit/rust-lightning#1924 (review)) ACKs for top commit: tcharding: ACK 7d1645a apoelstra: ACK 7d1645a Tree-SHA512: 5ac2a3359cadd303158c66ba45db8f4bf8cc80b6c19604262999ff361fd0bd98e2a4851c57da1962cb5c74f5789a85c8b3861f1742706a60ce1fbc57c3c200cc

lightning/src/ln/channel.rs

benthecarman · 2023-04-19T08:40:08Z

Still working on understanding the testing infra around the channel stuff but I think the logic for this is correct now

TheBlueMatt · 2023-04-19T17:40:06Z

lightning/src/ln/channel.rs

@@ -5142,6 +5145,15 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 							}
 						}
 					}
+
+					// if this is a coinbase transaction and not a 0-conf channel


This should go in the txid == funding_txo check, otherwise we'll do this any time we see a coinbase tx at all.

TheBlueMatt · 2023-05-12T16:54:41Z

Looks like this needs a rebase, would be good to move this forward. Can we help with test writing here?

benthecarman · 2023-05-13T04:23:09Z

Looks like this needs a rebase, would be good to move this forward. Can we help with test writing here?

yeah sorry haven't had time to get to this, if this is a priority feel free to write the test for me haha

TheBlueMatt · 2023-06-13T04:58:21Z

Grrrrr, alright, this is gonna slip to 117 :(.

dunxen · 2023-08-31T15:38:09Z

I've rebased and am writing a test for this case.

TheBlueMatt · 2023-08-31T19:27:40Z

Rebased....where?

dunxen · 2023-08-31T19:29:16Z

Rebased....where?

Just locally

lightning/src/ln/functional_tests.rs

alecchendev · 2023-09-02T19:01:47Z

lightning/src/ln/functional_tests.rs

+	{
+		let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
+		assert_eq!(added_monitors.len(), 1);
+		assert_eq!(added_monitors[0].0, funding_output);
+		added_monitors.clear();
+	}


nit: I don't think this is testing anything new, could probably just be check_added_monitors?

I forget all the utils lol.

alecchendev · 2023-09-02T19:02:19Z

lightning/src/ln/functional_tests.rs

+	{
+		let mut added_monitors = nodes[0].chain_monitor.added_monitors.lock().unwrap();
+		assert_eq!(added_monitors.len(), 1);
+		assert_eq!(added_monitors[0].0, funding_output);
+		added_monitors.clear();
+	}


nit: ^ same here

alecchendev · 2023-09-02T19:08:58Z

lightning/src/ln/functional_tests.rs

+	// Note that 0conf channels with coinbase funding transactions are unaffected and are
+	// immediately operational after opening.


Would it be worth adding the coinbase case to the 0conf test? FWIW it doesn't look like too much work since we already have functional_test_utils::open_zero_conf_channel

Yeah, we thought it would be good to add them, so will be doing that definitely in this PR. Maybe some closes on those channels before maturity too.

TheBlueMatt · 2023-09-03T20:05:41Z

LGTM, feel free to squash IMO.

tnull

LGTM

benthecarman force-pushed the handle-coinbase-funding-channel branch from 35ebc72 to 5155b21 Compare December 19, 2022 08:03

benthecarman changed the title ~~Handle if funding transaction is in a coinbase transaction~~ Handle if funding output is in a coinbase transaction Dec 19, 2022

tnull reviewed Dec 19, 2022

View reviewed changes

benthecarman mentioned this pull request Feb 28, 2023

Add constant for coinbase maturity rust-bitcoin/rust-bitcoin#1679

Merged

benthecarman force-pushed the handle-coinbase-funding-channel branch from 5155b21 to c5a477e Compare February 28, 2023 11:52

benthecarman force-pushed the handle-coinbase-funding-channel branch 2 times, most recently from 177e373 to 3b06a34 Compare February 28, 2023 12:09

TheBlueMatt reviewed Mar 2, 2023

View reviewed changes

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

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

benthecarman force-pushed the handle-coinbase-funding-channel branch from 3b06a34 to 956188b Compare April 19, 2023 08:38

TheBlueMatt reviewed Apr 19, 2023

View reviewed changes

TheBlueMatt added this to the 0.0.116 milestone May 12, 2023

benthecarman force-pushed the handle-coinbase-funding-channel branch from 7ed3790 to 0f01921 Compare May 13, 2023 04:25

TheBlueMatt modified the milestones: 0.0.116, 0.0.117 Jun 13, 2023

alecchendev mentioned this pull request Jul 19, 2023

Implement the UtxoSource interface for REST/RPC clients #2248

Merged

dunxen force-pushed the handle-coinbase-funding-channel branch from 0f01921 to 696725c Compare September 1, 2023 17:19

TheBlueMatt reviewed Sep 1, 2023

View reviewed changes

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

alecchendev reviewed Sep 2, 2023

View reviewed changes

dunxen force-pushed the handle-coinbase-funding-channel branch from d3fc6f8 to 418b0dc Compare September 2, 2023 19:48

benthecarman and others added 3 commits September 4, 2023 17:13

Handle if funding output is in a coinbase transaction

a2e01d5

Support creating coinbase funding transactions in tests

9a37135

Add test for coinbase funding transactions

b28bf0b

dunxen force-pushed the handle-coinbase-funding-channel branch from 418b0dc to b28bf0b Compare September 4, 2023 15:14

tnull approved these changes Sep 5, 2023

View reviewed changes

TheBlueMatt approved these changes Sep 5, 2023

View reviewed changes

TheBlueMatt merged commit eb44d99 into lightningdevkit:main Sep 5, 2023
12 of 14 checks passed

benthecarman deleted the handle-coinbase-funding-channel branch September 5, 2023 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle if funding output is in a coinbase transaction #1924

Handle if funding output is in a coinbase transaction #1924

benthecarman commented Dec 19, 2022

codecov-commenter commented Dec 19, 2022 •

edited

Loading

tnull left a comment

benthecarman commented Dec 20, 2022 •

edited

Loading

tnull commented Dec 20, 2022 •

edited

Loading

benthecarman commented Feb 28, 2023

benthecarman commented Apr 19, 2023 •

edited

Loading

TheBlueMatt Apr 19, 2023

TheBlueMatt commented May 12, 2023

benthecarman commented May 13, 2023

TheBlueMatt commented Jun 13, 2023

dunxen commented Aug 31, 2023

TheBlueMatt commented Aug 31, 2023

dunxen commented Aug 31, 2023

alecchendev Sep 2, 2023

dunxen Sep 2, 2023

alecchendev Sep 2, 2023

alecchendev Sep 2, 2023

dunxen Sep 2, 2023

TheBlueMatt commented Sep 3, 2023

tnull left a comment

		// Note that 0conf channels with coinbase funding transactions are unaffected and are
		// immediately operational after opening.

Handle if funding output is in a coinbase transaction #1924

Handle if funding output is in a coinbase transaction #1924

Conversation

benthecarman commented Dec 19, 2022

codecov-commenter commented Dec 19, 2022 • edited Loading

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

benthecarman commented Dec 20, 2022 • edited Loading

tnull commented Dec 20, 2022 • edited Loading

benthecarman commented Feb 28, 2023

benthecarman commented Apr 19, 2023 • edited Loading

TheBlueMatt Apr 19, 2023

Choose a reason for hiding this comment

TheBlueMatt commented May 12, 2023

benthecarman commented May 13, 2023

TheBlueMatt commented Jun 13, 2023

dunxen commented Aug 31, 2023

TheBlueMatt commented Aug 31, 2023

dunxen commented Aug 31, 2023

alecchendev Sep 2, 2023

Choose a reason for hiding this comment

dunxen Sep 2, 2023

Choose a reason for hiding this comment

alecchendev Sep 2, 2023

Choose a reason for hiding this comment

alecchendev Sep 2, 2023

Choose a reason for hiding this comment

dunxen Sep 2, 2023

Choose a reason for hiding this comment

TheBlueMatt commented Sep 3, 2023

tnull left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2022 •

edited

Loading

benthecarman commented Dec 20, 2022 •

edited

Loading

tnull commented Dec 20, 2022 •

edited

Loading

benthecarman commented Apr 19, 2023 •

edited

Loading