Remove `Outpoint::to_channel_id` method #2797

dunxen · 2023-12-15T11:45:57Z

To avoid confusion and for accuracy going forward, we remove this method as it is inconsistent with channel IDs generated during V2 channel establishment. If one wants to create a V1, funding outpoint-based channel ID, then ChannelId::v1_from_funding_outpoint should be used instead.

A large portion of the library has always made the assumption that having the funding outpoint will always allow us to generate the channel ID. This will not be the case anymore and we need to pass the channel ID along where appropriate. All channels that could have been persisted up to this point could only have used V1 establishment, so if some structures don't store a channel ID for them they can safely fall back to the funding outpoint-based version.

Summary by CodeRabbit

New Features
- Introduced ChannelId to enhance channel identification across various components.
Bug Fixes
- Corrected the conditional check logic within the do_test function.
Refactor
- Standardized the use of ChannelId across function signatures for improved consistency.
- Modified channel monitoring and update mechanisms to incorporate ChannelId.
- Adjusted test cases to align with the new ChannelId usage.
Style
- Updated logging behavior to accommodate new ChannelId references.
Documentation
- No visible changes to end-user documentation.
Tests
- Refined test suites to validate the integration of ChannelId.
Chores
- Updated .gitignore to prevent unnecessary tracking of specific directories.
Revert
- No reverts affecting end-user experience.

codecov-commenter · 2023-12-15T11:51:50Z

Codecov Report

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

Comparison is base (51d9ee3) 89.18% compared to head (8930f59) 89.18%.

❗ Current head 8930f59 differs from pull request most recent head cf2c278. Consider uploading reports for the commit cf2c278 to get more accurate results

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	85.21%	21 Missing ⚠️
lightning/src/chain/channelmonitor.rs	88.88%	3 Missing ⚠️
lightning/src/chain/chainmonitor.rs	90.00%	2 Missing ⚠️
lightning/src/ln/peer_handler.rs	0.00%	1 Missing ⚠️
lightning/src/util/macro_logger.rs	50.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2797      +/-   ##
==========================================
- Coverage   89.18%   89.18%   -0.01%     
==========================================
  Files         116      116              
  Lines       93098    93181      +83     
  Branches    93098    93181      +83     
==========================================
+ Hits        83034    83104      +70     
- Misses       7538     7553      +15     
+ Partials     2526     2524       -2

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

lightning-block-sync/src/init.rs

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channelmanager.rs

optout21 · 2023-12-18T09:38:27Z

ACK
When ChannelId was introduced, the OutPoint::to_channel_id() was preserved as a shortcut. Optionally that could be kept (with a rename to to_v1_channel_id()), but the more important point is that in places where the funding transaction was enough, now the channelId is often needed explicitly.

optout21

LGTM, some small comments added

lightning/src/util/macro_logger.rs

lightning/src/ln/shutdown_tests.rs

optout21 · 2024-01-10T08:40:07Z

lightning/src/ln/channelmanager.rs

 	},
 }
 impl_writeable_tlv_based_enum!(EventCompletionAction,
 	(0, ReleaseRAAChannelMonitorUpdate) => {
 		(0, channel_funding_outpoint, required),
 		(2, counterparty_node_id, required),
+		(3, channel_id, option),


Why 3 and not 4? Just wondering

optout21 · 2024-01-10T08:46:22Z

lightning/src/ln/channelmanager.rs

 					let mut updated_chan = false;
 					{
 						let per_peer_state = self.per_peer_state.read().unwrap();
 						if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
 							let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 							let peer_state = &mut *peer_state_lock;
-							match peer_state.channel_by_id.entry(funding_txo.to_channel_id()) {
+							match peer_state.channel_by_id.entry(channel_id.unwrap_or(ChannelId::v1_from_funding_outpoint(funding_txo))) {


Is this fallback needed here? channel_id initialization ~10 lines above also has a fallback.

Thanks, yeah just had some changes that should be up now but will address your other feedback soon.

Right now trying to figure out the fuzzing break for full_stack and not sure what's causing it or why anything would need to be changed there.

fuzz/src/full_stack.rs

lightning/src/chain/transaction.rs

dunxen · 2024-01-11T19:43:00Z

@coderabbitai review

coderabbitai

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 4b70921 and 49f6f21.

Files selected for processing (27)

.gitignore (1 hunks)
fuzz/src/chanmon_consistency.rs (7 hunks)
fuzz/src/full_stack.rs (2 hunks)
fuzz/src/utils/test_persister.rs (2 hunks)
lightning-background-processor/src/lib.rs (2 hunks)
lightning-block-sync/src/init.rs (2 hunks)
lightning-persister/src/fs_store.rs (4 hunks)
lightning/src/chain/chainmonitor.rs (22 hunks)
lightning/src/chain/channelmonitor.rs (20 hunks)
lightning/src/chain/mod.rs (4 hunks)
lightning/src/chain/transaction.rs (4 hunks)
lightning/src/ln/chanmon_update_fail_tests.rs (29 hunks)
lightning/src/ln/channel.rs (6 hunks)
lightning/src/ln/channel_id.rs (2 hunks)
lightning/src/ln/channelmanager.rs (92 hunks)
lightning/src/ln/functional_test_utils.rs (4 hunks)
lightning/src/ln/functional_tests.rs (17 hunks)
lightning/src/ln/monitor_tests.rs (6 hunks)
lightning/src/ln/payment_tests.rs (2 hunks)
lightning/src/ln/peer_handler.rs (1 hunks)
lightning/src/ln/priv_short_conf_tests.rs (3 hunks)
lightning/src/ln/reload_tests.rs (6 hunks)
lightning/src/ln/reorg_tests.rs (2 hunks)
lightning/src/ln/shutdown_tests.rs (16 hunks)
lightning/src/util/macro_logger.rs (2 hunks)
lightning/src/util/persist.rs (10 hunks)
lightning/src/util/test_utils.rs (7 hunks)

Files not summarized due to errors (1)

lightning/src/ln/channelmanager.rs: Error: Message exceeds token limit

Files skipped from review due to trivial changes (1)

.gitignore

Additional comments: 238

fuzz/src/utils/test_persister.rs (3)

5-5: The import of the ChannelId type is correctly added to be used in the TestPersister implementation.

14-14: The addition of the _channel_id: ChannelId parameter to the persist_new_channel function signature is correct and aligns with the PR objectives to explicitly use ChannelId.

18-18: The addition of the _channel_id: ChannelId parameter to the update_persisted_channel function signature is correct and aligns with the PR objectives to explicitly use ChannelId.

lightning/src/chain/transaction.rs (2)

82-82: The import of ChannelId in the test module is correctly updated to reflect the new location after the removal of the to_channel_id method.

91-98: The test test_channel_id_calculation has been correctly updated to use ChannelId::v1_from_funding_outpoint instead of the removed to_channel_id method.

lightning/src/ln/channel_id.rs (2)

14-14: The import of OutPoint is correctly added to support the new v1_from_funding_outpoint method.

47-49: The new method v1_from_funding_outpoint is correctly implemented to create a v1 channel ID from a funding transaction outpoint.

lightning/src/util/macro_logger.rs (5)

11-11: The import of ChannelId is correctly added to support the new logging behavior.

45-45: The fmt method of DebugFundingChannelId is correctly updated to use the v1_from_funding_outpoint method of ChannelId.

48-48: The log_funding_channel_id macro has been correctly renamed to log_v1_funding_channel_id to reflect the explicit use of v1 channel IDs.

54-54: The DebugFundingInfo struct is correctly updated to take a reference to ChannelId instead of OutPoint.

62-64: The log_funding_info macro is correctly updated to use the get_channel_id method of key_storage instead of get_funding_txo.

lightning-block-sync/src/init.rs (2)

52-52: The import of ChannelId is correctly added to support the updated watch_channel function call.

123-125: The watch_channel function call is correctly updated to include the channel_id parameter, aligning with the PR objectives to explicitly use ChannelId.

lightning-persister/src/fs_store.rs (3)

450-457: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [273-273]

The addition of channel_id: ChannelId to the watch_channel method signature is consistent with the PR's objective to explicitly pass the channel ID where previously the funding outpoint was used.

450-457: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [290-290]

The addition of channel_id: ChannelId to the update_channel method signature is consistent with the PR's objective to explicitly pass the channel ID where previously the funding outpoint was used.

450-457: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [301-301]

The addition of ChannelId to the return type of release_pending_monitor_events is consistent with the PR's objective to explicitly pass the channel ID where previously the funding outpoint was used.

lightning/src/chain/mod.rs (3)

273-273: The addition of channel_id: ChannelId to the watch_channel method signature aligns with the PR's goal of using explicit channel IDs for channel identification.

290-290: The addition of channel_id: ChannelId to the update_channel method signature aligns with the PR's goal of using explicit channel IDs for channel identification.

301-301: The inclusion of ChannelId in the return type of release_pending_monitor_events aligns with the PR's goal of using explicit channel IDs for channel identification.

lightning/src/ln/reorg_tests.rs (2)

28-28: The import statement has been updated to include ChannelId, which is consistent with the PR's objective to use ChannelId explicitly.

534-534: The assertion has been correctly updated to use the new ChannelId::v1_from_funding_outpoint() method, which aligns with the PR's objectives.

lightning/src/util/persist.rs (4)

197-197: The previous comment regarding the underscore prefix in _channel_id is still valid.

209-209: The previous comment regarding the underscore prefix in _channel_id is still valid.

610-610: The changes to persist_new_channel reflect the updated method signature to include the channel_id parameter, aligning with the PR's objectives.

654-654: The changes to update_persisted_channel correctly include the channel_id parameter in the method signature, consistent with the PR's objectives.
lightning/src/util/test_utils.rs (1)
296-307: The watch_channel method has been updated to include the channel_id parameter. The cloning and comparison of the monitor for a roundtrip test is still present, which was previously flagged as potentially inefficient. However, this is test code, and the explicitness may be preferred for clarity over performance. Ensure that the cloning does not introduce any unintended side effects in the test logic.

Verification successful

The search results indicate that the watch_channel method is used in various parts of the test code and the main library. The changes to the method signature in test_utils.rs to include the channel_id parameter are consistent with the updates across the library to handle channel IDs explicitly.

The cloning and comparison of the monitor within the watch_channel method in test_utils.rs are part of a roundtrip serialization test, ensuring that the serialization and deserialization of a monitor do not alter its state. This is a common practice in testing to verify the correctness of serialization logic.

Given that the cloning is part of the test logic and is intended to verify the serialization process, it is unlikely to introduce side effects that would affect the behavior of other tests. The tests are designed to be isolated, and the comparison of the monitor before and after serialization should not impact the state of the monitor used in other parts of the test suite.

Based on the information provided, there is no evidence of unintended side effects caused by the cloning of the monitor in the watch_channel method within the test code.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that the cloning of the monitor does not cause any side effects in other tests.
rg "watch_channel" --type rust
Length of output: 4735
lightning/src/ln/priv_short_conf_tests.rs (2)

21-21: The import of ChannelId is correctly added to support the new usage of ChannelId in the codebase.

620-620: The usage of ChannelId::v1_from_funding_outpoint is correct and aligns with the PR's objective to use ChannelId for V1 channel IDs.

lightning/src/chain/chainmonitor.rs (10)

34-34: The addition of ChannelId import is consistent with the PR's objective to use ChannelId explicitly.

162-162: The method persist_new_channel now includes a channel_id parameter, aligning with the PR's goal to explicitly pass ChannelId.

197-197: The method update_persisted_channel has been updated to include a channel_id parameter, which is in line with the PR's objectives.

325-326: The get_channel_id method is used to retrieve the channel ID, which is consistent with the changes made in the PR.

341-342: The get_channel_id method is used again here to retrieve the channel ID, which is consistent with the changes made in the PR.

362-362: The update_monitor_with_chain_data method now takes channel_id as a parameter, which is in line with the PR's objectives.

383-383: The update_persisted_channel method is called with channel_id as a parameter, which is consistent with the PR's objectives.

477-485: The list_monitors method now returns a vector of tuples containing OutPoint and ChannelId pairs, which is consistent with the PR's objectives.

723-723: The watch_channel method now includes a channel_id parameter, aligning with the PR's goal to explicitly pass ChannelId.

762-762: The update_channel method now includes a channel_id parameter, aligning with the PR's goal to explicitly pass ChannelId.

lightning/src/ln/reload_tests.rs (7)

19-19: The import changes to include ChannelId are correct and align with the PR's objectives.

204-204: The change from using OutPoint directly to creating a ChannelId using ChannelId::v1_from_funding_outpoint is consistent with the PR's objectives.

449-451: The updates to the watch_channel method calls to include the channel_id parameter are correct and necessary due to the removal of Outpoint::to_channel_id.

834-835: The retrieval of monitors using channel_id is consistent with the new changes and the PR's objectives.

854-855: The retrieval of monitors using channel_id is consistent with the new changes and the PR's objectives.

862-863: The retrieval of monitors using channel_id is consistent with the new changes and the PR's objectives.

1225-1225: The use of ChannelId::v1_from_funding_outpoint to create a ChannelId for the channel monitor retrieval is correct and aligns with the PR's objectives.
fuzz/src/chanmon_consistency.rs (11)
170-176: The addition of the channel_id parameter to the watch_channel function in the TestChainMonitor implementation aligns with the PR's objective to explicitly handle channel IDs.

167-182: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [179-191]

The update_channel function now correctly takes a channel_id parameter, which is consistent with the changes made to the watch_channel function.

194-194: The release_pending_monitor_events function's signature remains unchanged, which is expected as the function's behavior should not be affected by the explicit handling of channel IDs.

1066-1066: Ensure that the force_channel_monitor_updated function is correctly handling the new channel_id parameter by verifying its usage across the codebase.

Verification successful

The output from the scripts confirms that chan_1_id and chan_2_id are indeed being assigned channel ID values from the list of usable channels in the fuzz/src/chanmon_consistency.rs file. This suggests that the channel_id parameter is being handled correctly in the context of the force_channel_monitor_updated function calls within this file, as these variables are being used to pass channel IDs to the function.

Based on this information, it appears that the force_channel_monitor_updated function is correctly handling the new channel_id parameter in the context of the fuzz/src/chanmon_consistency.rs file.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that force_channel_monitor_updated is called with the correct channel_id
rg --type rust "force_channel_monitor_updated\("
Length of output: 4975

Script:
#!/bin/bash
# Check the definitions of chan_1_id and chan_2_id in the fuzz/src/chanmon_consistency.rs file
rg --type rust "let chan_1_id" fuzz/src/chanmon_consistency.rs
rg --type rust "let chan_2_id" fuzz/src/chanmon_consistency.rs
Length of output: 257
* 1072-1072: The previous comment regarding the `force_channel_monitor_updated` function applies here as well. * 1078-1078: The previous comment regarding the `force_channel_monitor_updated` function applies here as well. * 1084-1084: The previous comment regarding the `force_channel_monitor_updated` function applies here as well. * 1397-1397: The previous comment regarding the potential issue with `chan_1_id` and `chan_2_id` applies here as well. * 1401-1401: The previous comment regarding the potential issue with `chan_1_id` and `chan_2_id` applies here as well. * 1405-1405: The previous comment regarding the potential issue with `chan_1_id` and `chan_2_id` applies here as well. * 708-710: The assignment of `chan_1_id` and `chan_2_id` from the `list_usable_channels` function is correct and aligns with the new explicit channel ID handling.
lightning/src/ln/shutdown_tests.rs (10)

18-18: The import changes from use crate::ln::msgs to use crate::ln::{msgs, ChannelId} are correct and reflect the need for the ChannelId type in the updated function calls.

51-51: The update from using OutPoint::to_channel_id() to ChannelId::v1_from_funding_outpoint() is consistent with the PR's objective to remove the former method in favor of explicitly using ChannelId for V1 channel IDs.

721-721: The change in the test_upfront_shutdown_script test case correctly reflects the new method of obtaining a V1 channel ID from a funding outpoint.

736-736: The update in the test_upfront_shutdown_script test case is consistent with the previous comment and correctly uses the new ChannelId method.

750-750: The change in the test_upfront_shutdown_script test case for a non-signaling peer is correct and uses the ChannelId method as expected.

765-765: The update in the test_upfront_shutdown_script test case where the user opts out of providing a script at channel opening is correctly using the ChannelId method.

779-779: The change in the test_upfront_shutdown_script test case for closing the channel smoothly when the user opts out is correct.

1200-1200: The update in the simple_legacy_shutdown_test to use the ChannelId method for closing the channel is correct.

1240-1240: The change in the simple_target_feerate_shutdown test case to use the ChannelId method for closing the channel with a target feerate is correct.

1345-1345: The update in the outbound_update_no_early_closing_signed test case to use the ChannelId method after a force channel monitor update is correct.

lightning-background-processor/src/lib.rs (3)

932-932: The change from PaymentHash to ChannelId in the force_close_broadcasting_latest_txn call aligns with the PR's objective to use explicit channel IDs instead of deriving them from the funding outpoint.

1417-1417: The addition of the force_close_broadcasting_latest_txn call within the background processing loop is correct and uses the ChannelId as intended by the PR's objectives.

929-935: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [932-1417]

The test cases have been updated to use the ChannelId::v1_from_funding_outpoint method, which is consistent with the changes made in the main code. The tests also correctly simulate the force-closing of a channel using the new explicit channel ID.

fuzz/src/full_stack.rs (1)

25-25: The removal of specific imports related to the bitcoin::hashes module is consistent with the PR's objectives to remove the Outpoint::to_channel_id method.

lightning/src/ln/peer_handler.rs (1)

2010-2010: The update from log_funding_channel_id! to log_v1_funding_channel_id! is consistent with the PR's goal of using explicit V1 channel IDs for logging.

lightning/src/ln/monitor_tests.rs (6)

18-18: The addition of ChannelId to the ln module is in line with the PR's objectives to use ChannelId explicitly.

179-179: The update in the assert_eq! statement to use ChannelId::v1_from_funding_outpoint correctly reflects the removal of OutPoint::to_channel_id.

330-330: The assertion has been correctly updated to use the new ChannelId::v1_from_funding_outpoint method.

1124-1124: The assertion change is consistent with the PR's goal to use ChannelId explicitly.

1406-1406: The assert_eq! statement correctly uses the new method for generating V1 channel IDs.

1708-1708: The update to the assertion is in line with the changes made throughout the codebase to use ChannelId::v1_from_funding_outpoint.

lightning/src/ln/functional_test_utils.rs (2)

614-616: The explicit use of channel_id in the watch_channel function call aligns with the PR's objectives to use ChannelId explicitly.

1040-1042: The changes correctly use channel_id in the watch_channel function call and properly assert the expected status.

lightning/src/ln/chanmon_update_fail_tests.rs (29)

24-24: The import of ChannelId is correctly added to support the new changes in channel ID handling.

53-53: The channel_id is correctly derived from the channel creation function, aligning with the PR's objective to use explicit channel IDs.

84-84: The watch_channel method is updated to include the channel_id parameter, which is consistent with the changes described in the PR.

105-105: The update_channel method is correctly called with the channel_id parameter, and the logging behavior is consistent with the new logging macro usage.

Also applies to: 110-110

156-156: The force_channel_monitor_updated method is correctly updated to include the channel_id parameter.

316-316: The force_channel_monitor_updated method is consistently updated across the file to include the channel_id parameter.

625-625: The force_channel_monitor_updated method is used with the channel_id parameter, and the subsequent assertions are appropriate for the test context.

658-658: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

720-720: The force_channel_monitor_updated method is used with the channel_id parameter, and the test logic following the call is consistent with the expected outcomes.

782-782: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

908-908: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1163-1163: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1241-1241: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1363-1363: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1478-1478: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1571-1571: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1659-1659: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1759-1759: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1826-1826: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1865-1865: The ChannelId::v1_from_funding_outpoint method is correctly used to derive the channel_id from the funding outpoint, aligning with the PR's objective.

1876-1876: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

1922-1922: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

2023-2023: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

2398-2398: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

2617-2617: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

Also applies to: 2624-2624

2669-2669: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

Also applies to: 2672-2672

3116-3116: The channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

3144-3144: The channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

3313-3313: The force_channel_monitor_updated method is correctly called with the channel_id parameter, and the test logic is consistent with the expected behavior.

lightning/src/ln/payment_tests.rs (2)

1113-1113: The change correctly updates the retrieval of monitor updates to use the funding_txo as a key. This aligns with the PR's objective to explicitly use channel IDs.

1132-1132: The addition of chan_id to the channel_monitor_updated function call is consistent with the PR's goal of explicitly passing the channel ID where previously the funding outpoint was used.

lightning/src/chain/channelmonitor.rs (20)

161-162: The addition of the channel_id field to the Completed variant of the MonitorEvent enum is consistent with the PR's objective to use explicit channel IDs.

177-177: The update to the serialization logic to include the channel_id field is necessary due to the addition of the channel_id field in the Completed variant.

778-778: Adding an Option<ChannelId> field named channel_id to the ChannelMonitorImpl struct aligns with the PR's goal to explicitly handle channel IDs.

1104-1104: The serialization logic for ChannelMonitorImpl has been correctly updated to include the new channel_id field.

1168-1168: The from_impl function now correctly initializes the WithChannelMonitor struct with the channel_id obtained from monitor_impl.

1189-1190: The ChannelMonitor constructor has been updated to accept a channel_id parameter, which is a necessary change for explicit channel ID handling.

1244-1244: The ChannelMonitorImpl struct is correctly initialized with the channel_id passed to the constructor.

1396-1399: The get_channel_id method has been added to the ChannelMonitor struct to provide access to the channel ID, which is a logical addition following the removal of Outpoint::to_channel_id.

2849-2850: The get_latest_holder_commitment_txn method now uses self.get_channel_id() to log the channel ID, which is consistent with the new explicit channel ID handling.

2896-2898: The get_channel_id method in ChannelMonitorImpl now provides a fallback to ChannelId::v1_from_funding_outpoint if channel_id is None, which is a safe fallback for channels using V1 establishment.

3662-3662: The logging statement now correctly uses self.get_channel_id() to log the channel ID upon channel closure detection.

3832-3832: The Event::SpendableOutputs now includes the channel ID, which is necessary for identifying the channel associated with the spendable outputs.

4577-4577: The deserialization logic has been updated to include an optional channel_id, which is necessary due to the addition of the channel_id field in ChannelMonitorImpl.

4588-4588: The deserialization logic for ChannelMonitorImpl has been correctly updated to include the new channel_id field.

4613-4613: The ChannelMonitorImpl struct is correctly initialized with the channel_id during deserialization.

4688-4688: The import of ChannelId in the test module is necessary due to its usage in the updated test logic.

4864-4864: The test setup now correctly creates a ChannelId from the funding outpoint, reflecting the changes in the main codebase.

4884-4884: The test constructor for ChannelMonitor has been updated to include the channel_id parameter, which is consistent with the changes in the main codebase.

5114-5114: The test setup now correctly creates a ChannelId from the funding outpoint, which is consistent with the changes in the main codebase.

5132-5137: The test code now retrieves the channel_id from the ChannelMonitor and uses it in the context logger, which is a necessary update following the removal of Outpoint::to_channel_id.

lightning/src/ln/functional_tests.rs (17)

2431-2431: The update to the watch_channel method call to include the channel_id parameter aligns with the PR's objective to explicitly use ChannelId instead of the funding outpoint.

8472-8472: The introduction of the channel_id variable and its subsequent use in the test setup is consistent with the changes described in the PR.

8493-8493: The assertion for watch_channel now correctly includes the channel_id parameter, which is in line with the new method signature.

8515-8516: The update_channel method calls have been correctly updated to include the channel_id parameter, ensuring that the channel is properly identified during updates.

8543-8543: The channel_id variable is set up correctly for use in the test logic, which is consistent with the PR's changes.

8567-8567: The watch_channel method call is correctly updated to include the channel_id parameter, reflecting the changes made in the PR.

8599-8599: The watch_channel method call is correctly updated to include the channel_id parameter, reflecting the changes made in the PR.

8619-8621: The update_channel method calls are correctly updated to include the channel_id parameter, ensuring that the channel is properly identified during updates.

8688-8688: The creation of channel_id using ChannelId::v1_from_funding_outpoint and its use in the handle_error method is consistent with the PR's objectives.

9032-9032: The use of ChannelId::v1_from_funding_outpoint to check for closed events aligns with the PR's changes and the new channel ID handling.

9060-9060: The watch_channel method call is correctly updated to include the channel_id parameter, reflecting the changes made in the PR.

9093-9093: The assertion to check the equivalence of the ChannelId created from the funding outpoint with the real_channel_id is a good test to ensure the correctness of the ChannelId::v1_from_funding_outpoint method.

9185-9185: The creation of channel_id using ChannelId::v1_from_funding_outpoint is consistent with the PR's objectives and the new channel ID handling.

10639-10639: The update to the complete_sole_pending_chan_update method to include the channel_id parameter is in line with the PR's changes.

10696-10697: The creation of channel_id_1 and channel_id_2 using ChannelId::v1_from_funding_outpoint and their use in the test setup is consistent with the PR's objectives.

10770-10771: The creation of channel_id_1 and channel_id_2 using ChannelId::v1_from_funding_outpoint and their use in the force_close_broadcasting_latest_txn method is consistent with the PR's objectives.

10831-10831: The creation of chan_id using ChannelId::v1_from_funding_outpoint and its use in the test setup is consistent with the PR's objectives.

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

819-819: The addition of ChannelId to the monitor_update tuple in the ShutdownResult struct is consistent with the PR's objective to use explicit channel IDs.

2397-2397: The inclusion of self.channel_id() in the monitor update creation process aligns with the changes made to the ShutdownResult struct and is consistent with the PR's goal.

6478-6478: Setting self.context.channel_id using ChannelId::v1_from_funding_outpoint is appropriate and follows the new pattern established by the PR for handling channel IDs.

6817-6817: Passing self.context.channel_id() to the provide_holder_commitment_tx method is correct and ensures that the channel ID is explicitly used where needed.

7359-7359: The assignment of self.context.channel_id with ChannelId::v1_from_funding_outpoint is consistent with the PR's changes and the library's updated approach to channel IDs.

7377-7377: The use of self.context.channel_id() in the provide_holder_commitment_tx method is consistent with the other changes in the PR and correctly uses the explicit channel ID.

lightning/src/ln/channelmanager.rs (84)

290-290: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

326-326: Adding channel_id to the struct aligns with the changes made throughout the codebase to explicitly pass channel IDs.

367-367: The conversion from ClaimableHTLC to events::ClaimedHTLC now includes channel_id, which is in line with the PR's goal.

706-706: The tuple now includes ChannelId which is necessary for the explicit handling of channel IDs.

720-720: The addition of channel_id to MonitorUpdateRegeneratedOnStartup is consistent with the PR's objectives.

749-749: The optional tuple now includes ChannelId, which is a necessary change for explicit channel ID handling.

767-767: The addition of downstream_channel_id is consistent with the PR's objectives to use explicit channel IDs.

779-779: The inclusion of downstream_channel_id in the serialization is necessary for the explicit channel ID handling.

797-797: The addition of channel_id to ReleaseRAAChannelMonitorUpdate is consistent with the PR's objectives.

826-826: The addition of channel_id to RAAMonitorUpdateBlockingAction is consistent with the PR's objectives.

1630-1631: The comment clarifies the relationship between funding_txo and channel_id for V1-established channels, which is helpful for maintainability.

2289-2289: The macro now includes channel_id which is necessary for the explicit handling of channel IDs.

2300-2300: The update to the chain_monitor call to include channel_id is consistent with the PR's objectives.

2746-2746: The macro call now includes channel_id which is necessary for the explicit handling of channel IDs.

2857-2857: The update to the chain_monitor call to include channel_id is consistent with the PR's objectives.

3396-3396: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

3407-3407: The macro call now includes channel_id which is necessary for the explicit handling of channel IDs.

3956-3959: The previous comment regarding the macro usage in the context of batch funding for V1 channels is still valid.

4183-4183: The addition of prev_channel_id to the tuple is consistent with the PR's objective to use explicit channel IDs.

4211-4211: The addition of channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4235-4235: The addition of ChannelId to the phantom_receives vector is consistent with the PR's objectives.

4248-4248: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4326-4326: The addition of prev_channel_id to the tuple is consistent with the PR's objective to use explicit channel IDs.

4371-4371: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4383-4383: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4455-4455: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4490-4490: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4522-4522: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

4746-4746: The update to the chain_monitor call to include channel_id is consistent with the PR's objectives.

4751-4751: The update to the chain_monitor call to include channel_id is consistent with the PR's objectives.

5297-5297: The update to the logging context to include channel_id is consistent with the PR's objectives.

5344-5344: The addition of prev_channel_id to the event is consistent with the PR's objective to use explicit channel IDs.

5485-5485: The addition of prev_hop_chan_id to the local variable declarations is consistent with the PR's objectives.

5538-5538: The addition of chan_id to the local variable declarations is consistent with the PR's objectives.

5566-5566: The macro call now includes prev_hop_channel_id which is necessary for the explicit handling of channel IDs.

5577-5577: The addition of prev_hop_channel_id to the BackgroundEvent is consistent with the PR's objectives.

5592-5592: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

5608-5608: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

5640-5640: The addition of prev_hop_channel_id to the local variable declarations is consistent with the PR's objectives.

5667-5667: The addition of prev_hop_channel_id to the BackgroundEvent is consistent with the PR's objectives.

5686-5686: The addition of next_channel_id to the function parameters is consistent with the PR's objectives.

5696-5696: The addition of channel_id to the EventCompletionAction is consistent with the PR's objectives.

5704-5704: The addition of prev_channel_id to the local variable declarations is consistent with the PR's objectives.

5751-5751: The addition of channel_id to the BackgroundEvent is consistent with the PR's objectives.

5767-5767: The comparison of channel_id is consistent with the PR's objectives.

5777-5777: The addition of downstream_channel_id to the MonitorUpdateCompletionAction is consistent with the PR's objectives.

5791-5791: The addition of prev_channel_id to the event is consistent with the PR's objective to use explicit channel IDs.

5842-5842: The addition of channel_id to the MonitorUpdateCompletionAction is consistent with the PR's objectives.

5867-5867: The addition of channel_id to the return type is consistent with the PR's objectives.

5882-5882: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

5936-5936: The addition of channel_id to the channel_monitor_updated function parameters is consistent with the PR's objectives.

5958-5958: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

6376-6376: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

6435-6435: The addition of channel_id to the chain_monitor call is consistent with the PR's objectives.

6565-6565: The macro call now includes channel_id which is necessary for the explicit handling of channel IDs.

6772-6772: The addition of channel_id to the claim_funds_internal function parameters is consistent with the PR's objectives.

6847-6847: The macro call now includes channel_id which is necessary for the explicit handling of channel IDs.

6861-6861: The addition of channel_id to the forward_htlcs function parameters is consistent with the PR's objectives.

6881-6881: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

6899-6899: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

6928-6928: The addition of prev_channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

6972-6972: The addition of channel_id to the raa_monitor_updates_held function parameters is consistent with the PR's objectives.

6996-6996: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

7018-7018: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

7264-7264: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

7314-7314: The addition of channel_id to the MonitorEvent::Completed is consistent with the PR's objectives.

7367-7367: The macro call now includes channel_id which is necessary for the explicit handling of channel IDs.

7532-7532: The addition of channel_id to the BackgroundEvent is consistent with the PR's objectives.

8098-8098: The addition of channel_id to the handle_monitor_update_release function parameters is consistent with the PR's objectives.

8113-8113: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

8120-8120: The addition of channel_id to the raa_monitor_updates_held function parameters is consistent with the PR's objectives.

8130-8130: The addition of channel_id to the handle_new_monitor_update macro call is consistent with the PR's objectives.

8145-8145: The logging statement now includes channel_id which is necessary for the explicit handling of channel IDs.

8162-8162: The addition of channel_id to the EventCompletionAction is consistent with the PR's objectives.

8560-8560: The addition of channel_id to the struct is consistent with the PR's objective to use explicit channel IDs.

8572-8572: The update to the logging context to include channel_id is consistent with the PR's objectives.

9655-9655: The addition of channel_id to the serialization is necessary for the explicit channel ID handling.

9807-9807: The addition of prev_channel_id to the serialization is necessary for the explicit channel ID handling.

10321-10321: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

10358-10358: The addition of channel_id to the BackgroundEvent is consistent with the PR's objectives.

10439-10439: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

10618-10618: The addition of channel_id to the handle_in_flight_updates macro call is consistent with the PR's objectives.

10641-10641: The addition of channel_id to the BackgroundEvent is consistent with the PR's objectives.

10696-10696: The addition of channel_id to the local variable declarations is consistent with the PR's objectives.

fuzz/src/full_stack.rs

lightning/src/util/test_utils.rs

fuzz/src/chanmon_consistency.rs

dunxen · 2024-01-15T13:09:30Z

@coderabbitai: ignore

coderabbitai · 2024-01-15T13:09:45Z

@optout21 Noted, I will pause the automatic reviews on this pull request. If you need further assistance in the future, feel free to ask.

Action performed

Reviews paused.

lightning/src/chain/mod.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channelmanager.rs

dunxen · 2024-01-19T11:32:23Z

Can't reproduce this crash with that hex locally.

Fuzz crash

Crash: saved as 'hfuzz_workspace/chanmon_consistency_target/SIGABRT.PC.7ffff78969fc.STACK.c4c6d0d25.CODE.-6.ADDR.0.INSTR.mov____%eax,%r13d.fuzz'
Seen a crash. Terminating all fuzzing threads
Terminating thread no. #1, left: 1
Terminating thread no. #0, left: 0
Summary iterations:39008 time:685 speed:56 crashes_count:1 timeout_count:4 new_units_added:6 slowest_unit_ms:1062 guard_nb:0 branch_coverage_percent:0 peak_rss_mb:48
+ '[' -f hfuzz_workspace/chanmon_consistency_target/HONGGFUZZ.REPORT.TXT ']'
+ cat hfuzz_workspace/chanmon_consistency_target/HONGGFUZZ.REPORT.TXT
=====================================================================
TIME: 2024-01-18.12:59:46
=====================================================================
FUZZER ARGS:
 mutationsPerRun : 5
 externalCmd     : NULL
 fuzzStdin       : FALSE
 timeout         : 1 (sec)
 ignoreAddr      : (nil)
 ASLimit         : 0 (MiB)
 RSSLimit        : 0 (MiB)
 DATALimit       : 0 (MiB)
 wordlistFile    : NULL
 dynFileMethod   : 
 fuzzTarget      : hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target 
CRASH:
DESCRIPTION: 
ORIG_FNAME: 400ace291cf000003cb90edaa0b00000.00000005.honggfuzz.cov
FUZZ_FNAME: hfuzz_workspace/chanmon_consistency_target/SIGABRT.PC.7ffff78969fc.STACK.c4c6d0d25.CODE.-6.ADDR.0.INSTR.mov____%eax,%r13d.fuzz
PID: 11284
SIGNAL: SIGABRT (6)
PC: 0x7ffff78969fc
FAULT ADDRESS: 0x0
INSTRUCTION: mov____%eax,%r13d
STACK HASH: 0000000c4c6d0d25
STACK:
 <0x00007ffff7842476> [func:UNKNOWN file: line:0 module:/usr/lib/x86_64-linux-gnu/libc.so.6]
 <0x00007ffff78287f3> [func:UNKNOWN file: line:0 module:/usr/lib/x86_64-linux-gnu/libc.so.6]
 <0x00005555559f37a7> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a2af6> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a44e7> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555559f1229> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555559f0ca2> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555559ef4b4> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555559f0c39> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a3551> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a349d> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x0000555555974e1c> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x000055555597cf11> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a4338> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a4293> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a42a9> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555559eec9b> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00005555555a43b2> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
 <0x00007ffff7829d90> [func:UNKNOWN file: line:0 module:/usr/lib/x86_64-linux-gnu/libc.so.6]
 <0x00007ffff7829e40> [func:UNKNOWN file: line:0 module:/usr/lib/x86_64-linux-gnu/libc.so.6]
 <0x00005555555a41c5> [func:UNKNOWN file: line:0 module:/home/runner/work/rust-lightning/rust-lightning/fuzz/hfuzz_target/x86_64-unknown-linux-gnu/release/chanmon_consistency_target]
=====================================================================
+ for CASE in hfuzz_workspace/$FILE/SIG*
+ cat hfuzz_workspace/chanmon_consistency_target/SIGABRT.PC.7ffff78969fc.STACK.c4c6d0d25.CODE.-6.ADDR.0.INSTR.mov____%eax,%r13d.fuzz
+ xxd -p
3438380152ffffffffffff00002d7e32ca714f7574ffffffffffffffffff
ffffffffff626f756e64207570fffffffffffffffffffffffffffffffeba
aedce6af

wpaulino · 2024-01-19T18:33:00Z

3438380152ffffffffffff00002d7e32ca714f7574ffffffffffffffffff is the payload that's causing the crash. You should be able to reproduce it manually following https://github.com/lightningdevkit/rust-lightning/tree/main/fuzz#a-fuzz-test-failed-what-do-i-do.

This should fix it:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 32f2e3ad..a90d45ed 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -1397,11 +1397,11 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 					nodes[1].process_monitor_events();
 				}
 				if let Some((id, _)) = monitor_b.latest_monitors.lock().unwrap().get(&chan_2_funding) {
-					monitor_b.chain_monitor.force_channel_monitor_updated(chan_2_funding, chan_1_id, *id);
+					monitor_b.chain_monitor.force_channel_monitor_updated(chan_2_funding, chan_2_id, *id);
 					nodes[1].process_monitor_events();
 				}
 				if let Some((id, _)) = monitor_c.latest_monitors.lock().unwrap().get(&chan_2_funding) {
-					monitor_c.chain_monitor.force_channel_monitor_updated(chan_2_funding, chan_1_id, *id);
+					monitor_c.chain_monitor.force_channel_monitor_updated(chan_2_funding, chan_2_id, *id);
 					nodes[2].process_monitor_events();
 				}

lightning/src/chain/chainmonitor.rs

lightning-block-sync/src/init.rs

lightning/src/chain/chainmonitor.rs

lightning/src/chain/transaction.rs

lightning/src/ln/shutdown_tests.rs

lightning/src/util/macro_logger.rs

lightning/src/ln/channelmanager.rs

dunxen · 2024-01-19T21:35:15Z

3438380152ffffffffffff00002d7e32ca714f7574ffffffffffffffffff is the payload that's causing the crash. You should be able to reproduce it manually following https://github.com/lightningdevkit/rust-lightning/tree/main/fuzz#a-fuzz-test-failed-what-do-i-do.

Yeah I followed that and obviously did something wrong or my environment is whack. Thanks!

wpaulino

Almost there, just a few more minor cleanups but LGTM

lightning-persister/src/fs_store.rs

lightning/src/chain/chainmonitor.rs

lightning/src/util/persist.rs

fuzz/src/chanmon_consistency.rs

lightning/src/ln/shutdown_tests.rs

lightning/src/ln/channelmanager.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel_id.rs

lightning/src/ln/channelmanager.rs

lightning/src/chain/chainmonitor.rs

To avoid confusion and for accuracy going forward, we remove this method as it is inconsistent with channel IDs generated during V2 channel establishment. If one wants to create a V1, funding outpoint-based channel ID, then `ChannelId::v1_from_funding_outpoint` should be used instead. A large portion of the library has always made the assumption that having the funding outpoint will always allow us to generate the channel ID. This will not be the case anymore and we need to pass the channel ID along where appropriate. All channels that could have been persisted up to this point could only have used V1 establishment, so if some structures don't store a channel ID for them they can safely fall back to the funding outpoint-based version.

TheBlueMatt

Some minor comments, some of which should be addressed in a followup, but basically LGTM.

TheBlueMatt · 2024-01-30T22:26:26Z

lightning/src/ln/channelmanager.rs

@@ -1626,8 +1637,8 @@ pub struct ChannelDetails {
 	/// The Channel's funding transaction output, if we've negotiated the funding transaction with
 	/// our counterparty already.
 	///
-	/// Note that, if this has been set, `channel_id` will be equivalent to
-	/// `funding_txo.unwrap().to_channel_id()`.
+	/// Note that, if this has been set, `channel_id` for V1-established channels will be equivalent to


Almost worth just dropping the note now, no? I anticipate after we ship v2 it'll just be default and users wont really have a way to identify if a channel was v1 or v2 negotiated.

TheBlueMatt · 2024-01-30T22:27:43Z

lightning/src/ln/channelmanager.rs

@@ -2285,7 +2296,7 @@ macro_rules! handle_new_monitor_update {
 		handle_new_monitor_update!($self, $update_res, $chan, _internal,
 			handle_monitor_update_completion!($self, $peer_state_lock, $peer_state, $per_peer_state_lock, $chan))
 	};
-	($self: ident, $funding_txo: expr, $update: expr, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr) => { {
+	($self: ident, $funding_txo: expr, $channel_id: expr, $update: expr, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr) => { {


Looks like the new arg here is unused.

TheBlueMatt · 2024-01-30T22:45:41Z

lightning/src/ln/channelmanager.rs

@@ -9778,6 +9816,9 @@ impl_writeable_tlv_based!(PendingAddHTLCInfo, {
 	(2, prev_short_channel_id, required),
 	(4, prev_htlc_id, required),
 	(6, prev_funding_outpoint, required),
+	// Note that by the time we get past the required read for type 2 above, prev_funding_outpoint will be


Suggested change

// Note that by the time we get past the required read for type 2 above, prev_funding_outpoint will be

// Note that by the time we get past the required read for type 6 above, prev_funding_outpoint will be

TheBlueMatt · 2024-01-30T22:54:40Z

lightning/src/ln/channelmanager.rs

 						} = action {
 							if let Some(blocked_peer_state) = per_peer_state.get(&blocked_node_id) {
+								let channel_id = blocked_channel_id;


I don't see a reason to reassign this :)

TheBlueMatt · 2024-01-30T23:02:59Z

lightning/src/util/macro_logger.rs

@@ -41,24 +42,21 @@ macro_rules! log_bytes {
 pub(crate) struct DebugFundingChannelId<'a>(pub &'a Txid, pub u16);


This should either say v1 in the title or we should inline it in callsites/remove it.

v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan

optout21 reviewed Dec 18, 2023

View reviewed changes

dunxen force-pushed the 2023-12-purgetochannelid branch from 6397a45 to d5b05bc Compare January 8, 2024 08:18

dunxen marked this pull request as ready for review January 8, 2024 08:18

dunxen added the Dual-funding label Jan 8, 2024

dunxen marked this pull request as draft January 8, 2024 18:12

dunxen force-pushed the 2023-12-purgetochannelid branch 2 times, most recently from 78f4a4a to 426d1c4 Compare January 9, 2024 08:03

optout21 reviewed Jan 10, 2024

View reviewed changes

dunxen force-pushed the 2023-12-purgetochannelid branch from 426d1c4 to fb0fca5 Compare January 10, 2024 08:59

wpaulino reviewed Jan 10, 2024

View reviewed changes

fuzz/src/full_stack.rs Outdated Show resolved Hide resolved

wpaulino reviewed Jan 10, 2024

View reviewed changes

lightning/src/chain/transaction.rs Show resolved Hide resolved

dunxen force-pushed the 2023-12-purgetochannelid branch from fb0fca5 to 6b025f3 Compare January 11, 2024 08:08

dunxen marked this pull request as ready for review January 11, 2024 19:42

This comment was marked as resolved.

Sign in to view

dunxen force-pushed the 2023-12-purgetochannelid branch from 6b025f3 to 49f6f21 Compare January 15, 2024 05:47

coderabbitai bot reviewed Jan 15, 2024

View reviewed changes

fuzz/src/full_stack.rs Show resolved Hide resolved

lightning/src/util/test_utils.rs Outdated Show resolved Hide resolved

lightning/src/util/test_utils.rs Show resolved Hide resolved

fuzz/src/chanmon_consistency.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2023-12-purgetochannelid branch from 49f6f21 to 175de40 Compare January 15, 2024 14:01

TheBlueMatt reviewed Jan 16, 2024

View reviewed changes

lightning/src/chain/mod.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

dunxen force-pushed the 2023-12-purgetochannelid branch 3 times, most recently from ed3c7f5 to f9b7490 Compare January 18, 2024 12:32

wpaulino reviewed Jan 19, 2024

View reviewed changes

dunxen force-pushed the 2023-12-purgetochannelid branch 4 times, most recently from d26fd39 to b861d6e Compare January 22, 2024 15:12

wpaulino reviewed Jan 22, 2024

View reviewed changes

lightning-persister/src/fs_store.rs Show resolved Hide resolved

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

lightning/src/util/persist.rs Show resolved Hide resolved

dunxen force-pushed the 2023-12-purgetochannelid branch from b861d6e to 7fc8a56 Compare January 23, 2024 07:54

wpaulino reviewed Jan 23, 2024

View reviewed changes

fuzz/src/chanmon_consistency.rs Outdated Show resolved Hide resolved

lightning/src/ln/shutdown_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

optout21 reviewed Jan 23, 2024

View reviewed changes

dunxen force-pushed the 2023-12-purgetochannelid branch 2 times, most recently from 01aab17 to 32d7d55 Compare January 25, 2024 09:06

wpaulino previously approved these changes Jan 29, 2024

View reviewed changes

TheBlueMatt reviewed Jan 29, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

dunxen dismissed wpaulino’s stale review via 8930f59 January 30, 2024 10:08

dunxen force-pushed the 2023-12-purgetochannelid branch from 8930f59 to cf2c278 Compare January 30, 2024 10:31

wpaulino approved these changes Jan 30, 2024

View reviewed changes

TheBlueMatt approved these changes Jan 30, 2024

View reviewed changes

TheBlueMatt merged commit 7f177bb into lightningdevkit:main Jan 30, 2024
15 checks passed

dunxen mentioned this pull request Jan 31, 2024

Fix followups to PR2797 #2864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `Outpoint::to_channel_id` method #2797

Remove `Outpoint::to_channel_id` method #2797

dunxen commented Dec 15, 2023 •

edited by coderabbitai bot

Loading

codecov-commenter commented Dec 15, 2023 •

edited

Loading

optout21 commented Dec 18, 2023

optout21 left a comment

optout21 Jan 10, 2024

optout21 Jan 10, 2024

dunxen Jan 10, 2024

This comment was marked as resolved.

dunxen commented Jan 11, 2024

This comment was marked as resolved.

coderabbitai bot left a comment

dunxen commented Jan 15, 2024

coderabbitai bot commented Jan 15, 2024

dunxen commented Jan 19, 2024

wpaulino commented Jan 19, 2024

dunxen commented Jan 19, 2024

wpaulino left a comment

TheBlueMatt left a comment

TheBlueMatt Jan 30, 2024

TheBlueMatt Jan 30, 2024

TheBlueMatt Jan 30, 2024

TheBlueMatt Jan 30, 2024

TheBlueMatt Jan 30, 2024

	// Note that by the time we get past the required read for type 2 above, prev_funding_outpoint will be
	// Note that by the time we get past the required read for type 6 above, prev_funding_outpoint will be

		@@ -41,24 +42,21 @@ macro_rules! log_bytes {
		pub(crate) struct DebugFundingChannelId<'a>(pub &'a Txid, pub u16);

Remove Outpoint::to_channel_id method #2797

Remove Outpoint::to_channel_id method #2797

Conversation

dunxen commented Dec 15, 2023 • edited by coderabbitai bot Loading

Summary by CodeRabbit

codecov-commenter commented Dec 15, 2023 • edited Loading

Codecov Report

optout21 commented Dec 18, 2023

optout21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

dunxen commented Jan 11, 2024

This comment was marked as resolved.

coderabbitai bot left a comment

Choose a reason for hiding this comment

dunxen commented Jan 15, 2024

coderabbitai bot commented Jan 15, 2024

dunxen commented Jan 19, 2024

wpaulino commented Jan 19, 2024

dunxen commented Jan 19, 2024

wpaulino left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove `Outpoint::to_channel_id` method #2797

Remove `Outpoint::to_channel_id` method #2797

dunxen commented Dec 15, 2023 •

edited by coderabbitai bot

Loading

codecov-commenter commented Dec 15, 2023 •

edited

Loading