Misc Tweaks for bindings #2847
Misc Tweaks for bindings #2847
Conversation
|
Warning Rate Limit Exceeded@TheBlueMatt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 28 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates across several Rust files in a Lightning Network-related project suggest a refactoring that involves integrating a 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: 4
Configuration used: CodeRabbit UI
Files selected for processing (9)
- lightning-background-processor/src/lib.rs (2 hunks)
- lightning-rapid-gossip-sync/src/lib.rs (3 hunks)
- lightning-rapid-gossip-sync/src/processing.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/onion_message/functional_tests.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (6 hunks)
- lightning/src/routing/router.rs (14 hunks)
- lightning/src/util/logger.rs (4 hunks)
- lightning/src/util/test_utils.rs (6 hunks)
Files skipped from review due to trivial changes (2)
- lightning-rapid-gossip-sync/src/processing.rs
- lightning/src/onion_message/functional_tests.rs
Additional comments: 30
lightning/src/onion_message/messenger.rs (6)
- 100-100: The addition of
entropy_sourcetoOnionMessengeris consistent with the PR objectives to streamline the API and remove theEntropySourcetrait bound.- 296-302: The addition of
entropy_sourcetoDefaultMessageRouteris consistent with the PR objectives to streamline the API and remove theEntropySourcetrait bound.- 289-291: The removal of the
EntropySourcetrait bound fromcreate_blinded_pathsin theMessageRoutertrait is consistent with the PR objectives to streamline the API.- 369-383: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [316-380]
The use of
entropy_sourcein theMessageRouterimplementation forDefaultMessageRouteris consistent with the PR objectives to streamline the API and remove theEntropySourcetrait bound.
- 1064-1064: The update to
SimpleArcOnionMessengerandSimpleRefOnionMessengerto include theKeysManagertype is consistent with the PR objectives to streamline the API and remove theEntropySourcetrait bound.- 286-322: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [100-1083]
No further issues detected in the rest of the file. The changes are consistent with the PR objectives and AI-generated summary.
lightning/src/util/test_utils.rs (6)
- 69-69: The import of
core::ops::Derefhas been added. This is likely in preparation for use within the file, but without a corresponding use ofDerefin the diff, it's unclear why this was added.- 154-154: The last argument of
scorer.channel_penalty_msathas been changed from&()to&Default::default(). This change is more idiomatic Rust, as it uses theDefaulttrait to provide a default value for the last argument, which is more explicit and clear.- 166-166: The same change as above has been applied here, replacing
&()with&Default::default().- 181-181: Again, the same change as above has been applied here.
- 198-201: The signatures for
create_blinded_payment_pathsandcreate_blinded_pathshave been simplified by removing theES: EntropySource + ?Sizedparameter and its associated references. This change likely reflects an architectural decision to remove the need for an entropy source in these functions, which could simplify the API and its usage.- 215-217: The same simplification has been applied to the
create_blinded_pathsfunction as well.lightning-background-processor/src/lib.rs (3)
- 1264-1275: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [987-1270]
The addition of
Arc<KeysManager>as a parameter toDefaultRouteris a significant change. Ensure that all instances whereDefaultRouteris instantiated have been updated to pass this new parameter.Verification successful
The verification process has confirmed that there is only one instance of
DefaultRouter::newbeing instantiated, and it is indeed using five parameters, which aligns with the previous pattern search. This suggests that the instantiation ofDefaultRouterhas been updated to include the newArc<KeysManager>parameter as required.* 1267-1270: The introduction of the `now` variable to calculate the duration from the genesis block's time is a logical change that should not affect the correctness of the code, assuming that the `genesis_block`'s time is correctly defined elsewhere in the codebase. * 1269-1270: The creation of `keys_manager` before initializing the `router` is a logical change that ensures the `keys_manager` is available for the `router`'s construction. This change is correct as long as the `KeysManager::new` method does not depend on any state that would be modified by the `router`'s construction or usage.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances of DefaultRouter being instantiated without the new Arc<KeysManager> parameter. ast-grep --lang rust --pattern $'DefaultRouter::new($_, $_, $_, $_, $_)'Length of output: 270
Script:
#!/bin/bash # Search for all instances of DefaultRouter being instantiated to check if there are any other calls with a different number of parameters. ast-grep --lang rust --pattern $'DefaultRouter::new($$$)'Length of output: 255
lightning/src/routing/router.rs (11)
- 55-57: The
newmethod ofDefaultRouternow accepts anentropy_sourceparameter. Confirm that this change is reflected wherevernewis called.Verification successful
The verification process has confirmed that the
newmethod ofDefaultRouteris called with the updated parameters, including theentropy_source, in the single instance found in the codebase. No other calls to this method were found, indicating that the change has been correctly implemented where required.* 73-73: The `find_route` method now uses `self.entropy_source.get_secure_random_bytes()` instead of `random_seed_bytes`. Verify that `get_secure_random_bytes` is properly implemented and that the `EntropySource` trait is correctly integrated. * 83-86: The `create_blinded_payment_paths` method has been updated to accept `secp_ctx` directly. Ensure that all calls to this method have been updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to the new method of DefaultRouter and verify the updated parameters. ast-grep --lang rust --pattern $'DefaultRouter::new($_, $_, $_, $_, $_)'Length of output: 270
Script:
#!/bin/bash # Search for all calls to the new method of DefaultRouter throughout the entire codebase. rg "DefaultRouter::new" --multilineLength of output: 225
Verification successful
The
rgcommand has successfully identified several occurrences ofcreate_blinded_payment_pathsin the codebase. The relevant files and contexts where the method is mentioned or defined are listed in the output. To verify the review comment, we need to check the method calls inchannelmanager.rsto see if they have been updated to include thesecp_ctxparameter as per the updated method signature.Given the output, we can now focus on the calls in
channelmanager.rsto see if they match the updated signature with thesecp_ctxparameter.
The output from the
rgcommand shows thatcreate_blinded_payment_pathsis indeed called inchannelmanager.rs. The calls include thesecp_ctxparameter, which matches the updated method signature as per the review comment. Specifically, the callself.router.create_blinded_payment_paths(payee_node_id, first_hops, payee_tlvs, amount_msats, secp_ctx)confirms that thesecp_ctxparameter is being passed as expected.Based on this evidence, the review comment is verified as the calls to
create_blinded_payment_pathshave been updated to include thesecp_ctxparameter.* 137-137: Direct use of `entropy_source` in `BlindedPath::new_for_payment` is noted. Confirm that the `BlindedPath` struct and associated methods are updated to handle the `entropy_source` parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to create_blinded_payment_paths and verify the updated parameters. ast-grep --lang rust --pattern $'create_blinded_payment_paths($_, $_, $_, $_, $_)'Length of output: 83
Script:
#!/bin/bash # Search for calls to create_blinded_payment_paths with six parameters to verify the updated method signature. ast-grep --lang rust --pattern $'create_blinded_payment_paths($_, $_, $_, $_, $_, $_)'Length of output: 87
Script:
#!/bin/bash # Search for any calls to create_blinded_payment_paths regardless of the number of parameters. ast-grep --lang rust --pattern $'create_blinded_payment_paths($$$)'Length of output: 68
Script:
#!/bin/bash # Use ripgrep to search for any text occurrences of create_blinded_payment_paths in the codebase. rg 'create_blinded_payment_paths'Length of output: 808
Script:
#!/bin/bash # Extract the lines from channelmanager.rs where create_blinded_payment_paths is called to verify the parameters. rg 'create_blinded_payment_paths' lightning/src/ln/channelmanager.rs -A 3Length of output: 753
Verification successful
The output from the script provides the full method signature for
new_for_paymentin theBlindedPathimplementation, which includes theentropy_sourceparameter. The method body shows thatentropy_sourceis used to get secure random bytes for generating ablinding_secret. This confirms that theBlindedPathstruct and itsnew_for_paymentmethod have been updated to handle theentropy_sourceparameter as noted in the review comment.* 147-147: The `one_hop_for_payment` method now uses `entropy_source` directly. Verify that this method and any related calls are correctly updated to accommodate this change.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the BlindedPath struct and its new_for_payment method to verify the handling of entropy_source. rg --type rust 'impl BlindedPath'Length of output: 88
Script:
#!/bin/bash # Search for the new_for_payment method within the BlindedPath implementation to verify the handling of entropy_source. rg --type rust 'fn new_for_payment' lightning/src/blinded_path/mod.rsLength of output: 173
Script:
#!/bin/bash # Retrieve the full method signature and the relevant parts of the method body for new_for_payment in BlindedPath. rg --type rust -A 10 'fn new_for_payment' lightning/src/blinded_path/mod.rsLength of output: 804
Verification successful
The output from the shell script provides the implementation details of the
one_hop_for_paymentmethod within theBlindedPathstruct. According to the output, the method signature is as follows:pub fn one_hop_for_payment<ES: EntropySource + ?Sized, T: secp256k1::Signing + secp256k1::Verification>( payee_node_id: PublicKey, payee_tlvs: payment::ReceiveTlvs, entropy_source: &ES, secp_ctx: &Secp256k1<T> ) -> Result<(BlindedPayInfo, Self), ()> { // method implementation }This confirms that the
one_hop_for_paymentmethod indeed usesentropy_sourcedirectly as a parameter, which aligns with the review comment. The method takes four parameters:payee_node_id,payee_tlvs,entropy_source, andsecp_ctx. Theentropy_sourceis passed by reference, which is consistent with the usage shown in the snippet from the review comment.Based on this information, the review comment is verified to be accurate, and the
one_hop_for_paymentmethod and any related calls have been correctly updated to accommodate the change in usingentropy_sourcedirectly.* 169-173: The `create_blinded_paths` method has been updated to accept `secp_ctx` directly. Confirm that all calls to this method have been updated to pass `secp_ctx` as a parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to one_hop_for_payment and verify the updated parameters. ast-grep --lang rust --pattern $'one_hop_for_payment($_, $_, $_, $_)'Length of output: 70
Script:
#!/bin/bash # Search for the method signature and calls to one_hop_for_payment without specifying the number of parameters. ast-grep --lang rust --pattern $'one_hop_for_payment($$$)'Length of output: 59
Script:
#!/bin/bash # Search for any method calls within the BlindedPath struct to find the usage of one_hop_for_payment. ast-grep --lang rust --pattern $'impl BlindedPath { $$$ }'Length of output: 6056
Verification successful
The search using
rghas identified two calls to thecreate_blinded_pathsmethod within the codebase, both of which include thesecp_ctxparameter as part of the method call. This confirms that the calls to this method have been updated to passsecp_ctxas a parameter, in line with the changes mentioned in the review comment.* 1144-1149: Comments have been added to clarify that certain fields are not exported to bindings users. Ensure that the bindings generation process respects these comments and does not attempt to export these fields. * 1158-1159: Similar to the previous comment, verify that the bindings generation process does not export the `info` field of `PublicHopCandidate`.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to create_blinded_paths and verify the updated parameters. ast-grep --lang rust --pattern $'create_blinded_paths($_, $_, $_)'Length of output: 67
Script:
#!/bin/bash # Search for calls to create_blinded_paths with the updated number of parameters, which should now include secp_ctx. ast-grep --lang rust --pattern $'create_blinded_paths($_, $_, $_, $_)'Length of output: 71
Script:
#!/bin/bash # Search for any calls to create_blinded_paths regardless of the number of parameters. ast-grep --lang rust --pattern $'create_blinded_paths($$$)'Length of output: 60
Script:
#!/bin/bash # Use ripgrep to search for any calls to create_blinded_paths in the codebase. rg 'create_blinded_paths\('Length of output: 220
Verification successful
The output from the
rgcommand with context shows a comment directly above theinfofield within thePublicHopCandidatestruct definition stating:/// This is not exported to bindings users as lifetimes are not expressable in most languages.This comment confirms that the
infofield of thePublicHopCandidatestruct is indeed not exported to bindings users due to the limitations of expressing lifetimes in most languages. Therefore, the review comment is consistent with the codebase.* 1170-1175: Ensure that the `hint` and `target_node_id` fields of `PrivateHopCandidate` are not exported in the bindings, as indicated by the comments. * 1184-1185: Confirm that the `hint` field of `BlindedPathCandidate` is not exported in the bindings, in line with the added comments. * 1201-1202: As with the previous comments, verify that the `hint` field of the struct here is not exported in the bindings.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the bindings generation script or configuration to verify that it does not export the `info` field. rg --type rust 'bindings'Length of output: 20962
Script:
#!/bin/bash # Search for the `PublicHopCandidate` struct definition and check for the `info` field within the context of bindings. ast-grep --lang rust --pattern 'struct PublicHopCandidate { $$$ info: $_, $$$ }'Length of output: 82
Script:
#!/bin/bash # Search for the `PublicHopCandidate` struct definition in the codebase to find the `info` field. rg --type rust 'struct PublicHopCandidate'Length of output: 112
Script:
#!/bin/bash # Display the contents of the file where `PublicHopCandidate` struct is defined to check for the `info` field. cat lightning/src/routing/router.rs | rg 'PublicHopCandidate|info'Length of output: 6192
Script:
#!/bin/bash # Search for conditional compilation attributes related to bindings near the `PublicHopCandidate` struct definition. rg --type rust --context 5 'pub struct PublicHopCandidate' lightning/src/routing/router.rsLength of output: 482
Verification successful
The search results confirm that the
hintfield is preceded by a comment stating that it is not exported to bindings users because lifetimes are not expressible in most languages. This comment serves as an indicator that thehintfield is indeed not exported in the bindings, aligning with the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the bindings generation script or configuration to verify that it does not export the `hint` field. rg --type rust 'bindings'Length of output: 20962
Script:
#!/bin/bash # Search for the declaration of the `hint` field in the `router.rs` file and check for the `#[cfg(not(c_bindings))]` attribute. rg --type rust 'hint' lightning/src/routing/router.rsLength of output: 13490
Script:
#!/bin/bash # Refine the search to look for the `hint` field declaration along with the `#[cfg(not(c_bindings))]` attribute. rg --type rust 'pub hint: &' lightning/src/routing/router.rs -A 1 -B 1Length of output: 663
lightning/src/ln/channelmanager.rs (4)
- 967-967: The addition of
Arc<KeysManager>to theDefaultRouterdeclaration aligns with the PR objectives and summary.- 998-998: The addition of
&'c KeysManagerto theDefaultRouterdeclaration is consistent with the changes made in theArcversion and the PR objectives.- 7947-7947: The call to
create_blinded_pathsno longer includes anentropy_sourceparameter, which matches the PR objectives and summary.- 7970-7970: The call to
create_blinded_payment_pathshas been updated to remove theentropy_sourceparameter, consistent with the PR objectives and summary.
| ($($args: lifetime)?, $($nonstruct_args: lifetime)?) => { | ||
| /// A Record, unit of logging output with Metadata to enable filtering | ||
| /// Module_path, file, line to inform on log's source | ||
| #[derive(Clone, Debug)] | ||
| pub struct Record<'a> { | ||
| pub struct Record<$($args)?> { |
There was a problem hiding this comment.
The use of generic lifetime parameters in the Record struct definition is unconventional and may lead to confusion. It's not clear what the purpose of the generic parameters is, as they are not used within the struct fields or methods. This could be a mistake or an incomplete refactoring.
- pub struct Record<$($args)?> {
+ pub struct Record<'a> {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.
| ($($args: lifetime)?, $($nonstruct_args: lifetime)?) => { | |
| /// A Record, unit of logging output with Metadata to enable filtering | |
| /// Module_path, file, line to inform on log's source | |
| #[derive(Clone, Debug)] | |
| pub struct Record<'a> { | |
| pub struct Record<$($args)?> { | |
| ($($args: lifetime)?, $($nonstruct_args: lifetime)?) => { | |
| /// A Record, unit of logging output with Metadata to enable filtering | |
| /// Module_path, file, line to inform on log's source | |
| #[derive(Clone, Debug)] | |
| pub struct Record<'a> { |
| pub struct DefaultRouter<G: Deref<Target = NetworkGraph<L>> + Clone, L: Deref, ES: Deref, S: Deref, SP: Sized, Sc: ScoreLookUp<ScoreParams = SP>> where | ||
| L::Target: Logger, | ||
| S::Target: for <'a> LockableScore<'a, ScoreLookUp = Sc>, | ||
| ES::Target: EntropySource, | ||
| { | ||
| network_graph: G, | ||
| logger: L, | ||
| random_seed_bytes: Mutex<[u8; 32]>, | ||
| entropy_source: ES, | ||
| scorer: S, | ||
| score_params: SP, | ||
| message_router: DefaultMessageRouter<G, L>, | ||
| message_router: DefaultMessageRouter<G, L, ES>, |
There was a problem hiding this comment.
Tip
Codebase Verification
The instantiation of DefaultRouter in lightning-background-processor/src/lib.rs does not include the new entropy_source parameter. This needs to be addressed to align with the updated DefaultRouter struct.
Analysis chain
The entropy_source field has been added to the DefaultRouter struct. Ensure that all instances where DefaultRouter is instantiated have been updated to pass the entropy_source parameter.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for instantiations of DefaultRouter and verify the presence of the entropy_source parameter.
ast-grep --lang rust --pattern $'DefaultRouter::new($_, $_, $_, $_, $_)'
Length of output: 270
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2847 +/- ##
==========================================
+ Coverage 89.18% 90.04% +0.86%
==========================================
Files 116 115 -1
Lines 93098 98867 +5769
Branches 93098 98867 +5769
==========================================
+ Hits 83032 89029 +5997
+ Misses 7533 7343 -190
+ Partials 2533 2495 -38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
What's the motivation for this, i.e., why is it an issue for the bindings if the error has its own module?
There was a problem hiding this comment.
The bindings dont like re-exports very much. I could maybe fix that, but its a chunk of additional work and easier to patch it out. The particular change here I think stands on its own, however - having a separate file adds additional effort for developers to go have to look in a separate place to build context before they can read some code. In cases where a file is getting huge and you can't just look at the file on its own it doesnt matter (and there's other reasons to keep files small), but if a file is already small...
There was a problem hiding this comment.
The bindings dont like re-exports very much. I could maybe fix that, but its a chunk of additional work and easier to patch it out. The particular change here I think stands on its own, however - having a separate file adds additional effort for developers to go have to look in a separate place to build context before they can read some code. In cases where a file is getting huge and you can't just look at the file on its own it doesnt matter (and there's other reasons to keep files small), but if a file is already small...
Hmmm, I'd hold against this that we're trying to further modularize LDK's codebase as the files really get unwieldy over time and that breaking them up after the fact often only happens at a point where it requires significant refactoring work to do so. Consolidating files therefore seems generally counterintuitive to me, especially if there is seemingly no requirement to do so? Not sure if something changed, but the error type wasn't moved recently to a dedicated file?
There was a problem hiding this comment.
Hmmm, I'd hold against this that we're trying to further modularize LDK's codebase as the files really get unwieldy over time and that breaking them up after the fact often only happens at a point where it requires significant refactoring work to do so. Consolidating files therefore seems generally counterintuitive to me, especially if there is seemingly no requirement to do so?
Sure, we absolutely want to modularize, but there's a middle ground to be had here - we shouldn't have 10k LoC files, but also shouldn't have 30 LoC files. Both make working in the project a pain (the first due to compile times and lack of modularization, the second because you have to open multiple files just to get any context of what is going on) and we should strive to ensure things remain in the middle ground, not to small nor too large.
Not sure if something changed, but the error type wasn't moved recently to a dedicated file?
Doesn't look like it, it was added in a58ae4c when the crate was created.
There was a problem hiding this comment.
Sure, we absolutely want to modularize, but there's a middle ground to be had here - we shouldn't have 10k LoC files, but also shouldn't have 30 LoC files.
have to open multiple files just to get any context of what is going on
I think most/all editors/IDEs provide 'jump-to-definition', allowing to easily switch files if necessary. Sure, it could become annoying if we would go overboard with a thousand tiny files that are arbitrarily splitted, but I'm not sure why having some 30-50 LoC files is a huge deal if they a clearly separated by concern.
Anyways, as said below it's not blocking to this PR for me, so feel free to go ahead as is.
TheBlueMatt
force-pushed
the
2024-01-bindings-upstream
branch
from
0253d69
to
2d1af50
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (12)
- fuzz/src/chanmon_consistency.rs (2 hunks)
- fuzz/src/full_stack.rs (2 hunks)
- fuzz/src/onion_message.rs (1 hunks)
- lightning-background-processor/src/lib.rs (2 hunks)
- lightning-rapid-gossip-sync/src/lib.rs (3 hunks)
- lightning-rapid-gossip-sync/src/processing.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/onion_message/functional_tests.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (6 hunks)
- lightning/src/routing/router.rs (14 hunks)
- lightning/src/util/logger.rs (4 hunks)
- lightning/src/util/test_utils.rs (6 hunks)
Files skipped from review as they are similar to previous changes (7)
- lightning-background-processor/src/lib.rs
- lightning-rapid-gossip-sync/src/processing.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/onion_message/functional_tests.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/router.rs
- lightning/src/util/test_utils.rs
Additional comments: 7
lightning-rapid-gossip-sync/src/lib.rs (2)
- 84-93: The consolidation of the
GraphSyncErrorexport and the removal of themod error;declaration are correct and simplify the code by removing unnecessary indirection.- 293-293: The simplification of the error handling in the conditional statement is correct and improves readability by using the directly imported
GraphSyncErrorinstead of the full path.fuzz/src/onion_message.rs (1)
- 89-90: The change in the
create_blinded_pathsmethod signature to remove theEntropySourceparameter is correct. The method now only includes theSecp256k1parameter, which simplifies the function signature and potentially its internal logic ifEntropySourcewas not required.fuzz/src/chanmon_consistency.rs (2)
- 106-108: The signature of
create_blinded_payment_pathshas been modified to remove theEntropySourcetrait bound and its associated parameter_entropy_source. This change aligns with the PR's objective to streamline the API.- 121-122: Similarly, the
create_blinded_pathsfunction has also had theEntropySourcetrait bound and its associated parameter_secp_ctxremoved from its signature. This is consistent with the changes made tocreate_blinded_payment_pathsand the PR's overall goal.fuzz/src/full_stack.rs (2)
- 149-151: The generic parameter
ES: EntropySourceand its associated lifetime bound have been removed from thecreate_blinded_payment_pathsfunction signature, and the parameter_entropy_source: &EShas been removed. This change aligns with the PR's objective to removeEntropySourcetrait bounds.- 164-165: The generic parameter
ES: EntropySourceand its associated lifetime bound have been removed from thecreate_blinded_pathsfunction signature, and the parameter_entropy_source: &EShas been replaced with_secp_ctx: &Secp256k1<T>. This change is consistent with the PR's objective to refactor the method signatures to exclude theEntropySourcetrait bound and instead useSecp256k1.
Because log `Record`s are now being passed by ownership to `log`, the bindings get quite annoyed that there's a lifetime hanging around. We already don't use this lifetime in bindings (the `FormatArgs` is converted to a string and stored on the heap), so we can just drop the lifetime, even though it requires some macro'ing of the struct definition to do so.
There's not a lot of reason for downstream users to use the `WithContext` wrapper, it mostly exists for our own downstream crates anyway, and dealing with lifetimes in bindings isn't super practical, so simply no-export it.
The bindings cannot express lifetimes, so exposing any field which is a reference (and not `clone`-able, at least for garbage collected language bindings) is unsafe for those expecting a high-level interface. Thus, we simply no-export the `RouteHopCandidate` inner struct fields which are references (there are relevant accessors for them anyway).
TheBlueMatt
force-pushed
the
2024-01-bindings-upstream
branch
from
2d1af50
to
7c75ca4
Compare
|
Fixed new warnings and rebased. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (14)
- fuzz/src/chanmon_consistency.rs (2 hunks)
- fuzz/src/full_stack.rs (2 hunks)
- fuzz/src/onion_message.rs (1 hunks)
- lightning-background-processor/src/lib.rs (2 hunks)
- lightning-rapid-gossip-sync/src/lib.rs (3 hunks)
- lightning-rapid-gossip-sync/src/processing.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (6 hunks)
- lightning/src/ln/functional_test_utils.rs (1 hunks)
- lightning/src/ln/functional_tests.rs (1 hunks)
- lightning/src/onion_message/functional_tests.rs (2 hunks)
- lightning/src/onion_message/messenger.rs (6 hunks)
- lightning/src/routing/router.rs (14 hunks)
- lightning/src/util/logger.rs (4 hunks)
- lightning/src/util/test_utils.rs (10 hunks)
Files skipped from review as they are similar to previous changes (10)
- fuzz/src/chanmon_consistency.rs
- fuzz/src/full_stack.rs
- fuzz/src/onion_message.rs
- lightning-background-processor/src/lib.rs
- lightning-rapid-gossip-sync/src/lib.rs
- lightning-rapid-gossip-sync/src/processing.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/onion_message/functional_tests.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/routing/router.rs
Additional comments: 9
lightning/src/util/test_utils.rs (5)
- 73-73: The import of
RandomBytesis added, which is used later in theTestRouterstruct.- 110-115: The
TestRouterstruct now includes anArc<RandomBytes>field. This change aligns with the removal of theEntropySourcetrait bound from various functions, as mentioned in the summary.- 124-128: The
newfunction ofTestRouternow initializes theentropy_sourcewithRandomBytes::new([42; 32]). This hardcoded byte array should be reviewed to ensure it's appropriate for the test context.Verification successful
The hardcoded byte array
[42; 32]used for initializingRandomBytesin theTestRouteris consistent with common testing practices where deterministic behavior is desired. There are no indications in the code comments or documentation that this choice of seed is inappropriate for the test context. Therefore, the review comment is verified as consistent with the codebase.* 211-217: The `create_blinded_payment_paths` function signature has been simplified by removing the `EntropySource` trait bound. This change is consistent with the summary. * 230-234: The `create_blinded_paths` function signature has been simplified by removing the `EntropySource` trait bound. This change is consistent with the summary.Scripts Executed
The following scripts were executed for the analysis:
Script:
# Verify that the hardcoded byte array [42; 32] is a suitable choice for the entropy source in tests. # This script is a placeholder for a manual check. echo "Review the usage of hardcoded byte array [42; 32] for RandomBytes initialization in tests."Length of output: 190
Script:
#!/bin/bash # Search for the usage of RandomBytes::new with the hardcoded array [42; 32] rg "RandomBytes::new\(\[\s*42;\s*32\s*\]\)" --context 5 # Additionally, search for any comments or documentation related to RandomBytes rg "RandomBytes" -C 5Length of output: 11574
lightning/src/ln/functional_test_utils.rs (1)
- 2935-2935: The
message_routerfield in theNodeCfgstruct is being initialized with aTestMessageRouterinstance. This is expected and correct as it sets up the message routing for testing purposes.lightning/src/ln/functional_tests.rs (3)
- 5537-5537: The addition of
&keys_manageras a parameter totest_utils::TestMessageRouter::newis consistent with the PR's objective to integrateKeysManagerinto various parts of the codebase.- 5537-5537: The
NodeCfgstruct initialization now includeskeys_manager: &keys_manager, which aligns with the changes made tomessage_router. Ensure that all instances whereNodeCfgis instantiated are updated accordingly.Verification successful
The search for
NodeCfginstantiation in the Rust codebase has returned a single instance where thekeys_manageris indeed included in the initialization. This matches the expectation set by the review comment thatNodeCfgshould now includekeys_manager. However, the output only shows one instance, and without further context, it's not clear if this is the only place whereNodeCfgis instantiated or if there are other instances that were not captured by the regex pattern.To ensure thorough verification, it would be prudent to run a more comprehensive search that captures all possible instantiations of
NodeCfg, including those that might span multiple lines.
The extended search for
NodeCfginstantiation across multiple lines has provided two instances in the codebase whereNodeCfgis instantiated with thekeys_managerfield included. This confirms that in both instances found, thekeys_managerfield is present as part of theNodeCfgstruct initialization, which is consistent with the changes mentioned in the review comment.Based on the information provided by the scripts, it appears that the review comment is accurate, and the necessary updates to
NodeCfginstantiation have been made in the codebase.* 5537-5537: The removal of the first element from `node_cfgs` is noted in the summary but is not relevant to the exported or public entities. This change seems to be part of the test setup and does not require action unless it impacts the test logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for NodeCfg instantiation to ensure keys_manager is included everywhere. rg --type rust 'NodeCfg\s*{[^}]*}'Length of output: 147
Script:
#!/bin/bash # Search for NodeCfg instantiation to ensure keys_manager is included everywhere. # Escaping curly braces to correct the regex pattern rg --type rust 'NodeCfg\s*\{[^}]*\}'Length of output: 449
Script:
#!/bin/bash # Search for NodeCfg instantiation across multiple lines to ensure keys_manager is included everywhere. rg --type rust -U 'NodeCfg\s*\{[^}]*\}'Length of output: 1595
| macro_rules! impl_record { | ||
| ($($args: lifetime)?, $($nonstruct_args: lifetime)?) => { | ||
| /// A Record, unit of logging output with Metadata to enable filtering | ||
| /// Module_path, file, line to inform on log's source | ||
| #[derive(Clone, Debug)] | ||
| pub struct Record<'a> { | ||
| pub struct Record<$($args)?> { |
There was a problem hiding this comment.
The use of macro syntax for generic parameters in the Record struct is incorrect. Rust uses angle brackets for generics, and lifetime parameters are prefixed with a single quote.
- pub struct Record<$($args)?> {
+ pub struct Record<'a> {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.
| macro_rules! impl_record { | |
| ($($args: lifetime)?, $($nonstruct_args: lifetime)?) => { | |
| /// A Record, unit of logging output with Metadata to enable filtering | |
| /// Module_path, file, line to inform on log's source | |
| #[derive(Clone, Debug)] | |
| pub struct Record<'a> { | |
| pub struct Record<$($args)?> { | |
| macro_rules! impl_record { | |
| ($($args: lifetime)?, $($nonstruct_args: lifetime)?) => { | |
| /// A Record, unit of logging output with Metadata to enable filtering | |
| /// Module_path, file, line to inform on log's source | |
| #[derive(Clone, Debug)] | |
| pub struct Record<'a> { |
| impl<$($args)?> Record<$($args)?> { | ||
| /// Returns a new Record. | ||
| /// | ||
| /// This is not exported to bindings users as fmt can't be used in C | ||
| #[inline] | ||
| pub fn new( | ||
| pub fn new<$($nonstruct_args)?>( | ||
| level: Level, peer_id: Option<PublicKey>, channel_id: Option<ChannelId>, | ||
| args: fmt::Arguments<'a>, module_path: &'static str, file: &'static str, line: u32 | ||
| ) -> Record<'a> { | ||
| ) -> Record<$($args)?> { |
There was a problem hiding this comment.
The syntax for generic parameters in the impl block and new function for Record is incorrect. Rust syntax should be used for generic lifetime parameters.
- impl<$($args)?> Record<$($args)?> {
+ impl<'a> Record<'a> {- pub fn new<$($nonstruct_args)?>(
+ pub fn new<'a>(| impl_record!('a, ); | ||
| #[cfg(c_bindings)] | ||
| impl_record!(, 'a); |
There was a problem hiding this comment.
The invocations of the impl_record! macro are incorrect. The macro is expecting lifetime parameters, but the syntax provided does not match any known Rust syntax.
- impl_record!('a, );
- impl_record!(, 'a);
+ impl_record!('a);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.
| impl_record!('a, ); | |
| #[cfg(c_bindings)] | |
| impl_record!(, 'a); | |
| impl_record!('a); | |
| #[cfg(c_bindings)] | |
| impl_record!('a); |
|
Looks like the new tests in #2697 broke by changing the rng (with |
Hmmm... yeah, nothing about the change looks wrong. @valentinewallace Would you mind taking a look? |
|
This fixes the test failure: diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs
index 3232cd0d3..67bf5316a 100644
--- a/lightning/src/ln/blinded_payment_tests.rs
+++ b/lightning/src/ln/blinded_payment_tests.rs
@@ -508,6 +508,7 @@ fn three_hop_blinded_path_success() {
create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0);
let chan_upd_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0).0.contents;
let chan_upd_3_4 = create_announced_chan_between_nodes_with_value(&nodes, 3, 4, 1_000_000, 0).0.contents;
+ connect_blocks(&nodes[0], nodes[4].best_block_info().1 - nodes[0].best_block_info().1);
let amt_msat = 5000;
let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[4], Some(amt_msat), None);A big part of the problem was that nodes[0]'s best block height was 10 and nodes[4]'s best block height was 40. However, I think we may be setting the final hop's cltv too low, in general. I'm looking into that but I think it can be done separately. |
Hmm, wonder if we don't need to be getting all block heights in sync in the create channel methods :/. I'll take the minimal fix here but we should consider this more broadly later. |
In `three_hop_blinded_path_success`, the nodes in the test ended up at radically different block heights after channel opening. At that point, if the CLTV randomization is done slightly different the test payment may fail, which we fix here by ensuring all nodes are at the same height before we go to send a payment.
...as an arg to `Router`. Passing an `EntropySource` around all the time is a bit strange as the `Router` may or may not actually use it, and the `DefaultRouter` can just as easily store it.
The top-level module is only a few hundred lines, so there's not a lot of reason to hide the `GraphSyncError` in its own module. Instead, we simply move it to the top-level `lib.rs`, which doesn't change the public API as it was previously re-exported at the top level.
As it does the same thing as a derived `Debug` does anyway.
TheBlueMatt
force-pushed
the
2024-01-bindings-upstream
branch
from
7c75ca4
to
fb693ec
Compare
|
1.63 build is sad. |
Should be fixed by now, so should pass if CI is kicked. |
The usual various misc tweaks to the API and some no-exports which can go upstream to keep the bindings branch smaller.