funding: fix flake in itest caused by persistent fee param changes by guggero · Pull Request #7648 · lightningnetwork/lnd

guggero · 2023-04-28T14:03:16Z

Partially reverts changes of #7597, in order to fix an itest flake.

ellemouton

Very nice 🔥 i've never seen the funlen linter get triggered 😂 I think this is an OK place to put a nolint though

.golangci.yml

guggero · 2023-04-28T18:06:42Z

Looks like I broke something in a different itest. Will take a look on Monday.

guggero · 2023-05-08T14:47:15Z

Looks like I broke something in a different itest. Will take a look on Monday.

Looks to have been flakes, looks better after a rebase.

yyforyongyu

Looks to have been flakes, looks better after a rebase

Confirm they are flakes. Need to seriously fix it now as it seems to be the only one🧐 will check it later this week.

funding/manager.go

peer/brontide.go

lnpeer/peer.go

peer/brontide.go

guggero · 2023-05-26T10:42:14Z

@yyforyongyu there's still a bug in the unit test that I need to fix, so not ready for full review. But could you please quickly check if this new approach is more in line with what you had in mind?

yyforyongyu

But could you please quickly check if this new approach is more in line with what you had in mind?

Yes I like it! Thanks🙏

guggero · 2023-06-14T16:31:56Z

@yyforyongyu I found the unit test flake, was just a timing issue. So this PR is now ready for full review.
@ellemouton I'm re-requesting review from you as well, since quite a few things changed in the meantime.

yyforyongyu

Nice refactor🙏 And love how the build looks✅✅✅

Meanwhile I think we should save the policy to disk in handleFundingSigned for funder's side and handleFundingCreated in fundee's side, and only do them there in case anything goes wrong in the previous steps of the funding flow.

Also we should delete the saved policy when funding flow failed.

I don't think these are introduced in this PR tho, but need to be fixed nevertheless.

funding/manager.go

channeldb/forwarding_policy.go

peer/brontide.go

yyforyongyu · 2023-06-15T21:56:24Z

A second thought is we can go with this one and create a new PR to fix issues around saving and deleting policies. Btw I think this one needs to be in 0.17 so we can have a clean build for other PRs cc @saubyk

saubyk · 2023-06-16T03:27:56Z

tagged for 0.17.0

funding/manager.go

channeldb/forwarding_policy.go

lightninglabs-deploy · 2023-08-15T14:06:24Z

@ellemouton: review reminder
@guggero, remember to re-request review from reviewers when ready

yyforyongyu

Can we rebase to run the test again? I think the current failure might be fixed by #7518

channeldb/models/channel.go

Since the actual wallet backends might be different between the wallet DB and the actual channel state DB, we pass in the correct struct to the funding manager.

This will allow us to also set the TimeLockDelta and HTLC settings when creating the channel. We leave the actual implementation of the RPC and CLI changes to another PR, this is just to make things more consistent.

guggero · 2023-08-18T07:45:33Z

Rebased. Wasn't able to reproduce the Neutrino flake on my machine. Let's see how it looks now.

Because we now send the correct initial forwarding policy to Brontide, the correct initial values are inserted into the link and we no longer need to update the link in a later step.

This commit moves the deletion of the initial forwarding policy to the end of `stateStep` to make sure the router has persisted it to disk before the deletion.

This commit is a pure code move. We add a new method `handleChannelReadyReceived` to handle the channel's state change after the remote's channel ready message is received.

There is a race condition, - goroutine_1: performing `stateStep` under `channelReadySent`. Once `receivedChannelReady` returns true, it will mark the channel state as `addedToRouterGraph`, which will delete the initial forwarding policy for private channels. - goroutine_2: performing `handleChannelReady`, which processes the remote's channelReady message. It will ask brontide to `AddNewChannel` in the end. - goroutine_3: performing `handleNewActiveChannel` in brontide, which will query the initial forwarding policy and fail to find it because it's already deleted in goroutine_1. To fix it, we require `receivedChannelReady` to also check that the channel's barrier signal in the map `handleChannelReadyBarriers` doesn't exist, as this signal is removed once `handleChannelReady` finishes adding the channel in brontide.

yyforyongyu

LGTM🤓

guggero

I can't approve my own PR, but I'm leaving an explicit "LGTM 🎉" here since @yyforyongyu added some commits to it (thank you very much and great find with the barrier!).

guggero · 2023-08-22T09:55:04Z

@ellemouton this is ready for a final review, thanks!

ellemouton

Very nice fix 🔥 LGTM!

ellemouton · 2023-08-22T12:19:41Z

funding/manager.go

 		return "markedOpen"
 	case channelReadySent:
-		return "channelReady"
+		return "channelReadySent"


ellemouton · 2023-08-22T12:40:54Z

funding/manager.go

@@ -3070,14 +3070,27 @@ func (f *Manager) receivedChannelReady(node *btcec.PublicKey,
 		return false, err
 	}



really helpful commit message - thanks! 🙏

guggero added the no-changelog label Apr 28, 2023

guggero requested review from ellemouton and positiveblue April 28, 2023 14:03

ellemouton approved these changes Apr 28, 2023

View reviewed changes

yyforyongyu reviewed Apr 28, 2023

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

guggero force-pushed the initial-forwarding-policy branch from 25e7f1e to ea942f1 Compare April 28, 2023 17:53

guggero requested a review from yyforyongyu April 28, 2023 17:53

guggero force-pushed the initial-forwarding-policy branch from ea942f1 to 080f6f2 Compare May 8, 2023 12:11

yyforyongyu reviewed May 9, 2023

View reviewed changes

funding/manager.go Outdated Show resolved Hide resolved

funding/manager.go Outdated Show resolved Hide resolved

peer/brontide.go Outdated Show resolved Hide resolved

lnpeer/peer.go Outdated Show resolved Hide resolved

peer/brontide.go Outdated Show resolved Hide resolved

guggero force-pushed the initial-forwarding-policy branch from 080f6f2 to 908c33f Compare May 11, 2023 13:13

guggero requested a review from yyforyongyu May 11, 2023 13:15

guggero removed request for positiveblue and yyforyongyu May 25, 2023 15:01

guggero force-pushed the initial-forwarding-policy branch from 908c33f to 703b972 Compare May 26, 2023 09:36

yyforyongyu reviewed May 30, 2023

View reviewed changes

guggero force-pushed the initial-forwarding-policy branch from 703b972 to d66212f Compare June 14, 2023 13:44

guggero requested review from ellemouton and yyforyongyu June 14, 2023 16:31

yyforyongyu requested changes Jun 15, 2023

View reviewed changes

saubyk added this to the v0.17.0 milestone Jun 16, 2023

positiveblue reviewed Jun 19, 2023

View reviewed changes

funding/manager.go Outdated Show resolved Hide resolved

funding/manager.go Outdated Show resolved Hide resolved

channeldb/forwarding_policy.go Outdated Show resolved Hide resolved

saubyk assigned guggero Jun 27, 2023

saubyk modified the milestones: v0.17.0, v0.17.1 Jul 11, 2023

yyforyongyu reviewed Aug 17, 2023

View reviewed changes

channeldb/models/channel.go Outdated Show resolved Hide resolved

guggero added 5 commits August 18, 2023 09:44

funding+server: don't use wallet DB for channel operations

665179e

Since the actual wallet backends might be different between the wallet DB and the actual channel state DB, we pass in the correct struct to the funding manager.

funding: extract defaultForwardingPolicy

5b9aa63

manager: also store initial time lock delta and HTLC settings

4770cb0

This will allow us to also set the TimeLockDelta and HTLC settings when creating the channel. We leave the actual implementation of the RPC and CLI changes to another PR, this is just to make things more consistent.

channeldb: move fwding policy code to new file

eda34a8

funding+channeldb: rename fwding -> forwarding

36fcf65

guggero force-pushed the initial-forwarding-policy branch from fc8cd75 to bc27da0 Compare August 18, 2023 07:44

guggero and others added 6 commits August 22, 2023 06:22

channeldb+funding: move policy encoding into channel DB

59b5fb1

multi: use fwding policy from models pkg

d5c504c

funding: make sure initial policy is always set

7ac445c

peer: load initial forwarding policy from channel DB

fb36806

server+funding: remove previous link update mechanism

d822d75

Because we now send the correct initial forwarding policy to Brontide, the correct initial values are inserted into the link and we no longer need to update the link in a later step.

funding: rename channelReady to channelReadySent for clarity

38b106c

yyforyongyu force-pushed the initial-forwarding-policy branch from bc27da0 to ca4c16e Compare August 22, 2023 00:19

funding: defer initial forwarding policy deletion

d9e0464

This commit moves the deletion of the initial forwarding policy to the end of `stateStep` to make sure the router has persisted it to disk before the deletion.

yyforyongyu force-pushed the initial-forwarding-policy branch from ca4c16e to 605062a Compare August 22, 2023 00:22

yyforyongyu added 2 commits August 22, 2023 09:52

funding: add new method handleChannelReadyReceived

32d8208

This commit is a pure code move. We add a new method `handleChannelReadyReceived` to handle the channel's state change after the remote's channel ready message is received.

yyforyongyu force-pushed the initial-forwarding-policy branch from 605062a to 61ea29a Compare August 22, 2023 02:26

funding: put assertNoFwdingPolicy in wait.NoError

6820b08

yyforyongyu force-pushed the initial-forwarding-policy branch from 61ea29a to 6820b08 Compare August 22, 2023 03:10

yyforyongyu approved these changes Aug 22, 2023

View reviewed changes

guggero commented Aug 22, 2023

View reviewed changes

ellemouton approved these changes Aug 22, 2023

View reviewed changes

guggero merged commit e49f6fc into lightningnetwork:master Aug 22, 2023

guggero deleted the initial-forwarding-policy branch August 22, 2023 13:15

saubyk added this to the v0.17.0 milestone Aug 22, 2023

		@@ -3070,14 +3070,27 @@ func (f Manager) receivedChannelReady(node btcec.PublicKey,
		return false, err
		}

Conversation

guggero commented Apr 28, 2023

Uh oh!

ellemouton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guggero commented Apr 28, 2023

Uh oh!

guggero commented May 8, 2023

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guggero commented May 26, 2023

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

guggero commented Jun 14, 2023

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu commented Jun 15, 2023

Uh oh!

saubyk commented Jun 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lightninglabs-deploy commented Aug 15, 2023

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guggero commented Aug 18, 2023

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

guggero commented Aug 22, 2023

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

ellemouton Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

ellemouton Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ellemouton left a comment •

edited

Loading