2691 follow-ups

[wpaulino]

No description provided.

[shaavan]

ACK, the changes introduced in this PR look great and make the code robust.

Commit-wise-breakdown:

1. Manually defining PartialOrd makes the code robust and order-agnostic.
2. The renaming looks good, clearly expressing the function of the variable
3. Simplification of conditional improves code brevity and readability.
4. Funding transaction makes sense only for the outbound channel, and this commit enforces that.
5. Asserting to never write a pre-funded channel, makes sense. And also, the appropriate changes to the test addressing this update look good.
6. Correctly ensures that we cannot create initiate getter, setter, and clear() for channel states for which it doesn’t make sense. I tried to create an impl_state_flag!() for FundingNegotiated and got a compile time error as expected.
7. Ensures cleaner & clearer masking for copied flags.
8. Makes sense to not enable invalid bits, and instead mask them.

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (db81c65) 88.52% compared to head (25ec63f) 89.14%.
Report is 85 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channel.rs	95.83%	1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2780      +/-   ##
==========================================
+ Coverage   88.52%   89.14%   +0.62%     
==========================================
  Files         114      116       +2     
  Lines       92050    93122    +1072     
  Branches    92050    93122    +1072     
==========================================
+ Hits        81487    83016    +1529     
+ Misses       8058     7567     -491     
- Partials     2505     2539      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wpaulino]

Rebased to resolve a small conflict

[shaavan]

ACK

Updates since my last review:

1. Logic is corrected for the can_generate_new_commitment function. Since the new name is the exact opposite of should_force_holding_cell, it makes sense to flip the boolean result of the function.

[TheBlueMatt]

Dont think this strictly needs to make it in 119, the parent was fine and its all internal.

[TheBlueMatt]

It seems this needs rebase already :(

[coderabbitai]

Walkthrough

The recent update to the Lightning Network's codebase, specifically within lightning/src/ln/channel.rs, enhances channel state management through modifications to macros, enums, and the Channel struct. These refinements include the addition of parameters to a macro, introduction of new methods for enums, and adjustments to the channel state logic. This comprehensive overhaul aims to improve the flexibility and control over channel states, crucial for the network's efficiency and reliability.

Changes

File Path	Change Summary
lightning/src/ln/channel.rs	Updated define_state_flags macro, added/modified methods in enums, and refined Channel struct logic.

🐇✨
Changes in the code, swift and bright,
Flags and states, in the network's flight.
Through the tunnels of code, we hop and leap,
In the Lightning's glow, our progress we keep.
🌩️🚀

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[TheBlueMatt]

With the exception of the last commit, all the changes here are straightforward transformations of the existing code that doesn't change (debug_assert'd) behavior at all. The last commit is a bit more involved, but ultimately boils down to moving accessors and setters to using new methods, rather than directly manipulating flags, which is a bit safer. I don't see any reason to hold this on another reviewer.

[TheBlueMatt]

Oh, oops, nevermind, this is failing to build in the second-to-last commit - it contains diff hunks that should be in the last commit.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 51d9ee3 and 25ec63f.

Files selected for processing (1)

• lightning/src/ln/channel.rs (23 hunks)

Additional comments: 21

lightning/src/ln/channel.rs (21)

• 270-270: The modification to the define_state_flags macro introduces flexibility by allowing extra flags to be specified. Ensure that all usages of this macro throughout the codebase have been updated to accommodate or utilize this new parameter structure.
• 301-310: Added utility functions set, clear, and bitwise operations within the macro-generated structs enhance the manipulation of state flags. Ensure these methods are correctly utilized in the context where state flags are managed, and consider adding unit tests to verify their behavior.
• 329-350: The expansion of the define_state_flags macro to include specific flag behaviors and the addition of new flag methods for FundedStateFlags enhances the channel state management. Verify that these new methods are integrated and used correctly in the channel state transition logic.

Verification successful

---

The search results indicate that the new flag methods (is_peer_disconnected, set_peer_disconnected, clear_peer_disconnected) introduced for FundedStateFlags are indeed integrated and used within the channel state management logic in channel.rs. The methods are utilized in various conditions to check the connection state of a peer, to set the state when a peer is disconnected, and to clear this state when necessary. This confirms that the expansion of the define_state_flags macro and the addition of new flag methods are correctly integrated into the channel state transition logic.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify integration of new flag methods in channel state management.
rg --type rust 'is_peer_disconnected|set_peer_disconnected|clear_peer_disconnected'

Length of output: 2352

* 397-409: The definitions of `FundedStateFlags` and `NegotiatingFundingFlags` using the `define_state_flags` macro are clear and maintain the separation of concerns within channel state management. Ensure that these flags are used consistently across the channel module. * 428-437: The addition of `AwaitingChannelReadyFlags` further refines the granularity of channel state management. It's important to ensure that the logic surrounding these flags is correctly implemented, especially in the transition to the `ChannelReady` state. * 453-454: The comment about the implicit order of the `ChannelState` enum variants is crucial. Ensure that any new states added or modifications to existing ones respect this order to maintain the integrity of state transitions. * 495-511: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [475-508]

The impl_state_flag macro and its usage streamline the implementation of state flag manipulation methods across different channel states. Review the usage of these generated methods in state transition logic to ensure they are applied correctly.

• 559-579: The modification to can_generate_new_commitment and the addition of flag manipulation methods are significant for the channel's operational logic. Ensure that the conditions checked in can_generate_new_commitment are correctly reflecting the channel's ability to generate new commitments.
• 2415-2415: The logic change in handling the channel state for determining if a funding transaction output should be returned is significant. Ensure that this logic correctly reflects the intended behavior in all relevant channel states.
• 2731-2731: The assertion to prevent new commitments while certain conditions are met is crucial for channel safety. Verify that this assertion does not introduce any unintended side effects in the channel's operation.
• 2801-2801: The condition check before proceeding with certain channel operations is important for maintaining channel integrity. Ensure that this condition accurately reflects the channel's state and does not prevent legitimate operations.
• 2975-2975: The logic to force the use of the holding cell under certain conditions is a critical safety mechanism. Review the conditions under which force_holding_cell is set to true to ensure they are comprehensive and correct.
• 3071-3076: The logic for handling channel state transitions upon reconnection is complex. Ensure that the transitions between AwaitingChannelReady and ChannelReady states are handled correctly, especially in edge cases.
• 3592-3592: The condition for freeing HTLCs from the holding cell based on the channel's ability to generate new commitments is crucial. Verify that this logic does not inadvertently release HTLCs that should remain in the holding cell under certain conditions.
• 4202-4203: The logic for determining when the funding transaction is broadcastable is important for the channel's lifecycle. Ensure that this logic correctly accounts for all relevant channel states and does not allow premature broadcasting.
• 5211-5211: The condition for determining if a monitor update is awaited is critical for channel safety. Review this logic to ensure it accurately reflects the channel's state and does not bypass necessary updates.
• 5297-5304: The logic for determining the need for a commitment update based on channel state is significant. Ensure that this logic correctly triggers updates in the appropriate states and does not cause unnecessary updates.
• 5879-5879: The determination of the need for the holding cell based on the channel's ability to generate new commitments is crucial. Review this logic to ensure it correctly identifies scenarios where the holding cell should be used.
• 7497-7498: The logic for setting the peer_disconnected flag based on the channel state is important for managing channel states upon disconnection. Ensure that this flag is set correctly in all relevant channel states.
• 8400-8411: The test test_channel_state_order is crucial for ensuring the correct order of channel states. Verify that this test accurately reflects the intended state order and includes any new states introduced.
• 8919-8946: The test blinding_point_skimmed_fee_malformed_ser is important for ensuring correct serialization and deserialization of channel-related data. Ensure that this test covers all relevant scenarios and accurately reflects the data structures involved.