Three-hop blinded paths #2793
Three-hop blinded paths #2793
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2793 +/- ##
==========================================
- Coverage 89.84% 89.06% -0.78%
==========================================
Files 119 116 -3
Lines 97551 93294 -4257
Branches 97551 93294 -4257
==========================================
- Hits 87644 83092 -4552
- Misses 7332 7657 +325
+ Partials 2575 2545 -30 ☔ View full report in Codecov by Sentry. |
|
Needs rebase now that #2781 landed. |
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
78ddb01 to
6302480
Compare
|
Also need to sort the payment paths by some criteria. Earlier I figured we could use the |
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
6302480 to
ed86950
Compare
|
@coderabbitai review |
WalkthroughThe changes across the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 46
Configuration used: CodeRabbit UI
Files selected for processing (20)
- lightning/src/blinded_path/payment.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (5 hunks)
- lightning/src/ln/features.rs (1 hunks)
- lightning/src/ln/functional_test_utils.rs (11 hunks)
- lightning/src/ln/functional_tests.rs (1 hunks)
- lightning/src/ln/mod.rs (1 hunks)
- lightning/src/ln/offers_tests.rs (1 hunks)
- lightning/src/ln/outbound_payment.rs (11 hunks)
- lightning/src/ln/payment_tests.rs (2 hunks)
- lightning/src/ln/peer_handler.rs (2 hunks)
- lightning/src/offers/offer.rs (1 hunks)
- lightning/src/offers/refund.rs (1 hunks)
- lightning/src/onion_message/messenger.rs (9 hunks)
- lightning/src/routing/gossip.rs (2 hunks)
- lightning/src/routing/router.rs (9 hunks)
- lightning/src/routing/scoring.rs (1 hunks)
- lightning/src/sign/mod.rs (9 hunks)
- lightning/src/util/indexed_map.rs (1 hunks)
- lightning/src/util/test_utils.rs (5 hunks)
- lightning/src/util/time.rs (3 hunks)
Files skipped from review due to trivial changes (1)
- lightning/src/ln/peer_handler.rs
Additional comments: 95
lightning/src/ln/mod.rs (1)
- 82-84: The addition of the
offers_testsmodule gated with#[cfg(test)]is appropriate for including test-specific code. This ensures that the module is only compiled and included in test builds, which is a common and recommended practice.lightning/src/util/time.rs (1)
- 59-73: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [62-96]
The changes to conditionally compile the
MonotonicTimestruct,SHIFTconstant,Timetrait implementation forMonotonicTime, and the test functionmonotonic_time_subtractsunder the "std" feature are correct. This aligns with the shift from a no-std to std environment and is a standard practice for feature-based conditional compilation.lightning/src/util/indexed_map.rs (1)
- 61-65: The addition of the
get_key_valuemethod to theIndexedMapstruct is a useful enhancement. It provides a standard way to retrieve both the key and value from the map, which can be particularly useful in scenarios where both are needed and avoids separate lookups.lightning/src/blinded_path/payment.rs (1)
- 110-119: The implementation of
From<DirectedChannelInfo<'a>> for PaymentRelayis a logical addition, providing a clear conversion from channel information to payment relay parameters. This facilitates the construction of payment relays, which is an essential part of handling payments in the network.lightning/src/ln/offers_tests.rs (16)
44-55: The imports and macro definitions seem appropriate for the test module. No issues found here.
70-98: The
connect_peersanddisconnect_peersfunctions are utility functions used to manage peer connections within the tests. They appear to be correctly implemented, ensuring that both sides of the peer connection are established or disconnected.100-117: The
route_bolt12_paymentfunction is responsible for routing a BOLT 12 payment through a given path. It correctly monitors for added events and asserts the expected number of message events. The payment routing logic is encapsulated within thedo_pass_along_pathfunction, which is not shown here but assumed to be correct.119-128: The
claim_bolt12_paymentfunction is responsible for claiming a BOLT 12 payment. It correctly matches the expectedPaymentClaimableevent and callsclaim_payment. The test assumes that theclaim_paymentfunction works as expected.131-145: The
extract_invoice_request,extract_invoice, andextract_invoice_errorfunctions are responsible for extracting different types of messages from anOnionMessage. They correctly handle the peeled onion message and match against the expected message contents. The use of panics in the error handling is typical for test code where a failure to meet the expected conditions should stop the test immediately.180-267: The test
creates_and_pays_for_offer_using_two_hop_blinded_pathchecks the payment flow for an offer using a two-hop blinded path. The test setup includes creating channels and disconnecting certain peers to ensure the correct network topology. The test then proceeds through the steps of creating an offer, paying for it, and claiming the payment. The assertions check that the correct events are generated and that the payment details are as expected. The test is well-structured and covers the intended functionality.269-346: The test
creates_and_pays_for_refund_using_two_hop_blinded_pathis similar to the previous test but for a refund. It follows the same pattern of setting up the network, creating a refund, and then routing and claiming the payment. The assertions are in place to ensure the correct behavior.349-403: The test
creates_and_pays_for_offer_using_one_hop_blinded_pathchecks the payment flow for an offer using a one-hop blinded path. The test setup is simpler due to the smaller network topology. The test follows the same pattern of creating an offer, paying for it, and claiming the payment. The assertions validate the correct behavior and the use of ephemeral pubkeys.406-457: The test
creates_and_pays_for_refund_using_one_hop_blinded_pathis the refund counterpart to the previous test, with a one-hop blinded path. It follows the same structure and checks for the correct behavior with assertions.460-501: The test
pays_for_offer_without_blinded_pathschecks the behavior when paying for an offer that does not include any blinded paths. The test ensures that the payment can still be made using blinded paths as required by the spec. The assertions check that the payment is processed correctly.503-542: The test
pays_for_refund_without_blinded_pathsis similar to the previous test but for a refund. It ensures that the payment can be made without blinded paths in the refund, but still uses blinded paths for the payment itself, as per the spec.545-582: The test
fails_creating_offer_without_blinded_pathschecks that an error is returned when trying to create an offer without blinded paths, which is necessary to avoid exposing the node's id. The test correctly expects aBolt12SemanticError::MissingPathserror.561-617: The test
fails_creating_invoice_request_without_blinded_reply_pathand the subsequent testsfails_creating_refund_without_blinded_paths,fails_creating_invoice_request_with_duplicate_payment_id, andfails_creating_refund_with_duplicate_payment_idall check for error conditions when creating offers, refunds, and invoice requests. They correctly expect specific errors and assert that no recent payments are listed after the failed operations.688-751: The test
fails_sending_invoice_without_blinded_payment_paths_for_offerchecks that an error is returned when trying to send an invoice for an offer without blinded payment paths. The test setup includes disabling route blinding to simulate the error condition. The test correctly expects anInvoiceErrorwith aBolt12SemanticError::MissingPaths.754-798: The test
fails_sending_invoice_without_blinded_payment_paths_for_refundis similar to the previous test but for a refund. It also correctly expects an error when route blinding is disabled.802-892: The test
fails_paying_invoice_more_than_oncechecks that an invoice cannot be paid more than once. The test setup includes creating a refund and sending two invoices for it. The test correctly expects that the second invoice will result in an error, which is simulated by sending anInvoiceErrorwith a "DuplicateInvoice" message.lightning/src/onion_message/messenger.rs (10)
746-749: The
set_offers_handlerfunction has been added to allow setting a new offers handler. This function is marked with#[cfg(test)], indicating it's only available for testing purposes, which is appropriate if it's intended for use in test scenarios only.128-141: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [122-138]
The example usage in the comments is very helpful for understanding how to use the
OnionMessenger. However, it's important to ensure that the example code is kept up-to-date with the actual API to prevent confusion.
851-858: The
peel_onion_messagefunction is marked aspub(crate), which restricts its visibility to the current crate. This is good for encapsulation, but ensure that this visibility is intended and that the function does not need to be part of the public API.941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]
The
handle_onion_messagefunction logs at different levels (trace,error) based on the outcome of thepeel_onion_messagecall. Ensure that the logging levels are consistent with the severity of the events being logged.Verification successful
Based on the output from the
rgcommand, it is evident that the codebase uses a variety of logging levels (trace,debug,info,warn,error) in different contexts. The use oftracefor detailed and less critical information anderrorfor serious issues is consistent with common logging practices.In the specific context of the
handle_onion_messagefunction, the use of different logging levels based on the outcome of thepeel_onion_messagecall is likely appropriate. Thetracelevel is used for less severe events that are useful for detailed debugging, while theerrorlevel is reserved for significant issues that may require immediate attention.Therefore, the logging levels in the
handle_onion_messagefunction appear to be consistent with the severity of the events being logged, and the review comment does not seem to highlight an inconsistency with the project's logging practices.* 941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logging levels used throughout the codebase are consistent with the severity of the events. rg --type rust 'log_(trace|debug|info|warn|error)!'Length of output: 83426
The
handle_onion_messagefunction does not return any value, but it performs critical operations such as forwarding onion messages or handling received messages. Ensure that any necessary actions are taken based on the outcome of these operations, such as retrying message forwarding or alerting the user in case of errors.
- 941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]
The
handle_onion_messagefunction uses a#[cfg(fuzzing)]attribute, which is a conditional compilation flag for fuzz testing. Ensure that this flag is being used consistently across the codebase and that the fuzzing configuration is properly set up in the build system.Verification successful
The search results indicate that there are specific
Cargo.tomlfiles and shell scripts within thefuzzdirectories that reference fuzzing configurations and dependencies, such ashonggfuzz. The presence of these configurations inCargo.tomlfiles and the use of fuzzing-related commands inci-fuzz.shscripts suggest that the fuzzing configuration is indeed properly set up in the build system.Additionally, the
check-compiles.shscript includes a command that sets thefuzzingconfiguration flag when runningcargo check, which further supports the proper setup for fuzz testing.Based on this information, it can be concluded that the
#[cfg(fuzzing)]attribute is used consistently across the codebase and the fuzzing configuration is properly set up in the build system.* 941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the fuzzing configuration is properly set up and used consistently. rg --type rust '#\[cfg\(fuzzing\)\]'Length of output: 818
Script:
#!/bin/bash # Check for the presence of fuzzing configuration in Cargo.toml or other build scripts. rg --type toml --type sh 'fuzz'Length of output: 2305
The
handle_onion_messagefunction uses a lock onself.message_recipientsmultiple times within the same scope. This could potentially lead to deadlocks if not handled carefully. Review the locking strategy to ensure that it is safe and efficient.Verification successful
Based on the analysis of the
messenger.rsfile, the locking strategy formessage_recipientsappears to be safe in the context of thehandle_onion_messagefunction. The lock is acquired once at the beginning of the function and is held until the function returns, with no nested locks or reentrant calls that could lead to deadlocks. While the use ofunwrap()on lock acquisition could cause a panic if the lock is poisoned, this is a separate issue from deadlocks. The code uses theMutexin a typical and straightforward manner to protect shared data.* 941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the locking strategy on self.message_recipients is safe and does not lead to deadlocks. rg --type rust 'message_recipients.lock()'Length of output: 1104
Script:
#!/bin/bash # Examine the context of each lock acquisition to understand the locking strategy better. # We will look at the surrounding code for each instance where `message_recipients.lock().unwrap()` is called. rg --type rust -C 10 'message_recipients.lock().unwrap()' lightning/src/onion_message/messenger.rsLength of output: 100
Script:
#!/bin/bash # Attempting a different approach to examine the code surrounding the lock acquisitions. # We will use the `cat` command to output the entire file and then manually locate the relevant sections. cat lightning/src/onion_message/messenger.rsLength of output: 46230
The
handle_onion_messagefunction has a section of code under the#[cfg(fuzzing)]attribute. Ensure that this code is only for fuzz testing and that it does not affect the production code path.
- 941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]
The
handle_onion_messagefunction useslog_trace!andlog_error!macros for logging. Ensure that these macros are defined and used consistently throughout the codebase.Verification successful
The
log_trace!andlog_error!macros are used consistently throughout therust-lightningproject, as evidenced by the numerous instances across various files. This indicates that these macros are part of a common logging framework or utility used project-wide.* 941-947: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [944-1004]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logging macros are defined and used consistently. rg --type rust 'log_(trace|error)!'Length of output: 49369
The
handle_onion_messagefunction's error handling logs at different levels based on the error. Ensure that the severity of the log messages matches the severity of the errors.Verification successful
The log messages within the
handle_onion_messagefunction inmessenger.rsuse appropriate severity levels. Thelog_error!macro is used for actual error conditions, whilelog_trace!is used for informational messages that are not errors but may be relevant for detailed operational logging. The severity of the log messages matches the severity of the events they log.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the severity of log messages matches the severity of the errors. rg --type rust 'log_(trace|error)!'Length of output: 49369
lightning/src/offers/refund.rs (1)
- 300-303: The method
clear_pathshas been added to theRefundBuilderstruct. This method resets thepathsfield toNone, effectively clearing any blinded paths that have been added. This is a useful addition for cases where the paths need to be reset during the building process.lightning/src/ln/features.rs (2)
- 923-928: The implementation of
clear_route_blindingfor types that satisfy theRouteBlindingtrait seems to be correct. However, the use of#[cfg(test)]suggests that this function is only available for testing purposes. Ensure that this is the intended use case and that the function is not required in the non-test codebase.Verification successful
The
offers_testsmodule is indeed gated by the#[cfg(test)]attribute, indicating that it is part of the test suite. Therefore, the use ofclear_route_blindingwithinoffers_tests.rsis consistent with its intended use case for testing purposes only, as denoted by the#[cfg(test)]attribute in thefeatures.rsfile.* 929-929: The `unknown` function is added for types satisfying the `UnknownFeature` trait. This function is not shown in its entirety, but it appears to be a constructor for creating a `Features` instance with unknown features. It's important to ensure that this function is used correctly throughout the codebase and that it does not introduce any unexpected behavior, especially since it is not gated by `#[cfg(test)]`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `clear_route_blinding` function is not used outside of tests. rg --type rust --glob '!*_test.rs' 'clear_route_blinding'Length of output: 269
Script:
#!/bin/bash # Check if `offers_tests.rs` is included in a test configuration. rg --type rust 'mod offers_tests;' -- 'lightning/src/ln/mod.rs'Length of output: 81
Script:
#!/bin/bash # Check for the presence of a cfg(test) attribute above the offers_tests module declaration. rg --type rust -C 3 'mod offers_tests;' -- 'lightning/src/ln/mod.rs'Length of output: 200
Verification successful
The
Features::unknownfunction is used within test cases in therefund.rs,offer.rs, andinvoice_request.rsfiles to simulate scenarios with unknown features and to ensure that the system behaves correctly when encountering them. The usage is appropriate for the test contexts, and there is no indication ofFeatures::unknownbeing used in non-test code or introducing unexpected behavior. The review comment's concern is addressed by the current usage pattern of the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `unknown` function is used correctly. rg --type rust 'Features::unknown'Length of output: 1374
Script:
#!/bin/bash # Examine the context of usage of `Features::unknown` in `refund.rs` rg --type rust --context 5 'Features::unknown' lightning/src/offers/refund.rs # Examine the context of usage of `Features::unknown` in `offer.rs` rg --type rust --context 5 'Features::unknown' lightning/src/offers/offer.rs # Examine the context of usage of `Features::unknown` in `invoice_request.rs` rg --type rust --context 5 'Features::unknown' lightning/src/offers/invoice_request.rsLength of output: 4328
lightning/src/util/test_utils.rs (10)
35-38: The imports for
DefaultMessageRouterandDefaultRouterhave been added. This change aligns with the PR's objective to enhance routing functionality with new types.107-113: The
TestRouterstruct now includes arouterfield of typeDefaultRouter. This change seems to be part of the refactoring to use the newDefaultRoutertype. Ensure that all instances ofTestRouterare updated to handle the new field correctly.Verification successful
The
TestRouterstruct'snewmethod has been verified, and it initializes therouterfield with the necessary parameters. The calls toTestRouter::newacross various files in the codebase are consistent with this updated method signature, passing thenetwork_graph,logger, andscoreras arguments. Therefore, the instances ofTestRouterhave been correctly updated to handle the newrouterfield.* 204-204: The `find_route` method in `TestRouter` now delegates to the `router` field's `find_route` method. This change is consistent with the refactoring to use `DefaultRouter`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all instances of TestRouter are updated to handle the new router field. rg --type rust "TestRouter::new"Length of output: 1403
Script:
#!/bin/bash # View the implementation of the TestRouter::new method to verify the initialization of the router field. rg --type rust -A 10 "impl TestRouter" lightning/src/util/test_utils.rsLength of output: 73
Script:
#!/bin/bash # Broaden the search to find the TestRouter struct and associated methods in the test_utils.rs file. rg --type rust "struct TestRouter" -A 30 lightning/src/util/test_utils.rsLength of output: 965
- 210-215: The
create_blinded_payment_pathsmethod inTestRouterdelegates to therouterfield's corresponding method. This is part of the refactoring and should be checked to ensure that the delegation is correctly implemented.Verification successful
The output from the
rgcommand provides the information needed to verify the implementation of thecreate_blinded_payment_pathsmethod inTestRouterand the type of therouterfield withinTestRouter.From the output, we can see that
TestRoutercontains a field namedrouterwhich is of typeDefaultRouter. TheDefaultRouteris parameterized with several types, includingArc<NetworkGraph<&'a TestLogger>>,'a TestLogger,'a RwLock<TestScorer>, and().This confirms that the
routerfield inTestRouteris indeed of typeDefaultRouter, and the previous context provided shows that thecreate_blinded_payment_pathsmethod inTestRouterdelegates to therouterfield's corresponding method.Based on the information gathered, the delegation in
create_blinded_payment_pathswithinTestRouteris correctly implemented, as it correctly references therouterfield of typeDefaultRouter.* 221-223: The `find_path` method in `TestRouter` now delegates to the `router` field's `find_path` method. This change is consistent with the refactoring to use `DefaultRouter`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the delegation in create_blinded_payment_paths is correctly implemented. rg --type rust "create_blinded_payment_paths"Length of output: 897
Script:
#!/bin/bash # Verify the signature of the create_blinded_payment_paths method in DefaultRouter ast-grep --lang rust --pattern $'impl DefaultRouter { pub fn create_blinded_payment_paths<$$_>($$$) -> $_ { $$$ } }' # Verify that the router field in TestRouter is of type DefaultRouter ast-grep --lang rust --pattern $'struct TestRouter { router: DefaultRouter, $$$ }'Length of output: 213
Script:
#!/bin/bash # Adjusting the pattern to match the method signature in a more general way ast-grep --lang rust --pattern $'impl $_ { pub fn create_blinded_payment_paths<$$_>($$$) -> $_ { $$$ } }' # Adjusting the pattern to match the TestRouter struct in a more general way ast-grep --lang rust --pattern $'struct TestRouter { $$$ }'Length of output: 177
Script:
#!/bin/bash # Use ripgrep to find the TestRouter struct definition rg --type rust --context 5 "struct TestRouter" lightning/src/util/test_utils.rs # Use ripgrep to find the implementation of the create_blinded_payment_paths method within DefaultRouter context rg --type rust --context 5 "impl DefaultRouter" lightning/src/routing/router.rsLength of output: 437
- 229-232: The
create_blinded_pathsmethod inTestRouterdelegates to therouterfield's corresponding method. This is part of the refactoring and should be checked to ensure that the delegation is correctly implemented.Verification successful
To verify the correct implementation of the delegation in the
create_blinded_pathsmethod within theTestRouterstruct, we need to examine the method signatures in both theTestRouterand theDefaultRouter(or the type of therouterfield) to ensure they align and that the parameters are passed correctly.
The output from the scripts shows that the
create_blinded_pathsmethod signature inTestRoutermatches the signature inDefaultRouter. Both methods take the same parameters: a recipient of typePublicKey, a vector of peers of typeVec<PublicKey>, an entropy source that implements theEntropySourcetrait, and aSecp256k1context. They both return aResultwith a vector ofBlindedPathor an error of unit type().Given that the signatures match and the delegation in
TestRouterpasses the parameters directly to therouterfield'screate_blinded_pathsmethod, we can conclude that the delegation is correctly implemented.* 247-254: The `TestMessageRouter` struct has been added with an `inner` field of type `DefaultMessageRouter`. This addition is consistent with the PR's objective to enhance message routing functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the delegation in create_blinded_paths is correctly implemented. rg --type rust "create_blinded_paths"Length of output: 1563
Script:
#!/bin/bash # Check the method signature of create_blinded_paths in TestRouter and DefaultRouter rg --type rust -A 5 "fn create_blinded_paths<" lightning/src/util/test_utils.rs rg --type rust -A 5 "fn create_blinded_paths<" lightning/src/routing/router.rsLength of output: 903
258-262: The
find_pathmethod inTestMessageRouterdelegates to theinnerfield'sfind_pathmethod. This change is consistent with the introduction of theDefaultMessageRouter.265-271: The
create_blinded_pathsmethod inTestMessageRouterdelegates to theinnerfield's corresponding method. This is part of the refactoring and should be checked to ensure that the delegation is correctly implemented.Verification successful
The review of the
create_blinded_pathsmethod in bothTestRouterandTestMessageRouterwithinlightning/src/util/test_utils.rsconfirms that the delegation to therouterandinnerfields, respectively, is correctly implemented according to Rust's syntax and conventions. The method signatures match, and the delegation pattern is consistent with typical Rust practices.* 1434-1436: The `TestScorer` struct now implements the `Score` trait for the `c_bindings` configuration. This change should be verified to ensure that it aligns with the intended use of `TestScorer` in different build configurations.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the delegation in create_blinded_paths is correctly implemented. rg --type rust "create_blinded_paths"Length of output: 1563
Script:
#!/bin/bash # Verify the context of the delegation in create_blinded_paths within TestMessageRouter and TestRouter rg --type rust -C 10 "create_blinded_paths" lightning/src/util/test_utils.rsLength of output: 1550
lightning/src/offers/offer.rs (1)
- 342-345: The
clear_pathsmethod has been added to allow clearing the paths in the offer. The method is correctly usingpub(crate)visibility, which restricts its use within the crate, a good practice for encapsulation. The method takesselfby mutable reference and returnsSelf, allowing for method chaining. The implementation is straightforward and there are no apparent issues with logic, syntax, or performance.However, ensure that the
Offerstruct's invariants are maintained after paths are cleared, and consider if any additional actions should be taken when paths are cleared (e.g., updating related state or notifying other components).pub(crate) fn clear_paths(mut self) -> Self { self.offer.paths = None; self }lightning/src/sign/mod.rs (9)
825-826: The
InMemorySignerstruct now includes anentropy_sourcefield of typeRandomBytes. This change centralizes entropy generation, which is a good practice for cryptographic applications. Ensure that all instances ofInMemorySignerare updated to initialize this new field correctly.857-857: The
clonemethod forInMemorySignerhas been updated to use the newentropy_sourcefield. It's important to ensure that theget_secure_random_bytesmethod used here provides a fresh, secure source of entropy for each clone to prevent any cryptographic weaknesses.Verification successful
Based on the provided output, the
get_secure_random_bytesmethod in theRandomBytesstruct withinlightning/src/sign/mod.rsuses the ChaCha20 algorithm and an atomic counter to ensure that each call generates a unique and secure 32-byte value. This satisfies the requirement for providing a fresh source of entropy for each clone ofInMemorySigner.* 891-891: The `new` method for `InMemorySigner` has been modified to initialize the `entropy_source` with a given `rand_bytes_unique_start`. It's crucial to verify that the `rand_bytes_unique_start` is securely generated and unique for each signer to maintain cryptographic security.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that get_secure_random_bytes generates unique, secure random bytes for each clone. rg --type rust "fn get_secure_random_bytes" --context 5Length of output: 8749
1067-1067: The
get_secure_random_bytesmethod in theEntropySourceimplementation forInMemorySignernow delegates to theentropy_source. This is a good encapsulation of functionality. However, it's important to review theRandomBytes::get_secure_random_bytesmethod to ensure it meets the security requirements.1345-1345: The
InMemorySigneris being initialized with anentropy_sourceusing bytes from anotherEntropySource. This is a potential security risk if theEntropySourcepassed in does not provide secure random bytes. It's important to ensure that the source of entropy is secure and that this pattern does not introduce any vulnerabilities.Verification successful
The
EntropySourcetrait is implemented across various modules in the codebase, ensuring the generation of secure random bytes for cryptographic operations. TheRandomBytesstruct, which implementsEntropySource, utilizes the ChaCha20 algorithm, a secure method for random number generation.Regarding the review comment, initializing an
InMemorySignerwith anentropy_sourcederived from anotherEntropySourceis a pattern that relies on the security of the initial source. If the initialEntropySourceis secure, this pattern should not introduce vulnerabilities. The use of ChaCha20 in theRandomBytesimplementation suggests that the entropy source is likely secure.* 1373-1373: The `KeysManager` struct now includes an `entropy_source` field. This change is consistent with the updates to `InMemorySigner` and is a good practice for managing entropy in cryptographic systems. Ensure that all instances of `KeysManager` are updated to initialize this new field correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the EntropySource passed to InMemorySigner provides secure random bytes. rg --type rust "EntropySource" --context 5Length of output: 132911
- 1442-1442: The
newmethod forKeysManagerhas been updated to initialize theentropy_sourcewithrand_bytes_unique_start. Similar toInMemorySigner, it's important to verify that therand_bytes_unique_startis securely generated and unique for eachKeysManagerinstance.Verification successful
The review comment is concerned with the secure generation and uniqueness of
rand_bytes_unique_startfor eachKeysManagerinstance. The script output shows thatrand_bytes_unique_startis generated using a SHA256 hash function, which includes the current time in seconds and nanoseconds, a seed, and a constant string "LDK PRNG Seed" as inputs. This approach is typically sufficient to ensure thatrand_bytes_unique_startis both secure and unique per instance.* 1623-1623: The `get_secure_random_bytes` method in the `EntropySource` implementation for `KeysManager` now delegates to the `entropy_source`. This change is consistent with the encapsulation of entropy generation in `InMemorySigner` and should be reviewed to ensure it meets security requirements.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that rand_bytes_unique_start is securely generated and unique for each KeysManager instance. rg --type rust "rand_bytes_unique_start" --context 5Length of output: 2547
- 1877-1904: The new
RandomBytesstruct and its implementation of theEntropySourcetrait using the ChaCha20 algorithm is a significant change. It's important to ensure that the ChaCha20 algorithm is correctly implemented and that theindexis used securely to prevent any reuse of the nonce, which could lead to cryptographic vulnerabilities.lightning/src/ln/outbound_payment.rs (11)
26-26: The change to the
cfgattribute here is consistent with the PR's objective to adjust conditional compilation for standard library features. This looks good.285-299: The modifications to the
cfgattributes and theimpl_writeable_tlv_based_enum!macro usage are aligned with the changes to the feature flags. Ensure that theRetryenum is tested with both configurations (stdandnot(std)).312-315: The conditional compilation here is correctly updated to reflect the new feature flags. However, ensure that the time comparison logic is consistent and correct in both the
stdandtestconfigurations.Verification successful
To verify the consistency and correctness of the time comparison logic under both
stdandtestconfigurations inlightning/src/ln/outbound_payment.rs, we need to examine the actual usage ofduration_sincewithin the conditional compilation blocks. The output from the script indicates that there are relevant blocks to review. Let's proceed with examining the code surrounding these blocks.
The review comment concerning the consistency and correctness of the time comparison logic in both
stdandtestconfigurations inlightning/src/ln/outbound_payment.rshas been addressed. The logic is consistent across both configurations, using theduration_sincemethod. The difference in the time source (MonotonicTimefor standard andSinceEpochfor test configurations) is intentional and a common practice to facilitate testing. The conditional compilation attributes are correctly updated to reflect the new feature flags.* 341-371: The changes to the `PaymentAttemptsUsingTime` struct and its associated `Display` implementation are consistent with the conditional compilation updates. Ensure that the `Display` implementation is tested for both `std` and `not(std)` configurations.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the time comparison logic is consistent and correct in both configurations rg --type rust "duration_since" --files-with-matches | xargs rg --type rust "#\[cfg\(all\(feature = \"std\", (not\(test\)|test)\)\)\]"Length of output: 671
Script:
#!/bin/bash # Examine the usage of `duration_since` within the conditional compilation blocks in `outbound_payment.rs` rg --type rust -C 5 "#\[cfg\(all\(feature = \"std\", not\(test\)\)\)\]" lightning/src/ln/outbound_payment.rs rg --type rust -C 5 "#\[cfg\(all\(feature = \"std\", test\)\)\]" lightning/src/ln/outbound_payment.rsLength of output: 2374
1879-1879: This hunk shows test setup code. There are no changes to the code itself, only the context in which it is shown. No action is required.
1923-1923: Similar to the previous hunk, this is test setup code without any changes. No action is required.
1962-1962: Again, this hunk shows test setup code without any changes. No action is required.
2169-2169: This hunk is also showing test setup code without any changes. No action is required.
2220-2220: This hunk is showing test setup code without any changes. No action is required.
2279-2279: This hunk is showing test setup code without any changes. No action is required.
2338-2338: This hunk is showing test setup code without any changes. No action is required.
lightning/src/routing/gossip.rs (2)
1029-1029: The access level of the
directionfunction has been changed frompub(super)topub(crate). This widens the visibility of the function within the crate, which could be intended for increased internal reusability. Ensure that this change aligns with the intended encapsulation and usage within the crate.1848-1848: The conditional compilation directive has been changed from
#[cfg(feature = "no-std")]to#[cfg(not(feature = "std"))]. This change seems to be a correction to ensure that the code is included when thestdfeature is not enabled, which is more idiomatic for Rust crates that support bothstdandno-stdenvironments. Confirm that this change aligns with the project's feature management strategy.lightning/src/ln/functional_test_utils.rs (12)
22-31: The imports have been reorganized and new ones added. This is a common practice to maintain code cleanliness and ensure that all necessary modules are available for use. The added imports are related to message handling and entropy generation, which align with the new features being implemented.
48-52: Additional imports from the standard library have been added. These are foundational utilities that are often used in Rust for memory management and synchronization. Their addition suggests that the new code will involve reference counting and locking mechanisms.
394-394: A new field
message_routerof typetest_utils::TestMessageRouterhas been added to theNodeCfgstruct. This change is likely related to the new onion messaging functionality and is necessary for routing messages during tests.414-421: A new type alias
TestOnionMessengerhas been defined, which is a specialized version ofOnionMessengerfor testing purposes. It usesDedicatedEntropyfor entropy generation, which is defined later in the code. This is a good example of using type aliases to simplify complex type signatures.423-432: The
DedicatedEntropystruct is introduced to provide a specific entropy source for theOnionMessenger. It wraps theRandomBytesstruct, providing a dereference to the inner type. This encapsulation ensures that the entropy source can be controlled during testing.442-442: The
Nodestruct has been updated with a new fieldonion_messengerof typeTestOnionMessenger. This addition is consistent with the new functionality for onion messaging within the test utilities.462-465: A new method
init_featureshas been added to theNodestruct. This method combines the features provided by the node and the onion messenger, which is a logical step to ensure that all features are initialized correctly when establishing peer connections.635-635: The
routerfield is being initialized with aTestRouter. This is part of the setup for a node configuration and is necessary for testing the routing functionality.1090-1090: The
onion_messengeris being set with an offers handler using theset_offers_handlermethod. This is part of the node reloading process and is necessary to ensure that the onion messenger is correctly configured with the new channel manager.2915-2916: The
message_routerfield is being initialized with aTestMessageRouter. This is part of the node configuration setup and is necessary for testing the message routing functionality.2970-2982: The
onion_messengeris being initialized with a newOnionMessengerinstance usingDedicatedEntropy. This is part of the node setup and is necessary for testing the onion messaging functionality. TheIgnoringMessageHandleris used, which is appropriate for tests where message handling is not the focus.2997-3014: The code here is handling the peer connection initialization between nodes. It uses the
init_featuresmethod to get the features for each node and then callspeer_connectedon both thenodeandonion_messenger. This is a necessary step to simulate the peer connection process in a testing environment.lightning/src/routing/scoring.rs (1)
- 254-254: The conditional compilation directive added here ensures that the implementation of
LockableScoreforRwLock<T>is only compiled when the_test_utilsfeature or tests are being run. Please confirm that this feature flag is consistent with the project's conventions for test utilities and that it is being used elsewhere in the codebase as intended.Verification successful
The search results confirm that the feature flag
_test_utilsis used consistently throughout therust-lightningproject. It is applied in various files and contexts, often in conjunction with thetestconfiguration, to conditionally compile code for testing purposes. The usage of this feature flag in thescoring.rsfile is in line with the project's conventions.Based on the information gathered, the conditional compilation directive in
scoring.rsis correctly placed and the feature flag_test_utilsis appropriately used within the project's conventions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the feature flag '_test_utils' is used consistently across the project rg --type rust 'cfg\(.*_test_utils.*\)'Length of output: 2636
lightning/src/ln/payment_tests.rs (2)
45-50: The import statements have been reorganized, and new imports for
std::timeandcrate::util::time::tests::SinceEpochhave been added. Ensure that these new imports are used within the test module, otherwise, they should be removed to avoid unused imports.2318-2318: The addition of a conditional compilation block suggests that the enclosed code is only relevant when the
stdfeature is enabled. Verify that thestdfeature is indeed required for theDuration::from_secsmethod and that theRetry::Timeoutparameter is correctly implemented in thesend_paymentfunction.lightning/src/routing/router.rs (8)
93-94: The introduction of
recipient_node_idis a good practice for clarity and maintainability. It's important to ensure that theNodeIdtype andfrom_pubkeymethod are correctly implemented and thatrecipientis always a validPublicKey.103-116: The chaining of
filterandfilter_mapcalls is a common pattern in Rust for data transformation. However, it's important to ensure that the logic within these calls is correct and that the conditions used for filtering are appropriate for the intended functionality. The use ofunwrap_orwithu64::MAXand0is a sensible default but should be reviewed to ensure it aligns with the domain logic.145-228: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [139-150]
The construction of
forward_nodeand the subsequentSome((forward_node, counterparty_channels))tuple is a key part of the new logic. It's crucial to verify that the fields ofForwardNodeare being populated correctly and that thecounterparty_channelsbeing paired with it are the intended ones.
205-224: The construction of
two_hop_pathsand the subsequent logic to determine the final paths to return is critical. It's important to ensure that thecollectandand_thencalls are used correctly to handle the possible outcomes and that the fallback toone_hop_for_paymentis appropriate. The use ofok_or(())should be reviewed to ensure that it's the best way to handle errors in this context.6957-6957: The
generate_routestest function is usingProbabilisticScorerandProbabilisticScoringFeeParameters. It's important to ensure that the test covers the new logic introduced in this PR and that the scoring parameters are set up correctly for the test scenarios.6978-6978: Similar to the previous comment, the
generate_routes_mpptest function should be reviewed to ensure that it adequately tests the multi-path payment (MPP) logic with the new changes.6999-6999: The
generate_large_mpp_routestest function should be reviewed to ensure that it is testing the generation of large MPP routes correctly and that the test is robust enough to cover the new changes.8335-8335: The
bench_utilsmodule is being used for benchmarking and testing. It's important to ensure that the benchmarks are updated to reflect the new logic and that they are correctly measuring the performance of the relevant parts of the code.lightning/src/ln/functional_tests.rs (1)
- 5540-5542: The addition of
message_routerto therouterdeclaration and the subsequent update to thenodedeclaration seem to align with the PR's objectives of enhancing privacy through the implementation of blinded paths. Ensure that theTestMessageRouteris properly initialized and that its interactions with other components are correctly defined and tested.lightning/src/ln/channelmanager.rs (5)
7759-7767: In the conditional compilation blocks, ensure that the
create_blinded_payment_pathsfunction is correctly handling thestdfeature flag. The use ofmap_errto convert errors toBolt12SemanticError::MissingPathsis appropriate, but it's important to verify that this error type is being handled correctly wherever this function is called.9212-9222: The handling of
created_atusingself.highest_seen_timestampfor non-stdenvironments is a good fallback. However, it's crucial to ensure that thehighest_seen_timestampis being updated correctly elsewhere in the code to avoid potential timing issues.9231-9233: The conditional compilation for
respond_withandrespond_with_no_stdseems to be consistent with the previous patterns. It's important to check that therespond_with_no_stdfunction is implemented and handles thecreated_atparameter correctly.12473-12473: The removal of imports in this hunk suggests that there might have been code cleanup or refactoring. Since the PR instructions indicate not to flag removal of imports, this change is noted but does not require action.
12513-12513: The addition of
test_utils::TestRouterand the configuration ofmax_dust_htlc_exposurein the test setup is a good practice to ensure that the tests reflect the changes in the configuration. It's important to ensure that the tests cover the new code paths introduced by the PR.
| let three_hop_paths = counterparty_channels.clone() | ||
| // Pair counterparties with their other channels | ||
| .flat_map(|(forward_node, counterparty_channels)| | ||
| counterparty_channels | ||
| .iter() | ||
| .filter_map(|scid| network_graph.channels().get_key_value(scid)) | ||
| .filter_map(move |(scid, info)| info | ||
| .as_directed_to(&NodeId::from_pubkey(&forward_node.node_id)) | ||
| .map(|(info, source)| (source, *scid, info)) | ||
| ) | ||
| .filter(|(source, _, _)| **source != recipient_node_id) | ||
| .filter(|(source, _, _)| network_graph | ||
| .node(source) | ||
| .and_then(|info| info.announcement_info.as_ref()) | ||
| .map(|info| info.features.supports_route_blinding()) | ||
| .unwrap_or(false) | ||
| ) | ||
| .filter(|(_, _, info)| amount_msats >= info.direction().htlc_minimum_msat) | ||
| .filter(|(_, _, info)| amount_msats <= info.direction().htlc_maximum_msat) | ||
| .map(move |(source, scid, info)| (source, scid, info, forward_node.clone())) | ||
| ) | ||
| // Construct blinded paths where the counterparty's counterparty is the introduction | ||
| // node: | ||
| // | ||
| // source --- info ---> counterparty --- counterparty_forward_node ---> recipient | ||
| .map(|(introduction_node_id, scid, info, counterparty_forward_node)| { | ||
| let introduction_forward_node = { | ||
| let htlc_minimum_msat = info.direction().htlc_minimum_msat; | ||
| let htlc_maximum_msat = info.direction().htlc_maximum_msat; | ||
| let payment_relay: PaymentRelay = info.into(); | ||
| let payment_constraints = PaymentConstraints { | ||
| max_cltv_expiry: payment_relay.cltv_expiry_delta as u32 | ||
| + counterparty_forward_node.tlvs.payment_constraints.max_cltv_expiry, | ||
| htlc_minimum_msat, | ||
| }; | ||
| ForwardNode { | ||
| tlvs: ForwardTlvs { | ||
| short_channel_id: scid, | ||
| payment_relay, | ||
| payment_constraints, | ||
| features: BlindedHopFeatures::empty(), | ||
| }, | ||
| node_id: introduction_node_id.as_pubkey().unwrap(), | ||
| htlc_maximum_msat, | ||
| } | ||
| }; | ||
| BlindedPath::new_for_payment( | ||
| &[introduction_forward_node, counterparty_forward_node], recipient, | ||
| tlvs.clone(), u64::MAX, entropy_source, secp_ctx | ||
| ) | ||
| }) | ||
| .map(|forward_node| { | ||
| .take(MAX_PAYMENT_PATHS); | ||
|
|
There was a problem hiding this comment.
The logic for constructing three_hop_paths is complex and involves multiple filtering and mapping operations. It's important to ensure that the logic correctly implements the intended path construction, especially the conditions in the filter calls. The use of clone on counterparty_channels should be reviewed for potential performance implications, and it may be worth considering if there's a more efficient way to achieve the same result without cloning.
- let three_hop_paths = counterparty_channels.clone()
+ let three_hop_paths = counterparty_channels.iter()Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let three_hop_paths = counterparty_channels.clone() | |
| // Pair counterparties with their other channels | |
| .flat_map(|(forward_node, counterparty_channels)| | |
| counterparty_channels | |
| .iter() | |
| .filter_map(|scid| network_graph.channels().get_key_value(scid)) | |
| .filter_map(move |(scid, info)| info | |
| .as_directed_to(&NodeId::from_pubkey(&forward_node.node_id)) | |
| .map(|(info, source)| (source, *scid, info)) | |
| ) | |
| .filter(|(source, _, _)| **source != recipient_node_id) | |
| .filter(|(source, _, _)| network_graph | |
| .node(source) | |
| .and_then(|info| info.announcement_info.as_ref()) | |
| .map(|info| info.features.supports_route_blinding()) | |
| .unwrap_or(false) | |
| ) | |
| .filter(|(_, _, info)| amount_msats >= info.direction().htlc_minimum_msat) | |
| .filter(|(_, _, info)| amount_msats <= info.direction().htlc_maximum_msat) | |
| .map(move |(source, scid, info)| (source, scid, info, forward_node.clone())) | |
| ) | |
| // Construct blinded paths where the counterparty's counterparty is the introduction | |
| // node: | |
| // | |
| // source --- info ---> counterparty --- counterparty_forward_node ---> recipient | |
| .map(|(introduction_node_id, scid, info, counterparty_forward_node)| { | |
| let introduction_forward_node = { | |
| let htlc_minimum_msat = info.direction().htlc_minimum_msat; | |
| let htlc_maximum_msat = info.direction().htlc_maximum_msat; | |
| let payment_relay: PaymentRelay = info.into(); | |
| let payment_constraints = PaymentConstraints { | |
| max_cltv_expiry: payment_relay.cltv_expiry_delta as u32 | |
| + counterparty_forward_node.tlvs.payment_constraints.max_cltv_expiry, | |
| htlc_minimum_msat, | |
| }; | |
| ForwardNode { | |
| tlvs: ForwardTlvs { | |
| short_channel_id: scid, | |
| payment_relay, | |
| payment_constraints, | |
| features: BlindedHopFeatures::empty(), | |
| }, | |
| node_id: introduction_node_id.as_pubkey().unwrap(), | |
| htlc_maximum_msat, | |
| } | |
| }; | |
| BlindedPath::new_for_payment( | |
| &[introduction_forward_node, counterparty_forward_node], recipient, | |
| tlvs.clone(), u64::MAX, entropy_source, secp_ctx | |
| ) | |
| }) | |
| .map(|forward_node| { | |
| .take(MAX_PAYMENT_PATHS); | |
| let three_hop_paths = counterparty_channels.iter() | |
| // Pair counterparties with their other channels | |
| .flat_map(|(forward_node, counterparty_channels)| | |
| counterparty_channels | |
| .iter() | |
| .filter_map(|scid| network_graph.channels().get_key_value(scid)) | |
| .filter_map(move |(scid, info)| info | |
| .as_directed_to(&NodeId::from_pubkey(&forward_node.node_id)) | |
| .map(|(info, source)| (source, *scid, info)) | |
| ) | |
| .filter(|(source, _, _)| **source != recipient_node_id) | |
| .filter(|(source, _, _)| network_graph | |
| .node(source) | |
| .and_then(|info| info.announcement_info.as_ref()) | |
| .map(|info| info.features.supports_route_blinding()) | |
| .unwrap_or(false) | |
| ) | |
| .filter(|(_, _, info)| amount_msats >= info.direction().htlc_minimum_msat) | |
| .filter(|(_, _, info)| amount_msats <= info.direction().htlc_maximum_msat) | |
| .map(move |(source, scid, info)| (source, scid, info, forward_node.clone())) | |
| ) | |
| // Construct blinded paths where the counterparty's counterparty is the introduction | |
| // node: | |
| // | |
| // source --- info ---> counterparty --- counterparty_forward_node ---> recipient | |
| .map(|(introduction_node_id, scid, info, counterparty_forward_node)| { | |
| let introduction_forward_node = { | |
| let htlc_minimum_msat = info.direction().htlc_minimum_msat; | |
| let htlc_maximum_msat = info.direction().htlc_maximum_msat; | |
| let payment_relay: PaymentRelay = info.into(); | |
| let payment_constraints = PaymentConstraints { | |
| max_cltv_expiry: payment_relay.cltv_expiry_delta as u32 | |
| + counterparty_forward_node.tlvs.payment_constraints.max_cltv_expiry, | |
| htlc_minimum_msat, | |
| }; | |
| ForwardNode { | |
| tlvs: ForwardTlvs { | |
| short_channel_id: scid, | |
| payment_relay, | |
| payment_constraints, | |
| features: BlindedHopFeatures::empty(), | |
| }, | |
| node_id: introduction_node_id.as_pubkey().unwrap(), | |
| htlc_maximum_msat, | |
| } | |
| }; | |
| BlindedPath::new_for_payment( | |
| &[introduction_forward_node, counterparty_forward_node], recipient, | |
| tlvs.clone(), u64::MAX, entropy_source, secp_ctx | |
| ) | |
| }) | |
| .take(MAX_PAYMENT_PATHS); |
| } | ||
|
|
||
| #[cfg(not(feature = "no-std"))] | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
The random_init_seed function is using the default HashMap hasher for randomness. This is noted as a "(bad) RNG" in the comment, which suggests that it may not be suitable for cryptographic purposes. It's important to ensure that this RNG is only used in contexts where cryptographic strength is not required.
| pub fn new( | ||
| network_graph: Arc<NetworkGraph<&'a TestLogger>>, logger: &'a TestLogger, | ||
| scorer: &'a RwLock<TestScorer> | ||
| ) -> Self { | ||
| Self { | ||
| router: DefaultRouter::new(network_graph.clone(), logger, [42u8; 32], scorer, ()), | ||
| network_graph, | ||
| next_routes: Mutex::new(VecDeque::new()), | ||
| scorer, | ||
| } |
There was a problem hiding this comment.
The new method of TestRouter has been updated to initialize the router field with additional parameters, including a hardcoded entropy value [42u8; 32]. This hardcoded value should be replaced with a parameter or a constant defined elsewhere to avoid magic numbers in the code.
- router: DefaultRouter::new(network_graph.clone(), logger, [42u8; 32], scorer, ()),
+ // Assuming `DEFAULT_ENTROPY` is defined as a constant elsewhere
+ router: DefaultRouter::new(network_graph.clone(), logger, DEFAULT_ENTROPY, scorer, ()),Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn new( | |
| network_graph: Arc<NetworkGraph<&'a TestLogger>>, logger: &'a TestLogger, | |
| scorer: &'a RwLock<TestScorer> | |
| ) -> Self { | |
| Self { | |
| router: DefaultRouter::new(network_graph.clone(), logger, [42u8; 32], scorer, ()), | |
| network_graph, | |
| next_routes: Mutex::new(VecDeque::new()), | |
| scorer, | |
| } | |
| pub fn new( | |
| network_graph: Arc<NetworkGraph<&'a TestLogger>>, logger: &'a TestLogger, | |
| scorer: &'a RwLock<TestScorer> | |
| ) -> Self { | |
| Self { | |
| // Assuming `DEFAULT_ENTROPY` is defined as a constant elsewhere | |
| router: DefaultRouter::new(network_graph.clone(), logger, DEFAULT_ENTROPY, scorer, ()), | |
| network_graph, | |
| next_routes: Mutex::new(VecDeque::new()), | |
| scorer, | |
| } |
| const MIN_PEER_CHANNELS: usize = 3; | ||
|
|
||
| let network_graph = self.network_graph.deref().read_only(); | ||
| let paths = peers.iter() | ||
| let peer_channels = peers.iter() | ||
| .map(|pubkey| (pubkey, NodeId::from_pubkey(&pubkey))) | ||
| // Limit to peers with announced channels | ||
| .filter(|pubkey| | ||
| .filter_map(|(pubkey, node_id)| | ||
| network_graph | ||
| .node(&NodeId::from_pubkey(pubkey)) | ||
| .node(&node_id) | ||
| .map(|info| &info.channels[..]) | ||
| .map(|channels| channels.len() >= MIN_PEER_CHANNELS) | ||
| .unwrap_or(false) | ||
| .and_then(|channels| (channels.len() >= MIN_PEER_CHANNELS).then(|| channels)) | ||
| .map(|channels| (pubkey, node_id, channels)) | ||
| ); | ||
|
|
||
| let three_hop_paths = peer_channels.clone() | ||
| // Pair peers with their other peers | ||
| .flat_map(|(pubkey, node_id, channels)| | ||
| channels | ||
| .iter() | ||
| .filter_map(|scid| network_graph.channels().get(scid)) | ||
| .filter_map(move |info| info | ||
| .as_directed_to(&node_id) | ||
| .map(|(_, source)| source) | ||
| ) | ||
| .filter(|source| **source != recipient_node_id) | ||
| .filter(|source| network_graph | ||
| .node(source) | ||
| .and_then(|info| info.announcement_info.as_ref()) | ||
| .map(|info| info.features.supports_onion_messages()) | ||
| .unwrap_or(false) | ||
| ) | ||
| .filter_map(|source| source.as_pubkey().ok()) | ||
| .map(move |source_pubkey| (source_pubkey, *pubkey)) | ||
| ) | ||
| .map(|pubkey| vec![*pubkey, recipient]) | ||
| .map(|(source_pubkey, pubkey)| vec![source_pubkey, pubkey, recipient]) | ||
| .map(|node_pks| BlindedPath::new_for_message(&node_pks, entropy_source, secp_ctx)) | ||
| .take(MAX_PATHS) | ||
| .collect::<Result<Vec<_>, _>>(); | ||
|
|
||
| match paths { | ||
| Ok(paths) if !paths.is_empty() => Ok(paths), | ||
| _ => { | ||
| if network_graph.nodes().contains_key(&NodeId::from_pubkey(&recipient)) { | ||
| BlindedPath::one_hop_for_message(recipient, entropy_source, secp_ctx) | ||
| .map(|path| vec![path]) | ||
| } else { | ||
| Err(()) | ||
| } | ||
| }, | ||
| } | ||
| .take(MAX_PATHS); | ||
|
|
||
| let two_hop_paths = peer_channels | ||
| .map(|(pubkey, _, _)| vec![*pubkey, recipient]) | ||
| .map(|node_pks| BlindedPath::new_for_message(&node_pks, entropy_source, secp_ctx)) | ||
| .take(MAX_PATHS); | ||
|
|
||
| three_hop_paths | ||
| .collect::<Result<Vec<_>, _>>().ok() | ||
| .and_then(|paths| (!paths.is_empty()).then(|| paths)) | ||
| .or_else(|| two_hop_paths.collect::<Result<Vec<_>, _>>().ok()) | ||
| .and_then(|paths| (!paths.is_empty()).then(|| paths)) | ||
| .or_else(|| network_graph | ||
| .node(&NodeId::from_pubkey(&recipient)).ok_or(()) | ||
| .and_then(|_| BlindedPath::one_hop_for_message(recipient, entropy_source, secp_ctx)) | ||
| .map(|path| vec![path]) | ||
| .ok() | ||
| ) | ||
| .ok_or(()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [352-413]
The create_blinded_paths function within the DefaultMessageRouter struct has been modified. The logic appears to be sound, with checks for the number of channels a peer has and whether they support onion messages. However, the complexity of the nested closures and filters is quite high, which could impact maintainability. Consider refactoring for clarity.
| pub(crate) fn peel_onion_message( | ||
| &self, msg: &OnionMessage | ||
| ) -> Result<PeeledOnion<<<CMH>::Target as CustomOnionMessageHandler>::CustomMessage>, ()> { | ||
| peel_onion_message( | ||
| msg, &self.secp_ctx, &*self.node_signer, &*self.logger, &*self.custom_handler | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
The peel_onion_message function has been added to decode one layer of an incoming OnionMessage. The function uses a series of matches and error handling that seems appropriate. However, the error handling could be improved by returning more descriptive errors instead of a generic () to aid in debugging.
The peel_onion_message function uses a lot of nested logic and has multiple points where it returns Err(()). It would be beneficial to define a custom error type that can provide more context about the failure, which would be helpful for debugging and error handling by the callers of this function.
| #[cfg(not(c_bindings))] | ||
| use { | ||
| crate::sign::KeysManager, | ||
| crate::ln::channelmanager::{SimpleArcChannelManager, SimpleRefChannelManager}, |
There was a problem hiding this comment.
The import of IgnoringMessageHandler is not used anywhere in the file. Unused imports can be removed to clean up the code.
There was a problem hiding this comment.
Are you sure? I see it used on lines 1104 and 1123?
There was a problem hiding this comment.
@@ -42,6 +40,7 @@ use crate::prelude::*;
#[cfg(not(c_bindings))]
use {
crate::sign::KeysManager,
+ crate::ln::channelmanager::{SimpleArcChannelManager, SimpleRefChannelManager},@jkczyz, you are correct. The IgnoringMessageHandler is indeed used in the type aliases SimpleArcOnionMessenger and SimpleRefOnionMessenger on lines 1104 and 1123. Thank you for pointing that out. My initial comment was mistaken.
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
ed86950 to
a529010
Compare
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
a529010 to
707cc82
Compare
|
Rebased and added the filtering and sorting of three-hop blinded paths:
|
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
707cc82 to
f05ba31
Compare
| const MIN_PEER_CHANNELS: usize = 3; | ||
|
|
||
| // The minimum channel balance certainty required for using a channel in a blinded path. | ||
| const MIN_CHANNEL_CERTAINTY: f64 = 0.5; |
There was a problem hiding this comment.
Rather than having some arbitrary "certainty" constant in the router itself, should we not just let the scorer figure out what the right certainty threshold (or even certainty concept) is? Basically have it return Option<u64 (or f64)> and just use that to select the best path, don't apply any arbitrary limits here but make sure the docs are clear that the scorer should apply arbitrary limits itself.
There was a problem hiding this comment.
Hmmm... as currently written, the return value is used to compare against MIN_CHANNEL_CERTAINTY. If the scorer applies a limit itself, it really only needs to return a bool. Or are you suggesting we combine the two new methods into one returning Some success probability or None? Note that certainty is only used for filtering -- so it is not used to choose the best path -- while success probability is dependent on the amount and used for both filtering and sorting.
There was a problem hiding this comment.
Right, I'd imagine we probably return either an option to include the probability/hop cost or we use something like u64::MAX/f64::INF as a magic "dont use this channel" value (like we do now with the fee cost).
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
f05ba31 to
b3dc11d
Compare
|
Rebased on #3080 but will need to adjust the strategy now. |
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
2 times, most recently
from
d839dbd to
a0fe784
Compare
An upcoming change to the MessageRouter trait will require reusing DefaultMessageRouter::create_blinded_paths. Move the code to a utility function so facilitate this.
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
a0fe784 to
ecbc0eb
Compare
Using compact blinded paths isn't always necessary or desirable. For instance, reply paths are communicated via onion messages where space isn't a premium unlike in QR codes. Additionally, long-lived paths could become invalid if the channel associated with the SCID is closed. Refactor MessageRouter::create_blinded_paths into two methods: one for compact blinded paths and one for normal blinded paths.
There's no need to save space when creating reply paths since they are part of onion messages rather than in QR codes. Use normal blinded paths for these instead as they are less likely to become invalid in case of channel closure.
When an offer is short-lived, the likelihood of a channel used in a compact blinded path going away is low. Require passing the absolute expiry of an offer to ChannelManager::create_offer_builder so that it can be used to determine whether or not compact blinded path should be used. Use the same criteria for creating blinded paths for refunds as well.
When reconnecting nodes, make sure to notify OnionMessenger that the nodes are now connected.
When calling MessageRouter::create_blinded_path, ChannelManager was including disconnected peers. Filter peers such that only connected ones are included. Otherwise, the resulting BlindedPath may not work.
The docs assumed ChannelManager is parameterized by DefaultRouter, which may not be the case. Clarify the behavior is specific to using DefaultRouter.
How certain a scorer is about a channel's liquidity balance is useful in determining if the channel should be included in a blinded payment path. Channels with uncertain balances should be avoided to facilitate successful payments. Expand ScoreLookUp with a channel_balance_certainty method and use it in DefaultRouter::create_blinded_payment_paths.
How certain a scorer is about a channel's success probability is useful in determining if the channel should be included in a blinded payment path. Channels with low success probability for a given amount should be avoided to facilitate successful payments. Expand ScoreLookUp with a channel_success_probability method and use it in DefaultRouter::create_blinded_payment_paths.
jkczyz
force-pushed
the
2023-12-three-hop-paths
branch
from
ecbc0eb to
ee88951
Compare
4 participants
Work in progress split out from #2781.
Summary by CodeRabbit
New Features
Enhancements
Refactor
Bug Fixes
Documentation