rustfmt: Add CI scripts and format onion_utils.rs
#2877
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe project has introduced a new job within its CI workflow specifically for Rust code formatting, leveraging Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
rustfmt.tomlis excluded by:!**/*.toml
Files selected for processing (4)
- .github/workflows/build.yml (1 hunks)
- ci/rustfmt.sh (1 hunks)
- lightning/src/ln/onion_utils.rs (34 hunks)
- rustfmt_excluded_files (1 hunks)
Files not summarized due to errors (1)
- lightning/src/ln/onion_utils.rs: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- rustfmt_excluded_files
Additional comments: 18
.github/workflows/build.yml (1)
- 220-235: The
rustfmtjob is correctly set up to check Rust code formatting. However, ensure that theci/rustfmt.shscript is robust against all possible edge cases, such as handling filenames with spaces or special characters, as mentioned in the script review.lightning/src/ln/onion_utils.rs (17)
- 10-21: Imports are correctly organized and relevant to the file's functionality.
- 34-34: The
OnionKeysstruct is well-defined, ensuring that test-related fields are only compiled in test configurations.- 59-70: The
gen_rho_mu_from_shared_secretfunction correctly generates rho and mu values using HMAC-SHA256, adhering to cryptographic standards for onion routing.- 100-100: The
next_hop_pubkeyfunction correctly calculates the next hop's public key using ECDH, which is essential for onion routing in the Lightning Network.- 149-212: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-250]
The
build_onion_payloadsfunction correctly constructs onion payloads for each hop. Ensure error handling for channel fees and CLTV overflow is tested thoroughly to prevent potential issues during route construction.// Suggest adding tests to verify error handling for channel fees and CLTV overflow.
- 267-267: The constant
ONION_DATA_LENis correctly defined, aligning with the specifications for onion packet data length.- 283-295: The
construct_onion_packetfunction correctly initializes the onion packet with ChaCha20 encryption. Ensure the encryption logic is thoroughly tested, especially the initialization withprng_seedand the zero nonce.// Suggest adding tests to verify the encryption logic, especially the initialization with `prng_seed` and the zero nonce.
- 341-342: The
construct_onion_message_packetfunction's use of dynamic packet data length for different onion message types is a good practice for flexibility. Ensure that the dynamic sizing does not introduce any buffer overflow vulnerabilities.// Suggest adding tests or static analysis to ensure no buffer overflow vulnerabilities are introduced by dynamic packet data length handling.
- 360-367: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [352-378]
The
construct_onion_packet_with_init_noisefunction correctly handles the encryption and HMAC calculation for onion packets. Pay attention to the filler generation and ensure it complies with the specifications for preventing size analysis attacks.// Suggest reviewing the filler generation logic against the specifications to ensure compliance with size analysis attack prevention measures.
- 419-427: The
encrypt_failure_packetfunction correctly encrypts failure packets using ChaCha20. Ensure that the encryption key derivation (ammag) and the encryption process are secure and comply with the Lightning Network specifications.// Suggest reviewing the encryption key derivation and encryption process against the Lightning Network specifications for security compliance.
- 481-481: The
decrypt_onion_error_packetfunction's in-place decryption of the error packet is efficient. However, ensure that this approach does not inadvertently leak any sensitive information or introduce any side-channel vulnerabilities.// Suggest analyzing the in-place decryption approach for potential information leakage or side-channel vulnerabilities.
- 493-508: The
process_onion_failurefunction's handling of different failure scenarios is comprehensive. However, the complexity of this function warrants thorough testing, especially for the handling of blinded paths and the generation ofFailureLearnings.// Suggest adding comprehensive tests for the handling of different failure scenarios, especially for blinded paths and the generation of `FailureLearnings`.
- 813-814: The
HTLCFailReasonReprenum correctly represents different reasons for HTLC failure. Ensure that all possible failure scenarios are covered and that the enum's usage throughout the codebase is consistent.// Suggest reviewing all usages of `HTLCFailReasonRepr` throughout the codebase to ensure consistency and coverage of all possible failure scenarios.
- 903-916: The
get_encrypted_failure_packetfunction's handling of optional phantom shared secrets for additional encryption layers is a thoughtful design choice. Ensure that the use of phantom shared secrets is well-documented and tested.// Suggest adding documentation and tests for the use of optional phantom shared secrets in failure packet encryption.
- 900-941: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [934-964]
The
decode_onion_failurefunction's handling of differentHTLCFailReasonReprvariants to decode onion failures is correct. Ensure that the logic for handlingReasonvariants, especially the generation ofDecodedOnionFailure, is thoroughly tested.// Suggest adding tests for the logic handling `Reason` variants in `decode_onion_failure`, especially for the generation of `DecodedOnionFailure`.
- 976-976: The
NextPacketBytestrait implementation forFixedSizeOnionPacketandVec<u8>is correctly defined, allowing for flexible handling of packet bytes. Ensure that the implementations are used consistently and safely throughout the codebase.// Suggest reviewing all usages of `NextPacketBytes` implementations to ensure consistent and safe usage throughout the codebase.
- 1064-1067: The
decode_next_hopfunction's core logic for decoding the next hop in an onion packet is crucial. Pay special attention to the security of the HMAC check and the error handling logic to ensure they comply with the specifications.// Suggest reviewing the HMAC check and error handling logic against the specifications for security compliance.
| @@ -0,0 +1,13 @@ | |||
| #!/bin/bash | |||
| set -eox pipefail | |||
There was a problem hiding this comment.
The set -eox pipefail command includes the -x option, which prints commands and their arguments as they are executed. This can inadvertently leak sensitive information in logs. Consider removing -x unless debugging.
| set -eox pipefail | ||
|
|
||
| # Generate initial exclusion list | ||
| #find . -name '*.rs' -type f |sort >rustfmt_excluded_files |
There was a problem hiding this comment.
The commented-out find command (#find . -name '*.rs' -type f |sort >rustfmt_excluded_files) suggests an initial approach to generating the exclusion list. If this step is necessary for setup or future use, provide instructions or automate its execution rather than leaving it commented out.
ci/rustfmt.sh
Outdated
| for file in $(comm -23 $TMP_FILE rustfmt_excluded_files); do | ||
| echo "Checking formatting of $file" | ||
| rustfmt --check $file |
There was a problem hiding this comment.
The loop iterates over files to check formatting without handling potential spaces or special characters in filenames. Use while IFS= read -r file; do ... done < <(comm -23 $TMP_FILE rustfmt_excluded_files) to safely handle filenames.
- for file in $(comm -23 $TMP_FILE rustfmt_excluded_files); do
+ while IFS= read -r file; do ... done < <(comm -23 $TMP_FILE rustfmt_excluded_files)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.
| for file in $(comm -23 $TMP_FILE rustfmt_excluded_files); do | |
| echo "Checking formatting of $file" | |
| rustfmt --check $file | |
| while IFS= read -r file; do | |
| echo "Checking formatting of $file" | |
| rustfmt --check $file |
| secp_ctx: &Secp256k1<T>, path: &Path, session_priv: &SecretKey, mut callback: FType, | ||
| ) -> Result<(), secp256k1::Error> | ||
| where | ||
| T: secp256k1::Signing, | ||
| FType: FnMut(SharedSecret, [u8; 32], PublicKey, Option<&RouteHop>, usize) | ||
| FType: FnMut(SharedSecret, [u8; 32], PublicKey, Option<&RouteHop>, usize), | ||
| { | ||
| let mut blinded_priv = session_priv.clone(); | ||
| let mut blinded_pub = PublicKey::from_secret_key(secp_ctx, &blinded_priv); | ||
|
|
||
| let unblinded_hops_iter = path.hops.iter().map(|h| (&h.pubkey, Some(h))); | ||
| let blinded_pks_iter = path.blinded_tail.as_ref() | ||
| .map(|t| t.hops.iter()).unwrap_or([].iter()) | ||
| let blinded_pks_iter = path | ||
| .blinded_tail | ||
| .as_ref() | ||
| .map(|t| t.hops.iter()) | ||
| .unwrap_or([].iter()) |
There was a problem hiding this comment.
The construct_onion_keys_callback function correctly iterates over both unblinded and blinded hops, ensuring proper handling of blinded paths. However, consider adding more detailed comments explaining the logic, especially for the blinded paths handling, to improve maintainability.
// Add detailed comments explaining the handling of blinded paths.| @@ -797,6 +849,7 @@ | |||
| ;); | |||
|
|
|||
| impl HTLCFailReason { | |||
| #[rustfmt::skip] | |||
There was a problem hiding this comment.
The HTLCFailReason::reason function's extensive checks for failure codes and data lengths are good for ensuring correctness. However, consider simplifying this logic or breaking it into smaller functions for better readability and maintainability.
// Suggest simplifying the logic or breaking it into smaller functions for better readability and maintainability.
|
@coderabbitai pause |
|
@coderabbitai pause |
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
2 times, most recently
from
635ad02
to
23104e1
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2877 +/- ##
==========================================
+ Coverage 89.12% 90.06% +0.93%
==========================================
Files 115 115
Lines 93536 100169 +6633
Branches 93536 100169 +6633
==========================================
+ Hits 83368 90218 +6850
+ Misses 7639 7495 -144
+ Partials 2529 2456 -73 ☔ View full report in Codecov by Sentry. |
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
from
23104e1
to
ab17cf7
Compare
|
I'm ready to ACK whenever, though I think @jkczyz or @TheBlueMatt said they want to take a look first. |
| use bitcoin::hashes::{Hash, HashEngine}; | ||
| use bitcoin::hashes::cmp::fixed_time_eq; | ||
| use bitcoin::hashes::hmac::{Hmac, HmacEngine}; | ||
| use bitcoin::hashes::sha256::Hash as Sha256; | ||
| use bitcoin::hashes::{Hash, HashEngine}; |
There was a problem hiding this comment.
Little surprised to see this move.
There was a problem hiding this comment.
Probably just sorts by lexicographical order?
There was a problem hiding this comment.
Hmm... at very least it seems to go against the examples here:
There was a problem hiding this comment.
Note the Stable: No there, i.e., we're not using group_imports currently.
| if let Some(BlindedTail { | ||
| blinding_point, hops, final_value_msat, excess_final_cltv_expiry_delta, .. | ||
| }) = &path.blinded_tail { | ||
| blinding_point, | ||
| hops, | ||
| final_value_msat, | ||
| excess_final_cltv_expiry_delta, | ||
| .. | ||
| }) = &path.blinded_tail | ||
| { |
There was a problem hiding this comment.
Was there an option to keep the fields on the same line if it fits, similar to method parameters?
There was a problem hiding this comment.
I don't think so, I think rustfmt's verticality for cases like this and function calls was one of Matt's main dislikes.
I for one find one-argument-per-line more readable, but can see the argument regarding screen estate.
There was a problem hiding this comment.
God rustfmt is annoying sometimes.
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
from
ab17cf7
to
7606497
Compare
Well that would only happen if we then explicitly enable the packing option, but we won't get around this if we want to eventually enable some of the nice-to-have options that are currently unstable or even unimplemented. But, churn is a good point. Instead of formatting with the moving |
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
from
7606497
to
3132e51
Compare
| construct_onion_keys_callback( | ||
| secp_ctx, | ||
| &path, | ||
| session_priv, | ||
| |shared_secret, _blinding_factor, ephemeral_pubkey, _, _| { |
There was a problem hiding this comment.
| construct_onion_keys_callback( | |
| secp_ctx, | |
| &path, | |
| session_priv, | |
| |shared_secret, _blinding_factor, ephemeral_pubkey, _, _| { | |
| let callback = |shared_secret, _blinding_factor, ephemeral_pubkey, _, _| { | |
| ... | |
| }; | |
| construct_onion_keys_callback(secp_ctx, &path, session_priv, callback) |
There was a problem hiding this comment.
Although, I'm really preferring the previous version, as pulling these into variables requires specifying some types, resulting in:
let callback = |shared_secret: SharedSecret,
_blinding_factor,
ephemeral_pubkey,
_: Option<&RouteHop>,
_| {
...There was a problem hiding this comment.
Oh damn, I was thinking we'd be able to get it on one line. Indeed, if we can't don't bother.
Kinda surprised it needs the type info there.
There was a problem hiding this comment.
Now dropped the one bad case mentioned above, but kept one case further down, as pulling the callback to a dedicated variable there also results in rustfmt formatting it (interestingly, it seems to skip inlined closures, at least in this instance).
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
3 times, most recently
from
cfc59e0
to
4c73d51
Compare
.github/workflows/build.yml
Outdated
| rustfmt: | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| TOOLCHAIN: 1.75.0 |
There was a problem hiding this comment.
Do we need anything that violates our current MSRV (1.63)? Or can we MSRV this too?
There was a problem hiding this comment.
Mh, no, it appears that currently both could be used and the diff is really negligible. Generally, I'd prefer to loosely track the stable branch as we want to incorporate some of the upcoming changes as they become stabilized. But you're right, for now we should be good with using our MSRV version, which has the benefit of not requiring devs to install yet another special version.
There was a problem hiding this comment.
Now switched to use the MSRV.
There was a problem hiding this comment.
Yea, we can revisit once we want to depend on new features. (Also, I thought the whole point of stable rustfmt was that new versions wouldn't change the formatting for existing code......)
There was a problem hiding this comment.
Yeah, it should, but I'd rather be safe than sorry (i.e., encounter minor discrepancies that let our CI break when a new stable release is bumped). To quote the rustfmt github:
In general, we are looking to limit areas of instability; in particular, post-1.0, the formatting of most code should not change as Rustfmt improves. However, there are some things that Rustfmt can't do or can't do well (and thus where formatting might change significantly, even post-1.0). We would like to reduce the list of limitations over time.
Given for the entirety of rust-lightning there is a one-line diff between 1.63 and 1.75, it seems to be pretty stable, but probably still better to make bumping the formatter version a manual step we do from time-to-time / when we need new features.
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
3 times, most recently
from
3b3b973
to
7f5810b
Compare
|
Feel free to squash, IMO. Not sure what @jkczyz's views are on this currently. |
We add the previously discussed `rustfmt.toml` and enforce it in CI for any files that are not contained in an exclusion list. To start, we add all current Rust files to this exclusion list. This means that formatter rules will be enforced for any newly introduced files, and we'll then start going through the codebase file-by-file, removing them from the list as we go.
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
from
7f5810b
to
846e139
Compare
|
Squashed fixups without further changes. |
Pulling out expressions into variables for the some of the more egregious seems to have helped. |
.. now continously checking its formatting in CI.
tnull
force-pushed
the
2024-02-start-rustfmt-journey
branch
from
846e139
to
5d50e9e
Compare
|
Force-pushed including the following changes: > git diff-tree -U2 846e139fa 5d50e9e8a
diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs
index ba4095790..c705c4afd 100644
--- a/lightning/src/ln/onion_utils.rs
+++ b/lightning/src/ln/onion_utils.rs
@@ -1274,6 +1274,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "2ec2e5da605776054187180343287683aa6a51b4b1c04d6dd49c45d8cffb3c36";
assert_eq!(onion_keys[0].blinding_factor[..], <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619";
assert_eq!(
@@ -1281,6 +1283,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "ce496ec94def95aadd4bec15cdb41a740c9f2b62347c4917325fcc6fb0453986";
assert_eq!(onion_keys[0].rho, <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "b57061dc6d0a2b9f261ac410c8b26d64ac5506cbba30267a649c28c179400eba";
assert_eq!(onion_keys[0].mu, <Vec<u8>>::from_hex(hex).unwrap()[..]);
@@ -1291,6 +1295,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "bf66c28bc22e598cfd574a1931a2bafbca09163df2261e6d0056b2610dab938f";
assert_eq!(onion_keys[1].blinding_factor[..], <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "028f9438bfbf7feac2e108d677e3a82da596be706cc1cf342b75c7b7e22bf4e6e2";
assert_eq!(
@@ -1298,6 +1304,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "450ffcabc6449094918ebe13d4f03e433d20a3d28a768203337bc40b6e4b2c59";
assert_eq!(onion_keys[1].rho, <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "05ed2b4a3fb023c2ff5dd6ed4b9b6ea7383f5cfe9d59c11d121ec2c81ca2eea9";
assert_eq!(onion_keys[1].mu, <Vec<u8>>::from_hex(hex).unwrap()[..]);
@@ -1308,6 +1316,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "a1f2dadd184eb1627049673f18c6325814384facdee5bfd935d9cb031a1698a5";
assert_eq!(onion_keys[2].blinding_factor[..], <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "03bfd8225241ea71cd0843db7709f4c222f62ff2d4516fd38b39914ab6b83e0da0";
assert_eq!(
@@ -1315,6 +1325,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "11bf5c4f960239cb37833936aa3d02cea82c0f39fd35f566109c41f9eac8deea";
assert_eq!(onion_keys[2].rho, <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "caafe2820fa00eb2eeb78695ae452eba38f5a53ed6d53518c5c6edf76f3f5b78";
assert_eq!(onion_keys[2].mu, <Vec<u8>>::from_hex(hex).unwrap()[..]);
@@ -1325,6 +1337,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "7cfe0b699f35525029ae0fa437c69d0f20f7ed4e3916133f9cacbb13c82ff262";
assert_eq!(onion_keys[3].blinding_factor[..], <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "031dde6926381289671300239ea8e57ffaf9bebd05b9a5b95beaf07af05cd43595";
assert_eq!(
@@ -1332,6 +1346,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "cbe784ab745c13ff5cffc2fbe3e84424aa0fd669b8ead4ee562901a4a4e89e9e";
assert_eq!(onion_keys[3].rho, <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "5052aa1b3d9f0655a0932e50d42f0c9ba0705142c25d225515c45f47c0036ee9";
assert_eq!(onion_keys[3].mu, <Vec<u8>>::from_hex(hex).unwrap()[..]);
@@ -1342,6 +1358,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "c96e00dddaf57e7edcd4fb5954be5b65b09f17cb6d20651b4e90315be5779205";
assert_eq!(onion_keys[4].blinding_factor[..], <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "03a214ebd875aab6ddfd77f22c5e7311d7f77f17a169e599f157bbcdae8bf071f4";
assert_eq!(
@@ -1349,6 +1367,8 @@ mod tests {
<Vec<u8>>::from_hex(hex).unwrap()[..]
);
+
let hex = "034e18b8cc718e8af6339106e706c52d8df89e2b1f7e9142d996acf88df8799b";
assert_eq!(onion_keys[4].rho, <Vec<u8>>::from_hex(hex).unwrap()[..]);
+
let hex = "8e45e5c61c2b24cb6382444db6698727afb063adecd72aada233d4bf273d975a";
assert_eq!(onion_keys[4].mu, <Vec<u8>>::from_hex(hex).unwrap()[..]); |
|
Ship it! |
| if let Some(BlindedTail { | ||
| blinding_point, hops, final_value_msat, excess_final_cltv_expiry_delta, .. | ||
| }) = &path.blinded_tail { | ||
| blinding_point, | ||
| hops, | ||
| final_value_msat, | ||
| excess_final_cltv_expiry_delta, | ||
| .. | ||
| }) = &path.blinded_tail | ||
| { |
There was a problem hiding this comment.
God rustfmt is annoying sometimes.
| > update_opt.as_ref().unwrap().contents.htlc_minimum_msat | ||
| }, | ||
| 12 => { | ||
| update_opt.is_ok() |
There was a problem hiding this comment.
Dear
god
what
the
hell
is
this?
This is the start of the
rustfmtjourney, as previously discussed in #2648.Here, we first add a CI script that checks formatting for all but a list of excluded files. To begin with, we add all current Rust files in the codebase to the exclusion list.
Then, we make some manual adjustments before running
rustfmton the initial fileonion_utils.rs.Finally, we remove
onion_utils.rsfrom the exclusion list, ensuring its formatting will be checked continuously from now on.Please note that this PR for now will be the last chance to make final adjustments to our
rustfmt.tomlbefore we'll start introducing it to our codebase.