-
Notifications
You must be signed in to change notification settings - Fork 378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address 2609 follow-up comments #2624
Conversation
Three levels of descendant transactions starting from the channel's funding transaction should cover all potential spendable outputs. The first level covers the commitment transaction. The second level covers the to_self claims, to_remote claims, second-stage HTLC claims and justice transactions. The third levels covers the justice transactions on second-stage HTLCs, and to_self claims on second-stage HTLCs.
1f9ede3
to
901571a
Compare
Otherwise, we could give users a descriptor ahead of time that will result in an invalid transaction spend/broadcast.
901571a
to
f267a30
Compare
Codecov ReportAll modified lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2624 +/- ##
==========================================
- Coverage 89.03% 88.97% -0.06%
==========================================
Files 112 112
Lines 86351 86358 +7
Branches 86351 86358 +7
==========================================
- Hits 76879 76836 -43
- Misses 7235 7281 +46
- Partials 2237 2241 +4
☔ View full report in Codecov by Sentry. |
@@ -1680,8 +1682,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> { | |||
/// missed/unhandled descriptors. For the purpose of gathering historical records, if the | |||
/// channel close has fully resolved (i.e., [`ChannelMonitor::get_claimable_balances`] returns | |||
/// an empty set), you can retrieve all spendable outputs by providing all descendant spending | |||
/// transactions starting from the channel's funding or closing transaction that have at least | |||
/// [`ANTI_REORG_DELAY`] confirmations. | |||
/// transactions starting from the channel's funding transaction and going down three levels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, do the three levels here mean looking at these txns?
- Funding tx
- Closing tx
- Tx spending the closing tx outputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The funding transaction will never match. It's the closing transaction, all spends of the closing transaction, and all spends of those which spend the closing transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense that the funding transaction isn't needed. Why is the third level needed? Isn't the closing transaction and all spends of the closing transaction enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because HTLCs are resolved in two stages when you broadcast your own commitment transaction. The first spends the HTLC output in the commitment transaction via the timeout or preimage path. The second spends the output in that timeout/preimage transaction after a CSV delay to give the counterparty a chance to claim it instead if the HTLC came from a revoked commitment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's right, there could be the revocation tx as well!
So this scenario would be:
- Commitment tx (closing tx)
- Revocation tx (spends the commitment tx)
- Tx spending the revocation tx
We need the third layer because we want to make sure that the output is spent right?
Thanks for the speedy reply btw 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have LN-related outputs to claim within each of those transactions. Any spends after that will be from your wallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because HTLCs are resolved in two stages when you broadcast your own commitment transaction.
I didn't fully grok the two stage HTLC protocol until now. The way I look at it, I still only see two layers that we need to check. I.e
- Commitment Tx
- HTLC-timeout Tx (which contains the LDK spendable output)
![Screenshot 2023-10-30 at 2 15 57 PM](https://private-user-images.githubusercontent.com/5805570/280112045-c696e484-ddfb-4832-a6dd-8cf47965c78a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwMTQ1NzUsIm5iZiI6MTczOTAxNDI3NSwicGF0aCI6Ii81ODA1NTcwLzI4MDExMjA0NS1jNjk2ZTQ4NC1kZGZiLTQ4MzItYTZkZC04Y2Y0Nzk2NWM3OGEucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDhUMTEzMTE1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZWM2N2Q5MDEyNGIxZTEwMTVjMmEyYmJlZWMwYTdiMzhlNWE4MmRiMzU1NzkwMWE3NGI0YWY0MmNjYjI5ZGZkMSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.dpuHv05IK5VxDc5nb0eG5tMO8zQOQjmi2gaD3ypR55o)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the HTLC-timeout/success transaction is from a revoked commitment, then LDK will claim it with a new transaction via the revocation path, yielding another SpendableOutputDescriptor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, that makes sense
Fixes #2619.