Include pending HTLC's in ChannelDetails #2442
Conversation
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
67e872b
to
a31802a
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2442 +/- ##
==========================================
- Coverage 89.18% 89.11% -0.07%
==========================================
Files 115 115
Lines 93379 93522 +143
Branches 93379 93522 +143
==========================================
+ Hits 83276 83345 +69
- Misses 7575 7643 +68
- Partials 2528 2534 +6 ☔ View full report in Codecov by Sentry. |
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
a31802a
to
9ca0041
Compare
lightning/src/ln/channel.rs
Outdated
| dust_buffer_feerate * htlc_success_tx_weight(self.get_channel_type()) / 1000 | ||
| }; | ||
| let holder_dust_limit_success_sat = htlc_success_dust_limit + self.holder_dust_limit_satoshis; | ||
| for ref htlc in self.pending_inbound_htlcs.iter() { |
There was a problem hiding this comment.
I'm not sure about the rationale for not including claimed/failed holding cell HTLCs. It seems like we should include them (and indicate whether they're successful or failed), and maybe add a note in ChannelDetails::balance_msat about the considerations from the PR description, if necessary?
There was a problem hiding this comment.
Thinking about it more, I think there's two approaches we can take to pending HTLCs and balances if we want a current view independent from commitment transactions:
- Being as conservative as possible and considering as many pending HTLCs as encumbering the balance as possible. The pending HTLCs will therefore include both added holding cell HTLCs on one end and HTLCs being partially resolved on the other end. These pending HTLCs will subtract from the balance. Resolutions (claimed/failed) in the holding cell won't be considered.
- Being as aggressive as possible and considering as many HTLCs as resolved as possible and removing them from the pending HTLCs and adjusting the balance appropriately. The pending HTLCs will therefore not include HTLCs that are in a partially resolved state or for which the resolution is in the holding cell. One question here is around partially failed HTLCs where one party can still broadcast a previous state and fulfill the HTLC.
Currently, we are very close to being fully consistent with the former, conservative option, minus the inclusion of inbound-claimed HTLCs in AvailableBalances::balance_msat due to https://github.com/lightningdevkit/rust-lightning/pull/1268/files.
However, the root cause of the underflow that was the motivation of that PR does not exist anymore. At the time of the PR, the amount in send_htlc was verified against the CommitmentStats (and this commitment transaction resolves the pending HTLC in the InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) state). Currently, we're verifying against AvailableBalances::next_outbound_htlc_limit_msat which does not resolve such pending HTLCs. I copied and reran the test in #1268 from the main branch and it is not possible to send an HTLC back anymore and cause an underflow (sending the return payment fails with Cannot send more than our next-HTLC maximum). This is likely why the test has been removed.
Therefore, I want to propose the conservative approach and remove the inclusion of inbound-claimed HTLCs in AvailableBalances::balance_msat. This will also cause the sum of the reserves, inbound capacity, outbound capacity and pending HTLCs to be equal to the channel size when the reserves are met. What do you think @valentinewallace @TheBlueMatt?
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
9ca0041
to
dbcd317
Compare
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
dbcd317
to
dc6a109
Compare
wvanlint
force-pushed
the
list_pending_htlcs
branch
3 times, most recently
from
529da61
to
a9bdb45
Compare
|
After discussing offline, there were the following conclusions:
I updated the PR to expose all the information Channel has around pending HTLC's including HTLC state and holding cell state (and whether the HTLC is dust) without any further processing. Downstream users can then do debugging or any computation they would like. One implementation alternative I considered here was reusing the existing types ( |
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
a9bdb45
to
a8fd24c
Compare
lightning/src/ln/channel.rs
Outdated
| RemoteAnnouncedFail, | ||
| AwaitingRemoteRevokeToAnnounceForward, | ||
| AwaitingRemoteRevokeToAnnounceFail, | ||
| AwaitingAnnouncedRemoteRevokeForward, |
There was a problem hiding this comment.
Do you have a strong feeling on if we differentiate between the two awaiting-remote-revoke states? They're pretty different in the state machine itself, but do users really ever care whether we're waiting on the remote to go to state 2/3 or if we're in state 2/3?
There was a problem hiding this comment.
If the use cases can vary and include debugging, I was thinking to expose all states that are available. What do you think? Would there be debugging use cases around stuck HTLCs?
There was a problem hiding this comment.
Even for debugging I'm not sure there's a real difference here. Both indicate we're waiting on the peer to send an RAA for a previous state, the exact place we are in the state machine isn't all that interesting, at some point you go look at logs (or, really, once you see you're just waiting on an RAA for a while you can yell at the other peer).
There was a problem hiding this comment.
Collapsed these states where we are waiting for remote revoke_and_ack's.
| @@ -152,6 +152,50 @@ enum InboundHTLCState { | |||
| LocalRemoved(InboundHTLCRemovalReason), | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug, PartialEq)] | |||
| pub enum InboundHTLCStateDetails { | |||
There was a problem hiding this comment.
Once we make this pub we'll need to write a ton of docs - IMO each variant should have, at least, a description of whether this HTLC is included in the current local commitment transaction (ie the one signed by our peer which we can broadcast at any time) and whether it will be included in the next remote commitment transaction (ie the next thing we need to sign that determines if we can send another HTLC or not).
There was a problem hiding this comment.
Added documentation here based on InboundHTLCState and OutboundHTLCState.
lightning/src/ln/channel.rs
Outdated
| RemoteRemovedFailure, | ||
| AwaitingRemoteRevokeToRemoveSuccess, | ||
| AwaitingRemoteRevokeToRemoveFailure, | ||
| AwaitingRemovedRemoteRevokeSuccess, |
There was a problem hiding this comment.
Same here, do we want to merge the AwaitingRemoteRevoke... states?
There was a problem hiding this comment.
Collapsed these states where we are waiting for remote revoke_and_ack's.
lightning/src/ln/channel.rs
Outdated
| LocalAnnounced, | ||
| Committed, | ||
| RemoteRemovedSuccess, | ||
| RemoteRemovedFailure, |
There was a problem hiding this comment.
I wonder if we want to drop the RemoteRemoved... states - they're really just the remote side indicating that they wish to remove, but they haven't actually committed to it yet. The states shouldn't hang around long, so I dunno if they really communicate much additional information.
There was a problem hiding this comment.
Collapsed this state into Committed.
lightning/src/ln/channel.rs
Outdated
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum InboundHTLCStateDetails { | ||
| RemoteAnnouncedForward, | ||
| RemoteAnnouncedFail, |
There was a problem hiding this comment.
Same as on the outbound case - not sure if RemoteAnnounced... adds much value to expose.
There was a problem hiding this comment.
I kept this state around as otherwise the pending HTLC would be dropped.
There was a problem hiding this comment.
I'm still a bit dubious of it because I anticipate no one will ever see this - generally nodes send update_add_htlc and then commitment_signed immediately, at which point we'll have already failed this HTLC back and we'll be in another state. Still, if we keep it we need to describe it in way more detail - something about how "we intend to immediately fail the HTLC" and what cases this can arise in.
There was a problem hiding this comment.
I think I mainly wanted the state details to be a .map() of internal state instead of a .filter_map(). Added documentation around RemoteAnnounced states. Alternatively, I can collapse this into the subsequent state.
There was a problem hiding this comment.
I don't quite get why - this is a public interface that should be easy for developers to deal with, there's no specific requirement that it be 1:1 with the internal state, and not being 1:1 forces developers to handle cases that they might otherwise not care about. Can you provide a bit more detail on why you want it to be 1:1?
We'll also, now, want to drop the *Forward/*Fail distinction, I think - post #2845 it'll go away in future HTLCs, so exposing it via the public interface doesn't make much sense.
There was a problem hiding this comment.
One of the reasons for adding this PR was also observability, so I was leaning towards exposing information we have, especially since the states are caused by and are dependent on external messages. It might for example be possible to get stuck at the RemoteAnnounced state if the counterparty does not send commitment_signed due to an implementation bug.
I agree that information on announcements adds less value than the rest though and I probably should be consistent with #2442 (comment) as well, added a commit to not expose this.
Dropped the *Forward/*Fail distinction as well.
I like it the way you have it - having different public types gives us the ability to hide some nuance that users may not need to care about. |
lightning/src/ln/channel.rs
Outdated
| @@ -249,6 +371,39 @@ enum HTLCUpdateAwaitingACK { | |||
| }, | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug, PartialEq)] | |||
| pub enum HTLCUpdateAwaitingACKDetails { | |||
| AddHTLC { | |||
There was a problem hiding this comment.
IMO we should just include this in the outbound HTLC set with a different state flag.
There was a problem hiding this comment.
Moved holding cell updates as separate states into pending inbound and outbound HTLCs.
lightning/src/ln/channel.rs
Outdated
| skimmed_fee_msat: Option<u64>, | ||
| is_dust: bool, | ||
| }, | ||
| ClaimHTLC { |
There was a problem hiding this comment.
I wonder if we shouldn't just include this and fail in the inbound HTLC sets as a bool indicating our intent to remove.
There was a problem hiding this comment.
I agree, I think we can get rid of holding_cell_updates and not reference the holding cell explicitly.
There was a problem hiding this comment.
Moved holding cell updates as separate states into pending inbound and outbound HTLCs.
lightning/src/ln/channel.rs
Outdated
| skimmed_fee_msat: Option<u64>, | ||
| is_dust: bool, | ||
| }, | ||
| ClaimHTLC { |
There was a problem hiding this comment.
I agree, I think we can get rid of holding_cell_updates and not reference the holding cell explicitly.
wvanlint
force-pushed
the
list_pending_htlcs
branch
5 times, most recently
from
1a3a29f
to
d8efd56
Compare
There was a problem hiding this comment.
Mostly some comments on the docs - at a high level we try hard to avoid writing docs which assume users of LDK understand the inner workings of the lightning state machine. Also, all documentation should strive to include both a what but also a why you'd care about this and a how you'd use this information (at least for things that aren't super duper obvious).
lightning/src/ln/channel.rs
Outdated
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum InboundHTLCStateDetails { | ||
| RemoteAnnouncedForward, | ||
| RemoteAnnouncedFail, |
There was a problem hiding this comment.
I'm still a bit dubious of it because I anticipate no one will ever see this - generally nodes send update_add_htlc and then commitment_signed immediately, at which point we'll have already failed this HTLC back and we'll be in another state. Still, if we keep it we need to describe it in way more detail - something about how "we intend to immediately fail the HTLC" and what cases this can arise in.
lightning/src/ln/channel.rs
Outdated
| /// We are awaiting the appropriate revoke_and_ack's from the remote before this HTLC is included | ||
| /// on the remote commitment transaction, possibly for a prior state first. | ||
| AwaitingRemoteRevokeToAddForward, | ||
| /// See above. |
There was a problem hiding this comment.
Same here, at least the documentation part. I think I agree that we should keep this one, because it at least sticks around longer that people might see it, but if the documentation is "see above" that probably implies we shouldn't have it - we're telling users to handle it the same as the other case :).
There was a problem hiding this comment.
Added documentation on this state around the intent to fail the HTLC.
lightning/src/ln/channel.rs
Outdated
| /// transactions, but we are awaiting the appropriate revoke_and_ack's to remove this HTLC from | ||
| /// the remote commitment transaction, possibly for a prior state first. | ||
| AwaitingRemoteRevokeToRemoveFulfill, | ||
| /// See above. |
There was a problem hiding this comment.
Same here, on the documentation end.
There was a problem hiding this comment.
Added documentation on this state around the intent to fail the HTLC.
lightning/src/ln/channel.rs
Outdated
| /// See above. | ||
| AwaitingRemoteRevokeToAddFail, | ||
| /// Committed indicates that this HTLC has been included in the commitment_signed and | ||
| /// revoke_and_ack flow on both sides and is included in both commitment transactions. |
There was a problem hiding this comment.
Can we mention something about how the HTLC may have been forwarded to another channel, or may be awaiting such forwarding or claim by us, or other parts for an MPP payment?
lightning/src/ln/channel.rs
Outdated
| /// Exposes details around pending inbound HTLCs. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct InboundHTLCDetails { | ||
| /// The corresponding HTLC ID. |
There was a problem hiding this comment.
We should describe how someone might use this - something about once its past RemoteAnnounced* this is unique for this channel and no further inbound HTLCs will have the same ID. If an HTLC only appears as announced we may have a collision, and outbound HTLCs have a separate ID namespace.
There was a problem hiding this comment.
Added. I think this will be useful as well if there are ever HTLC-specific (analytics) events that can be correlated by HTLC id.
| /// The state of the HTLC in the update_*_htlc, commitment_signed, revoke_and_ack flow. | ||
| /// Informs on which commitment transactions the HTLC is included. | ||
| pub state: InboundHTLCStateDetails, | ||
| /// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed |
There was a problem hiding this comment.
Something about burning to fee on closure?
lightning/src/ln/channel.rs
Outdated
| pub cltv_expiry: u32, | ||
| /// The payment hash. | ||
| pub payment_hash: PaymentHash, | ||
| /// The state of the HTLC in the update_*_htlc, commitment_signed, revoke_and_ack flow. |
There was a problem hiding this comment.
Should we just say something about the HTLC's state machine? I think this is the only place in the public documentation that we mention the state machine in terms of the messages being exchanged.
There was a problem hiding this comment.
I described the state machine in more detail in InboundHTLCStateDetails and OutboundHTLCStateDetails and left a reference here.
| @@ -152,6 +152,72 @@ enum InboundHTLCState { | |||
| LocalRemoved(InboundHTLCRemovalReason), | |||
| } | |||
|
|
|||
| /// Exposes the state of pending inbound HTLCs. | |||
| /// | |||
There was a problem hiding this comment.
Can we get a brief description of the HTLC state machine at a high level? I dont think we have anywhere else in the API which describes it, and we generally try to avoid APIs which assume knowledge of how lightning works under the hood.
lightning/src/ln/channel.rs
Outdated
| AwaitingRemoteRevokeToAdd, | ||
| /// The Committed state indicates that this HTLC is included on the remote's commitment | ||
| /// transaction. This includes the subsequent state where we include it in the local commitment | ||
| /// transaction, as: |
There was a problem hiding this comment.
This part is super confusing to me - I can figure out what you mean by reading the code (its a reference to the remote party having indicated intent to remove the HTLC, but not having yet committed to it), but its too verbose and assumes some level of knowing the HTLC state machine (to figure out what "the subsequent state" is).
There was a problem hiding this comment.
I trimmed this down. It was mainly copied from
rust-lightning/lightning/src/ln/channel.rs
Lines 171 to 182 in 6b0ba8c
lightning/src/ln/channel.rs
Outdated
| /// will be removed from the remote's commitmen transaction as well, possibly for a prior state | ||
| /// first. | ||
| AwaitingRemoteRevokeToRemoveSuccess, | ||
| /// See above. |
There was a problem hiding this comment.
Please at least copy the docs - not every language lists these things in order, and eg in an IDE you may load the documentation on the specific variant and be kinda lost.
|
Is this still on your radar @wvanlint? |
|
@wpaulino Sorry, it dropped off my priority list for a bit while I was thinking about how to reword the documentation to not assume knowledge from the user and still be precise. I would like to wrap this up but I might not be able to get back to it in the short-term. Is this a blocker for anything? |
Nope, just wondering :) |
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
d8efd56
to
7a44af3
Compare
|
Warning Rate Limit Exceeded@wvanlint has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 33 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent modifications enhance the Lightning network's channel management by introducing detailed tracking and management of pending inbound and outbound Hashed Time-Locked Contracts (HTLCs). This includes the establishment of new enums and structs to represent the states and details of HTLCs, alongside updates to serialization and testing frameworks to accommodate these changes. These enhancements aim to provide clearer insights into HTLC states, improving the network's reliability and diagnostic capabilities. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- fuzz/src/router.rs (1 hunks)
- lightning/src/ln/channel.rs (5 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/routing/router.rs (2 hunks)
Additional comments: 22
fuzz/src/router.rs (1)
- 245-246: Initialization of
pending_inbound_htlcsandpending_outbound_htlcsas empty vectors is correct and aligns with the PR's objective to provide more granular information about the state of HTLCs.lightning/src/routing/router.rs (2)
- 3288-3289: Initialization of
pending_inbound_htlcsandpending_outbound_htlcsas empty vectors is correct and aligns with the PR's objective to track pending HTLCs without altering existing balances.- 8431-8432: The addition of
pending_inbound_htlcsandpending_outbound_htlcsto theRouterstruct in this context is consistent with the previous hunk and the PR's objectives.lightning/src/ln/channel.rs (13)
- 161-175: The documentation provided for
InboundHTLCStateDetailsis comprehensive and aligns with the previous comments about needing a high-level description of the HTLC state machine. This should help users understand the lifecycle of an HTLC.- 176-220: The enum
InboundHTLCStateDetailsincludes detailed comments for each variant, which is good for clarity. However, ensure that the descriptions are accurate and reflect the actual behavior of the code. The comments from TheBlueMatt suggest that some states may not be seen often, such asRemoteAnnounced..., and if kept, they need detailed descriptions. Verify that these descriptions have been added and are sufficient.- 222-247: The
Fromtrait implementation forInboundHTLCStateDetailslooks correct and follows Rust conventions. It's good to see that the conversion logic is encapsulated in a way that keeps the code clean and maintainable.- 249-257: The
impl_writeable_tlv_based_enum!macro usage forInboundHTLCStateDetailsis appropriate and follows the existing codebase patterns. Ensure that the TLV fields are correctly mapped and that no additional data is required for each state.- 267-294: The
InboundHTLCDetailsstruct is well-documented, with clear descriptions for each field. The inclusion ofis_dustand its implications on the commitment transaction fee is particularly important for users to understand the financial consequences of channel operations.- 296-303: The
impl_writeable_tlv_based!macro forInboundHTLCDetailsseems correct. Ensure that all required fields are present and that the TLV mapping is accurate.- 338-372: The documentation for
OutboundHTLCStateDetailsis thorough and mirrors the structure provided for inbound HTLCs. This consistency is good for maintainability and understanding of the code. Ensure that the state descriptions are accurate and complete.- 374-395: The
Fromtrait implementation forOutboundHTLCStateDetailsis consistent with the inbound counterpart and follows Rust best practices. The handling of transient states is noted, which is important for understanding the state transitions.- 397-402: The
impl_writeable_tlv_based_enum!macro forOutboundHTLCStateDetailsis used correctly. Confirm that the TLV fields are correctly mapped and that no additional data is required for each state.- 442-473: The
OutboundHTLCDetailsstruct is well-documented, and the inclusion ofskimmed_fee_msatis a good addition for transparency in fee handling. Theis_dustfield is also important for understanding the financial implications of channel operations.- 475-483: The
impl_writeable_tlv_based!macro forOutboundHTLCDetailsappears correct. Ensure that all required fields are present and that the TLV mapping is accurate.- 2214-2253: The method
get_pending_inbound_htlc_detailsis well-implemented, taking into account holding cell updates and dust limits. The logic for determiningis_dustis clear and should work as intended. Ensure that the calculation of dust limits is correct and consistent with the rest of the codebase.Verification successful
The
rgcommand has provided the context around the calculation ofholder_dust_limit_success_satin theget_pending_inbound_htlc_detailsmethod. The calculation is consistent with the rest of the codebase. The variableholder_dust_limit_success_satis computed based onhtlc_success_dust_limitandself.holder_dust_limit_satoshis. Thehtlc_success_dust_limitis calculated based on thedust_buffer_feerateandself.get_channel_type(). Theholder_dust_limit_success_satis then used to determine whether an HTLC is considered dust.The calculation of
holder_dust_limit_success_satis consistent with the rest of the codebase and should work as intended. Therefore, the logic for determiningis_dustin theget_pending_inbound_htlc_detailsmethod is correct and consistent with the rest of the codebase.The verification has provided the necessary information to confirm that the calculation of dust limits is correct and consistent with the rest of the codebase.
Conclusion
The calculation of dust limits in the
get_pending_inbound_htlc_detailsmethod is consistent with the rest of the codebase. The logic for determiningis_dustshould work as intended.* 2255-2296: The method `get_pending_outbound_htlc_details` mirrors the inbound method and handles holding cell updates appropriately. The calculation of `is_dust` is consistent with the inbound method, which is good for maintainability. Confirm that the dust limit calculation is correct and consistent.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the calculation of dust limits in get_pending_inbound_htlc_details is consistent with the rest of the codebase. rg --type rust --context 5 'holder_dust_limit_success_sat'Length of output: 2640
Verification successful
The
rgcommand has provided the context around the calculation ofholder_dust_limit_timeout_satin theget_pending_outbound_htlc_detailsmethod. The calculation ofholder_dust_limit_timeout_satis consistent with the rest of the codebase. It's derived fromhtlc_timeout_dust_limitandself.holder_dust_limit_satoshis, and it's used to determine whether an HTLC is considered dust.The calculation is based on the amount of the HTLC and the
holder_dust_limit_timeout_sat. If the amount of the HTLC is less thanholder_dust_limit_timeout_sat, it's considered dust. The same calculation is used consistently throughout the method, ensuring maintainability and consistency.The verification confirms that the dust limit calculation is correct and consistent with the rest of the codebase.
Conclusion
The calculation of the dust limit in the
get_pending_outbound_htlc_detailsmethod is consistent with the rest of the codebase. Therefore, the review comment has been verified and the code is correct.Verifying the review comment
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the calculation of dust limits in get_pending_outbound_htlc_details is consistent with the rest of the codebase. rg --type rust --context 5 'holder_dust_limit_timeout_sat'Length of output: 4186
lightning/src/ln/channelmanager.rs (6)
- 47-47: The re-export of
InboundHTLCDetails,InboundHTLCStateDetails,OutboundHTLCDetails, andOutboundHTLCStateDetailsis consistent with the PR's objective to make these types publicly available. This change aligns with the previous discussion and the decision to use re-exports.- 1797-1804: The addition of
pending_inbound_htlcsandpending_outbound_htlcsto theChannelDetailsstruct is a significant change. It's important to ensure that all serialization and deserialization paths handle these new fields correctly, especially with the versioning comments indicating compatibility with specific LDK versions.- 1882-1883: The implementation of
get_pending_inbound_htlc_detailsandget_pending_outbound_htlc_detailsmethods in the context object is correct and aligns with the PR's objectives. Ensure that these methods are covered by unit tests to validate their behavior.- 9432-9433: The serialization logic for
pending_inbound_htlcsandpending_outbound_htlcsusingoptional_vecis correct. It's important to ensure that the serialization logic is backward compatible and that there are tests to verify the serialization and deserialization processes.Verification successful
The shell script output indicates that the serialization and deserialization tests for the new fields
pending_inbound_htlcsandpending_outbound_htlcsare present in the following files:
lightning/src/ln/channelmanager.rslightning/src/ln/channel.rsThe presence of these tests suggests that the serialization and deserialization processes for the new fields have been verified. Therefore, the changes in the serialization logic for
pending_inbound_htlcsandpending_outbound_htlcsusingoptional_vecare covered by the tests.The verification has provided evidence that the serialization and deserialization tests include the new fields, confirming that the serialization logic is backward compatible and that there are tests to verify the serialization and deserialization processes.
Conclusion
The verification has been successful, and the serialization and deserialization tests for the new fields are present in the relevant files.
* 9472-9473: The deserialization logic for `pending_inbound_htlcs` and `pending_outbound_htlcs` is present. Ensure that there are tests for deserialization that include scenarios where these fields are not present, to maintain backward compatibility. * 9510-9511: Defaulting `pending_inbound_htlcs` and `pending_outbound_htlcs` to empty vectors if they are not present in the serialized data is a good approach for backward compatibility. Confirm that this behavior is tested, especially for data serialized with older versions of LDK.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that serialization and deserialization tests include the new fields. rg --type rust 'optional_vec' --files-with-matches | xargs rg --type rust '#\[test\]'Length of output: 6195
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
7a44af3
to
55335f1
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- fuzz/src/router.rs (1 hunks)
- lightning/src/ln/channel.rs (5 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/routing/router.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- fuzz/src/router.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/routing/router.rs
wvanlint
force-pushed
the
list_pending_htlcs
branch
3 times, most recently
from
7a89973
to
44920ff
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- fuzz/src/router.rs (1 hunks)
- lightning/src/ln/channel.rs (5 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/routing/router.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- fuzz/src/router.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/routing/router.rs
|
Apologies for the delay here. Observability into pending HTLCs came up recently again, so I will be picking this back up. Added more documentation throughout. |
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
44920ff
to
58e8e16
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- fuzz/src/router.rs (1 hunks)
- lightning/src/ln/channel.rs (5 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/routing/router.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- fuzz/src/router.rs
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/routing/router.rs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/channel.rs (5 hunks)
Additional comments: 4
lightning/src/ln/channel.rs (4)
- 161-198: The documentation and implementation of
InboundHTLCStateDetailsprovide a clear and detailed explanation of the HTLC state machine, which is crucial for understanding the lifecycle of HTLCs. However, ensure that the documentation is kept up-to-date with any future changes to the HTLC state machine to prevent confusion.- 235-261: The
InboundHTLCDetailsstruct is well-defined, with clear documentation for each field. This struct will be useful for downstream users to get detailed information about inbound HTLCs. Ensure that any changes to the HTLC lifecycle are reflected in the state field to maintain the accuracy of the information provided.- 306-339: Similar to the inbound HTLCs, the
OutboundHTLCStateDetailsenum is well-documented and provides a comprehensive view of the outbound HTLC state machine. This consistency in handling both inbound and outbound HTLCs is good practice. Keep the documentation updated with any changes to the outbound HTLC state machine.- 410-450: The
OutboundHTLCDetailsstruct is well-structured and documented, providing essential information about outbound HTLCs. The optionalhtlc_idfield and the inclusion ofskimmed_fee_msatoffer a nuanced view of the HTLCs, which is valuable for debugging and analysis. Ensure that the handling ofNonevalues forhtlc_idis consistent and well-documented in the codebase.
lightning/src/ln/channel.rs
Outdated
| impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> { | ||
| fn from(state: &InboundHTLCState) -> Option<InboundHTLCStateDetails> { | ||
| match state { | ||
| InboundHTLCState::RemoteAnnounced(_) => None, | ||
| InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => | ||
| Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd), | ||
| InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => | ||
| Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd), | ||
| InboundHTLCState::Committed => | ||
| Some(InboundHTLCStateDetails::Committed), | ||
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) => | ||
| Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail), | ||
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed(_)) => | ||
| Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail), | ||
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) => | ||
| Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl_writeable_tlv_based_enum!(InboundHTLCStateDetails, | ||
| (0, AwaitingRemoteRevokeToAdd) => {}, | ||
| (2, Committed) => {}, | ||
| (4, AwaitingRemoteRevokeToRemoveFulfill) => {}, | ||
| (6, AwaitingRemoteRevokeToRemoveFail) => {}; | ||
| ); |
There was a problem hiding this comment.
The implementation of From<&InboundHTLCState> for Option<InboundHTLCStateDetails> is logically sound, providing a clear mapping from InboundHTLCState to InboundHTLCStateDetails. This abstraction allows for easier handling of HTLC states in the higher-level logic. Consider adding unit tests to cover all possible state transitions to ensure the correctness of this mapping.
Would you like me to help with adding unit tests for this conversion?
lightning/src/ln/channel.rs
Outdated
| impl From<&OutboundHTLCState> for OutboundHTLCStateDetails { | ||
| fn from(state: &OutboundHTLCState) -> OutboundHTLCStateDetails { | ||
| match state { | ||
| OutboundHTLCState::LocalAnnounced(_) => | ||
| OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd, | ||
| OutboundHTLCState::Committed => | ||
| OutboundHTLCStateDetails::Committed, | ||
| // RemoteRemoved states are ignored as the state is transient and the remote has not committed to | ||
| // the state yet. | ||
| OutboundHTLCState::RemoteRemoved(_) => | ||
| OutboundHTLCStateDetails::Committed, | ||
| OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) => | ||
| OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess, | ||
| OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Failure(_)) => | ||
| OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFailure, | ||
| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) => | ||
| OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess, | ||
| OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Failure(_)) => | ||
| OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFailure, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl_writeable_tlv_based_enum!(OutboundHTLCStateDetails, | ||
| (0, AwaitingRemoteRevokeToAdd) => {}, | ||
| (2, Committed) => {}, | ||
| (4, AwaitingRemoteRevokeToRemoveSuccess) => {}, | ||
| (6, AwaitingRemoteRevokeToRemoveFailure) => {}; |
There was a problem hiding this comment.
The conversion from OutboundHTLCState to OutboundHTLCStateDetails is implemented correctly. As with the inbound HTLC states, consider adding unit tests for these conversions to ensure their correctness across all possible state transitions.
Would you like assistance in creating unit tests for these state conversions?
lightning/src/ln/channel.rs
Outdated
| /// Returns information on all pending inbound HTLCs. | ||
| pub fn get_pending_inbound_htlc_details(&self) -> Vec<InboundHTLCDetails> { | ||
| let mut holding_cell_states = HashMap::new(); | ||
| for holding_cell_update in self.holding_cell_htlc_updates.iter() { | ||
| match holding_cell_update { | ||
| HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { | ||
| holding_cell_states.insert( | ||
| htlc_id, | ||
| InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill, | ||
| ); | ||
| }, | ||
| HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => { | ||
| holding_cell_states.insert( | ||
| htlc_id, | ||
| InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail, | ||
| ); | ||
| }, | ||
| HTLCUpdateAwaitingACK::FailMalformedHTLC { htlc_id, .. } => { | ||
| holding_cell_states.insert( | ||
| htlc_id, | ||
| InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail, | ||
| ); | ||
| }, | ||
| _ => {}, | ||
| } | ||
| } | ||
| let mut inbound_details = Vec::new(); | ||
| let htlc_success_dust_limit = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { | ||
| 0 | ||
| } else { | ||
| let dust_buffer_feerate = self.get_dust_buffer_feerate(None) as u64; | ||
| dust_buffer_feerate * htlc_success_tx_weight(self.get_channel_type()) / 1000 | ||
| }; | ||
| let holder_dust_limit_success_sat = htlc_success_dust_limit + self.holder_dust_limit_satoshis; | ||
| for htlc in self.pending_inbound_htlcs.iter() { | ||
| if let Some(state_details) = (&htlc.state).into() { | ||
| inbound_details.push(InboundHTLCDetails{ | ||
| htlc_id: htlc.htlc_id, | ||
| amount_msat: htlc.amount_msat, | ||
| cltv_expiry: htlc.cltv_expiry, | ||
| payment_hash: htlc.payment_hash, | ||
| state: holding_cell_states.remove(&htlc.htlc_id).unwrap_or(state_details), | ||
| is_dust: htlc.amount_msat / 1000 < holder_dust_limit_success_sat, | ||
| }); | ||
| } | ||
| } | ||
| inbound_details | ||
| } | ||
|
|
||
| /// Returns information on all pending outbound HTLCs. | ||
| pub fn get_pending_outbound_htlc_details(&self) -> Vec<OutboundHTLCDetails> { | ||
| let mut outbound_details = Vec::new(); | ||
| let htlc_timeout_dust_limit = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() { | ||
| 0 | ||
| } else { | ||
| let dust_buffer_feerate = self.get_dust_buffer_feerate(None) as u64; | ||
| dust_buffer_feerate * htlc_success_tx_weight(self.get_channel_type()) / 1000 | ||
| }; | ||
| let holder_dust_limit_timeout_sat = htlc_timeout_dust_limit + self.holder_dust_limit_satoshis; | ||
| for htlc in self.pending_outbound_htlcs.iter() { | ||
| outbound_details.push(OutboundHTLCDetails{ | ||
| htlc_id: Some(htlc.htlc_id), | ||
| amount_msat: htlc.amount_msat, | ||
| cltv_expiry: htlc.cltv_expiry, | ||
| payment_hash: htlc.payment_hash, | ||
| skimmed_fee_msat: htlc.skimmed_fee_msat, | ||
| state: (&htlc.state).into(), | ||
| is_dust: htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat, | ||
| }); | ||
| } | ||
| for holding_cell_update in self.holding_cell_htlc_updates.iter() { | ||
| if let HTLCUpdateAwaitingACK::AddHTLC { | ||
| amount_msat, | ||
| cltv_expiry, | ||
| payment_hash, | ||
| skimmed_fee_msat, | ||
| .. | ||
| } = *holding_cell_update { | ||
| outbound_details.push(OutboundHTLCDetails{ | ||
| htlc_id: None, | ||
| amount_msat: amount_msat, | ||
| cltv_expiry: cltv_expiry, | ||
| payment_hash: payment_hash, | ||
| skimmed_fee_msat: skimmed_fee_msat, | ||
| state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd, | ||
| is_dust: amount_msat / 1000 < holder_dust_limit_timeout_sat, | ||
| }); | ||
| } | ||
| } | ||
| outbound_details | ||
| } |
There was a problem hiding this comment.
The methods get_pending_inbound_htlc_details and get_pending_outbound_htlc_details are crucial for retrieving detailed information about pending HTLCs. The logic appears sound, but it's important to ensure that these methods are covered by comprehensive tests, especially to verify the correct handling of dust limits and the mapping of HTLC states.
Would you like me to help with adding tests for these methods to ensure they handle all edge cases correctly?
lightning/src/ln/channel.rs
Outdated
| /// The state of the HTLC in the state machine. | ||
| /// Determines on which commitment transactions the HTLC is included and what message the HTLC is | ||
| /// waiting for to advance to the next state. | ||
| /// See [InboundHTLCStateDetails] for information on the specific states. |
There was a problem hiding this comment.
nit
| /// See [InboundHTLCStateDetails] for information on the specific states. | |
| /// See [`InboundHTLCStateDetails`] for information on the specific states. |
lightning/src/ln/channel.rs
Outdated
| pub state: InboundHTLCStateDetails, | ||
| /// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed | ||
| /// from the local commitment transaction and added to the commitment transaction fee. | ||
| /// This takes into account the second-stage HTLC transactions as well. |
There was a problem hiding this comment.
| /// This takes into account the second-stage HTLC transactions as well. | |
| /// For non-anchor channels, this takes into account the cost of the second-stage HTLC transactions as well. |
lightning/src/ln/channel.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl_writeable_tlv_based_enum!(InboundHTLCStateDetails, |
There was a problem hiding this comment.
Lets use the upgradeable version for all the new serialization.
| impl_writeable_tlv_based_enum!(InboundHTLCStateDetails, | |
| impl_writeable_tlv_based_enum_upgradable!(InboundHTLCStateDetails, |
There was a problem hiding this comment.
Done. This changes the field into Option<_> right?
lightning/src/ln/channel.rs
Outdated
| /// This can be used to inspect what next message an HTLC is waiting for to advance its state. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum OutboundHTLCStateDetails { | ||
| /// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is be added |
There was a problem hiding this comment.
| /// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is be added | |
| /// We are awaiting the appropriate revoke_and_ack's from the remote before the HTLC is added |
lightning/src/ln/channel.rs
Outdated
| /// and we removed the HTLC from our commitment transaction through a commitment_signed and | ||
| /// revoke_and_ack exchange. We are awaiting the appropriate revoke_and_ack's from the remote for |
There was a problem hiding this comment.
Here and below, we might not actually have sent the CS yet, if we're waiting on the remote to revoke their latest state.
| /// and we removed the HTLC from our commitment transaction through a commitment_signed and | |
| /// revoke_and_ack exchange. We are awaiting the appropriate revoke_and_ack's from the remote for | |
| /// and we removed the HTLC from our commitment transaction through a revoke_and_ack. | |
| /// We are awaiting the appropriate revoke_and_ack's from the remote for |
There was a problem hiding this comment.
Ah, the exchange I mentioned here is receiving the commitment_signed message from the counterparty and returning the revoke_and_ack message. Added clarification by explicitly mentioning sending and receiving.
lightning/src/ln/channel.rs
Outdated
| /// The state of the HTLC in the state machine. | ||
| /// Determines on which commitment transactions the HTLC is included and what message the HTLC is | ||
| /// waiting for to advance to the next state. | ||
| /// See [OutboundHTLCStateDetails] for information on the specific states. |
There was a problem hiding this comment.
nit
| /// See [OutboundHTLCStateDetails] for information on the specific states. | |
| /// See [`OutboundHTLCStateDetails`] for information on the specific states. |
lightning/src/ln/channel.rs
Outdated
| pub skimmed_fee_msat: Option<u64>, | ||
| /// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed | ||
| /// from the local commitment transaction and added to the commitment transaction fee. | ||
| /// This takes into account the second-stage HTLC transactions as well. |
There was a problem hiding this comment.
| /// This takes into account the second-stage HTLC transactions as well. | |
| /// For non-anchor channels, this takes into account the cost of the second-stage HTLC transactions as well. |
lightning/src/ln/channel.rs
Outdated
| InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail, | ||
| ); | ||
| }, | ||
| _ => {}, |
There was a problem hiding this comment.
nit: make the ignored branch explicit with HTLCUpdateAwaitingACK::AddHTLC given there's only one option left and its nice to force ourselves to check the code if we ever add a new variant.
| /// Whether the HTLC has an output below the local dust limit. If so, the output will be trimmed | ||
| /// from the local commitment transaction and added to the commitment transaction fee. | ||
| /// This takes into account the second-stage HTLC transactions as well. | ||
| /// |
There was a problem hiding this comment.
Is it worth noting here (and in the inbound field for the same) that an HTLC could be dust for our commitment tx but not our counterparty's commitment tx or vice versa?
There was a problem hiding this comment.
Added a note that this is specific to each party for the inbound and outbound case.
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
98df707
to
bb85b33
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- fuzz/src/router.rs (1 hunks)
- lightning/src/ln/channel.rs (5 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/routing/router.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/channel.rs
Additional comments: 7
fuzz/src/router.rs (1)
- 245-246: Initialization of
pending_inbound_htlcsandpending_outbound_htlcsas empty vectors aligns with the PR's objective to enhanceChannelDetailsfunctionality. This change is necessary for the fuzz test to compile and run successfully after the modifications toChannelDetails. Ensure that all other necessary initializations or adjustments related to these new fields are also made throughout the test suite to maintain consistency.lightning/src/routing/router.rs (2)
- 3288-3289: Initialization of
pending_inbound_htlcsandpending_outbound_htlcswith empty vectors is correct and follows Rust conventions for initializing collections. This change aligns with the PR's objective to enhanceChannelDetailsfunctionality by including details about pending HTLCs.- 8431-8432: Similar to the previous comment, the addition of
pending_inbound_htlcsandpending_outbound_htlcsfields with empty vector initializations in this context is appropriate. It ensures that the new fields are accounted for in struct initializations within both test and benchmark setups.lightning/src/ln/channelmanager.rs (4)
- 1890-1891: Correctly initializes
pending_inbound_htlcsandpending_outbound_htlcsin theChannelDetailsconstructor. Ensure that the methodsget_pending_inbound_htlc_detailsandget_pending_outbound_htlc_detailsare implemented efficiently and correctly.- 9474-9475: The serialization of
pending_inbound_htlcsandpending_outbound_htlcsusesoptional_vec, which is appropriate for optional fields. Verify that this approach maintains backward compatibility and correctly handles empty vectors.- 9514-9515: Similar to serialization, deserialization of
pending_inbound_htlcsandpending_outbound_htlcsusesoptional_vec. Ensure deserialization logic correctly handles cases where these fields are not present in older serialized data.- 9552-9553: Properly handles the case where
pending_inbound_htlcsandpending_outbound_htlcsare not present in the serialized data by defaulting to empty vectors. This ensures backward compatibility with data serialized by older versions.
| @@ -44,6 +44,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa | |||
| // construct one themselves. | |||
| use crate::ln::{inbound_payment, ChannelId, PaymentHash, PaymentPreimage, PaymentSecret}; | |||
| use crate::ln::channel::{self, Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext}; | |||
| pub use crate::ln::channel::{InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails}; | |||
There was a problem hiding this comment.
Re-exporting types from a module that is not publicly accessible outside the crate could lead to confusion. Consider moving these types to a more appropriate location if they are intended for public use.
| /// Pending inbound HTLCs. | ||
| /// | ||
| /// This field is empty for objects serialized with LDK versions prior to 0.0.122. | ||
| pub pending_inbound_htlcs: Vec<InboundHTLCDetails>, | ||
| /// Pending outbound HTLCs. | ||
| /// | ||
| /// This field is empty for objects serialized with LDK versions prior to 0.0.122. | ||
| pub pending_outbound_htlcs: Vec<OutboundHTLCDetails>, |
There was a problem hiding this comment.
Ensure that the handling of pending_inbound_htlcs and pending_outbound_htlcs fields in ChannelDetails is backward compatible and that these fields are properly documented, including their behavior in versions prior to 0.0.122.
Thanks! 🙏 Squashed. |
This exposes details around pending HTLCs in ChannelDetails. The state of the HTLC in the state machine is also included, so it can be determined which protocol message the HTLC is waiting for to advance.
wvanlint
force-pushed
the
list_pending_htlcs
branch
from
bb85b33
to
67e788e
Compare
| /// The state of the HTLC in the state machine. | ||
| /// Determines on which commitment transactions the HTLC is included and what message the HTLC is | ||
| /// waiting for to advance to the next state. | ||
| /// See [`InboundHTLCStateDetails`] for information on the specific states. |
There was a problem hiding this comment.
Docs should say when this can be None.
This exposes details around pending HTLCs in ChannelDetails. The state of the HTLC in the state machine is also included, so it can be determined which protocol message the HTLC is waiting for to advance.