[7/?] - funding: update funding manager w/ new musig2+taproot funding flow

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR

[Roasbeef]

Change Description

Depends on #7345.

Only the last 6 commits are new.

In this PR, we build on the prior PR that updated the channel state machine to update the funding manager to be aware of the new funding flow.

This flow is identical to the existing flow, but both sides need to exchange local nonces up front, and then signatures sent are now partial signatures instead of regular signatures.

The funding manager also gains some new state of the local nonces it needs to generate in order to send the funding locked message, and also process the funding locked message from the remote party.

In order to allow the funding manger to generate the nonces that need to be applied to each channel, then AddNewChannel method has been modified to accept a set of options that the peer will then use to bind the nonces to a new channel.

We also update the link+peer interaction to know how to initialize the new channel, as well as the extra information that needs to be sent in funding locked and the channel reest message.

As a stop gap before Gossip 1.5, we need to set a new feature bit in the channel announcement itself. Otherwise, the router won't know how to validate the funding script of the newly created channel.

Steps to Test

Steps for reviewers to follow to test the change.

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.

[joostjager]

Do you expect the same feature bit to be used eventually for public channels?

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[Roasbeef]

No, this is a temp hack so we're able to signal to the gossiper exactly what type of channel this is. When things get to the router, we'll still verify the channel on-chain (compared the output to the expected script), so we need this in order to signal that this is a different type of channel.

[joostjager]

Shallow-reviewed all the PRs in the series and overall the addition of simple taproot channels does not look too involved at all! Of course relative to the expectation, because obviously it is still a large diff.

General comments:

• Perhaps it is good to make the changes so that taproot channels follow the default flow instead of being the exception. So if !taproot { ... } rather than if taproot { ...}
• Related to the first point: would it be feasible to take a more object-oriented approach for adding taproot channels? More grouping of taproot-channel-specific code in a single location and fewer selects on chan type could possibly improve code quality.
• For public channels, gossip needs to change. Is there already anything concrete in terms of spec for that? Would be helpful to complete the mental model, even if not implemented.
• To reduce complexity, it would be great if all non-taproot channels would just go away 😬

[ellemouton]

Did first pass - looks good! Sooo cool how the funding manager could be made taproot-chan-ready with such a slim diff 🔥

[ellemouton]

can use channel.GenMusigNonces() as is done for createFundingLocked

[Crypt-iQ]

agree

[Roasbeef]

So can't use it here as we don't yet have the full channel state machine created. So instead we need to call the function manually. Also spotted a bug here (will be covered by the eventual itests): it uses the current commitment height, but should actually use the next commitment height. I think @ellemouton ran into this in the past.

[Crypt-iQ]

I think it gets confusing because they may send a channel_ready and channel_reestablish

[Roasbeef]

Goa here is to have the "last" one override the nonce that we'll store for them.

[Roasbeef]

Goal here is to have the "last" one override the nonce that we'll store for them.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Crypt-iQ]

Should this modify the feature bit dependency mapping to use "INC+" instead of "IN"?

[positiveblue]

First pass done.

The code looks good and the changes are pretty straight forward 🎉

There are some issues to resolve before merging (like make it panic proof). I also did not see any tests in the commits, are they added later in the PR chain?

[litbot-9000]

@Roasbeef, remember to re-request review from reviewers when ready

[Crypt-iQ]

I think that this might be an outdated branch because there are three panics?

• msg.LocalNonce in handleFundingOpen
• msg.LocalNonce in handleFundingAccept
• sig.Nonce in VerifyCommitSig

[Crypt-iQ]

think we need to include nonces here. Also think the spec needs to clarify what to do when we receive another FundingLocked that rotates the alias, do we ignore the nonces?

[Roasbeef]

Yes as is we'll ignore the second one (existing logic). We'll use the same nonce each time until the channel state has moved forward one.

[Crypt-iQ]

I think that means that the channel_ready will override the nonces previously set in channel_reestablish? IMO if channel_reestablish has been exchanged, we should ignore nonces in channel_ready and then we never need to worry about overriding nonces with scid alias rotation

[Roasbeef]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

[ellemouton]

LGTM pending resolution on @Crypt-iQ's comment re RegisterSpendNtfn & CI fixes.
Left one comment re allowing unsetting of the new feature bit.

[Crypt-iQ]

sig.Nonce in VerifyCommitSig

sig.Nonce isn't a pointer here.

if sig is nil then it will be nil-ptr-dereference

[yyforyongyu]

Think it's very close. Got one question - should we update the commit weight and fee here when calling NewChannelReservation?

[yyforyongyu]

nit: would put this check after L1968 to align with other checks such as if !resCtx.reservation.IsZeroConf() && msg.MinAcceptDepth == 0 { .

[yyforyongyu]

This is probably a bug, not introduced from this PR tho. At L2320 we already called f.deleteReservationCtx which removes the channel from f.activeReservations. Hence when we call failFundingFlow here, it will error out because f.cancelReservationCtx won't find the reservation.

[ellemouton]

should we update the commit weight and fee here when calling NewChannelReservation?

I think we can just change that to use the lnwallet.CommitWeight method instead which is updated with a taproot case here

[Crypt-iQ]

LGTM pending two comments:

• #7346 (comment)
• #7346 (review)

[Roasbeef]

Pushed up a final set of commits adding unit test coverage for the new funding manager flows, cc @positiveblue

[Crypt-iQ]

@ellemouton comment was that channelFeatures.OnlyContains can be called with ZeroConfRequired and ScidAliasRequired, but then hasFeatures doesn't contain ScidAliasOptional. I think that the ZeroConfRequired+ScidAliasRequired case is missing from this PR