Combine common fields for OpenChannel and AcceptChannel V1 & V2 #2871
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe Lightning Network's codebase underwent a significant restructuring, focusing on the Changes
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: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- lightning/src/ln/channel.rs (24 hunks)
- lightning/src/ln/channelmanager.rs (15 hunks)
- lightning/src/ln/functional_test_utils.rs (3 hunks)
- lightning/src/ln/functional_tests.rs (19 hunks)
- lightning/src/ln/msgs.rs (13 hunks)
- lightning/src/ln/peer_handler.rs (3 hunks)
- lightning/src/ln/priv_short_conf_tests.rs (8 hunks)
- lightning/src/ln/shutdown_tests.rs (4 hunks)
Files not reviewed due to errors (2)
- lightning/src/ln/functional_test_utils.rs (Error: unable to parse review)
- lightning/src/ln/msgs.rs (Error: unable to parse review)
Additional comments: 68
lightning/src/ln/priv_short_conf_tests.rs (10)
- 296-298: The manipulation of
channel_typedirectly in the testtest_scid_privacy_on_pub_channelto force SCID privacy on a public channel is a good simulation of incorrect behavior from a peer. However, it's important to ensure that the production code properly handles such cases and rejects them, as done here. This test effectively validates the expected behavior when a peer attempts to set SCID privacy on a public channel.- 320-320: In
test_scid_privacy_negotiation, the assertion thatinit_open_channel.common_fields.channel_typesupports SCID privacy is crucial for ensuring that the negotiation logic for SCID privacy is correctly implemented. This test case effectively covers the scenario where a node attempts to negotiate SCID privacy but the counterparty does not support it, leading to a fallback to a non-SCID-privacy channel type.- 326-326: The use of an
ErrorMessageto simulate a peer's rejection of SCID privacy intest_scid_privacy_negotiationis a good test strategy. It accurately represents a real-world scenario where a peer might not support or agree to use SCID privacy, ensuring the code properly handles such rejections.- 332-332: The assertion in
test_scid_privacy_negotiationthat thesecond_open_channel.common_fields.channel_typedoes not support SCID privacy after receiving an error message from the counterparty is a critical part of testing the negotiation fallback mechanism. This ensures that the channel falls back to a type without SCID privacy if the counterparty does not support it.- 366-366: In
test_inbound_scid_privacy, asserting thatopen_channel.common_fields.channel_typerequires SCID privacy is essential for testing the acceptance of channels with the SCID privacy feature. This test case effectively ensures that channels can be established with SCID privacy when both parties agree to it.- 608-608: The assertion in
test_0conf_channel_with_async_monitorthataccept_channel.common_fields.minimum_depthis 0 is crucial for testing the acceptance of zero-confirmation channels. This ensures that the channel is recognized as a zero-confirmation channel and handled accordingly.- 888-888: In
test_zero_conf_accept_reject, modifyingopen_channel_msg.common_fields.channel_typeto require zero confirmation is a good way to test the node's behavior when receiving a zero-confirmation channel request. This test case effectively checks that nodes reject zero-confirmation channels by default unless explicitly configured to accept them.- 916-916: The attempt to manually accept a zero-confirmation channel using a non-zero-confirmation method in
test_zero_conf_accept_rejectis a good test for ensuring that the appropriate method is used for accepting such channels. This test case highlights the importance of using the correct API for accepting zero-confirmation channels.- 948-948: The successful manual acceptance of a zero-confirmation channel using the correct method in
test_zero_conf_accept_rejectis an important test case. It ensures that nodes can manually accept zero-confirmation channels when configured to do so, using the appropriate API call.- 999-999: In
test_connect_before_funding, settingaccept_channel.common_fields.minimum_depthto 0 is crucial for testing the behavior of nodes when connecting before the funding transaction is confirmed. This ensures that the channel is recognized as a zero-confirmation channel and that the nodes can proceed with the connection before the funding transaction is confirmed.lightning/src/ln/peer_handler.rs (7)
- 225-228: The method
handle_open_channelis correctly updated to accesstemporary_channel_idthroughcommon_fields. This change aligns with the PR's objective to refactor message structures for V1 and V2 compatibility.- 228-228: The method
handle_accept_channelis correctly updated to accesstemporary_channel_idthroughcommon_fields. This change aligns with the PR's objective to refactor message structures for V1 and V2 compatibility.- 318-322: The methods
handle_open_channel_v2andhandle_accept_channel_v2are correctly updated to accesstemporary_channel_idthroughcommon_fields. This change aligns with the PR's objective to refactor message structures for V1 and V2 compatibility.- 1982-1984: The log message in
SendAcceptChannelevent handling uses the correct channel ID fromcommon_fields. This ensures accurate logging information, which is crucial for debugging and monitoring.- 1988-1990: The log message in
SendAcceptChannelV2event handling uses the correct channel ID fromcommon_fields. This ensures accurate logging information, which is crucial for debugging and monitoring.- 1994-1996: The log message in
SendOpenChannelevent handling uses the correct channel ID fromcommon_fields. This ensures accurate logging information, which is crucial for debugging and monitoring.- 2000-2002: The log message in
SendOpenChannelV2event handling uses the correct channel ID fromcommon_fields. This ensures accurate logging information, which is crucial for debugging and monitoring.lightning/src/ln/functional_tests.rs (19)
- 110-125: The refactoring to use
common_fieldsfor channel message fields ininsane_open_helpercalls is consistent with the PR's objectives. This change centralizes the access and modification of common fields, improving code maintainability.- 169-177: The conditional modification of
max_htlc_value_in_flight_msatwithincommon_fieldsbased on thesend_from_initiatorflag demonstrates a clear understanding of the context in which these fields are used. This ensures that the logic for setting channel parameters remains intact post-refactoring.- 5837-5856: The assertions added to validate
OpenChannelmessage fields against the BOLT Fix typos #2 specifications, using thecommon_fieldsstructure, are correctly implemented. These checks ensure that the message adheres to the protocol's requirements, enhancing the robustness of channel creation logic.- 5870-5870: Modifying
dust_limit_satoshisandchannel_reserve_satoshisdirectly within theOpenChannelmessage, as seen here, is a straightforward application of the newcommon_fieldsstructure. This change simplifies the code by centralizing common field access, aligning with the PR's goal of improving code maintainability.- 7210-7210: Setting
to_self_delaywithincommon_fieldsfor channel creation tests the enforcement of configuration constraints. This usage ofcommon_fieldsmaintains the original logic while adhering to the refactor's aim of centralizing common field access.- 7225-7225: The modification of
to_self_delayin theAcceptChannelmessage throughcommon_fieldsis consistent with the refactor's objectives. This change ensures that the logic for handling channel acceptance parameters remains intact, demonstrating an understanding of the protocol's requirements.- 7242-7242: Adjusting
to_self_delaywithincommon_fieldsfor enforcing configuration constraints in channel creation logic showcases the refactor's effectiveness. This approach simplifies the codebase by centralizing the access to common fields, aligning with the PR's goals.- 7927-7928: Assertions on
channel_flagsandto_self_delayusingcommon_fieldsin the channel creation process validate the correct application of overridden configurations. This demonstrates the refactor's success in centralizing field access without altering the underlying logic.- 7942-7946: The consistency in setting and asserting
htlc_minimum_msatthroughcommon_fieldsacross different channel messages highlights the refactor's effectiveness in centralizing common field access. This maintains the integrity of channel parameter validation.- 8958-8958: Using
common_fields.temporary_channel_idto ensure unique identification of channels, even when manipulating thetemporary_channel_id, aligns with the refactor's objectives. This change simplifies the handling of channel identifiers by centralizing their access.- 8969-8969: Asserting the equality of
temporary_channel_idthroughcommon_fieldsin message send events ensures the correct channel identification logic is preserved. This usage ofcommon_fieldsaligns with the refactor's goal of improving code maintainability.- 8982-8982: The assertion on
temporary_channel_idwithincommon_fieldsfor channel acceptance messages demonstrates the refactor's success in centralizing common field access. This maintains the correct logic for channel identification and handling.- 9102-9106: Manipulating
temporary_channel_idwithincommon_fieldsfor testing channel creation scenarios showcases the refactor's effectiveness. This approach simplifies the code by centralizing access to common fields, aligning with the PR's objectives.- 9164-9164: Asserting channel identification through
common_fields.temporary_channel_idin message handling events ensures the refactor maintains the correct logic for channel management. This usage ofcommon_fieldsaligns with the PR's goal of improving code organization.- 9198-9198: The handling of
temporary_channel_idwithincommon_fieldsfor channel opening scenarios demonstrates the refactor's success in centralizing common field access. This maintains the integrity of channel identification logic, aligning with the PR's objectives.- 9207-9207: Asserting
channel_idthroughcommon_fields.temporary_channel_idin error handling scenarios ensures the refactor does not alter the underlying logic for channel management. This demonstrates the effectiveness of centralizing common field access.- 9228-9228: The removal of a channel based on
temporary_channel_idfromcommon_fieldsin error recovery scenarios highlights the refactor's effectiveness. This approach simplifies the code by centralizing access to common fields, aligning with the PR's goals.- 9895-9896: Modifying
max_htlc_value_in_flight_msatandmax_accepted_htlcswithincommon_fieldsfor channel creation tests the application of specific configurations. This usage ofcommon_fieldsmaintains the original logic while adhering to the refactor's aim of centralizing common field access.- 9943-9946: Calculating HTLC-related metrics based on
dust_limit_satoshisfromcommon_fieldsin channel creation scenarios demonstrates the refactor's success in centralizing common field access. This maintains the correct logic for handling channel parameters and constraints.lightning/src/ln/channel.rs (10)
- 6618-6640: The encapsulation of common fields into
CommonOpenChannelFieldsfor theOpenChannelmessage is a positive change for maintainability and readability. However, ensure that all instances where these fields are accessed have been updated to use the new structure.- 6724-6724: The handling of
shutdown_scriptpubkeybased on upfront shutdown script support is logical. However, ensure that the zero-length script logic aligns with the protocol's expectations and security considerations.- 6743-6768: Updating the context with counterparty's parameters from
AcceptChannelmessage is correctly implemented. It's important to ensure that all fields are correctly mapped and that the logic for settingminimum_depthbased ontrust_own_funding_0confis sound and secure.- 6895-6901: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [6898-6913]
The validation of
channel_typeinOpenChannelmessages against optional bits and subset of supported features is crucial for protocol compliance and security. This logic appears correct, but ensure thatsupports_any_optional_bitsandis_subsetfunctions accurately reflect the protocol specifications.
- 6940-7015: The construction of a new channel from an
OpenChannelmessage involves several validation steps and state updates. The logic for these operations seems sound, but it's important to review the configuration limits and protocol rules being enforced to ensure they are appropriate and secure.- 7281-7300: The construction of the
AcceptChannelmessage usingCommonAcceptChannelFieldsis well-implemented. This change enhances the code's maintainability by reducing redundancy. Ensure that theshutdown_scriptpubkeyhandling and the optionalchannel_typeare correctly implemented according to the protocol's requirements.- 8532-8532: The test assertion for
commitment_feerate_sat_per_1000_weightbeing equal to the original fee after a fee estimation adjustment is a good practice for ensuring that changes in fee estimation are correctly reflected in the message.- 8563-8563: Explicitly setting the
dust_limit_satoshisin theAcceptChannelmessage for testing purposes is appropriate. Ensure that the test reflects realistic scenarios and that the dust limit set is within acceptable protocol limits.- 9854-9854: Setting the
channel_typein theOpenChannelmessage to test zero-conf channel creation is a valid approach. Ensure that the test covers all relevant scenarios and that thechannel_typeset aligns with the capabilities of both parties.- 9937-9937: The test logic for feature negotiation by setting
channel_typetoNoneis sound. It's important to ensure comprehensive coverage of feature negotiation scenarios to validate that the channel fails as expected when incompatible features are present.lightning/src/ln/channelmanager.rs (22)
- 6181-6183: The update to reference
msg.common_fields.chain_hashandmsg.common_fields.temporary_channel_idaligns with the PR's objective to refactor message structures for better maintainability. This change correctly accesses the common fields for theOpenChannelmessage.- 6187-6188: Correctly updated to use
msg.common_fields.temporary_channel_idfor error handling, ensuring consistency with the new structure. This is a good practice for maintainability and readability.- 6217-6217: The use of
msg.common_fields.temporary_channel_idfor error handling is consistent and correct. This change supports the refactor's aim to streamline message handling.- 6224-6224: Correctly utilizes
msg.common_fields.temporary_channel_idin error messages, aligning with the refactor's objectives. This ensures that error handling is consistent with the new message structures.- 6230-6232: The update to use
msg.common_fields.temporary_channel_idfor error handling in case of atemporary_channel_idcollision is appropriate and aligns with the refactor's goals.- 6240-6240: The error handling here correctly uses
msg.common_fields.temporary_channel_id, which is consistent with the refactor's objectives. This ensures that the new structure is utilized throughout the codebase.- 6266-6266: The error handling correctly references
msg.common_fields.temporary_channel_id, aligning with the refactor's goals. This consistency is crucial for maintainability.- 6273-6275: Correctly updated to use
msg.common_fields.temporary_channel_idfor error handling, ensuring consistency with the new structure. This is a good practice for maintainability and readability.- 6278-6280: The update to use
msg.common_fields.temporary_channel_idfor error handling is appropriate and aligns with the refactor's goals.- 6314-6314: Correctly utilizes
msg.common_fields.temporary_channel_idin error messages, aligning with the refactor's objectives. This ensures that error handling is consistent with the new message structures.- 6318-6318: The use of
msg.common_fields.temporary_channel_idfor error handling is consistent and correct. This change supports the refactor's aim to streamline message handling.- 8712-8712: The handling of
OpenChannelV2messages with a clear error message usingmsg.common_fields.temporary_channel_idis consistent with the refactor's objectives. This ensures that the new structure is utilized throughout the codebase.- 8728-8728: Correctly updated to use
msg.common_fields.temporary_channel_idfor error handling inhandle_accept_channel_v2, ensuring consistency with the new structure. This is a good practice for maintainability and readability.- 12006-12006: The update to set
temporary_channel_idusingmsg.common_fieldsin test scenarios aligns with the refactor's objective to improve code organization. This ensures that tests are consistent with the new message structures.- 12010-12011: Correctly sets
temporary_channel_idusingmsg.common_fieldsin test scenarios, aligning with the refactor's objectives. This ensures that tests are consistent with the new message structures.- 12062-12062: The update to set
temporary_channel_idusingmsg.common_fieldsin test scenarios aligns with the refactor's objective to improve code organization. This ensures that tests are consistent with the new message structures.- 12102-12102: Correctly sets
temporary_channel_idusingmsg.common_fieldsin test scenarios, aligning with the refactor's objectives. This ensures that tests are consistent with the new message structures.- 12118-12118: The update to set
temporary_channel_idusingmsg.common_fieldsin test scenarios aligns with the refactor's objective to improve code organization. This ensures that tests are consistent with the new message structures.- 12154-12154: Correctly sets
temporary_channel_idusingmsg.common_fieldsin test scenarios, aligning with the refactor's objectives. This ensures that tests are consistent with the new message structures.- 12176-12176: The update to set
temporary_channel_idusingmsg.common_fieldsin test scenarios aligns with the refactor's objective to improve code organization. This ensures that tests are consistent with the new message structures.- 12321-12321: Asserting that the
channel_typesupportsanchors_zero_fee_htlc_txusingmsg.common_fieldsin test scenarios is consistent with the refactor's objectives. This ensures that tests are aligned with the new message structures and functionalities.- 12336-12336: The assertion on
channel_typenot supportinganchors_zero_fee_htlc_txusingmsg.common_fieldsin test scenarios aligns with the refactor's objectives. This ensures that tests are consistent with the new message structures and functionalities.
Comment on lines
+6657
to
+6705
| if msg.common_fields.dust_limit_satoshis > 21000000 * 100000000 { | ||
| return Err(ChannelError::Close(format!("Peer never wants payout outputs? dust_limit_satoshis was {}", msg.common_fields.dust_limit_satoshis))); | ||
| } | ||
| if msg.channel_reserve_satoshis > self.context.channel_value_satoshis { | ||
| return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than ({})", msg.channel_reserve_satoshis, self.context.channel_value_satoshis))); | ||
| } | ||
| if msg.dust_limit_satoshis > self.context.holder_selected_channel_reserve_satoshis { | ||
| return Err(ChannelError::Close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); | ||
| if msg.common_fields.dust_limit_satoshis > self.context.holder_selected_channel_reserve_satoshis { | ||
| return Err(ChannelError::Close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.common_fields.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); | ||
| } | ||
| if msg.channel_reserve_satoshis > self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis { | ||
| return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than channel value minus our reserve ({})", | ||
| msg.channel_reserve_satoshis, self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis))); | ||
| } | ||
| let full_channel_value_msat = (self.context.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000; | ||
| if msg.htlc_minimum_msat >= full_channel_value_msat { | ||
| return Err(ChannelError::Close(format!("Minimum htlc value ({}) is full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat))); | ||
| if msg.common_fields.htlc_minimum_msat >= full_channel_value_msat { | ||
| return Err(ChannelError::Close(format!("Minimum htlc value ({}) is full channel value ({})", msg.common_fields.htlc_minimum_msat, full_channel_value_msat))); | ||
| } | ||
| let max_delay_acceptable = u16::min(peer_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT); | ||
| if msg.to_self_delay > max_delay_acceptable { | ||
| return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.to_self_delay))); | ||
| if msg.common_fields.to_self_delay > max_delay_acceptable { | ||
| return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.common_fields.to_self_delay))); | ||
| } | ||
| if msg.max_accepted_htlcs < 1 { | ||
| if msg.common_fields.max_accepted_htlcs < 1 { | ||
| return Err(ChannelError::Close("0 max_accepted_htlcs makes for a useless channel".to_owned())); | ||
| } | ||
| if msg.max_accepted_htlcs > MAX_HTLCS { | ||
| return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.max_accepted_htlcs, MAX_HTLCS))); | ||
| if msg.common_fields.max_accepted_htlcs > MAX_HTLCS { | ||
| return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.common_fields.max_accepted_htlcs, MAX_HTLCS))); | ||
| } | ||
|
|
||
| // Now check against optional parameters as set by config... | ||
| if msg.htlc_minimum_msat > peer_limits.max_htlc_minimum_msat { | ||
| return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); | ||
| if msg.common_fields.htlc_minimum_msat > peer_limits.max_htlc_minimum_msat { | ||
| return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.common_fields.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); | ||
| } | ||
| if msg.max_htlc_value_in_flight_msat < peer_limits.min_max_htlc_value_in_flight_msat { | ||
| return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); | ||
| if msg.common_fields.max_htlc_value_in_flight_msat < peer_limits.min_max_htlc_value_in_flight_msat { | ||
| return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.common_fields.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); | ||
| } | ||
| if msg.channel_reserve_satoshis > peer_limits.max_channel_reserve_satoshis { | ||
| return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg.channel_reserve_satoshis, peer_limits.max_channel_reserve_satoshis))); | ||
| } | ||
| if msg.max_accepted_htlcs < peer_limits.min_max_accepted_htlcs { | ||
| return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); | ||
| if msg.common_fields.max_accepted_htlcs < peer_limits.min_max_accepted_htlcs { | ||
| return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.common_fields.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); | ||
| } | ||
| if msg.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { | ||
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); | ||
| if msg.common_fields.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { | ||
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); | ||
| } | ||
| if msg.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { | ||
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); | ||
| if msg.common_fields.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { | ||
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); | ||
| } | ||
| if msg.minimum_depth > peer_limits.max_minimum_depth { | ||
| return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.minimum_depth))); | ||
| if msg.common_fields.minimum_depth > peer_limits.max_minimum_depth { | ||
| return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.common_fields.minimum_depth))); |
There was a problem hiding this comment.
Validation logic for AcceptChannel messages correctly uses the new CommonOpenChannelFields structure. Pay attention to the hardcoded Bitcoin supply limit in line 6666, which might be better placed as a constant for clarity and maintainability.
- if msg.common_fields.dust_limit_satoshis > 21000000 * 100000000 {
+ const TOTAL_BITCOIN_SUPPLY_SATOSHIS: u64 = 21000000 * 100000000;
+ if msg.common_fields.dust_limit_satoshis > TOTAL_BITCOIN_SUPPLY_SATOSHIS {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| if msg.common_fields.dust_limit_satoshis > 21000000 * 100000000 { | |
| return Err(ChannelError::Close(format!("Peer never wants payout outputs? dust_limit_satoshis was {}", msg.common_fields.dust_limit_satoshis))); | |
| } | |
| if msg.channel_reserve_satoshis > self.context.channel_value_satoshis { | |
| return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than ({})", msg.channel_reserve_satoshis, self.context.channel_value_satoshis))); | |
| } | |
| if msg.dust_limit_satoshis > self.context.holder_selected_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); | |
| if msg.common_fields.dust_limit_satoshis > self.context.holder_selected_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.common_fields.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); | |
| } | |
| if msg.channel_reserve_satoshis > self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than channel value minus our reserve ({})", | |
| msg.channel_reserve_satoshis, self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis))); | |
| } | |
| let full_channel_value_msat = (self.context.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000; | |
| if msg.htlc_minimum_msat >= full_channel_value_msat { | |
| return Err(ChannelError::Close(format!("Minimum htlc value ({}) is full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat))); | |
| if msg.common_fields.htlc_minimum_msat >= full_channel_value_msat { | |
| return Err(ChannelError::Close(format!("Minimum htlc value ({}) is full channel value ({})", msg.common_fields.htlc_minimum_msat, full_channel_value_msat))); | |
| } | |
| let max_delay_acceptable = u16::min(peer_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT); | |
| if msg.to_self_delay > max_delay_acceptable { | |
| return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.to_self_delay))); | |
| if msg.common_fields.to_self_delay > max_delay_acceptable { | |
| return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.common_fields.to_self_delay))); | |
| } | |
| if msg.max_accepted_htlcs < 1 { | |
| if msg.common_fields.max_accepted_htlcs < 1 { | |
| return Err(ChannelError::Close("0 max_accepted_htlcs makes for a useless channel".to_owned())); | |
| } | |
| if msg.max_accepted_htlcs > MAX_HTLCS { | |
| return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.max_accepted_htlcs, MAX_HTLCS))); | |
| if msg.common_fields.max_accepted_htlcs > MAX_HTLCS { | |
| return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.common_fields.max_accepted_htlcs, MAX_HTLCS))); | |
| } | |
| // Now check against optional parameters as set by config... | |
| if msg.htlc_minimum_msat > peer_limits.max_htlc_minimum_msat { | |
| return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); | |
| if msg.common_fields.htlc_minimum_msat > peer_limits.max_htlc_minimum_msat { | |
| return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.common_fields.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); | |
| } | |
| if msg.max_htlc_value_in_flight_msat < peer_limits.min_max_htlc_value_in_flight_msat { | |
| return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); | |
| if msg.common_fields.max_htlc_value_in_flight_msat < peer_limits.min_max_htlc_value_in_flight_msat { | |
| return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.common_fields.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); | |
| } | |
| if msg.channel_reserve_satoshis > peer_limits.max_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg.channel_reserve_satoshis, peer_limits.max_channel_reserve_satoshis))); | |
| } | |
| if msg.max_accepted_htlcs < peer_limits.min_max_accepted_htlcs { | |
| return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); | |
| if msg.common_fields.max_accepted_htlcs < peer_limits.min_max_accepted_htlcs { | |
| return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.common_fields.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); | |
| } | |
| if msg.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { | |
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); | |
| if msg.common_fields.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { | |
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); | |
| } | |
| if msg.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { | |
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); | |
| if msg.common_fields.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { | |
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); | |
| } | |
| if msg.minimum_depth > peer_limits.max_minimum_depth { | |
| return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.minimum_depth))); | |
| if msg.common_fields.minimum_depth > peer_limits.max_minimum_depth { | |
| return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.common_fields.minimum_depth))); | |
| const TOTAL_BITCOIN_SUPPLY_SATOSHIS: u64 = 21000000 * 100000000; | |
| if msg.common_fields.dust_limit_satoshis > TOTAL_BITCOIN_SUPPLY_SATOSHIS { | |
| return Err(ChannelError::Close(format!("Peer never wants payout outputs? dust_limit_satoshis was {}", msg.common_fields.dust_limit_satoshis))); | |
| } | |
| if msg.channel_reserve_satoshis > self.context.channel_value_satoshis { | |
| return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than ({})", msg.channel_reserve_satoshis, self.context.channel_value_satoshis))); | |
| } | |
| if msg.common_fields.dust_limit_satoshis > self.context.holder_selected_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("Dust limit ({}) is bigger than our channel reserve ({})", msg.common_fields.dust_limit_satoshis, self.context.holder_selected_channel_reserve_satoshis))); | |
| } | |
| if msg.channel_reserve_satoshis > self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must not be greater than channel value minus our reserve ({})", | |
| msg.channel_reserve_satoshis, self.context.channel_value_satoshis - self.context.holder_selected_channel_reserve_satoshis))); | |
| } | |
| let full_channel_value_msat = (self.context.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000; | |
| if msg.common_fields.htlc_minimum_msat >= full_channel_value_msat { | |
| return Err(ChannelError::Close(format!("Minimum htlc value ({}) is full channel value ({})", msg.common_fields.htlc_minimum_msat, full_channel_value_msat))); | |
| } | |
| let max_delay_acceptable = u16::min(peer_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT); | |
| if msg.common_fields.to_self_delay > max_delay_acceptable { | |
| return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_delay_acceptable, msg.common_fields.to_self_delay))); | |
| } | |
| if msg.common_fields.max_accepted_htlcs < 1 { | |
| return Err(ChannelError::Close("0 max_accepted_htlcs makes for a useless channel".to_owned())); | |
| } | |
| if msg.common_fields.max_accepted_htlcs > MAX_HTLCS { | |
| return Err(ChannelError::Close(format!("max_accepted_htlcs was {}. It must not be larger than {}", msg.common_fields.max_accepted_htlcs, MAX_HTLCS))); | |
| } | |
| // Now check against optional parameters as set by config... | |
| if msg.common_fields.htlc_minimum_msat > peer_limits.max_htlc_minimum_msat { | |
| return Err(ChannelError::Close(format!("htlc_minimum_msat ({}) is higher than the user specified limit ({})", msg.common_fields.htlc_minimum_msat, peer_limits.max_htlc_minimum_msat))); | |
| } | |
| if msg.common_fields.max_htlc_value_in_flight_msat < peer_limits.min_max_htlc_value_in_flight_msat { | |
| return Err(ChannelError::Close(format!("max_htlc_value_in_flight_msat ({}) is less than the user specified limit ({})", msg.common_fields.max_htlc_value_in_flight_msat, peer_limits.min_max_htlc_value_in_flight_msat))); | |
| } | |
| if msg.channel_reserve_satoshis > peer_limits.max_channel_reserve_satoshis { | |
| return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is higher than the user specified limit ({})", msg.channel_reserve_satoshis, peer_limits.max_channel_reserve_satoshis))); | |
| } | |
| if msg.common_fields.max_accepted_htlcs < peer_limits.min_max_accepted_htlcs { | |
| return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.common_fields.max_accepted_htlcs, peer_limits.min_max_accepted_htlcs))); | |
| } | |
| if msg.common_fields.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { | |
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); | |
| } | |
| if msg.common_fields.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { | |
| return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.common_fields.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); | |
| } | |
| if msg.common_fields.minimum_depth > peer_limits.max_minimum_depth { | |
| return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.common_fields.minimum_depth))); |
Comment on lines
+6201
to
+6203
| MsgHandleErrInternal::send_err_msg_no_close( | ||
| format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), | ||
| msg.common_fields.temporary_channel_id.clone()) |
There was a problem hiding this comment.
The error handling here has been updated to use msg.common_fields.temporary_channel_id, which is consistent with the refactor's goal. However, using format! for error messages is efficient only when necessary. Consider if static messages could be used instead to avoid runtime formatting costs in error paths that are expected to be rare.
- format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
+ "Can't find a peer matching the passed counterparty node_id".to_owned(),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| MsgHandleErrInternal::send_err_msg_no_close( | |
| format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), | |
| msg.common_fields.temporary_channel_id.clone()) | |
| MsgHandleErrInternal::send_err_msg_no_close( | |
| "Can't find a peer matching the passed counterparty node_id".to_owned(), | |
| msg.common_fields.temporary_channel_id.clone()) |
| @@ -6289,28 +6299,28 @@ | |||
| let peer_state_mutex = per_peer_state.get(counterparty_node_id) | |||
| .ok_or_else(|| { | |||
| debug_assert!(false); | |||
| MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.temporary_channel_id) | |||
| MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.common_fields.temporary_channel_id) | |||
There was a problem hiding this comment.
The error handling here has been updated to use msg.common_fields.temporary_channel_id, which is consistent with the refactor's goal. However, consider if static messages could be used instead to avoid runtime formatting costs in error paths that are expected to be rare.
- format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
+ "Can't find a peer matching the passed counterparty node_id".to_owned(),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.common_fields.temporary_channel_id) | |
| MsgHandleErrInternal::send_err_msg_no_close("Can't find a peer matching the passed counterparty node_id".to_owned(), msg.common_fields.temporary_channel_id) |
dunxen
force-pushed
the
2024-02-msgcommonfields
branch
from
502527c
to
0a7a5f9
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (9)
- lightning/src/ln/async_signer_tests.rs (1 hunks)
- lightning/src/ln/channel.rs (24 hunks)
- lightning/src/ln/channelmanager.rs (15 hunks)
- lightning/src/ln/functional_test_utils.rs (3 hunks)
- lightning/src/ln/functional_tests.rs (19 hunks)
- lightning/src/ln/msgs.rs (13 hunks)
- lightning/src/ln/peer_handler.rs (3 hunks)
- lightning/src/ln/priv_short_conf_tests.rs (8 hunks)
- lightning/src/ln/shutdown_tests.rs (4 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/msgs.rs: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review as they are similar to previous changes (7)
- lightning/src/ln/channel.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/ln/priv_short_conf_tests.rs
- lightning/src/ln/shutdown_tests.rs
Additional comments: 1
lightning/src/ln/async_signer_tests.rs (1)
- 200-200: The update to use
accept_channel.common_fields.minimum_depthaligns with the PR's objective of refactoring common fields into shared structures. This change correctly accesses theminimum_depthfield through the newcommon_fieldsstructure, ensuring consistency with the new data organization. The logic appears correct, and the change is directly related to the PR's goal of improving code organization and maintainability by introducingCommonOpenChannelFieldsandCommonAcceptChannelFields.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2871 +/- ##
==========================================
- Coverage 89.18% 88.95% -0.23%
==========================================
Files 116 116
Lines 93252 93465 +213
Branches 93252 93465 +213
==========================================
- Hits 83166 83144 -22
- Misses 7560 7723 +163
- Partials 2526 2598 +72 ☔ View full report in Codecov by Sentry. |
wpaulino
approved these changes
Feb 5, 2024
shaavan
approved these changes
Feb 6, 2024
optout21
approved these changes
Feb 8, 2024
There was a problem hiding this comment.
My only concern to mention is naming:
OpenChannelCommonFieldsseems a tiny bit more consistent withcommon_fields(vs.CommonOpenChannelFields).- Somehow the 'common fields' does not sound the most appropriate, though I can't think of a much better one. Some alternatives:
common struct,common info, or maybe justcommon(CommonOpenChannel/CommonAcceptChannel/common)
The names used in the PR are not bad, but maybe some more consideration should be given, as likely they will not be changed later.
TheBlueMatt
approved these changes
Feb 8, 2024
| }); | ||
| impl Writeable for OpenChannel { | ||
| fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> { | ||
| self.common_fields.chain_hash.write(w)?; |
There was a problem hiding this comment.
here and a few other places - tabs not spaces :)
There was a problem hiding this comment.
oh I know what happened here. I used rust-analyzer to fill out the missing trait methods and it insists on space 🤦♂️
Will put up a quick fixup.
There was a problem hiding this comment.
oh, you already got them. thanks and sorry for messing up the git blame!
There was a problem hiding this comment.
Oh lol. Oh well, not a big deal, really.
| self.common_fields.first_per_commitment_point.write(w)?; | ||
| self.common_fields.channel_flags.write(w)?; | ||
| encode_tlv_stream!(w, { | ||
| (0, self.common_fields.shutdown_scriptpubkey.as_ref().map(|s| WithoutLength(s)), option), // Don't encode length twice. |
There was a problem hiding this comment.
Similarly, this is indented really far?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.