Preliminary refactoring & structure for dual-funded channels #2770
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
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
2 times, most recently
from
6055dd1
to
fc0dff5
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2770 +/- ##
==========================================
+ Coverage 89.05% 89.13% +0.08%
==========================================
Files 117 117
Lines 94244 94413 +169
Branches 94244 94413 +169
==========================================
+ Hits 83925 84158 +233
+ Misses 7835 7779 -56
+ Partials 2484 2476 -8 ☔ View full report in Codecov by Sentry. |
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
fc0dff5
to
86bce60
Compare
optout21
approved these changes
Dec 6, 2023
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
3 times, most recently
from
b0224a0
to
23febee
Compare
|
Sorry this is gonna need a rebase after #2782 to include the new |
TheBlueMatt
reviewed
Dec 11, 2023
There was a problem hiding this comment.
Basically LGTM but man that first commit is gonna require a followup with a ton of work. I think we should wholesale remove OutPoint::to_channel_id(), as nearly every callsite of it will be broken by v2 channels and it'd be nice to not expose the confusion to our users, plus be guaranteed we'll touch/rewrite every line that uses it. It'll be a slog but should be doable, in part cause we can add new fields that we can guarantee are filled in for v2 channels, so we can always do self.channel_id.unwrap_or(ChannelId::from_v1_outpoint(self.funding_txo)).
Hmm didn't consider this, thanks, but I think they do belong in this PR, maybe? I'll see how much work it involves. Edit (after looking at the slog): cries |
I love rebasing for breakfast! |
Yea, uhhhh, it definitely is big enough to be its own PR. Sorry. Luckily most of it will be formulaic once you get started, I think. |
Ok I'm mostly done with fixing this and it doesn't seem too bad. Just have to get through getting all tests to compile again. I'll open another PR so it can get more focused review. |
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
23febee
to
7d10850
Compare
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughA significant update has been made across various files, primarily introducing and integrating 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: 14
Configuration used: CodeRabbit UI
Files selected for processing (30)
- .gitignore (1 hunks)
- ci/check-cfg-flags.py (1 hunks)
- ci/ci-tests.sh (1 hunks)
- fuzz/src/chanmon_consistency.rs (8 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 (22 hunks)
- lightning/src/ln/channel_id.rs (3 hunks)
- lightning/src/ln/channel_keys.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (99 hunks)
- lightning/src/ln/functional_test_utils.rs (4 hunks)
- lightning/src/ln/functional_tests.rs (18 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 (2)
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
- lightning/src/ln/channelmanager.rs: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 336
fuzz/src/utils/test_persister.rs (2)
- 14-14: The addition of the
_channel_idparameter to thepersist_new_channelfunction aligns with the PR's objective to refactor the codebase for dual-funded channel support.- 18-18: The addition of the
_channel_idparameter to theupdate_persisted_channelfunction is consistent with the changes made topersist_new_channeland is necessary for the dual-funded channel feature.lightning/src/chain/transaction.rs (2)
- 82-82: The removal of the
OutPoint::to_channel_idmethod is in line with the PR's objectives to avoid confusion with the introduction of v2 channels.- 91-98: The test cases have been updated to use
ChannelIddirectly, which is consistent with the removal of theOutPoint::to_channel_idmethod.ci/check-cfg-flags.py (1)
- 95-96: The inclusion of the "dual_funding" configuration in the
check_cfg_tagfunction is necessary to support the new feature flag introduced by the PR.lightning/src/ln/channel_id.rs (3)
- 52-54: The
v1_from_funding_outpointmethod has been correctly added to theChannelIdstruct to facilitate the creation of v1 channel IDs from funding transaction outpoints.- 81-92: The
v2_from_revocation_basepointsmethod has been added to support the creation of v2 channel IDs using revocation basepoints, aligning with the PR's objectives for dual-funded channel support.- 96-97: The
temporary_v2_from_revocation_basepointmethod provides a way to create temporary v2 channel IDs, which is consistent with the new channel identification mechanisms introduced in the PR.ci/ci-tests.sh (1)
- 179-179: The addition of the test command with the
dual_fundingconfiguration flag is necessary to ensure that the new feature is tested properly.lightning/src/util/macro_logger.rs (2)
- 45-45: The update to the
DebugFundingChannelIdstructure to useChannelIdinstead ofOutPointis consistent with the PR's objectives and the broader shift in channel identification.- 57-57: The
DebugFundingInfostructure has been correctly updated to useChannelId, which aligns with the changes made to the logging functionality.lightning/src/ln/channel_keys.rs (1)
- 167-167: The addition of the
PartialOrdtrait to theRevocationBasepointstruct is a minor change that enhances the functionality of the struct without affecting existing behavior.lightning-block-sync/src/init.rs (2)
- 52-52: The import of
ChannelIdis correct and necessary for the updatedwatch_channelfunction call.- 123-125: The
watch_channelfunction call has been correctly updated to include thechannel_idparameter, aligning with the changes in theWatchtrait.lightning-persister/src/fs_store.rs (4)
- 453-454: The retrieval of
channel_idfrom theChannelMonitoris correct and aligns with the updatedpersist_new_channelfunction signature.- 468-468: The updated
persist_new_channelfunction call correctly includes thechannel_idparameter.- 493-495: The changes to retrieve
funding_outpointandchannel_idfor thepersist_new_channelfunction call are correct.- 507-507: The
persist_new_channelfunction call has been correctly updated to include thechannel_idparameter in the test case.lightning/src/chain/mod.rs (3)
- 273-273: The
watch_channelmethod signature has been correctly updated to include achannel_idparameter.- 290-290: The
update_channelmethod signature has been correctly updated to include achannel_idparameter.- 301-301: The
release_pending_monitor_eventsmethod now correctly includes thechannel_idin its return type.lightning/src/ln/reorg_tests.rs (2)
- 28-28: The import statement has been correctly updated to include
ChannelId, which is consistent with the changes described in the PR objectives.- 534-534: The assertion has been updated to use
ChannelId::v1_from_funding_outpoint, which aligns with the new channel identification mechanism introduced in the PR.lightning/src/util/persist.rs (6)
- 197-197: The addition of the
_channel_idparameter in thepersist_new_channelmethod signature is consistent with the PR's objectives to support dual-funded channels.- 209-209: Similarly, the addition of the
_channel_idparameter in theupdate_persisted_channelmethod signature aligns with the PR's goals.- 610-610: The
persist_new_channelmethod in theMonitorUpdatingPersisterstruct correctly includes the new_channel_idparameter, aligning with the PR's objectives.- 654-654: The
update_persisted_channelmethod in theMonitorUpdatingPersisterstruct also correctly includes the newchannel_idparameter.- 1069-1069: The test
unrecoverable_error_on_write_failurecorrectly asserts thatpersist_new_channelshould return anUnrecoverableErrorwhen the underlying store is read-only.- 1080-1080: The test
unrecoverable_error_on_write_failurealso correctly asserts thatupdate_persisted_channelshould return anUnrecoverableErrorwhen the underlying store is read-only.lightning/src/util/test_utils.rs (6)
- 292-292: The method
complete_sole_pending_chan_updatenow uses thechannel_idparameter in thechannel_monitor_updatedcall. Verify that this change is consistent with the new channel identification mechanism and that all instances where this method is used have been updated accordingly.- 296-307: The addition of the
channel_idparameter to thewatch_channelmethod signature is consistent with the changes described in the PR summary.- 289-332: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [310-347]
The addition of the
channel_idparameter to theupdate_channelmethod signature and the related logic to handle channel force closure and monitor round trip failure is consistent with the changes described in the PR summary.
- 347-347: The addition of the
channel_idparameter to therelease_pending_monitor_eventsmethod signature is consistent with the changes described in the PR summary.- 399-408: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [402-427]
The changes to the
persist_new_channelandupdate_persisted_channelmethods in theWatchtowerPersisterstruct, including the addition of thechannel_idparameter, are consistent with the PR's objectives.
- 482-498: The changes to the
persist_new_channelandupdate_persisted_channelmethods in theTestPersisterstruct, including the addition of thechannel_idparameter and the handling of monitor update IDs, are consistent with the PR's objectives.lightning/src/ln/priv_short_conf_tests.rs (3)
- 620-620: The line
let channel_id = ChannelId::v1_from_funding_outpoint(funding_output);introduces a new way to derive theChannelId. We need to ensure that this method is consistent with the rest of the codebase and that theChannelIdis being used correctly in all instances.Verification successful
The search for usages of
v1_from_funding_outpointacross the codebase has returned multiple instances where this method is used to derive aChannelIdfrom a funding outpoint. The method appears in various contexts, including channel closure, channel monitoring, and channel updates, which is consistent with the expected usage pattern for channel identification in the context of the Lightning Network.Given the widespread and consistent use of
ChannelId::v1_from_funding_outpointacross different files and scenarios, it seems that the usage of this method inpriv_short_conf_tests.rsis in line with the rest of the codebase. This suggests that the method is being used correctly and is part of the refactoring effort to support dual-funded channels.* 728-728: The call to `chain_monitor.chain_monitor.channel_monitor_updated(outpoint, channel_id, latest_update)` is critical for channel state consistency. It's important to verify that the `channel_id` and `outpoint` are being correctly identified and updated across reorgs and other chain changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of `v1_from_funding_outpoint` to ensure consistency. ast-grep --lang rust --pattern $'ChannelId::v1_from_funding_outpoint($_)'Length of output: 6150
Verification successful
The
channel_monitor_updatedmethod is used consistently throughout the codebase with the expected parameters, includingOutPointandChannelId, which aligns with the review comment's concern for maintaining channel state consistency. The method's usage in the provided code snippet is in line with its usage in other parts of the codebase, suggesting that thechannel_idandoutpointare being correctly identified and updated.* 21-21: The tests should be thoroughly checked to ensure that they are correctly using the `ChannelId` where appropriate, and that the logic of the tests reflects the intended behavior with the new changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `channel_monitor_updated` method is consistently used with the correct parameters. ast-grep --lang rust --pattern $'chain_monitor.chain_monitor.channel_monitor_updated($_, $_, $_)'Length of output: 98
Script:
#!/bin/bash # Search for all instances of the `channel_monitor_updated` method to analyze its usage. ast-grep --lang rust --pattern $'chain_monitor.chain_monitor.channel_monitor_updated($$$)'Length of output: 91
Script:
#!/bin/bash # Use ripgrep to search for the `channel_monitor_updated` method in the codebase. rg 'channel_monitor_updated'Length of output: 9616
lightning/src/chain/chainmonitor.rs (12)
- 291-291: The addition of
channel_idto thepending_monitor_eventsfield in theMonitorHolderstruct is consistent with the summary and the overall goal of the PR to support dual-funded channels.- 325-326: The update to use
channel_idwithin theChainMonitorstruct's methods is consistent with the PR's objectives and the AI-generated summary.- 341-342: The use of
channel_idin theChainMonitorstruct's methods is correctly implemented and aligns with the PR's objectives.- 362-362: The addition of
channel_idto theupdate_monitor_with_chain_datamethod is consistent with the PR's objectives and the AI-generated summary.- 383-383: The call to
update_persisted_channelwithchannel_idis correctly implemented and aligns with the PR's objectives.- 481-485: The
list_monitorsmethod correctly returns a vector of tuples containingOutPointandChannelId, which is consistent with the PR's objectives.- 723-723: The
watch_channelmethod correctly takeschannel_idin addition tofunding_outpoint, which is consistent with the PR's objectives.- 762-762: The
update_channelmethod correctly takeschannel_idin addition tofunding_txo, which is consistent with the PR's objectives.- 825-825: The
release_pending_monitor_eventsmethod correctly returns a vector of tuples containingOutPoint,ChannelId,MonitorEvent, andOption<PublicKey>, which is consistent with the PR's objectives.- 915-915: The test
test_async_ooo_offchain_updatescorrectly asserts the behavior of theChainMonitorwhen dealing with out-of-order offchain updates.- 1048-1050: The test
do_chainsync_pauses_eventscorrectly simulates the behavior of theChainMonitorwhen dealing with chainsync monitor updates.- 940-940: The test
chainsync_pauses_eventscorrectly asserts the behavior of theChainMonitorwhen dealing with chainsync events.lightning/src/ln/reload_tests.rs (6)
- 19-19: The import changes to include
ChannelIdare consistent with the PR's goal to refactor the codebase for dual-funded channel support.- 449-451: The update to use
ChannelIddirectly when watching a channel in thechain_monitoris correct and aligns with the changes made throughout the codebase.- 834-835: The serialization of the
ChannelMonitorfor a specific channel usingChannelIdis a critical operation. It's important to verify that the serialization and deserialization processes are correctly handling the newChannelIdstructure to prevent any data corruption or loss.- 854-855: The retrieval and serialization of the updated
ChannelMonitorusing the newChannelIdis consistent with the changes and the PR's objectives.- 862-863: The conditional logic to serialize the second
ChannelMonitorifpersist_both_monitorsis set, is a good safety check to ensure that both monitors are updated when required.- 1225-1225: The reloading of the node with a subset of channels having persisted monitors needs to be carefully verified to ensure that the state is consistent and no data is lost or corrupted during the reload process. This is especially important for the correct handling of channel states and funds.
Verification successful
Based on the information gathered from the executed scripts, the following observations have been made:
The
reload_nodefunction is used in various test files, indicating that the reloading process is indeed tested in different scenarios. This is seen in files likechanmon_update_fail_tests.rs,reorg_tests.rs,payment_tests.rs, andreload_tests.rs.The
ChannelMonitorstruct and related functionality are extensively used throughout the codebase, including in the context of reloading nodes. This is evident from the numerous mentions inpersist.rs,test_utils.rs,functional_test_utils.rs, andreload_tests.rs.The
v1_from_funding_outpointfunction is used to create aChannelIdfrom a funding outpoint. This function is utilized in various parts of the code, including in tests that likely simulate channel creation and operations related to channel identifiers.The
test.*reloadpattern matches several test files, suggesting that there are tests specifically designed to verify the reloading process and its impact on the state consistency of nodes.Given these observations, it can be concluded that the reloading process, particularly with respect to the persistence of
ChannelMonitorinstances, is an area covered by tests in the codebase. The use ofv1_from_funding_outpointin creatingChannelIdinstances is consistent with the changes in the PR, which aim to shift towards usingChannelIdfor channel identification.The review comment about verifying the reloading of the node with a subset of channels having persisted monitors to ensure state consistency and correct handling of channel states and funds is aligned with the observed testing patterns in the codebase. The tests found in
reload_tests.rsand other files likely cover scenarios that would detect issues related to state consistency during the reload process.Therefore, the review comment is consistent with the codebase's approach to testing and the importance of verifying the reload process, especially in the context of
ChannelMonitorpersistence and state consistency.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the reload process with partial channel monitor persistence. rg --type rust "reload_node.*ChannelMonitor"Length of output: 46
Script:
#!/bin/bash # Since the direct search for 'reload_node.*ChannelMonitor' did not yield results, # let's try a broader search for 'reload_node' to find relevant context. rg --type rust "reload_node"Length of output: 7781
Script:
#!/bin/bash # Additionally, let's search for the usage of 'ChannelMonitor' to understand how it's being used in the context of reloading. rg --type rust "ChannelMonitor"Length of output: 104354
Script:
#!/bin/bash # It might also be helpful to look for the function 'v1_from_funding_outpoint' to see how it's used in the context of reloading. rg --type rust "v1_from_funding_outpoint"Length of output: 7195
Script:
#!/bin/bash # Lastly, let's check for any tests that might be related to reloading the node to ensure the state consistency. rg --type rust --files-with-matches "test.*reload"Length of output: 295
fuzz/src/chanmon_consistency.rs (24)
- 43-43: The import of
ChannelIdis necessary due to the changes in the channel monitoring functions to include thechannel_idparameter.- 170-176: The
watch_channelfunction has been correctly updated to include thechannel_idparameter, which is necessary for the new channel identification mechanism.- 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_channelfunction has been correctly updated to include thechannel_idparameter. This change is consistent with the new channel identification mechanism.
- 194-194: The return type of
release_pending_monitor_eventshas been correctly updated to includeChannelId, aligning with the changes in channel identification.- 1066-1066: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1072-1072: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1078-1078: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1084-1084: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1298-1298: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1305-1305: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1312-1312: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1320-1320: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1327-1327: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1334-1334: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1342-1342: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1349-1349: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1356-1356: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1364-1364: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1371-1371: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1378-1378: The call to
channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1393-1393: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1397-1397: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1401-1401: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.- 1405-1405: The call to
force_channel_monitor_updatedwith thechannel_idparameter is consistent with the updated channel monitoring logic.lightning/src/ln/shutdown_tests.rs (24)
- 15-21: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-17]
The imports and initial test setup look correct and are consistent with the typical Rust testing pattern.
- 15-21: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [18-50]
The test
pre_funding_lock_shutdown_testis well-structured, follows the typical pattern for testing in Rust, and uses the appropriate assertions and test utilities.
- 48-54: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [51-120]
The test
expect_channel_shutdown_statecorrectly tests the channel shutdown state transitions and uses the appropriate assertions.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [121-240]
The test
expect_channel_shutdown_state_with_htlccorrectly tests the channel shutdown state with outstanding HTLCs and follows the expected behavior for handling HTLCs during shutdown.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [241-320]
The test
test_lnd_bug_6039is designed to emulate a specific bug behavior and ensure compatibility. The test is correctly implemented and uses the expected message handling.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [321-360]
The test
shutdown_on_unfunded_channelcorrectly tests the behavior of receiving a shutdown message before the channel is funded, which is an edge case that should be handled properly.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [361-380]
The test
close_on_unfunded_channelcorrectly tests the behavior of the user requesting to close a channel before it is funded, which is another important edge case.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [381-420]
The test
expect_channel_shutdown_state_with_force_closurecorrectly tests the channel shutdown state transitions in the case of a force closure, ensuring that the state transitions are as expected.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [421-520]
The test
updates_shutdown_waitcorrectly tests the behavior of sending a shutdown with outstanding updates pending, which is a critical scenario to ensure proper channel closure.
- 15-21: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [521-640]
The test
htlc_fail_async_shutdowncorrectly tests the behavior of HTLC failure during an asynchronous shutdown process, which is a complex scenario that needs to be handled correctly.
- 718-724: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [641-720]
The helper function
do_htlc_fail_async_shutdownis used within thehtlc_fail_async_shutdowntest and is correctly implemented to support the test logic.
- 718-724: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [721-736]
The test
test_shutdown_rebroadcastcorrectly tests the rebroadcast logic of shutdown and closing_signed messages upon reconnection, which is important for ensuring robustness in the face of network issues.
- 747-753: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [737-750]
The test
test_upfront_shutdown_scriptcorrectly tests the behavior of upfront shutdown scripts, ensuring that the implementation adheres to the BOLT 2 specification.
- 762-768: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [751-765]
The test
test_unsupported_anysegwit_upfront_shutdown_scriptcorrectly tests the behavior when an unsupported anysegwit script is used in the upfront shutdown script, which is a good negative test case.
- 776-782: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [766-779]
The test
test_invalid_upfront_shutdown_scriptcorrectly tests the behavior when an invalid upfront shutdown script is provided, ensuring that the error handling is correct.
- 891-897: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [780-894]
The test
test_segwit_v0_shutdown_scriptcorrectly tests the behavior with a segwit v0 script, which is a valid script type for shutdown, ensuring compatibility with the BOLT 2 specification.
- 926-932: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [895-929]
The test
test_anysegwit_shutdown_scriptcorrectly tests the behavior with a non-v0 segwit script supported by option_shutdown_anysegwit, ensuring that the implementation is compliant with the extended BOLT 2 specification.
- 972-985: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [930-982]
The test
test_unsupported_anysegwit_shutdown_scriptcorrectly tests the behavior when an unsupported anysegwit script is used in the shutdown process, ensuring that the error handling is correct.
- 1004-1010: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [983-1007]
The test
test_invalid_shutdown_scriptcorrectly tests the behavior when an invalid shutdown script is used, ensuring that the error handling is correct and that non-standard scripts are rejected.
- 1038-1044: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1008-1041]
The test
test_user_shutdown_scriptcorrectly tests the behavior when a user-provided shutdown script is used, ensuring that the implementation allows for user customization of the shutdown script.
- 1065-1071: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1042-1068]
The test
test_already_set_user_shutdown_scriptcorrectly tests the behavior when a user attempts to override an already set shutdown script, ensuring that the API misuse error is correctly returned.
- 1197-1203: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1069-1200]
The test
do_test_closing_signed_reinit_timeoutcorrectly tests the fundee side of the closing_signed restart mechanism, ensuring that the implementation can handle a funder restarting the closing_signed negotiation with a new fee range.
- 1237-1243: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1201-1240]
The test
test_closing_signed_reinit_timeoutcorrectly tests the closing_signed reinitialization timeout, ensuring that the implementation can handle timeouts during the closing_signed negotiation.
- 1342-1348: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1241-1345]
The test
do_simple_legacy_shutdown_testcorrectly tests the legacy shutdown fee negotiation logic, ensuring that the implementation can handle the legacy closing_signed negotiation process.lightning-background-processor/src/lib.rs (70)
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-1]
The file header comment provides a clear description of the module's purpose.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [2-2]
The
#![deny(rustdoc::broken_intra_doc_links)]attribute is correctly used to ensure all intra-doc links are valid.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [3-3]
The
#![deny(rustdoc::private_intra_doc_links)]attribute is correctly used to prevent linking to private items in documentation.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [4-4]
The
#![deny(missing_docs)]attribute is correctly used to ensure all public items have documentation.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [5-5]
The
#![cfg_attr(not(feature = "futures"), deny(unsafe_code))]attribute is correctly used to deny the use of unsafe code when thefuturesfeature is not enabled.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [7-7]
The
#![cfg_attr(docsrs, feature(doc_auto_cfg))]attribute is correctly used to automatically document feature flags in the generated documentation.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [9-9]
The
#![cfg_attr(all(not(feature = "std"), not(test)), no_std)]attribute is correctly used to make the libraryno_stdcompatible when neither thestdnortestfeatures are enabled.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [11-13]
The conditional external crate declarations are correctly used to include
coreandallocin non-stdenvironments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [15-15]
The
#[macro_use] extern crate lightning;statement is correctly used to import macros from thelightningcrate.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [16-16]
The
extern crate lightning_rapid_gossip_sync;statement is correctly used to import thelightning_rapid_gossip_synccrate.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [18-39]
The import statements are correctly organized and provide the necessary modules for the functionality of this file.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [41-41]
The use of
core::ops::Derefis appropriate for trait implementations that require dereferencing.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [42-42]
The use of
core::time::Durationis appropriate for representing time durations.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [44-48]
The conditional compilation for
stdfeature and usage ofArc,AtomicBool,Ordering,thread, andInstantare correctly handled.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [50-50]
The conditional compilation for non-
stdfeature and usage ofalloc::vec::Vecis correctly handled.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [52-90]
The documentation for
BackgroundProcessoris comprehensive and provides a clear explanation of its responsibilities and behavior.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [92-92]
The
BackgroundProcessorstruct is correctly marked with#[must_use]to indicate that it should not be left unused.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [94-94]
The
FRESHNESS_TIMERconstant is correctly defined with different values for testing and non-testing environments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [96-101]
The
PING_TIMERconstant is correctly defined with different values for testing, non-testing, and debug assertion environments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [103-103]
The
ONION_MESSAGE_HANDLER_TIMERconstant is correctly defined with different values for testing and non-testing environments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [105-105]
The
NETWORK_PRUNE_TIMERconstant is correctly set to prune the network graph of stale entries hourly.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [107-107]
The
SCORER_PERSIST_TIMERconstant is correctly defined with different values for testing and non-testing environments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [109-109]
The
FIRST_NETWORK_PRUNE_TIMERconstant is correctly defined with different values for testing and non-testing environments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [111-111]
The
REBROADCAST_TIMERconstant is correctly defined with different values for testing and non-testing environments.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [113-117]
The
min_u64function andFASTEST_TIMERconstant are correctly defined for use with thefuturesfeature.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [119-128]
The
GossipSyncenum is correctly defined to represent different gossip sync strategies.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [130-144]
The
GossipSyncimplementation block is correctly using generics and provides a method to retrieve theNetworkGraph.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [146-156]
The
prunable_network_graphmethod is correctly implemented to handle the case where rapid gossip sync is not complete.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [158-166]
The
GossipSync::P2Pvariant constructor is correctly implemented.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [168-176]
The
GossipSync::Rapidvariant constructor is correctly implemented.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [178-186]
The
GossipSync::Nonevariant constructor is correctly implemented.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [188-197]
The
handle_network_graph_updatefunction is correctly implemented to handle network updates based on payment path failures.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [199-224]
The
update_scorerfunction is correctly implemented to update the scorer based on various payment path events.
- 929-935: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [226-932]
The
define_run_bodymacro is correctly defined and encapsulates the main loop of the background processor, handling various timer-based events and persistence logic.
- 934-934: The
futures_utilmodule is correctly defined under thefuturesfeature flag and provides utilities for futures processing.- 1414-1420: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [936-1416]
The
process_events_asyncfunction is correctly implemented to process background events in a future, with appropriate handling for mobile platforms and time fetching.
- 1418-1418: The
process_onion_message_handler_events_asyncfunction is correctly implemented to process onion message handler events asynchronously.- 1420-1420: The
BackgroundProcessorimplementation for thestdfeature is correctly defined with methods to start, join, stop, and drop the background processor.- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1422-1422]
The
startmethod is correctly implemented to start the background processor thread with appropriate error handling and event processing.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1424-1424]
The
joinmethod is correctly implemented to join the background processor's thread and return any errors that occurred.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1426-1426]
The
stopmethod is correctly implemented to stop the background processor's thread and return any errors that occurred.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1428-1428]
The
stop_and_join_threadandjoin_threadprivate methods are correctly implemented to handle stopping and joining the background processor's thread.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1430-1430]
The
Dropimplementation forBackgroundProcessoris correctly implemented to ensure the thread is stopped and joined when the processor is dropped.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1434-1434]
The
TestDescriptorstruct is correctly implemented for use in tests as aSocketDescriptor.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1436-1436]
The
LockingWrappertype alias is correctly defined based on thec_bindingsfeature flag.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1438-1438]
The
ChannelManagerandChainMonitortype aliases are correctly defined for use in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1440-1440]
The
PGSandRGStype aliases for gossip sync are correctly defined.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1442-1462]
The
Nodestruct is correctly implemented to encapsulate the components of a node for testing.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1464-1464]
The
Dropimplementation forNodeis correctly implemented to clean up the test store directory.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1466-1466]
The
Persisterstruct is correctly implemented to simulate persistence operations in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1468-1468]
The
TestScorerstruct is correctly implemented to simulate scoring in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1470-1470]
The
TestResultenum is correctly defined to represent expected test outcomes.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1472-1472]
The
get_full_filepathfunction is correctly implemented to construct file paths for persistence in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1474-1474]
The
create_nodesfunction is correctly implemented to set up nodes for testing.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1476-1476]
The
open_channelmacro is correctly defined to facilitate channel opening in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1478-1478]
The
begin_open_channelmacro is correctly defined to initiate the channel opening process in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1480-1480]
The
handle_funding_generation_readymacro is correctly defined to handleFundingGenerationReadyevents in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1482-1482]
The
confirm_transaction_depthandconfirm_transactionfunctions are correctly implemented to confirm transactions in tests.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1484-1484]
The
test_background_processortest function is correctly implemented to test the background processor's functionality.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1486-1486]
The
test_timer_tick_calledtest function is correctly implemented to test timer-based event handling.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1488-1488]
The
test_channel_manager_persist_errortest function is correctly implemented to test error handling during manager persistence.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1490-1490]
The
test_channel_manager_persist_error_asynctest function is correctly implemented to test error handling during manager persistence in an async context.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1492-1492]
The
test_network_graph_persist_errortest function is correctly implemented to test error handling during network graph persistence.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1494-1494]
The
test_scorer_persist_errortest function is correctly implemented to test error handling during scorer persistence.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1496-1496]
The
test_background_event_handlingtest function is correctly implemented to test event handling by the background processor.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1498-1498]
The
test_scorer_persistencetest function is correctly implemented to test the persistence of the scorer.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1500-1500]
The
test_not_pruning_network_graph_until_graph_sync_completiontest function is correctly implemented to test the pruning behavior of the network graph.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1502-1502]
The
test_not_pruning_network_graph_until_graph_sync_completion_asynctest function is correctly implemented to test the pruning behavior of the network graph in an async context.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1504-1504]
The
test_payment_path_scoringtest function is correctly implemented to test the scoring of payment paths.
- 929-935: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1506-1506]
The
test_payment_path_scoring_asynctest function is correctly implemented to test the scoring of payment paths in an async context.fuzz/src/full_stack.rs (2)
- 25-25: The wildcard import for
bitcoin::hashesis a common pattern in Rust when multiple items from the same module are used.- 653-653: The change from comparing
chan.channel_idwithoutpoint.to_channel_id()tochan.funding_txowithSome(outpoint)is consistent with the PR's goal to refactor the codebase for dual-funded channel support.lightning/src/ln/peer_handler.rs (1)
- 2010-2010: The change from
log_funding_channel_id!tolog_v1_funding_channel_id!is consistent with the PR's objectives to support dual-funded channels and the distinction between v1 and v2 channel formats.lightning/src/ln/monitor_tests.rs (6)
- 18-18: The addition of the
ChannelIdtype to thelnmodule is consistent with the PR's objectives to support dual-funded channels.- 179-179: The assertion change to use
ChannelId::v1_from_funding_outpointis correct and aligns with the refactoring goal to useChannelIdfor channel identification.- 330-330: The assertion update to use
ChannelId::v1_from_funding_outpointis appropriate and reflects the new method of obtaining channel IDs.- 1124-1124: The assertion is correctly updated to use the new
ChannelIdtype, ensuring consistency with the updated channel identification mechanism.- 1406-1406: The use of
ChannelId::v1_from_funding_outpointfor the assertion is correct, indicating the proper transition to the new channel identification method.- 1708-1708: The assertion correctly uses
ChannelId::v1_from_funding_outpoint, which is in line with the changes made across the codebase for channel identification.lightning/src/ln/functional_test_utils.rs (4)
- 252-252: The change to iterate over a tuple of
funding_outpointand_channel_idis consistent with the PR's objectives to handle new channel identification mechanisms.- 571-571: The iteration over a tuple of
outpointand_channel_idaligns with the PR's goal of updating components to handle thechannel_idfield, even though_channel_idis not used here.- 614-616: The retrieval and use of
funding_outpointandchannel_idfromdeserialized_monitorfor thewatch_channelcall is correct and reflects the PR's intent to useChannelIdfor channel identification.- 1040-1042: The use of
funding_outpointandchannel_idin thewatch_channelcall and the subsequent monitor check are appropriate and align with the PR's changes.lightning/src/ln/chanmon_update_fail_tests.rs (26)
- 24-24: The import changes are correct and align with the PR's objectives to refactor the codebase for dual-funded channel support.
- 53-53: The variable
channel_idis correctly assigned from the channel tuple, which is consistent with the PR's goal to useChannelIdfor channel identification.- 84-84: The
watch_channelfunction is correctly updated to include thechannel_idparameter, which is in line with the changes described in the PR.- 105-110: The
update_channelfunction is correctly updated to include thechannel_idparameter, and the test logic is consistent with the expected behavior of theChainMonitor.- 156-156: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 316-316: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 625-625: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 658-658: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 720-720: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 782-782: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1241-1241: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1363-1363: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1478-1478: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1571-1571: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1659-1659: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1826-1826: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1865-1865: The
ChannelId::v1_from_funding_outpointfunction is correctly used to derive thechannel_idfrom the funding outpoint, which is consistent with the PR's objectives.- 1876-1876: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 1922-1922: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 2023-2023: The
force_channel_monitor_updatedfunction is correctly updated to include thechan_2_idparameter, and the test checks are appropriate.- 2398-2398: The
force_channel_monitor_updatedfunction is correctly updated to include thechan_idparameter, and the test checks are appropriate.- 2617-2624: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 2669-2672: The
force_channel_monitor_updatedfunction is correctly updated to include thechannel_idparameter, and the test checks are appropriate.- 3116-3116: The
channel_monitor_updatedfunction is correctly updated to include thechan_id_abparameter, and the test checks are appropriate.- 3144-3144: The
channel_monitor_updatedfunction is correctly updated to include thechan_id_abparameter, and the test checks are appropriate.- 3313-3313: The
force_channel_monitor_updatedfunction is correctly updated to include thechan_id_abparameter, and the test checks are appropriate.lightning/src/ln/payment_tests.rs (1)
- 1132-1132: The addition of
chan_idas a parameter to thechannel_monitor_updatedcall aligns with the PR's objective to refactor the codebase for dual-funded channel support.lightning/src/chain/channelmonitor.rs (20)
- 161-162: The addition of the
channel_idfield to theCompletedvariant of theMonitorEventenum is consistent with the PR's objectives to refactor the codebase for dual-funded channel support.- 177-177: The inclusion of
channel_idin the macro pattern for deserialization is correct and ensures that thechannel_idwill be properly deserialized when loadingMonitorEvents.- 778-778: The addition of an optional
channel_idfield to theChannelMonitorImplstruct is appropriate, as it allows for the possibility that thechannel_idmay not be known at the time of struct creation.- 1104-1104: The serialization of the optional
channel_idfield inChannelMonitorImplis handled correctly, ensuring that it will be included in the serialized data when present.- 1168-1168: The
from_implfunction correctly retrieves thechannel_idfrom theChannelMonitorImpland includes it in theWithChannelMonitorstruct, maintaining consistency with the new structure.- 1189-1190: The
ChannelMonitorconstructor now correctly takes achannel_idas a parameter, aligning with the updated structure and ensuring that thechannel_idis available for new monitors.- 1244-1244: The
channel_idis correctly set as anOption<ChannelId>within theChannelMonitorImplstruct, allowing for the possibility that it may not be set at the time of struct instantiation.- 1396-1399: The
get_channel_idmethod is implemented correctly, providing a way to retrieve thechannel_idfrom aChannelMonitor.- 2849-2850: The logging statements have been updated to include the
channel_id, which is retrieved using the newget_channel_idmethod. This change improves the clarity of log messages.- 2896-2898: The
get_channel_idmethod inChannelMonitorImplprovides a fallback to generate a v1ChannelIdfrom the funding outpoint if thechannel_idis not set, which is a sensible default behavior.- 3662-3662: The log message correctly uses the
get_channel_idmethod to include thechannel_idin the log output, which will be useful for debugging and monitoring.- 3832-3832: The
channel_idis now included in theEvent::SpendableOutputsevent, which is consistent with the changes made to theMonitorEventenum.- 4577-4577: The deserialization process has been updated to include an optional
channel_id, ensuring that it can be correctly loaded from serialized data.- 4588-4588: The
channel_idis included in the TLV fields for deserialization, which is necessary for backward compatibility and future extensibility.- 4613-4613: The
channel_idis now a non-optional field in theChannelMonitorImplstruct, which is appropriate for the new structure where thechannel_idis expected to be known at the time of instantiation.- 4688-4688: The import of
ChannelIdis necessary due to its usage throughout the file following the refactoring.- 4864-4864: The creation of a
channel_idfrom a funding outpoint is correct and aligns with the new structure wherechannel_idis used for channel identification.- 4884-4884: The
channel_idis correctly passed to theChannelMonitorconstructor, ensuring that it is set upon creation of the monitor.- 5114-5114: The
channel_idis generated from the funding outpoint, which is consistent with the changes made to theChannelMonitorandChannelMonitorImplstructures.- 5132-5137: The retrieval of the
channel_idfor logging purposes is done correctly, and theWithChannelMonitorstruct is used to provide context for the log messages.lightning/src/ln/functional_tests.rs (18)
- 193-193: The change to the match arm correctly asserts a failure state, which is a good practice for unreachable code paths in tests.
- 2431-2431: The addition of
channel_idto thewatch_channelfunction call aligns with the PR's objective to refactor the codebase for dual-funded channel support.- 8472-8472: The assignment of
channel_idfromchan_1.2is consistent with the new channel identification mechanism.- 8493-8493: The assertion for
watch_channelwith the newchannel_idparameter is correct and follows the updated function signature.- 8515-8516: The update to
update_channelcalls withchannel_idis in line with the changes made to theChainMonitorstruct and its methods.- 8543-8543: Repeating the assignment of
channel_idfromchan_1.2is consistent with the new channel identification mechanism.- 8567-8567: The assertion for
watch_channelwith the newchannel_idparameter is correct and follows the updated function signature.- 8599-8599: The assertion for
watch_channelwith the newchannel_idparameter is correct and follows the updated function signature.- 8619-8621: The assertions for
update_channelwith thechannel_idparameter are correct and follow the updated function signatures.- 8688-8688: The creation of
channel_idusingChannelId::v1_from_funding_outpointis consistent with the new channel identification mechanism.- 9032-9032: The use of
ChannelId::v1_from_funding_outpointfor generating achannel_idand passing it tocheck_closed_eventsis consistent with the updated channel identification mechanism.- 9060-9060: The call to
watch_channelwithChannelId::v1_from_funding_outpointas thechannel_idparameter is correct and aligns with the updated function signature.- 9093-9093: The assertion that checks the equality of
ChannelIdderived from the funding outpoint with thereal_channel_idis correct and ensures consistency in channel identification.- 9185-9185: The creation of
channel_idusingChannelId::v1_from_funding_outpointis consistent with the new channel identification mechanism.- 10639-10639: The call to
complete_sole_pending_chan_updatewithChannelId::v1_from_funding_outpointis correct and aligns with the updated channel identification mechanism.- 10696-10697: The creation of
channel_id_1andchannel_id_2usingChannelId::v1_from_funding_outpointis consistent with the new channel identification mechanism.- 10770-10771: The creation of
channel_id_1andchannel_id_2usingChannelId::v1_from_funding_outpointand their subsequent use inforce_close_broadcasting_latest_txnis correct.- 10831-10831: The creation of
chan_idusingChannelId::v1_from_funding_outpointand its use in asserting the number of channels is consistent with the updated channel identification mechanism.lightning/src/ln/channel.rs (18)
- 908-911: The addition of
UnfundedOutboundV2andUnfundedInboundV2to theChannelPhaseenum is consistent with the introduction of dual-funded channel support.- 924-927: The match arms for
UnfundedOutboundV2andUnfundedInboundV2in thecontextmethod are correctly gated with thedual_fundingfeature flag and return the expected context reference.- 936-939: The mutable references to the context for the new
UnfundedOutboundV2andUnfundedInboundV2channel phases are correctly implemented.- 2966-2966: The monitor update generation with the
ChannelMonitorUpdatestruct is correctly updated to include thecounterparty_node_idandchannel_id.- 3080-3091: The
get_v2_channel_reserve_satoshisfunction correctly implements the channel reserve calculation for v2 channels, adhering to the spec's fixed 1% requirement and ensuring it does not exceed the channel value or fall below the dust limit.- 3108-3120: The
DualFundingChannelContextstruct is correctly defined and gated with thedual_fundingfeature flag, containing the necessary fields for dual-funded channel support.- 6917-6917: The update of the
channel_idto useChannelId::v1_from_funding_outpointis consistent with the new channel identification mechanism.- 7233-7233: The call to
provide_initial_counterparty_commitment_txwith the correct parameters is consistent with the expected behavior when receiving afunding_signedmessage.- 7253-7257: The creation of a
Channelinstance with the updated context and the absence ofdual_funding_channel_contextwhen receivingfunding_signedis correct.- 7287-7288: The
channel_type_from_open_channelfunction correctly derives theChannelTypeFeaturesfrom anOpenChannelmessage.- 7318-7318: The validation of the
announced_channelflag against therequires_scid_privacychannel type feature is correct and ensures that privacy channels are not announced publicly.- 7536-7536: The update of the
channel_statetoAwaitingChannelReadyand the setting of thechannel_idare correctly implemented.- 7554-7554: The call to
provide_initial_counterparty_commitment_txwith the correct parameters is consistent with the expected behavior when receiving afunding_createdmessage.- 7569-7570: The creation of a
Channelinstance with the updated context and the absence ofdual_funding_channel_contextwhen receiving afunding_createdmessage is correct.- 7579-7585: The definition of
OutboundV2Channelwith thedual_funding_contextfield is consistent with the dual-funded channel feature.- 7848-7869: The
get_initial_channel_typefunction correctly determines the initial channel type based on the user's configuration and the counterparty's features.- 8806-8808: The deserialization of a
Channelinstance without adual_funding_channel_contextis consistent with the current channel state expectations.- 9371-9375: The creation of a
Channelinstance from anOutboundV1Channelfor testing purposes is correctly implemented without adual_funding_channel_context.lightning/src/ln/channelmanager.rs (66)
- 290-290: The addition of
prev_channel_idto the structure is consistent with the PR's goal to useChannelIdfor channel identification.- 326-326: The addition of
channel_idto the structure is consistent with the PR's goal to useChannelIdfor channel identification.- 367-367: The use of
channel_idin the conversion toevents::ClaimedHTLCis correct and aligns with the changes made in the rest of the codebase.- 706-706: The tuple
ClosedMonitorUpdateRegeneratedOnStartupnow includesChannelId, which is consistent with the changes to useChannelIdfor channel identification.- 720-720: The
MonitorUpdateRegeneratedOnStartupstruct now includeschannel_id, which is consistent with the changes to useChannelIdfor channel identification.- 749-749: The addition of
ChannelIdin the tuple withinEmitEventAndFreeOtherChannelis consistent with the PR's goal to useChannelIdfor channel identification.- 767-767: The addition of
downstream_channel_idto the structure is consistent with the PR's goal to useChannelIdfor channel identification.- 779-779: The addition of
downstream_channel_idin the serialization structure is consistent with the PR's goal to useChannelIdfor channel identification.- 797-797: The addition of
channel_idin theReleaseRAAChannelMonitorUpdatestructure is consistent with the PR's goal to useChannelIdfor channel identification.- 826-826: The addition of
channel_idin theRAAMonitorUpdateBlockingActionstructure is consistent with the PR's goal to useChannelIdfor channel identification.- 1630-1631: The comment regarding
channel_idfor V1-established channels is informative and aligns with the changes made in the rest of the codebase.- 2073-2080: The addition of
ChannelPhase::UnfundedOutboundV2andChannelPhase::UnfundedInboundV2with the#[cfg(dual_funding)]attribute is consistent with the PR's goal to support dual-funded channels.- 2297-2297: The macro usage for handling new monitor updates with the inclusion of
channel_idis consistent with the PR's goal to useChannelIdfor channel identification.- 2308-2308: The call to
chain_monitor.update_channelwithchannel_idis consistent with the changes to useChannelIdfor channel identification.- 2754-2754: The use of
channel_idinhandle_new_monitor_updateis consistent with the changes to useChannelIdfor channel identification.- 2865-2865: The inclusion of
channel_idin the shutdown process is consistent with the changes to useChannelIdfor channel identification.- 2946-2952: The addition of
ChannelPhase::UnfundedOutboundV2andChannelPhase::UnfundedInboundV2with the#[cfg(dual_funding)]attribute is consistent with the PR's goal to support dual-funded channels.- 3411-3411: The use of
channel_idin the sending process is consistent with the changes to useChannelIdfor channel identification.- 3422-3422: The use of
channel_idin the handling of monitor updates after sending an HTLC is consistent with the changes to useChannelIdfor channel identification.- 3971-3974: The comment regarding the current limitation of batch funding for V1 channels and the potential need for changes to support V2 channels is informative and aligns with the PR's goal to support dual-funded channels.
- 4198-4198: The addition of
prev_channel_idto the structure is consistent with the PR's goal to useChannelIdfor channel identification.- 4226-4226: The addition of
channel_idin theHTLCPreviousHopDatastructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4250-4250: The addition of
ChannelIdin thephantom_receivesvector is consistent with the PR's goal to useChannelIdfor channel identification.- 4263-4264: The addition of
prev_channel_idin theHTLCForwardInfostructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4271-4271: The use of
prev_channel_idin the logging context is consistent with the changes to useChannelIdfor channel identification.- 4341-4341: The addition of
prev_channel_idin thephantom_receivesvector is consistent with the PR's goal to useChannelIdfor channel identification.- 4386-4387: The addition of
prev_channel_idin theHTLCForwardInfostructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4398-4398: The addition of
prev_channel_idin theHTLCSourcestructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4470-4471: The addition of
prev_channel_idin theHTLCForwardInfostructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4505-4505: The addition of
prev_channel_idin theHTLCPreviousHopDatastructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4537-4537: The addition of
prev_channel_idin theHTLCSourcestructure is consistent with the PR's goal to useChannelIdfor channel identification.- 4761-4761: The use of
channel_idin theBackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupis consistent with the changes to useChannelIdfor channel identification.- 4766-4766: The addition of
channel_idin theBackgroundEvent::MonitorUpdateRegeneratedOnStartupis consistent with the changes to useChannelIdfor channel identification.- 4789-4789: The use of
channel_idin theBackgroundEvent::MonitorUpdateRegeneratedOnStartupis consistent with the changes to useChannelIdfor channel identification.- 5029-5038: The addition of
ChannelPhase::UnfundedInboundV2andChannelPhase::UnfundedOutboundV2with the#[cfg(dual_funding)]attribute is consistent with the PR's goal to support dual-funded channels.- 5322-5322: The use of
channel_idin the logging context is consistent with the changes to useChannelIdfor channel identification.- 5369-5369: The addition of
prev_channel_idin theevents::Event::HTLCHandlingFailedis consistent with the changes to useChannelIdfor channel identification.- 5510-5510: The use of
prev_hop_chan_idin theclaim_funds_from_hopfunction is consistent with the changes to useChannelIdfor channel identification.- 5563-5563: The use of
chan_idin the context ofshort_to_chan_infois consistent with the changes to useChannelIdfor channel identification.- 5591-5591: The use of
prev_hop_channel_idin thehandle_new_monitor_updatemacro is consistent with the changes to useChannelIdfor channel identification.- 5602-5602: The inclusion of
channel_idin theBackgroundEvent::MonitorUpdateRegeneratedOnStartupis consistent with the changes to useChannelIdfor channel identification.- 5617-5617: The use of
channel_idin the context ofMonitorUpdateCompletionAction::FreeOtherChannelImmediatelyis consistent with the changes to useChannelIdfor channel identification.- 5633-5633: The use of
channel_idin the context ofactions_blocking_raa_monitor_updatesis consistent with the changes to useChannelIdfor channel identification.- 5664-5664: The use of
prev_hop_channel_idin the context ofchain_monitor.update_channelis consistent with the changes to useChannelIdfor channel identification.- 5692-5692: The inclusion of
channel_idin theBackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupis consistent with the changes to useChannelIdfor channel identification.- 5710-5711: The use of
next_channel_idin theclaim_funds_internalfunction is consistent with the changes to useChannelIdfor channel identification.- 5721-5721: The inclusion of
channel_idin theEventCompletionAction::ReleaseRAAChannelMonitorUpdateis consistent with the changes to useChannelIdfor channel identification.- 5729-5729: The use of
prev_channel_idin the context ofRAAMonitorUpdateBlockingAction::from_prev_hop_datais consistent with the changes to useChannelIdfor channel identification.- 5776-5776: The use of
channel_idin the context ofBackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupis consistent with the changes to useChannelIdfor channel identification.- 5792-5792: The use of
channel_idin the context ofBackgroundEvent::MonitorUpdatesCompleteis consistent with the changes to useChannelIdfor channel identification.- 5802-5802: The inclusion of
downstream_channel_idin theMonitorUpdateCompletionAction::FreeOtherChannelImmediatelyis consistent with the changes to useChannelIdfor channel identification.- 5816-5816: The addition of
prev_channel_idandnext_channel_idin theevents::Event::PaymentForwardedis consistent with the changes to useChannelIdfor channel identification.- 5867-5867: The use of
channel_idin the context ofhandle_monitor_update_releaseis consistent with the changes to useChannelIdfor channel identification.- 5892-5892: The use of
channel_idin the context of channel resumption is consistent with the changes to useChannelIdfor channel identification.- 5907-5907: The use of
channel_idin the context of handling pending forwards is consistent with the changes to useChannelIdfor channel identification.- 5961-5961: The use of
channel_idin thechannel_monitor_updatedfunction is consistent with the changes to useChannelIdfor channel identification.- 5983-5983: The use of
channel_idin the context ofpeer_state.channel_by_idis consistent with the changes to useChannelIdfor channel identification.- 6180-6200: The addition of
ChannelPhase::UnfundedInboundV2andChannelPhase::UnfundedOutboundV2with the#[cfg(dual_funding)]attribute is consistent with the PR's goal to support dual-funded channels.- 6414-6414: The use of
channel_idin the context ofchain_monitor.watch_channelis consistent with the changes to useChannelIdfor channel identification.- 6473-6473: The use of
channel_idin the context ofchain_monitor.watch_channelis consistent with the changes to useChannelIdfor channel identification.- 6603-6603: The use of
channel_idin the context ofhandle_new_monitor_updateis consistent with the changes to useChannelIdfor channel identification.- 6614-6621: The addition of
ChannelPhase::UnfundedInboundV2andChannelPhase::UnfundedOutboundV2with the#[cfg(dual_funding)]attribute is consistent with the PR's goal to support dual-funded channels.- 6818-6818: The use of
channel_idin the context ofclaim_funds_internalis consistent with the changes to useChannelIdfor channel identification.- 6893-6893: The use of
channel_idin the context ofhandle_new_monitor_updateis consistent with the changes to useChannelIdfor channel identification.- 6907-6907: The use of
channel_idin theforward_htlcsfunction is consistent with the changes to useChannelIdfor channel identification.- 6927-6927: The addition of
prev_channel_idin theHTLCForwardInfostructure is consistent with the PR's goal to useChannelIdfor channel identification.
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
7d10850
to
37158f9
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (27)
- .gitignore (1 hunks)
- ci/check-cfg-flags.py (1 hunks)
- ci/ci-tests.sh (1 hunks)
- fuzz/src/chanmon_consistency.rs (4 hunks)
- fuzz/src/full_stack.rs (2 hunks)
- lightning-background-processor/src/lib.rs (2 hunks)
- lightning-persister/src/fs_store.rs (2 hunks)
- lightning/src/chain/chainmonitor.rs (10 hunks)
- lightning/src/chain/channelmonitor.rs (20 hunks)
- lightning/src/chain/mod.rs (3 hunks)
- lightning/src/chain/transaction.rs (4 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (4 hunks)
- lightning/src/ln/channel.rs (22 hunks)
- lightning/src/ln/channel_id.rs (3 hunks)
- lightning/src/ln/channel_keys.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (97 hunks)
- lightning/src/ln/functional_test_utils.rs (4 hunks)
- lightning/src/ln/functional_tests.rs (13 hunks)
- lightning/src/ln/monitor_tests.rs (6 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/ln/priv_short_conf_tests.rs (2 hunks)
- lightning/src/ln/reload_tests.rs (6 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
- lightning/src/ln/shutdown_tests.rs (14 hunks)
- lightning/src/util/macro_logger.rs (2 hunks)
- lightning/src/util/persist.rs (1 hunks)
- lightning/src/util/test_utils.rs (3 hunks)
Files not summarized due to errors (2)
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
- lightning/src/ln/channelmanager.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (24)
- .gitignore
- ci/check-cfg-flags.py
- ci/ci-tests.sh
- fuzz/src/chanmon_consistency.rs
- fuzz/src/full_stack.rs
- lightning-background-processor/src/lib.rs
- lightning-persister/src/fs_store.rs
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/mod.rs
- lightning/src/chain/transaction.rs
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/channel_keys.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/ln/priv_short_conf_tests.rs
- lightning/src/ln/reload_tests.rs
- lightning/src/ln/reorg_tests.rs
- lightning/src/ln/shutdown_tests.rs
- lightning/src/util/macro_logger.rs
- lightning/src/util/persist.rs
- lightning/src/util/test_utils.rs
Additional comments: 95
lightning/src/ln/channel_id.rs (4)
- 51-54: The method
v1_from_funding_outpointcorrectly implements the creation of a v1 channel ID from a funding transaction outpoint.- 78-92: The method
v2_from_revocation_basepointsuses the revocation basepoints to generate a v2 channel ID. Ensure that the sorting of basepoints and the subsequent concatenation and hashing are correctly implemented according to the protocol specifications.- 94-97: The method
temporary_v2_from_revocation_basepointcreates a temporary v2 channel ID by hashing a zeroed out basepoint with the holder's revocation basepoint. Confirm that this approach is consistent with the intended use of temporary channel IDs and that using a zeroed out basepoint does not introduce any security concerns.- 98-98: Ensure that the trait implementations for
WriteableandReadableand thefmt::Displayimplementation forChannelIdare correct and that they handle the new channel ID formats appropriately.Verification successful
The trait implementations for
Writeable,Readable, andfmt::DisplayforChannelIdhave been verified. They follow standard patterns for handling fixed-size byte arrays in Rust. There is no special handling for different channel ID formats evident in the code provided, which suggests that the internal representation ofChannelIddoes not vary between v1 and v2. However, without specific knowledge of the requirements for the new channel ID formats, it cannot be definitively stated that no additional handling is required.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the trait implementations for ChannelId are correct and handle new channel ID formats. # This script is a placeholder for the actual verification steps which would involve checking trait implementations.Length of output: 0
Script:
#!/bin/bash # Search for the implementations of Writeable and Readable traits for ChannelId ast-grep --lang rust --pattern $'impl Writeable for ChannelId { $$$ }' ast-grep --lang rust --pattern $'impl Readable for ChannelId { $$$ }' # Search for the implementation of fmt::Display trait for ChannelId ast-grep --lang rust --pattern $'impl fmt::Display for ChannelId { $$$ }'Length of output: 1236
lightning/src/ln/channel.rs (17)
- 908-911: The addition of
UnfundedOutboundV2andUnfundedInboundV2to theChannelPhaseenum is consistent with the introduction of dual-funded channel support.- 924-927: The conditional compilation flags
#[cfg(dual_funding)]are correctly used to ensure that the new dual-funded channel types are only compiled when the feature is enabled.- 936-939: The changes here mirror those in hunk 3, correctly using conditional compilation for dual-funded channel support.
- 2966-2966: The
ChannelMonitorUpdatenow includes acounterparty_node_idfield, which is a good addition for clarity and tracking purposes.- 3087-3090: The calculation for
get_v2_channel_reserve_satoshisis correct and follows the specification for a fixed 1% of the total channel value.- 3110-3120: The
DualFundingChannelContextstruct is a good encapsulation of the dual-funding specific context needed for channels. It includes all necessary fields for managing dual-funded channels.- 6917-6917: The update of the
channel_idto useChannelId::v1_from_funding_outpointis correct and aligns with the new channel identification mechanism.- 7233-7233: The call to
provide_initial_counterparty_commitment_txcorrectly includes the counterparty node ID and channel ID, which is necessary for tracking and identification purposes.- 7253-7257: The construction of the
Channelstruct without thedual_funding_channel_contextfield when not in dual-funding mode is correct.- 7287-7303: The function
channel_type_from_type_and_flagscorrectly checks for optional bits in the channel type and ensures that the channel type is a subset of supported features.- 7318-7318: The check for SCID privacy on a public channel is correct and prevents a configuration that would be against the protocol rules.
- 7536-7536: The update of the
channel_statetoChannelState::AwaitingChannelReadyand thechannel_idassignment are correct and align with the channel state progression.- 7554-7554: The call to
provide_initial_counterparty_commitment_txwith the correct transaction ID and commitment number is appropriate for setting up the initial channel state.- 7569-7570: The construction of the
Channelstruct without thedual_funding_channel_contextfield when not in dual-funding mode is consistent with the previous similar changes.- 7580-7870: The introduction of
OutboundV2ChannelandInboundV2Channelstructs, along with the associated methods, correctly encapsulates the logic for dual-funded channels. Theget_initial_channel_typefunction is well-implemented, considering the user's configuration and the counterparty's features.- 8806-8808: The construction of the
Channelstruct without thedual_funding_channel_contextfield when not in dual-funding mode is consistent with the previous similar changes.- 9371-9375: The construction of the
Channelstruct without thedual_funding_channel_contextfield when not in dual-funding mode is consistent with the previous similar changes.lightning/src/ln/channelmanager.rs (74)
- 291-291: The addition of
prev_channel_idto the struct is consistent with the PR's goal of usingChannelIdfor channel identification.- 332-332: The addition of
channel_idto the struct is consistent with the PR's goal of usingChannelIdfor channel identification.- 373-373: The use of
val.prev_hop.channel_idin theFromimplementation forevents::ClaimedHTLCis correct and aligns with the PR's objectives.- 712-712: The tuple
ClosedMonitorUpdateRegeneratedOnStartupnow includesChannelId, which is consistent with the PR's objectives.- 726-726: The struct
MonitorUpdateRegeneratedOnStartupnow includesChannelId, which is consistent with the PR's objectives.- 755-755: The addition of
ChannelIdin the tupledownstream_counterparty_and_funding_outpointis consistent with the PR's objectives.- 773-773: The addition of
downstream_channel_idto the struct is consistent with the PR's objectives.- 787-787: The use of
ChannelId::v1_from_funding_outpointto provide a default value fordownstream_channel_idis correct and aligns with the PR's objectives.- 805-805: The struct
ReleaseRAAChannelMonitorUpdatenow includesChannelId, which is consistent with the PR's objectives.- 814-814: The use of
ChannelId::v1_from_funding_outpointto provide a default value forchannel_idis correct and aligns with the PR's objectives.- 836-836: The addition of
channel_idto theRAAMonitorUpdateBlockingActionstruct is consistent with the PR's objectives.- 1640-1641: The comment regarding
channel_idfor V1-established channels is informative and aligns with the PR's objectives.- 2083-2090: The conditional compilation with
#[cfg(dual_funding)]is used to include code specific to dual-funded channels, which is consistent with the PR's objectives.- 2307-2307: The macro
handle_new_monitor_update!is being used correctly to handleChannelMonitorUpdates, and the inclusion ofchannel_idis consistent with the PR's objectives.- 2318-2318: The call to
chain_monitor.update_channelwithchannel_idis correct and aligns with the PR's objectives.- 2764-2764: The use of
handle_new_monitor_update!macro withchannel_idis correct and aligns with the PR's objectives.- 2875-2875: The inclusion of
channel_idin the tuple forshutdown_res.monitor_updateis consistent with the PR's objectives.- 2956-2962: The conditional compilation with
#[cfg(dual_funding)]is used to include code specific to dual-funded channels, which is consistent with the PR's objectives.- 3422-3422: The use of
channel_idobtained fromchan.context.channel_id()is correct and aligns with the PR's objectives.- 3433-3433: The use of
handle_new_monitor_update!macro withchannel_idis correct and aligns with the PR's objectives.- 3982-3985: The comment regarding the current limitation to V1 channels for batch funding and the potential need for changes to support V2 channels is insightful and suggests future work that may be needed.
- 4210-4210: The inclusion of
prev_channel_idin the tuple is consistent with the PR's objectives.- 4238-4238: The inclusion of
channel_idin theHTLCPreviousHopDatastruct is consistent with the PR's objectives.- 4262-4262: The addition of
ChannelIdto thephantom_receivesvector is consistent with the PR's objectives.- 4275-4276: The inclusion of
prev_channel_idin theHTLCForwardInfo::AddHTLCvariant is consistent with the PR's objectives.- 4283-4283: The use of
WithContext::fromwithprev_channel_idfor logging is correct and aligns with the PR's objectives.- 4353-4353: The inclusion of
prev_channel_idin thephantom_receivesvector is consistent with the PR's objectives.- 4398-4399: The inclusion of
prev_channel_idin theHTLCForwardInfo::AddHTLCvariant is consistent with the PR's objectives.- 4410-4410: The inclusion of
channel_idin theHTLCSource::PreviousHopDatastruct is consistent with the PR's objectives.- 4482-4483: The inclusion of
prev_channel_idin theHTLCForwardInfo::AddHTLCvariant is consistent with the PR's objectives.- 4517-4517: The inclusion of
channel_idin theHTLCSource::PreviousHopDatastruct is consistent with the PR's objectives.- 4549-4549: The inclusion of
channel_idin theHTLCSource::PreviousHopDatastruct is consistent with the PR's objectives.- 4773-4773: The use of
chain_monitor.update_channelwithchannel_idis correct and aligns with the PR's objectives.- 4785-4785: The use of
handle_new_monitor_update!macro withchannel_idis correct and aligns with the PR's objectives.- 4801-4801: The use of
chain_monitor.update_channelwithchannel_idis correct and aligns with the PR's objectives.- 5041-5050: The conditional compilation with
#[cfg(dual_funding)]is used to include code specific to dual-funded channels, which is consistent with the PR's objectives.- 5334-5334: The use of
WithContext::fromwithchannel_idfor logging is correct and aligns with the PR's objectives.- 5381-5381: The inclusion of
prev_channel_idin theevents::Event::HTLCHandlingFailedis consistent with the PR's objectives.- 5522-5522: The use of
prev_hop_chan_idin theclaim_funds_from_hopcall is correct and aligns with the PR's objectives.- 5575-5575: The use of
channel_idobtained fromprev_hop.channel_idis correct and aligns with the PR's objectives.- 5603-5603: The use of
handle_new_monitor_update!macro withprev_hop_channel_idis correct and aligns with the PR's objectives.- 5614-5614: The use of
BackgroundEvent::MonitorUpdateRegeneratedOnStartupwithprev_hop.channel_idis correct and aligns with the PR's objectives.- 5629-5629: The use of
MonitorUpdateCompletionAction::FreeOtherChannelImmediatelywithdownstream_channel_idis correct and aligns with the PR's objectives.- 5645-5645: The check for
channel_idinactions_blocking_raa_monitor_updatesis correct and aligns with the PR's objectives.- 5676-5676: The use of
chain_monitor.update_channelwithprev_hop_channel_idis correct and aligns with the PR's objectives.- 5704-5704: The use of
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupwithprev_hop.channel_idis correct and aligns with the PR's objectives.- 5722-5723: The use of
claim_funds_internalwithnext_channel_idis correct and aligns with the PR's objectives.- 5733-5733: The use of
EventCompletionAction::ReleaseRAAChannelMonitorUpdatewithnext_channel_idis correct and aligns with the PR's objectives.- 5741-5741: The use of
RAAMonitorUpdateBlockingAction::from_prev_hop_datawithprev_channel_idis correct and aligns with the PR's objectives.- 5788-5788: The use of
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupwithchannel_idis correct and aligns with the PR's objectives.- 5804-5804: The check for
channel_idinMonitorUpdatesCompleteis correct and aligns with the PR's objectives.- 5814-5814: The use of
MonitorUpdateCompletionAction::FreeOtherChannelImmediatelywithdownstream_channel_idis correct and aligns with the PR's objectives.- 5828-5828: The inclusion of
prev_channel_idandnext_channel_idin theevents::Event::PaymentForwardedis consistent with the PR's objectives.- 5879-5879: The use of
handle_monitor_update_releasewithchannel_idis correct and aligns with the PR's objectives.- 5904-5904: The use of
channel_idin the return type of the function is correct and aligns with the PR's objectives.- 5919-5919: The use of
channel_idin theforward_htlcsfunction is correct and aligns with the PR's objectives.- 5973-5973: The use of
channel_monitor_updatedwithchannel_idis correct and aligns with the PR's objectives.- 5995-5995: The check for
ChannelPhase::Fundedand subsequent use ofchannel_idis correct and aligns with the PR's objectives.- 6192-6212: The conditional compilation with
#[cfg(dual_funding)]is used to include code specific to dual-funded channels, which is consistent with the PR's objectives.- 6614-6614: The use of
handle_new_monitor_update!macro withchannel_idis correct and aligns with the PR's objectives.- 6625-6632: The conditional compilation with
#[cfg(dual_funding)]is used to include code specific to dual-funded channels, which is consistent with the PR's objectives.- 6829-6829: The use of
claim_funds_internalwithchannel_idis correct and aligns with the PR's objectives.- 6904-6904: The use of
handle_new_monitor_update!macro withchannel_idis correct and aligns with the PR's objectives.- 6918-6918: The use of
forward_htlcswithchannel_idis correct and aligns with the PR's objectives.- 6938-6938: The inclusion of
prev_channel_idin theHTLCForwardInfo::AddHTLCvariant is consistent with the PR's objectives.- 6956-6956: The use of
WithContext::fromwithprev_channel_idfor logging is correct and aligns with the PR's objectives.- 6985-6985: The inclusion of
prev_channel_idin theHTLCForwardInfo::AddHTLCvariant is consistent with the PR's objectives.- 7029-7029: The function
raa_monitor_updates_heldnow includeschannel_id, which is consistent with the PR's objectives.- 7053-7053: The use of
raa_monitor_updates_heldwithchannel_idis correct and aligns with the PR's objectives.- 7075-7075: The use of
raa_monitor_updates_heldwithchannel_idis correct and aligns with the PR's objectives.- 7321-7321: The use of
chain_monitor.release_pending_monitor_eventswithchannel_idis correct and aligns with the PR's objectives.- 7352-7352: The use of
peer_state.channel_by_id.entry(channel_id)is correct and aligns with the PR's objectives.- 7371-7371: The struct
MonitorEvent::Completednow includeschannel_id, which is consistent with the PR's objectives.- 7424-7424: The use of
handle_new_monitor_update!macro withchannel_idis correct and aligns with the PR's objectives.
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
37158f9
to
36390c8
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (27)
- .gitignore (1 hunks)
- ci/check-cfg-flags.py (1 hunks)
- ci/ci-tests.sh (1 hunks)
- fuzz/src/chanmon_consistency.rs (3 hunks)
- fuzz/src/full_stack.rs (2 hunks)
- lightning-background-processor/src/lib.rs (2 hunks)
- lightning-persister/src/fs_store.rs (2 hunks)
- lightning/src/chain/chainmonitor.rs (10 hunks)
- lightning/src/chain/channelmonitor.rs (20 hunks)
- lightning/src/chain/mod.rs (3 hunks)
- lightning/src/chain/transaction.rs (4 hunks)
- lightning/src/ln/chanmon_update_fail_tests.rs (4 hunks)
- lightning/src/ln/channel.rs (22 hunks)
- lightning/src/ln/channel_id.rs (3 hunks)
- lightning/src/ln/channel_keys.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (96 hunks)
- lightning/src/ln/functional_test_utils.rs (4 hunks)
- lightning/src/ln/functional_tests.rs (13 hunks)
- lightning/src/ln/monitor_tests.rs (6 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/ln/priv_short_conf_tests.rs (2 hunks)
- lightning/src/ln/reload_tests.rs (6 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
- lightning/src/ln/shutdown_tests.rs (14 hunks)
- lightning/src/util/macro_logger.rs (2 hunks)
- lightning/src/util/persist.rs (1 hunks)
- lightning/src/util/test_utils.rs (3 hunks)
Files not summarized due to errors (2)
- lightning/src/ln/channel.rs: Error: Message exceeds token limit
- lightning/src/ln/channelmanager.rs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (24)
- .gitignore
- ci/check-cfg-flags.py
- ci/ci-tests.sh
- fuzz/src/chanmon_consistency.rs
- fuzz/src/full_stack.rs
- lightning-background-processor/src/lib.rs
- lightning-persister/src/fs_store.rs
- lightning/src/chain/chainmonitor.rs
- lightning/src/chain/channelmonitor.rs
- lightning/src/chain/mod.rs
- lightning/src/chain/transaction.rs
- lightning/src/ln/chanmon_update_fail_tests.rs
- lightning/src/ln/channel_keys.rs
- lightning/src/ln/functional_test_utils.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/peer_handler.rs
- lightning/src/ln/priv_short_conf_tests.rs
- lightning/src/ln/reload_tests.rs
- lightning/src/ln/reorg_tests.rs
- lightning/src/ln/shutdown_tests.rs
- lightning/src/util/macro_logger.rs
- lightning/src/util/persist.rs
- lightning/src/util/test_utils.rs
Additional comments: 86
lightning/src/ln/channel_id.rs (3)
- 48-50: The method
v1_from_funding_outpointhas been added to create a v1 channel ID from a funding transaction outpoint. This method is a straightforward wrapper around the existingv1_from_funding_txidmethod, which is good for API consistency.- 74-88: The
v2_from_revocation_basepointsmethod has been added to create v2 channel IDs by hashing the concatenated revocation basepoints. The logic to sort the basepoints before concatenation is correct and ensures a consistent channel ID regardless of the order of arguments.- 90-94: The
temporary_v2_from_revocation_basepointmethod has been added to create temporary v2 channel IDs. The method correctly concatenates a zeroed out basepoint with the holder's revocation basepoint and hashes the result. This approach is consistent with the temporary nature of the ID.lightning/src/ln/channel.rs (17)
- 908-911: The addition of
UnfundedOutboundV2andUnfundedInboundV2to theChannelPhaseenum aligns with the implementation of dual-funded channels.- 924-927: The addition of
UnfundedOutboundV2andUnfundedInboundV2to the match expression forChannelPhasealigns with the implementation of dual-funded channels.- 936-939: The addition of
UnfundedOutboundV2andUnfundedInboundV2to the mutable match expression forChannelPhasealigns with the implementation of dual-funded channels.- 2966-2966: The code correctly replaces
OutPointwithChannelIdin theChannelMonitorUpdatetuple, aligning with the new channel identification mechanism.- 3087-3091: The addition of
get_v2_channel_reserve_satoshismethod is consistent with the PR's goal of supporting dual-funded channels by calculating the channel reserve for v2 channels.- 3108-3120: The addition of
DualFundingChannelContextstruct and its inclusion in theChannelstruct is consistent with the PR's goal of supporting dual-funded channels.- 6917-6917: The code correctly updates the
channel_idto use the newChannelIdtype, aligning with the new channel identification mechanism.- 7233-7233: The code correctly updates the
channel_idin theprovide_initial_counterparty_commitment_txcall, aligning with the new channel identification mechanism.- 7253-7257: The code correctly initializes a
Channelstruct without thedual_funding_channel_contextfor a non-dual-funded channel, which is consistent with the PR's goal of supporting dual-funded channels.- 7287-7303: The method
channel_type_from_open_channelcorrectly checks for optional bits in the channel type, which is consistent with the PR's goal of supporting dual-funded channels.- 7318-7318: The code correctly checks for unsupported features in the channel type and for the incompatibility of SCID privacy with public channels, which is consistent with the PR's goal of supporting dual-funded channels.
- 7536-7536: The code correctly updates the
channel_idto use the newChannelIdtype and adjusts the commitment transaction numbers, aligning with the new channel identification mechanism.- 7554-7554: The code correctly updates the
channel_idin theprovide_initial_counterparty_commitment_txcall, aligning with the new channel identification mechanism.- 7569-7570: The code correctly initializes a
Channelstruct without thedual_funding_channel_contextfor a non-dual-funded channel, which is consistent with the PR's goal of supporting dual-funded channels.- 7579-7870: The addition of
OutboundV2ChannelandInboundV2Channelstructs and related methods is consistent with the PR's goal of supporting dual-funded channels.- 8806-8808: The code correctly initializes a
Channelstruct without thedual_funding_channel_contextfor a non-dual-funded channel, which is consistent with the PR's goal of supporting dual-funded channels.- 9371-9375: The code correctly initializes a
Channelstruct without thedual_funding_channel_contextfor a non-dual-funded channel, which is consistent with the PR's goal of supporting dual-funded channels.lightning/src/ln/channelmanager.rs (66)
- 291-291: The addition of
prev_channel_idto the struct is consistent with the PR's goal of usingChannelIdfor channel identification. This change should be cross-verified with other parts of the codebase to ensure that the new field is properly utilized and does not introduce any inconsistencies.- 332-332: The addition of
channel_idto the struct is appropriate and aligns with the changes mentioned in the PR summary. Ensure that the new field is used correctly wherever this struct is instantiated or modified.- 373-373: The conversion from
ClaimableHTLCtoevents::ClaimedHTLCnow includeschannel_id, which is in line with the changes. Verify that all instances where this conversion occurs are updated to handle the new field.- 712-712: The tuple
ClosedMonitorUpdateRegeneratedOnStartupnow includesChannelId, which is consistent with the PR's objectives. Ensure that the tuple is correctly used in the context of monitor updates.- 726-726: The struct
MonitorUpdateRegeneratedOnStartupnow includeschannel_id. This change should be verified across the codebase to ensure that the new field is properly handled in monitor updates.- 755-755: The addition of
downstream_counterparty_and_funding_outpointwhich includesChannelIdto theEmitEventAndFreeOtherChannelenum variant is consistent with the PR's objectives. Verify that this change is properly integrated with the event handling logic.- 773-773: The addition of
downstream_channel_idto the struct is appropriate. Ensure that the new field is used correctly wherever this struct is instantiated or modified.- 787-787: The default value for
downstream_channel_idis set usingChannelId::v1_from_funding_outpoint. Ensure that this default value is appropriate for all cases, especially considering the introduction of v2 channels.- 814-814: The default value for
channel_idis set usingChannelId::v1_from_funding_outpoint. Verify that this default value is appropriate for all cases, especially considering the introduction of v2 channels.- 836-836: The method
from_prev_hop_datanow includeschannel_idin the returnedRAAMonitorUpdateBlockingAction. Verify that this change is properly integrated with the rest of the codebase.- 1640-1641: The comment about
channel_idfor V1-established channels being equivalent toChannelId::v1_from_funding_outpoint(funding_txo.unwrap())is informative. Ensure that this logic is consistently applied throughout the codebase.- 2083-2090: The introduction of
ChannelPhase::UnfundedOutboundV2andChannelPhase::UnfundedInboundV2is part of the dual funding feature. Verify that these new channel phases are handled correctly in all relevant parts of the code.- 2307-2307: The macro
handle_new_monitor_update!is being used with additional parameters includingchannel_id. Verify that the macro is updated to handle these parameters correctly.- 2318-2318: The call to
chain_monitor.update_channelnow includeschannel_id. Ensure that theupdate_channelmethod is updated to handle this new parameter.- 2764-2764: The call to
handle_new_monitor_update!within the conditional block formonitor_update_optnow includeschannel_id. Verify that the monitor update handling logic is correctly updated to include this parameter.- 2875-2875: The tuple unpacking now includes
channel_idin theshutdown_res.monitor_update. Ensure that the monitor update logic correctly handles thechannel_id.- 2956-2962: The introduction of
ChannelPhase::UnfundedOutboundV2andChannelPhase::UnfundedInboundV2within the match arm and the associated logic is part of the dual funding feature. Verify that these new channel phases are handled correctly in all relevant parts of the code.- 3422-3422: The variable
channel_idis now being used in the context of sending an HTLC. Verify that thechannel_idis correctly utilized in this context and that the logic for sending HTLCs is correctly updated.- 3433-3433: The match arm now includes
channel_idin the context of handling a monitor update. Verify that the monitor update handling logic is correctly updated to include this parameter.- 3982-3985: The comment about batch funding only being done for V1 channels at the moment and the potential need to fix this for V2 channels is important. Ensure that the logic for batch funding is correctly updated when V2 channels are introduced.
- 4210-4210: The tuple now includes
prev_channel_id. Verify that the logic for handling pending forwards is correctly updated to include this new field.- 4238-4238: The struct now includes
channel_idandhtlc_id. Verify that the logic for handling HTLCs is correctly updated to include these new fields.- 4262-4262: The vector
phantom_receivesnow includesChannelId. Verify that the logic for handling phantom receives is correctly updated to include this new field.- 4275-4276: The macro
failure_handlernow includesprev_channel_id. Verify that the failure handling logic is correctly updated to include this new field.- 4353-4353: The tuple now includes
prev_channel_id. Verify that the logic for handling phantom receives is correctly updated to include this new field.- 4398-4399: The match arm now includes
prev_channel_id. Verify that the logic for handling HTLC forwards is correctly updated to include this new field.- 4410-4410: The struct
HTLCSource::PreviousHopDatanow includeschannel_id. Verify that the logic for handling HTLC sources is correctly updated to include this new field.- 4482-4483: The match arm now includes
prev_channel_id. Verify that the logic for handling HTLC forwards is correctly updated to include this new field.- 4517-4517: The struct
HTLCPreviousHopDatanow includeschannel_id. Verify that the logic for handling HTLC forwards is correctly updated to include this new field.- 4549-4549: The struct
HTLCPreviousHopDatanow includeschannel_id. Verify that the logic for handling HTLC failures is correctly updated to include this new field.- 4773-4773: The enum variant
ClosedMonitorUpdateRegeneratedOnStartupnow includeschannel_id. Verify that the logic for handling monitor updates is correctly updated to include this new field.- 4776-4776: The call to
chain_monitor.update_channelnow includeschannel_id. Ensure that theupdate_channelmethod is updated to handle this new parameter.- 4785-4785: The match arm now includes
channel_idin the context of handling a monitor update. Verify that the monitor update handling logic is correctly updated to include this parameter.- 4801-4801: The call to
chain_monitor.update_channelnow includeschannel_id. Ensure that theupdate_channelmethod is updated to handle this new parameter.- 5041-5050: The introduction of
ChannelPhase::UnfundedInboundV2andChannelPhase::UnfundedOutboundV2is part of the dual funding feature. Verify that these new channel phases are handled correctly in all relevant parts of the code.- 5334-5334: The struct
HTLCPreviousHopDatanow includeschannel_id. Verify that the logic for handling HTLC failures is correctly updated to include this new field.- 5381-5381: The struct
events::Event::HTLCHandlingFailednow includesprev_channel_id. Verify that the logic for handling HTLC failures is correctly updated to include this new field.- 5522-5522: The variable
prev_hop_chan_idis now being used in the context of claiming funds from a hop. Verify that theclaim_funds_from_hopmethod is updated to handle this new variable.- 5575-5575: The variable
chan_idis now being used in the context of handling HTLCs. Verify that thechan_idis correctly utilized in this context and that the logic for handling HTLCs is correctly updated.- 5603-5603: The call to
handle_new_monitor_update!within the conditional block forduring_initnow includesprev_hop.channel_id. Verify that the monitor update handling logic is correctly updated to include this parameter.- 5613-5613: The struct
BackgroundEvent::MonitorUpdateRegeneratedOnStartupnow includesprev_hop.channel_id. Verify that the logic for handling monitor updates is correctly updated to include this new field.- 5628-5628: The tuple unpacking now includes
channel_id. Verify that the logic for handling monitor update completion actions is correctly updated to include this new field.- 5644-5644: The logic for handling blockers for monitor updates now includes
channel_id. Verify that the logic for handling blockers is correctly updated to include this new field.- 5675-5675: The variable
prev_hop_channel_idis now being used in the context of handling HTLCs. Verify that theprev_hop_channel_idis correctly utilized in this context and that the logic for handling HTLCs is correctly updated.- 5680-5680: The call to
chain_monitor.update_channelnow includesprev_hop_channel_id. Ensure that theupdate_channelmethod is updated to handle this new parameter.- 5703-5703: The struct
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupnow includesprev_hop.channel_id. Verify that the logic for handling monitor updates is correctly updated to include this new field.- 5721-5722: The method
claim_funds_internalnow includesnext_channel_id. Verify that this change is properly integrated with the rest of the codebase.- 5732-5732: The struct
EventCompletionAction::ReleaseRAAChannelMonitorUpdatenow includeschannel_id. Verify that the logic for handling event completion actions is correctly updated to include this new field.- 5740-5740: The variable
prev_channel_idis now being used in the context of claiming funds from a hop. Verify that theclaim_funds_from_hopmethod is updated to handle this new variable.- 5787-5787: The struct
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartupnow includeschannel_id. Verify that the logic for handling monitor updates is correctly updated to include this new field.- 5803-5803: The logic for handling
BackgroundEvent::MonitorUpdatesCompletenow includeschannel_id. Verify that the logic for handling monitor updates is correctly updated to include this new field.- 5903-5903: The method
handle_channel_resumptionnow includeschannel_id. Verify that this change is properly integrated with the rest of the codebase.- 5918-5918: The method
forward_htlcsnow includesprev_channel_id. Verify that this change is properly integrated with the rest of the codebase.- 6937-6937: The logic for handling
HTLCForwardInfo::AddHTLCnow includesprev_channel_id. Verify that the logic for handling HTLC forwards is correctly updated to include this new field.- 6955-6955: The logic for handling
HTLCForwardInfo::AddHTLCnow includesprev_channel_id. Verify that the logic for handling HTLC forwards is correctly updated to include this new field.- 6984-6984: The logic for handling
HTLCForwardInfo::AddHTLCnow includesprev_channel_id. Verify that the logic for handling HTLC forwards is correctly updated to include this new field.- 7028-7028: The method
raa_monitor_updates_heldnow includeschannel_id. Verify that this change is properly integrated with the rest of the codebase.- 7052-7052: The method
raa_monitor_updates_heldnow includeschannel_id. Verify that this change is properly integrated with the rest of the codebase.- 7074-7074: The logic for handling
RAAMonitorUpdateBlockingActionnow includeschannel_id. Verify that the logic for handling blockers is correctly updated to include this new field.- 7320-7320: The struct
MonitorEvent::HTLCEventnow includeschannel_id. Verify that the logic for handling monitor events is correctly updated to include this new field.- 7351-7351: The logic for handling
MonitorEvent::HolderForceClosednow includeschannel_id. Verify that the logic for handling monitor events is correctly updated to include this new field.- 7370-7370: The struct
MonitorEvent::Completednow includeschannel_id. Verify that the logic for handling monitor events is correctly updated to include this new field.- 7423-7423: The logic for handling
MonitorEventnow includeschannel_id. Verify that the logic for handling monitor events is correctly updated to include this new field.- 7588-7588: The struct
BackgroundEvent::MonitorUpdateRegeneratedOnStartupnow includeschannel_id. Verify that the logic for handling monitor updates is correctly updated to include this new field.- 8154-8154: The method
handle_monitor_update_releasenow includeschannel_id. Verify that this change is properly integrated with the rest of the codebase.- 8169-8169: The logic for handling blockers for monitor updates now includes
channel_id. Verify that the logic for handling blockers is correctly updated to include this new field.
|
@coderabbitai pause |
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
4 times, most recently
from
9f5eeb6
to
85f3155
Compare
wpaulino
reviewed
Jan 31, 2024
optout21
reviewed
Feb 1, 2024
₿ git diff-tree -U1 6a88e68 cff175a 2024/02/15 09:15:55 AM
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index e995af78..cada2941 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1566,3 +1566,3 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if msg_channel_reserve_satoshis > channel_value_satoshis {
- return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must be not greater than funding_satoshis: {}", msg_channel_reserve_satoshis, channel_value_satoshis)));
+ return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must be no greater than channel_value_satoshis: {}", msg_channel_reserve_satoshis, channel_value_satoshis)));
}
@@ -1573,3 +1573,3 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if open_channel_fields.dust_limit_satoshis > channel_value_satoshis {
- return Err(ChannelError::Close(format!("dust_limit_satoshis {} was larger than funding_satoshis {}. Peer never wants payout outputs?", open_channel_fields.dust_limit_satoshis, channel_value_satoshis)));
+ return Err(ChannelError::Close(format!("dust_limit_satoshis {} was larger than channel_value_satoshis {}. Peer never wants payout outputs?", open_channel_fields.dust_limit_satoshis, channel_value_satoshis)));
}
@@ -7650,10 +7650,3 @@ pub(super) fn channel_type_from_open_channel(
) -> Result<ChannelTypeFeatures, ChannelError> {
- channel_type_from_type_and_flags(&common_fields.channel_type, common_fields.channel_flags,
- their_features, our_supported_features)
-}
-
-fn channel_type_from_type_and_flags(msg_channel_type: &Option<ChannelTypeFeatures>,
- msg_channel_flags: u8, their_features: &InitFeatures, our_supported_features: &ChannelTypeFeatures
-) -> Result<ChannelTypeFeatures, ChannelError> {
- if let Some(channel_type) = &msg_channel_type {
+ if let Some(channel_type) = &common_fields.channel_type {
if channel_type.supports_any_optional_bits() {
@@ -7672,3 +7665,3 @@ fn channel_type_from_type_and_flags(msg_channel_type: &Option<ChannelTypeFeature
}
- let announced_channel = if (msg_channel_flags & 1) == 1 { true } else { false };
+ let announced_channel = if (common_fields.channel_flags & 1) == 1 { true } else { false };
if channel_type.requires_scid_privacy() && announced_channel { |
|
Do we need to update this from #2725? rust-lightning/lightning/src/ln/channelmanager.rs Lines 907 to 909 in cff175a There may be another place that uses |
Ah I was thinking of a different change when you mentioned this. Yeah, we'll need to account for outbound V2. |
Hm, yeah maybe we should discourage its use project-wide. Rust enums force you to explicitly cover all variants to prevent bugs like the one you pointed out. While using the macro is nice in certain cases, perhaps we should move towards helper methods that do the explicit match across all variants. |
Yip, changing to a simple match is good. Don't think we need an additional helper method in this PR. |
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
cff175a
to
d23ade2
Compare
|
This is the only ₿ git diff-tree -U1 cff175a d23ade2 2024/02/15 07:56:23 PM
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index d4163756..6eed5a49 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -907,3 +907,10 @@ impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
!self.channel_by_id.iter().any(|(_, phase)|
- matches!(phase, ChannelPhase::Funded(_) | ChannelPhase::UnfundedOutboundV1(_))
+ match phase {
+ ChannelPhase::Funded(_) | ChannelPhase::UnfundedOutboundV1(_) => true,
+ ChannelPhase::UnfundedInboundV1(_) => false,
+ #[cfg(dual_funding)]
+ ChannelPhase::UnfundedOutboundV2(_) => true,
+ #[cfg(dual_funding)]
+ ChannelPhase::UnfundedInboundV1(_) => false,
+ }
) |
Hmm... it's really not specific to
There is an existing one in
Does it matter if an input was contributed to an |
Good point. Unfunded inbound V2 channels we contribute to are special in at least the sense that they don’t contribute to unfunded channel counts for anti-DoS limits. Although, here I believe it’s consistent to keep it on purely if we initiated. I stand to be corrected though… |
|
CI is mega-sad. |
We can't contribute to a v1 inbound channel, only v2. I went through all uses of
We also may need to re-visit this once we have the state machine done just to make sure we don't have any other gaps, specifically around when a v2 channel becomes funded. With v1 channels, we remain unfunded until we send/receive |
Right... sometimes I count from 0 and mix up the numbers. 😛 |
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
d23ade2
to
db0bc60
Compare
|
Added a new penultimate commit for the channel downgrade method for V2 outbound. Some other minor fixes: Diff₿ git diff-tree -U1 d23ade2 db0bc60 -1 2024/02/16 08:29:46 AM
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index cada2941..028b100d 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3378,2 +3378,45 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
+
+ /// If we receive an error message when attempting to open a channel, it may only be a rejection
+ /// of the channel type we tried, not of our ability to open any channel at all. We can see if a
+ /// downgrade of channel features would be possible so that we can still open the channel.
+ pub(crate) fn maybe_downgrade_channel_features<F: Deref>(
+ &mut self, fee_estimator: &LowerBoundedFeeEstimator<F>
+ ) -> Result<(), ()>
+ where
+ F::Target: FeeEstimator
+ {
+ if !self.is_outbound() ||
+ !matches!(
+ self.channel_state, ChannelState::NegotiatingFunding(flags)
+ if flags == NegotiatingFundingFlags::OUR_INIT_SENT
+ )
+ {
+ return Err(());
+ }
+ if self.channel_type == ChannelTypeFeatures::only_static_remote_key() {
+ // We've exhausted our options
+ return Err(());
+ }
+ // We support opening a few different types of channels. Try removing our additional
+ // features one by one until we've either arrived at our default or the counterparty has
+ // accepted one.
+ //
+ // Due to the order below, we may not negotiate `option_anchors_zero_fee_htlc_tx` if the
+ // counterparty doesn't support `option_scid_privacy`. Since `get_initial_channel_type`
+ // checks whether the counterparty supports every feature, this would only happen if the
+ // counterparty is advertising the feature, but rejecting channels proposing the feature for
+ // whatever reason.
+ if self.channel_type.supports_anchors_zero_fee_htlc_tx() {
+ self.channel_type.clear_anchors_zero_fee_htlc_tx();
+ self.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee);
+ assert!(!self.channel_transaction_parameters.channel_type_features.supports_anchors_nonzero_fee_htlc_tx());
+ } else if self.channel_type.supports_scid_privacy() {
+ self.channel_type.clear_scid_privacy();
+ } else {
+ self.channel_type = ChannelTypeFeatures::only_static_remote_key();
+ }
+ self.channel_transaction_parameters.channel_type_features = self.channel_type.clone();
+ Ok(())
+ }
}
@@ -7316,33 +7359,3 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
{
- if !self.context.is_outbound() ||
- !matches!(
- self.context.channel_state, ChannelState::NegotiatingFunding(flags)
- if flags == NegotiatingFundingFlags::OUR_INIT_SENT
- )
- {
- return Err(());
- }
- if self.context.channel_type == ChannelTypeFeatures::only_static_remote_key() {
- // We've exhausted our options
- return Err(());
- }
- // We support opening a few different types of channels. Try removing our additional
- // features one by one until we've either arrived at our default or the counterparty has
- // accepted one.
- //
- // Due to the order below, we may not negotiate `option_anchors_zero_fee_htlc_tx` if the
- // counterparty doesn't support `option_scid_privacy`. Since `get_initial_channel_type`
- // checks whether the counterparty supports every feature, this would only happen if the
- // counterparty is advertising the feature, but rejecting channels proposing the feature for
- // whatever reason.
- if self.context.channel_type.supports_anchors_zero_fee_htlc_tx() {
- self.context.channel_type.clear_anchors_zero_fee_htlc_tx();
- self.context.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee);
- assert!(!self.context.channel_transaction_parameters.channel_type_features.supports_anchors_nonzero_fee_htlc_tx());
- } else if self.context.channel_type.supports_scid_privacy() {
- self.context.channel_type.clear_scid_privacy();
- } else {
- self.context.channel_type = ChannelTypeFeatures::only_static_remote_key();
- }
- self.context.channel_transaction_parameters.channel_type_features = self.context.channel_type.clone();
+ self.context.maybe_downgrade_channel_features(fee_estimator)?;
Ok(self.get_open_channel(chain_hash))
@@ -7980,2 +7993,15 @@ impl<SP: Deref> OutboundV2Channel<SP> where SP::Target: SignerProvider {
+ /// If we receive an error message, it may only be a rejection of the channel type we tried,
+ /// not of our ability to open any channel at all. Thus, on error, we should first call this
+ /// and see if we get a new `OpenChannelV2` message, otherwise the channel is failed.
+ pub(crate) fn maybe_handle_error_without_close<F: Deref>(
+ &mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>
+ ) -> Result<msgs::OpenChannelV2, ()>
+ where
+ F::Target: FeeEstimator
+ {
+ self.context.maybe_downgrade_channel_features(fee_estimator)?;
+ Ok(self.get_open_channel_v2(chain_hash))
+ }
+
pub fn get_open_channel_v2(&self, chain_hash: ChainHash) -> msgs::OpenChannelV2 {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 6eed5a49..7ceb19c5 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -913,3 +913,3 @@ impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
#[cfg(dual_funding)]
- ChannelPhase::UnfundedInboundV1(_) => false,
+ ChannelPhase::UnfundedInboundV2(_) => false,
}
@@ -9265,10 +9265,25 @@ where
let peer_state = &mut *peer_state_lock;
- if let Some(ChannelPhase::UnfundedOutboundV1(chan)) = peer_state.channel_by_id.get_mut(&msg.channel_id) {
- if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator) {
- peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
- node_id: *counterparty_node_id,
- msg,
- });
- return;
- }
+ match peer_state.channel_by_id.get_mut(&msg.channel_id) {
+ Some(ChannelPhase::UnfundedOutboundV1(ref mut chan)) => {
+ if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator) {
+ peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
+ node_id: *counterparty_node_id,
+ msg,
+ });
+ return;
+ }
+ },
+ #[cfg(dual_funding)]
+ Some(ChannelPhase::UnfundedOutboundV2(ref mut chan)) => {
+ if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator) {
+ peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannelV2 {
+ node_id: *counterparty_node_id,
+ msg,
+ });
+ return;
+ }
+ },
+ None | Some(ChannelPhase::UnfundedInboundV1(_) | ChannelPhase::Funded(_)) => (),
+ #[cfg(dual_funding)]
+ Some(ChannelPhase::UnfundedInboundV2(_)) => (),
} |
|
😭 |
|
That's fixed by #2891 which could use another reviewer. |
dunxen
force-pushed
the
2023-12-dualfundingpreliminaries
branch
from
db0bc60
to
efec33f
Compare
wpaulino
approved these changes
Feb 20, 2024
TheBlueMatt
approved these changes
Feb 21, 2024
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.
Pulled some of the work that could be on its own out of #2302.