lnd: compute FundingExpiryBlocks in PendingOpenChannel message #7480

maxwellsayles · 2023-03-03T23:37:24Z

Change Description

This adds the FundingExpiryBlocks field to the PendingOpenChannel message. The FundingExpiryBlocks field is computed from the broadcast height of the funding transaction, the current height of the chain and MaxWaitNumBlocksFundingConf. This is visible via the RPC and lncli pendingchannels.

Resolves #7434

Steps to Test

Added an integration test (testFundingExpiryBlocksOnPending) that opens a channel between Alice and Bob, invokes the RPC "pendingchannels", checks for the FundingExpiryBlocks field in the result message, mines a block, and then loops checking that the value decreases as expected until the funding transaction is completed.

$ make itest icase=funding_expiry_blocks_on_pending

Also manually tested the results of lncli pendingchannels:

Created a simnet between Alice and Bob with lncli openchannel.
Check that lncli pendingchannels has "funding_expiry_blocks": 2016.
Mine a block with btcd generate 1.
Check that lncli pendingchannels has "funding_expiry_blocks": 2015. The value decreased by one as expected.
Mine two more blocks and the channel moves from pending to open and does not show up with lncli pendingchannels, but rather does show up with lncli listchannels.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

yyforyongyu

Thanks for the PR! Please refer to our contribution guidelines on how to create commit messages. Plus the linter needs to be fixed.

maxwellsayles · 2023-03-14T23:33:29Z

@yyforyongyu thanks for the review!

Cleaned up the commit stack. Ran make fmt on each commit. (Is this what the linter wants?) Looks like previously the workflow was waiting for the lint code status to be reported, so not sure what the issue/fix is. Let me know if there's something else, and I'll check to see if the workflow is updated.

funding/manager.go

maxwellsayles · 2023-03-15T18:50:46Z

Fixing lint. Found this style guide. (y)

guggero · 2023-03-16T09:12:33Z

When you create a PR, there's a checklist: https://github.com/lightningnetwork/lnd/blob/master/.github/pull_request_template.md
It links to all the guides. So what's still missing is the commit message format (prefix, length, body).
Also, it seems you're using the wrong version to compile the proto files. Please make sure you only use make rpc (which runs a docker based build that uses the correct versions).

maxwellsayles · 2023-04-16T18:47:22Z

@guggero @yyforyongyu would love to get a review or feedback on this. I can abandon the PR if it's not relevant, or wrap this up if you want it included. Thanks!

yyforyongyu

The code looks good👍 Also like the added itest! The only issue is the format of the commit messages - that we wanna the format of pkg: description. For instance, the first commit would need to be changed into funding: make MaxWaitNumBlocksFundingConf public to..., and so do the rest.

itest/lnd_misc_test.go

rpcserver.go

maxwellsayles · 2023-04-19T19:48:31Z

Made suggested changes. Re-requesting review @guggero @yyforyongyu. Thanks!

guggero

Great feature and nice test!
Mostly nits from my side.

guggero · 2023-04-20T11:17:14Z

lnrpc/lightning.proto

+        // channel should be fee bumped using CPFP (see walletrpc.BumpFee) to
+        // ensure that the channel confirms in time. Otherwise a force-close
+        // will be necessary if the channel confirms after the funding
+        // transaction expires.


Add "A negative value means the channel responder has very likely canceled the funding and the channel will never become fully operational."

@guggero @yyforyongyu : We should be able to test for a negative value of FundExpiryBlocks. Any tips on how to construct this state? I was considering something like:

Alice opens a pending channel to Bob

Alice force closes the channel

Mine more blocks than what Bob expects for a confirmation

Verify the negative value of FundExpiryBlocks

I'll try it out, but it's not clear to me how a "pending" channel might transition after the requisite number of confirmations or a force close. Thanks!

There is a ht.MineEmptyBlocks method you can use, which mines blocks, but not includes any txes from the mempool.

You could try it out locally, and I'm suggesting not adding it to the itest builds here. Unfortunately, we are still trying to find out an efficient way to run itests faster, and mining 2016 blocks will dramatically slow them down.

itest/lnd_misc_test.go

guggero · 2023-04-20T11:22:21Z

itest/lnd_misc_test.go

+// before the funding transaction is confirmed, that the FundingExpiryBlocks
+// field of a PendingChannels decreases.
+func testFundingExpiryBlocksOnPending(ht *lntest.HarnessTest) {
+	// Configure bob to require 5 confs for a channel to be considered open


nit: missing fulls top at end of sentence.

Also, this comment just describes what the code does. But it doesn't say why the default confs need to be overwritten (because the default in all integration tests is set to 1).
We should probably also decrease the value to 3 since the more blocks we mine here, the slower subsequent tests become because the nodes need to sync to the extra blocks mined.

@guggero

We should probably also decrease the value to 3 since the more blocks we mine here, the slower subsequent tests become because the nodes need to sync to the extra blocks mined.

Is the test not isolated in this instance? If so, would it make sense to restart the node with the defaults when the test ends (both on success and failure)?

Side note: Do tests need to do their own cleanup here? I did a quick search for test setup, but I didn't see how this was handled.

there is a cleanup in Subtest. And nope, currently all tests in the same tranche share the same blockchain state, which is something I'm looking into for a change.

itest/lnd_misc_test.go

maxwellsayles · 2023-05-30T19:31:56Z

Updated test to mine empty blocks
Added release notes

Re-requesting review. At your leisure. Thanks @guggero and @yyforyongyu !

guggero

Very nice, LGTM 🎉

lightninglabs-deploy · 2023-06-07T08:41:03Z

@yyforyongyu: review reminder

yyforyongyu

Very nice! Could you do a rebase so we can let the build run again? The newly added itest funding_expiry_blocks_on_pending has failed, and the logs are expired.

maxwellsayles · 2023-06-07T18:36:14Z

I rebased to master and reran make itest icase=funding_expiry_blocks_on_pending to verify that the test succeeds.

cc @yyforyongyu

yyforyongyu

This is the error from the builds,

harness_assertion.go:972: 
        	Error Trace:	/home/runner/work/lnd/lnd/itest/harness_assertion.go:972
        	            				/home/runner/work/lnd/lnd/itest/lnd_open_channel_test.go:653
        	            				/home/runner/work/lnd/lnd/itest/harness.go:286
        	            				/home/runner/work/lnd/lnd/itest/lnd_test.go:136
        	Error:      	Received unexpected error:
        	            	Alice: assert pending open channels failed: want 0, got: 1, total: 1, previously had: 0
        	Test:       	TestLightningNetworkDaemon/tranche00/14-of-133/bitcoind/funding_expiry_blocks_on_pending
        	Messages:   	num of pending open channels not match
    harness.go:339: finished test: funding_expiry_blocks_on_pending, start height=974, end height=978, mined blocks=4
    harness.go:345: test failed, skipped cleanup

Can take a deeper look tomorrow.

maxwellsayles · 2023-06-08T18:53:42Z

itest/lnd_open_channel_test.go

+	ht.MineBlocks(1)
+	ht.AssertNumPendingOpenChannels(alice, 0)
+	ht.AssertNumPendingOpenChannels(bob, 0)


@yyforyongyu , the build failure seems strange. This test passes when run locally and in isolation. I'm testing if the complete make itest passes locally. Maybe it's a test isolation issue. That being said though, is there some difference between the local run and the build run? The failure seems to be after mining a single block and checking for the number of pending open channels (the lines above). On a local run, mining one block moves the channel from pending to open. In the build environment, is this still the case?

In any event, the intention of the test is to check the decreasing value of FundingExpiryBlocks and not that channels transition from pending to open after mining a block. I'll check if the test still passes locally without these lines, and I think it should be fine to remove them altogether. Thoughts?

maxwellsayles · 2023-06-08T19:12:14Z

itest/lnd_open_channel_test.go

+	ht.MineBlocksAndAssertNumTxes(1, 1)
+	chanPoint := lntest.ChanPointFromPendingUpdate(update)
+	ht.CloseChannel(alice, chanPoint)


@yyforyongyu, I went for a slightly different approach. Still need to mine a block to gracefully close the channel, but I don't think we need to check that it transitions to open. This works on an isolated local run, and I'm testing if the complete make itest passes locally. Let me know if you have any thoughts. Thanks.

You can always refer to the build results below to see the errors. And you can test it against different backend nodes as documented here. Our build runs against btcd, neutrino and multiple modes of bitcoind mixed with different database options.

As for why the previous test was flaky, it was because the method ht.MineBlocks(1) is used instead of ht.MineBlocksAndAssertNumTxes(1). Because we use regtest, mining blocks can be very fast, thus the funding tx may have not entered the mempool yet but the block is already mined, which explains why the test failed for some of the backends.

The current approach works because ht.MineBlocksAndAssertNumTxes is used.

@yyforyongyu thanks for the explanation. Good to know about ht.MineBlocksAndAssertNumTxes. I rebased this against the latest master.

yyforyongyu

LGTM🎉

yyforyongyu · 2023-06-09T10:15:21Z

itest/lnd_open_channel_test.go

+	ht.MineBlocksAndAssertNumTxes(1, 1)
+	chanPoint := lntest.ChanPointFromPendingUpdate(update)
+	ht.CloseChannel(alice, chanPoint)


You can always refer to the build results below to see the errors. And you can test it against different backend nodes as documented here. Our build runs against btcd, neutrino and multiple modes of bitcoind mixed with different database options.

As for why the previous test was flaky, it was because the method ht.MineBlocks(1) is used instead of ht.MineBlocksAndAssertNumTxes(1). Because we use regtest, mining blocks can be very fast, thus the funding tx may have not entered the mempool yet but the block is already mined, which explains why the test failed for some of the backends.

The current approach works because ht.MineBlocksAndAssertNumTxes is used.

guggero · 2023-06-10T12:00:00Z

Needs a rebase.

cc @saubyk

`FundingExpiryBlocks`.

…essage.

pending channels that are waiting for funding confirmation.

`PendingOpenChannel`.

saubyk

@maxwellsayles does this pr covers the change to REST API response as well?

maxwellsayles · 2023-06-12T20:14:23Z

@saubyk this is the first of heard of the REST API. I would have to investigate to answer your question. Any pointers or documentation on the REST API?

saubyk · 2023-06-12T20:24:33Z

@saubyk this is the first of heard of the REST API. I would have to investigate to answer your question. Any pointers or documentation on the REST API?

You can see the REST API for the pending channels rpc: https://lightning.engineering/api-docs/api/lnd/lightning/pending-channels

My guess is it's covered as I see the changes made to the lightning.swagger.json as well, but wanted to be sure.

guggero · 2023-06-13T10:07:05Z

@saubyk the REST API is always automatically updated from changes to any of the .proto files.

saubyk

LGTM 🕐

saubyk · 2023-06-13T15:35:45Z

Congrats @maxwellsayles on getting your first pr merged in LND! 🎉

maxwellsayles · 2023-06-13T16:14:35Z

@guggero any recommended followup 'issues' that might be a continuation of this PR? I'll have a look at the open issues and see if anything jumps out.

Thanks everyone for the help throughout!

guggero · 2023-06-13T17:18:40Z

@guggero any recommended followup 'issues' that might be a continuation of this PR?

Not really related to this issue, just a thought. I should probably write this into an actual issue, but one thing I was thinking today that would be very helpful is an RPC that returns a certain number of lines from the current log file. So basically a GetLogs RPC that allows you to specify the number of lines (would default to "all lines in the file" if not specified).

This was referenced Mar 3, 2023

[feature]: show channel funding TX expiry in listchannels #7434

Closed

[WIP] Show channel funding TX expiry in listchannels #7479

Closed

maxwellsayles changed the title ~~[WIP] Show channel funding TX expiry in pendingchannels~~ lnd: compute FundingExpiryBlocks in PendingOpenChannel message Mar 7, 2023

saubyk assigned maxwellsayles Mar 13, 2023

yyforyongyu requested changes Mar 13, 2023

View reviewed changes

maxwellsayles force-pushed the fundExpiryOnPending branch 3 times, most recently from 5e4499a to 692654a Compare March 14, 2023 23:28

maxwellsayles requested a review from yyforyongyu March 14, 2023 23:33

maxwellsayles commented Mar 14, 2023

View reviewed changes

funding/manager.go Outdated Show resolved Hide resolved

maxwellsayles force-pushed the fundExpiryOnPending branch 2 times, most recently from 7063c3d to 1a4a978 Compare March 14, 2023 23:48

maxwellsayles marked this pull request as draft March 15, 2023 18:50

maxwellsayles force-pushed the fundExpiryOnPending branch 2 times, most recently from 59a87b6 to ede9514 Compare March 15, 2023 23:14

maxwellsayles marked this pull request as ready for review March 15, 2023 23:17

maxwellsayles marked this pull request as draft March 16, 2023 16:05

maxwellsayles force-pushed the fundExpiryOnPending branch from ede9514 to e8ad5cc Compare March 16, 2023 16:54

maxwellsayles marked this pull request as ready for review March 16, 2023 16:55

maxwellsayles force-pushed the fundExpiryOnPending branch from e8ad5cc to 0d51d79 Compare March 30, 2023 16:59

guggero self-requested a review April 17, 2023 07:51

yyforyongyu requested changes Apr 18, 2023

View reviewed changes

itest/lnd_misc_test.go Outdated Show resolved Hide resolved

rpcserver.go Outdated Show resolved Hide resolved

maxwellsayles force-pushed the fundExpiryOnPending branch from 0d51d79 to 01562c2 Compare April 19, 2023 19:45

maxwellsayles requested a review from yyforyongyu April 19, 2023 19:47

guggero reviewed Apr 20, 2023

View reviewed changes

maxwellsayles requested a review from guggero May 30, 2023 19:30

guggero approved these changes May 31, 2023

View reviewed changes

yyforyongyu requested changes Jun 7, 2023

View reviewed changes

maxwellsayles force-pushed the fundExpiryOnPending branch from 2e32ab0 to 7d164dc Compare June 7, 2023 18:35

maxwellsayles requested a review from yyforyongyu June 7, 2023 18:35

yyforyongyu reviewed Jun 8, 2023

View reviewed changes

maxwellsayles commented Jun 8, 2023

View reviewed changes

maxwellsayles force-pushed the fundExpiryOnPending branch from 7d164dc to de9f778 Compare June 8, 2023 19:09

maxwellsayles commented Jun 8, 2023

View reviewed changes

yyforyongyu approved these changes Jun 9, 2023

View reviewed changes

maxwellsayles added 4 commits June 12, 2023 10:04

funding: make MaxWaitNumBlocksFundingConf public to be used to compute

9dea695

`FundingExpiryBlocks`.

funding: provide FundingExpiryBlock field in PendingOpenChannel m…

663b288

…essage.

funding: integration test to verify the value of FundingExpiryBlock on

f7bdbb9

pending channels that are waiting for funding confirmation.

funding: update release notes to mention funding_expiry_blocks in

987b54f

`PendingOpenChannel`.

maxwellsayles force-pushed the fundExpiryOnPending branch from de9f778 to 987b54f Compare June 12, 2023 17:10

saubyk reviewed Jun 12, 2023

View reviewed changes

saubyk self-requested a review June 13, 2023 15:20

saubyk approved these changes Jun 13, 2023

View reviewed changes

guggero merged commit f97f54e into lightningnetwork:master Jun 13, 2023
21 of 23 checks passed

maxwellsayles deleted the fundExpiryOnPending branch June 13, 2023 16:14

lnd: compute FundingExpiryBlocks in PendingOpenChannel message #7480

lnd: compute FundingExpiryBlocks in PendingOpenChannel message #7480

Conversation

maxwellsayles commented Mar 3, 2023 • edited

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

yyforyongyu left a comment

Choose a reason for hiding this comment

maxwellsayles commented Mar 14, 2023

maxwellsayles commented Mar 15, 2023 • edited

guggero commented Mar 16, 2023

maxwellsayles commented Apr 16, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

maxwellsayles commented Apr 19, 2023

guggero left a 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

Choose a reason for hiding this comment

maxwellsayles commented May 30, 2023 • edited

guggero left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Jun 7, 2023

yyforyongyu left a comment

Choose a reason for hiding this comment

maxwellsayles commented Jun 7, 2023 • edited

yyforyongyu left a 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

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Jun 10, 2023

saubyk left a comment

Choose a reason for hiding this comment

maxwellsayles commented Jun 12, 2023

saubyk commented Jun 12, 2023 • edited

guggero commented Jun 13, 2023

saubyk left a comment

Choose a reason for hiding this comment

saubyk commented Jun 13, 2023

maxwellsayles commented Jun 13, 2023

guggero commented Jun 13, 2023

maxwellsayles commented Mar 3, 2023 •

edited

maxwellsayles commented Mar 15, 2023 •

edited

maxwellsayles commented May 30, 2023 •

edited

maxwellsayles commented Jun 7, 2023 •

edited

saubyk commented Jun 12, 2023 •

edited