Expose confirmation count for pending 'channel open' transactions #9677
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
NishantBansal2003
changed the title
NishantBansal2003
force-pushed
the
conf-count
branch
from
43bc2b9 to
0e4bc79
Compare
channeldb/channel_test.go
Outdated
| // TestPendingChannelConfirmation verifies that the confirmed state is updated | ||
| // upon calling MarkConfirmedScid and that Refresh updates the in-memory state | ||
| // of another OpenChannel to reflect a preceding call to MarkConfirmedScid on | ||
| // a different OpenChannel. |
There was a problem hiding this comment.
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.
rpcserver.go
Outdated
| @@ -3919,6 +3919,27 @@ func (r *rpcServer) fetchPendingOpenChannels() (pendingOpenChannels, error) { | |||
| pendingChan.BroadcastHeight() | |||
| fundingExpiryBlocks := int32(maxFundingHeight) - currentHeight | |||
|
|
|||
| // ConfirmationUntilConfirmed is the number of blocks remaining | |||
There was a problem hiding this comment.
nit: referring to the variable 'ConfirmationUntilConfirmed' but at this point of the code you are using 'remainingConfs'.
rpcserver.go
Outdated
| // confirmation height. This is calculated as distance from the | ||
| // current block height to the block height where the funding | ||
| // transaction is located + required number of confirmations. |
There was a problem hiding this comment.
The comment doesn't accurately reflect what the code does. Please review and clarify it. The code itself looks correct.
rpcserver.go
Outdated
| CommitFee: int64(localCommitment. | ||
| CommitFee), | ||
| FeePerKw: int64(localCommitment. | ||
| FeePerKw), |
There was a problem hiding this comment.
You don't need to split each one into two lines.
channeldb/channel.go
Outdated
| // MarkConfirmedScid updates the channel's ShortChannelID after the channel | ||
| // has been confirmed but before it is fully opened. |
There was a problem hiding this comment.
I think it’s more accurate to say that the channel opening tx gets one confirmation, rather than calling it a channel confirmation.
channeldb/channel_test.go
Outdated
| @@ -978,6 +978,81 @@ func TestChannelStateTransition(t *testing.T) { | |||
| require.Empty(t, fwdPkgs, "no forwarding packages should exist") | |||
| } | |||
|
|
|||
| // TestPendingChannelConfirmation verifies that the confirmed state is updated | |||
There was a problem hiding this comment.
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.
funding/manager.go
Outdated
| errorChan := make(chan error, 1) | ||
|
|
||
| // If the channel is not a zero-conf channel, we add the SCID to the | ||
| // database once the channel is confirmed but not fully opened. This |
There was a problem hiding this comment.
I think it’s more accurate to say that the channel opening tx gets one confirmation, rather than say that channel is confirmed.
funding/manager.go
Outdated
| // The fundingManager is shutting down, and will resume wait on | ||
| // startup. | ||
| return nil, ErrFundingManagerShuttingDown | ||
| // If the channel confirmation is handled successfully, |
There was a problem hiding this comment.
I think it’s more accurate to say that the channel opening tx gets one confirmation, rather than calling it a channel confirmation.
funding/manager.go
Outdated
| @@ -3075,6 +3100,87 @@ func makeFundingScript(channel *channeldb.OpenChannel) ([]byte, error) { | |||
| return input.WitnessScriptHash(multiSigScript) | |||
| } | |||
|
|
|||
| // handleChannelConfirmation manages the confirmation process of a channel's | |||
There was a problem hiding this comment.
Maybe change the name of the function to handleOpenChanTxConfirmation
lnrpc/lightning.proto
Outdated
| // The number of blocks remaining before the pending channel open | ||
| // transaction is fully confirmed. A value of 0 indicates that the | ||
| // transaction has reached the required number of confirmations. | ||
| int32 confirmation_until_confirmed = 7; |
There was a problem hiding this comment.
This variable should be named confirmations_until_active instead, since what we are tracking is channel status change from 'pending' to 'active'
lnrpc/lightning.proto
Outdated
| @@ -2847,6 +2847,11 @@ message PendingChannelsResponse { | |||
| // very likely canceled the funding and the channel will never become | |||
| // fully operational. | |||
| int32 funding_expiry_blocks = 3; | |||
|
|
|||
| // The number of blocks remaining before the pending channel open | |||
| // transaction is fully confirmed. A value of 0 indicates that the | |||
There was a problem hiding this comment.
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.
NishantBansal2003
force-pushed
the
conf-count
branch
from
0e4bc79 to
161d639
Compare
channeldb/channel_test.go
Outdated
| FundingBroadcastHeight, "broadcast height mismatch") | ||
|
|
||
| confirmedScid := lnwire.ShortChannelID{ | ||
| BlockHeight: 5, |
There was a problem hiding this comment.
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. 😉
funding/manager_test.go
Outdated
| sixConfChannel: make(chan *chainntnfs.TxConfirmation, 1), | ||
| sixConfChannel: make(chan *chainntnfs.TxConfirmation, 6), |
There was a problem hiding this comment.
Can you explain the reasoning behind this change?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, understood. I have reverted this change. PTAL
| // We send two notifications: | ||
| // 1. The first adds the SCID to the database, allowing calculation of | ||
| // the number of confirmations before the channel is fully opened. | ||
| // 2. The second marks the channel as open. | ||
| alice.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ | ||
| Tx: fundingTx, | ||
| } | ||
| bob.mockNotifier.oneConfChannel <- &chainntnfs.TxConfirmation{ | ||
| Tx: fundingTx, | ||
| } |
There was a problem hiding this comment.
It works, but this doesn't reflect the actual open channel process, right? Have you considered sending the channel confirmation after the transaction confirmation?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Tested three scenarios:
- Regular channel, default confirmations required for active is 3.
confirmations_until_activevalue inpendingchannelsresponse tracks, until the channel is active. Test pass. - Wumbo channel, default confirmations required for active is 6.
confirmations_until_activevalue inpendingchannelsresponse tracks, until the channel is active. Test pass. - Updated the node config with
bitcoin.defaultchanconfs=4.confirmations_until_activevalue inpendingchannelsresponse doesn't track. It still starts off with a value of 3 (should start with 4) and channels gets active after 3 confirmations. Test fail.
Don't think that the issue with last scenario has been introduced with this change. But it would be good to investigate further why the setting in config is not having any bearing on the default behavior.
I checked this locally and it works for me. I have also added an itest for this case. Not entirely sure, but it seems like you might have missed what So:
|
Good clarification. Tested again by updating the peer's setting and was able to get the correct values for |
Signed-off-by: Nishant Bansal <[email protected]>
This change ensures that a channel's ShortChannelID is recorded in the database once its funding transaction receives its initial confirmation. By doing so, we establish a reliable reference point to monitor the channel's progress toward the required confirmation depth. Signed-off-by: Nishant Bansal <[email protected]>
Introduce ConfirmationsUntilActive in PendingChannelsResponse_PendingChannel. This field indicates the remaining confirmations needed for the channel to become active. If the funding transaction is unconfirmed, ConfirmationsUntilActive defaults to the total required confirmations (NumConfsRequired). Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
NishantBansal2003
force-pushed
the
conf-count
branch
from
161d639 to
8a13679
Compare
|
@NishantBansal2003, remember to re-request review from reviewers when ready |
|
|
||
| // Create a pending channel that was broadcast at height 99. | ||
| const broadcastHeight = uint32(99) | ||
| channelState := createTestChannel(t, cdb, |
| require.Len(t, pendingChannels, 1, "expected one pending channel") | ||
|
|
||
| // Verify the broadcast height of the pending channel. | ||
| require.Equal(t, broadcastHeight, pendingChannels[0]. |
| TxIndex: 10, | ||
| TxPosition: 15, |
There was a problem hiding this comment.
can we leave them empty, seems like they are ot used anyways ?
| @@ -1501,6 +1501,37 @@ func (c *OpenChannel) fullSync(tx kvdb.RwTx) error { | |||
| return putOpenChannel(chanBucket, c) | |||
| } | |||
|
|
|||
| // MarkConfirmedScid updates the channel's ShortChannelID once the channel | |||
| // opening transaction receives one confirmation. | |||
| func (c *OpenChannel) MarkConfirmedScid(scid lnwire.ShortChannelID) error { | |||
There was a problem hiding this comment.
What we need to do is to split what we are currently doing in
func (c *OpenChannel) MarkAsOpen(openLoc lnwire.ShortChannelID) error {
We need to split this function into two:
-
MarkShortChannelID -
MarkAsOpen=> removing the logic where we persit the short channel id there otherwise we make it two times for the non-zeroconf-channel.
| // database once the channel opening transaction receives one | ||
| // confirmation. This enables us to calculate the number of | ||
| // confirmations before the pending channel becomes active. | ||
| if !ch.IsZeroConf() { |
There was a problem hiding this comment.
This is not necessary, for can use the function waitForFundingConfirmation to update the short channel id.
| FeePerKw: int64(localCommitment. | ||
| FeePerKw), | ||
| FundingExpiryBlocks: fundingExpiryBlocks, | ||
| ConfirmationsUntilActive: remainingConfs, |
There was a problem hiding this comment.
we should also add the confirmation height here.
| // Restart both nodes to test that the appropriate state has been | ||
| // persisted and that both nodes recover gracefully. | ||
| ht.RestartNode(alice) | ||
| ht.RestartNode(bob) |
There was a problem hiding this comment.
Also need the ensure connected directive.
| require.Equal(ht, numConfs, ht.AssertNumPendingOpenChannels(alice, | ||
| 1)[0].ConfirmationsUntilActive) |
There was a problem hiding this comment.
Nit: Not necessary this line, I mean we are mining an empty block.
|
|
||
| // ConfirmationsUntilActive field should decrease as each block is | ||
| // mined until the required number of confirmations is reached. Let's | ||
| // mine a few empty blocks and verify the value of |
There was a problem hiding this comment.
we should mine normal blocks here, because there is no tx in the mempool anyways ?
| // mined until the required number of confirmations is reached. Let's | ||
| // mine a few empty blocks and verify the value of | ||
| // ConfirmationsUntilActive at each step. | ||
| for i := int32(1); i < numConfs; i++ { |
There was a problem hiding this comment.
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
Change Description
Fixes: #9452
This PR enhances the
PendingChannelsResponse_PendingChannelstructure to include theConfirmationsUntilActivefield. This new field provides users with the exact number of confirmations required for a pending channel's transition to an active state.Changes:
Database Update:
The channel's
shortchannelIDis now persisted in the database once its funding transaction receives 1 confirmation. This ensures we have a reference point to track progress toward the required confirmation depth.API Enhancement:
Introduced the
ConfirmationsUntilActivefield in thePendingChannelsResponse_PendingChannelresponse. This field indicates the remaining confirmations needed for the channel to become active.ConfirmationsUntilActivedefaults to the total required confirmations (NumConfsRequired).Steps to Test
itest: itest have been added to validate this new functionality.
Manual Verification: To manually test:
Establish a channel between two peers without confirming the funding transaction.
Execute
lncli pendingchannelsto observe theconfirmations_until_activefield, which should display the remaining confirmations needed for the pending channel to transition to an active state.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.