add-close-confirmation-info#10509
add-close-confirmation-info#10509ziggie1984 wants to merge 4 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello @ziggie1984, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the channel closure mechanism to enhance its resilience against blockchain reorganizations and provide more transparent status updates to users. It introduces dynamic confirmation scaling for cooperative closes, where the number of required confirmations is now proportional to the channel's value, offering a better balance between security and user experience. A state-machine-based approach is implemented in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Looks like this: 
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a more robust confirmation handling mechanism for channel closures, particularly for cooperative closes, to better withstand chain reorganizations. This is achieved by implementing a state machine in the chain_watcher to track spend confirmations and handle reorgs gracefully. The number of confirmations required for a cooperative close is now scaled based on channel capacity, with a minimum of 3 confirmations in production for reorg safety.
The changes also include:
- A new
CloseConfirmationHeightfield in the channel database to track the confirmation height of the closing transaction. - A new
blocks_til_closedfield in thePendingChannelsRPC response to show the remaining confirmations until a channel is fully closed. - An increase of
MinCLTVDeltafrom 18 to 24 blocks for a larger safety margin. - Extensive new tests, including property-based tests, to validate the new reorg handling logic.
The code is well-structured, thoroughly tested, and the new logic for handling reorgs is a significant improvement in robustness. I have a couple of minor comments regarding a documentation inconsistency and a style guide violation.
itest/lnd_coop_close_rbf_test.go
Outdated
| // confirmations until the channel is considered fully closed. It also tests | ||
| // that the field resets correctly after a chain reorg removes the closing | ||
| // transaction's confirmations. | ||
| func testWaitingCloseBlocksTilClosed(ht *lntest.HarnessTest) { |
There was a problem hiding this comment.
Alternatively can add this assertion to the existing test?
There was a problem hiding this comment.
yes deleted the separated tests and added it to the existent ones.
…mation This commit adds a new CloseConfirmationHeight field to the OpenChannel struct which records the block height at which the closing transaction was first confirmed. This is stored using TLV encoding (TlvType9) for backwards compatibility. A new MarkCloseConfirmationHeight method is added to persist this value, which can be called when the closing tx confirms and also supports updates in case of chain reorgs.
ziggie1984
force-pushed
the
cnct-reorg-v2
branch
2 times, most recently
from
c064bad to
2628c3e
Compare
Update the chain watcher to set and reset the CloseConfirmationHeight field when monitoring a channel close. When a spend is detected, we record the spending height so users can see remaining confirmations. When a reorg removes the close tx from the chain, we reset the height to 0 to reflect that the transaction is no longer confirmed.
Add a new field blocks_til_closed to the WaitingCloseChannel message in PendingChannels RPC response. This shows users how many more blocks until their waiting close channel will be fully closed and removed. The required confirmations are determined by CloseConfsForCapacity which scales based on channel capacity for reorg safety. If the close tx is not yet confirmed, the full required confirmations are shown.
Add an integration test that verifies the blocks_til_closed field in the WaitingCloseChannel RPC response. The test covers: 1. Initial state: shows full required confirmations when tx unconfirmed 2. Countdown: decrements as blocks are mined 3. Reorg handling: resets to full confirmations when close tx is reorged out of the chain 4. Recovery: countdown resumes correctly after close tx is re-mined
ziggie1984
force-pushed
the
cnct-reorg-v2
branch
from
2628c3e to
47f0539
Compare
| reorg safety factor which is determined by the channel capacity. As | ||
| long as the closing transaction is not confirmed, this value will be | ||
| the number of confirmations required to consider the channel fully | ||
| closed. |
There was a problem hiding this comment.
we should run make rpc again as the comment here is inconsistent with lightning.swagger.json and lightning.pb.go: "Will be 0 if the closing transaction is not yet confirmed"
| @@ -162,6 +162,8 @@ func testRBFCoopCloseDisconnect(ht *lntest.HarnessTest) { | |||
| // testCoopCloseRBFWithReorg tests that when a cooperative close transaction | |||
There was a problem hiding this comment.
missing a release-note entry
|
@Roasbeef: review reminder |
5 participants
Add the close confirmation info which is now needed since channels are not immediately considered closed now.