nit: Two sentences improve the readability.

// TestOpeningChannelTxConfirmation verifies that calling MarkConfirmedScid
// correctly updates the confirmed state. It also ensures that calling Refresh
// on a different OpenChannel updates its in-memory state to reflect the prior
// MarkConfirmedScid call.

nit: referring to the variable 'ConfirmationUntilConfirmed' but at this point of the code you are using 'remainingConfs'.

The comment doesn't accurately reflect what the code does. Please review and clarify it. The code itself looks correct.

You don't need to split each one into two lines.

I think it’s more accurate to say that the channel opening tx gets one confirmation, rather than calling it a channel confirmation.

I think that TestOpeningChannelTxConfirmation might be more accurate for the name of the test as we are not testing the Channel confirmation, but its funding tx confirmation.

I think it’s more accurate to say that the channel opening tx gets one confirmation, rather than say that channel is confirmed.

I think it’s more accurate to say that the channel opening tx gets one confirmation, rather than calling it a channel confirmation.

Maybe change the name of the function to handleOpenChanTxConfirmation

This variable should be named confirmations_until_active instead, since what we are tracking is channel status change from 'pending' to 'active'

its really the number of blocks remaining before the channel status is 'active', not really "full confirmed". Important to distinguish between transaction confirmations and channel status.

nit: since the opening tx was broadcast at broadcastHeight, we could confirm it at broadcastHeight + 1.
It’s not mandatory and doesn’t change the test behavior, but I think it makes the test a bit more elegant. 😉

Done👍

Can you explain the reasoning behind this change?

Based on their names, I initially thought it was a typo; however, I'm not sure if it serves a specific purpose. If the naming is indeed intentional, could you explain why both oneConfChannel and sixConfChannel are the same(just for my understanding)?

The make(..., 6) call here is only sizing the buffer, but the test doesn't actually need a buffer of size 6. The name sixConfChannel refers to the event (i.e., sixth confirmation received), not the buffer size.

Thanks, understood. I have reverted this change. PTAL

It works, but this doesn't reflect the actual open channel process, right? Have you considered sending the channel confirmation after the transaction confirmation?

Sorry, I didn't quite understand what you meant. My understanding is that if we wait until the transaction is confirmed (after 3 or 6 blocks, etc.), then by the time we mark the channel as confirmed, it will already be open. Also, since we need the details of the SCID, we have to mark the channel as confirmed (after 1 block) even before the transaction is fully confirmed. Am I thinking in the right direction, or is there anything I should read first to better align my thoughts with your points?

I meaning that you are sending the confirmation tx event twice to Alice and Bob funding managers. I'm not sure if this is happening during the normal workflow when a channel is opening.

I meaning that you are sending the confirmation tx event twice to Alice and Bob funding managers. I'm not sure if this is happening during the normal workflow when a channel is opening.

Ok, so what I understand is that in a normal, real-world workflow, whenever a confirmation tx notification is registered, the notification is sent to all the goroutines that registered for it as soon as the block is mined. However, since this is a unit test, I need to explicitly send the confirmation tx event twice.

Have you considered sending the channel confirmation after the transaction confirmation?

Since we need the details of the SCID, we have to mark the channel as confirmed (after one block) even before the transaction is fully confirmed.
Also, if I send the goroutine for channel confirmation after the transaction confirmation, I still need to send the confirmation tx event twice because of the select statement:

2. If one or more of the communications can proceed, a single one that can proceed is chosen via a uniform pseudo-random selection.

See: https://go.dev/ref/spec#Select_statements

To avoid flaky tests, I send the confirmation tx event twice.
I hope this is the correct way of handling the process. Please suggest any better method you have in mind.

Agree this testHarness could be better mocked, however both nodes need to receive the confirmation to finally set the channel to active.

Tho this changes with my proposal so I am not going to review this for now.

This is not necessary, for can use the function waitForFundingConfirmation to update the short channel id.

we should also add the confirmation height here.

Nit: Not necessary this line, I mean we are mining an empty block.

we should mine normal blocks here, because there is no tx in the mempool anyways ?

I think for these cases a reverse loop fits better:

// we could also decrease the numConfs immediately when confirming the tx above which is maybe even better.

for i := numConfs-1; i > 0; i-- {
		expectedConfirmationsLeft := i

hmm I am not really a fan of catching raise condition like this, probably we need to change the design a bit to avoid this case.

What do you think about this instead:

	// Monitor the confirmation event for the funding transaction. And
	// return when the funding tx has received the required number of
	// confirmations.
	for {
		select {
		case updDetails := <-confNtfn.Updates:
			log.Tracef("funding tx %s received a confirmation, %d "+
				" number of confirmations still required", txid,
				updDetails.NumConfsLeft)

			// Handle first confirmation of the funding transaction.
			if updDetails.NumConfsLeft ==
				uint32(completeChan.NumConfsRequired-1) {

				log.Infof("funding tx %s received first "+
					"confirmation", txid)

				err := completeChan.MarkConfirmationHeight(
					updDetails.BlockHeight,
				)
				if err != nil {
					log.Errorf("failed to mark "+
						"confirmation height: %v", err)

					return
				}
			}

			// If we haven't reached final confirmation, continue
			// waiting for confirmations.
			if updDetails.NumConfsLeft > 0 {
				continue
			}

			log.Infof("funding tx %s received all confirmations",
				txid)

			// We've reached final confirmation. Wait for the the
			// confirmation event to trigger and then send the
			// result to the confirmation channel.
			err := f.handleFinalConfirmation(
				confNtfn, confChan, completeChan,
			)
			if err != nil {
				log.Errorf("failed to handle final "+
					"confirmation: %v", err)
			}

			return

		case <-confNtfn.NegativeConf:
			log.Warnf("funding tx %s was reorged out; channel "+
				"point: %s", txid, completeChan.FundingOutpoint)

			// Reset the confirmation height to 0 because the
			// funding transaction was reorged out.
			err := completeChan.MarkConfirmationHeight(uint32(0))
			if err != nil {
				log.Errorf("failed to update state for "+
					"ChannelPoint(%v): %v",
					completeChan.FundingOutpoint, err)

				return
			}

		case <-cancelChan:
			log.Warnf("canceled waiting for funding confirmation, "+
				"stopping funding flow for ChannelPoint(%v)",
				completeChan.FundingOutpoint)

			return

		case <-f.quit:
			log.Warnf("fundingManager shutting down, stopping "+
				"funding flow for ChannelPoint(%v)",
				completeChan.FundingOutpoint)

			return
		}
	}
}

// handleFinalConfirmation is a helper function that listens for the final
// confirmation event and sends the final confirmation to the confirmation
// channel which was provided by the caller.
func (f *Manager) handleFinalConfirmation(
	confNtfn *chainntnfs.ConfirmationEvent,
	confChan chan<- *confirmedChannel,
	completeChan *channeldb.OpenChannel) error {

	select {
	case confDetails, ok := <-confNtfn.Confirmed:
		if !ok {
			return fmt.Errorf("confirmation channel closed " +
				"unexpectedly")
		}

		// Send the result to the confirmation channel.
		shortChanID := lnwire.ShortChannelID{
			BlockHeight: confDetails.BlockHeight,
			TxIndex:     confDetails.TxIndex,
			TxPosition:  uint16(completeChan.FundingOutpoint.Index),
		}

		if !fn.SendOrQuit(confChan, &confirmedChannel{
			shortChanID: shortChanID,
			fundingTx:   confDetails.Tx,
		}, f.quit) {
			return fmt.Errorf("failed to send confirmation")
		}

		return nil

	case <-f.quit:
		return fmt.Errorf("fundingManager shutting down")
	}
}

This way we avoid this race condition of above.

I was thinking we don’t need this case (though I tried running the code with it and ran into issues because the confirmation notification was sent before anyone was listening, causing a block). We only really need this case when numConfs is 1, which leads to two scenarios:

1. The transaction isn’t confirmed yet, so we’ll have ConfirmationsUntilActive = 1 and ConfirmationHeight = 0.
2. The transaction is already confirmed, meaning the channel is no longer pending (it’s active).

In either case, we don’t need to persist the confirmation height in the database. wdyt?

So I’m going to remove the code that marks ConfirmationHeight in the database in the Confirmed case of that select.

hmm but we still want our channel struct to have the correct blockheight for this case, so I don't recommend neglecting this case. Normally it should not block because the confirmed is a buffered channel, can you be more specific which test made problems ?

Normally it should not block because the confirmed is a buffered channel, can you be more specific which test made problems

The TestFundingManagerNormalWorkflow unit test in manager_test.go is failing on this code.
I think it’s because we never send any update notifications—we only send the chainntnfs.TxConfirmation notification (So I don’t think there’s an issue with the code; we just need to update the test, and I’ll do that now).

Nit: gossiped-it => gossiped. It ...

I find this code part lacks a bit readability what about this ?

// calcRemainingConfs calculates how many more confirmations are needed for a
// pending channel to be fully confirmed. It takes into account:
// 1. The current blockchain height
// 2. The block height at which the funding transaction was first confirmed
// 3. The total number of confirmations required for the channel.
func calcRemainingConfs(pendingChan *channeldb.OpenChannel,
	currentHeight uint32) uint32 {

	// If the funding transaction hasn't been confirmed yet,
	// we need all the required confirmations
	if pendingChan.ConfirmationHeight == 0 {
		return uint32(pendingChan.NumConfsRequired)
	}

	// Calculate the target height at which the channel will be fully
	// confirmed. The -1 is because the confirmation height of the first
	// confirmation has to be taken into account.
	targetConfirmationHeight := pendingChan.ConfirmationHeight +
		uint32(pendingChan.NumConfsRequired) - 1

	// In case the current height is already past the target, return 0. This
	// should never happen because the channel should already be moved from
	// pending to open state but we handle this case in case of timing
	// issues.
	if currentHeight >= targetConfirmationHeight {
		return 0
	}

	return targetConfirmationHeight - currentHeight
}

Apologies for writing unreadable code. I’ll try to follow good practices and learn from the codebase. I’m sorry for wasting your time, especially after you shared so many code patches.

Nit: testConfirmationsUntilActiveOnPending => testPendingChannelConfirmationUntilActive verifies the value for the rpc filed ConfirmationUntilActive decreases as expected as soon as blocks are confirmed.

Nit: confirms the funding transaction but the channel should still remain pending ...

Q: Not sure if this case here makes sense, because the confirmation height is not related to the pending status directly.

See comment above.

Nit: missing point at the end.

Nit: before becoming active ...