rustfmt non-test ln module's not-currently-being-edited files
#3763
Conversation
|
👋 I see @valentinewallace was un-assigned. |
| let mac_check = | ||
| chacha.variable_time_decrypt(&cyphertext[0..cyphertext.len() - 16], res, hmac); | ||
| mac_check.map_err(|()| LightningError { |
There was a problem hiding this comment.
nitpicky non-blocking personal opinion that this looks better without chacha + mac_check:
ChaCha20Poly1305RFC::new(key, &nonce, h)
.variable_time_decrypt(hmac, res, &cyphertext[cyphertext.len() - 16..])
.map_err(|()| LightningError {
err: "Bad MAC".to_owned(),
action: msgs::ErrorAction::DisconnectPeer { msg: None },
})
There was a problem hiding this comment.
So first I went and replaced the two &cyphertext[..] with a cyphertext.split_at which is how it should have been to begin with. Sadly, condensing it the way you suggest makes rustfmt push the whole thing onto one line which is kinda hard to read:
let (data, hmac) = cyphertext.split_at(cyphertext.len() - 16);
ChaCha20Poly1305RFC::new(key, &nonce, h).variable_time_decrypt(&data, res, hmac).map_err(
|()| LightningError {
err: "Bad MAC".to_owned(),
action: msgs::ErrorAction::DisconnectPeer { msg: None },
}
)
so instead I dropped the chacha but kept the hmac_check (which is vaguely nice because it highlights that we're not decrypting, but only checking the MAC):
let (data, hmac) = cyphertext.split_at(cyphertext.len() - 16);
let mac_check =
ChaCha20Poly1305RFC::new(key, &nonce, h).variable_time_decrypt(&data, res, hmac);
mac_check.map_err(|()| LightningError {
err: "Bad MAC".to_owned(),
action: msgs::ErrorAction::DisconnectPeer { msg: None },
})
| ); | ||
| expect_channel_ready_event(&nodes[0], &nodes[2].node.get_our_node_id()); | ||
| expect_channel_ready_event(&nodes[2], &nodes[0].node.get_our_node_id()); | ||
| let as_channel_ready = |
There was a problem hiding this comment.
Changing as_channel_ready from the message that C omits to send to A to the message that A omits to send C seems consistent with naming elsewhere in the codebase (at least the majority of cases) 👌
lightning/src/ln/onion_payment.rs
Outdated
| use bitcoin::hashes::sha256::Hash as Sha256; | ||
| use bitcoin::secp256k1::{self, PublicKey, Secp256k1}; | ||
| use bitcoin::hashes::Hash; |
There was a problem hiding this comment.
There was a problem hiding this comment.
That last commit was just a vanilla rustfmt, wasn't it?
I think (...) that wouldn't be a problem when using the fix script #3749 (comment)? Just apply rustfmt to onion_payment.rs for each commit of #3711.
At least then all the new code added in that PR can be rustfmt-optimized immediately and doesn't need to be touched later again potentially.
There was a problem hiding this comment.
In my experience with rebasing #3751 git's detection of which hunks are a part of a diff is pretty bad (even without indentation changes) so I'm somewhat skeptical of formatting files that are being super actively worked on. It also doesn't seem like a big deal - waiting an extra few weeks for onion_payment.rs seems perfectly fine?
These methods are super trivial to replace with the `ChannelManager` equivalent, so keeping them around for a while doesn't give us anything.
This leaves some mess in the tests, but they're mostly encoding tests that should never fail and are generally quite short.
Via sed: s/\(\t*\)\(.*\)<Vec<u8>>::from_hex(\(["0-9a-f]*\)).unwrap()/\1let hex = \3;\r\1\2<Vec<u8>>::from_hex(hex).unwrap()/g
TheBlueMatt
force-pushed
the
2025-05-rustfmt-untouched-ln
branch
from
27ca228 to
01f50bc
Compare
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
| @@ -578,39 +578,52 @@ mod tests { | |||
| use crate::util::test_utils::TestNodeSigner; | |||
|
|
|||
| fn get_outbound_peer_for_initiator_test_vectors() -> PeerChannelEncryptor { | |||
| let their_node_id = PublicKey::from_slice(&<Vec<u8>>::from_hex("028d7500dd4c12685d1f568b4c2b5048e8534b873319f3a8daa612b469132ec7f7").unwrap()[..]).unwrap(); | |||
| let hex = "028d7500dd4c12685d1f568b4c2b5048e8534b873319f3a8daa612b469132ec7f7"; | |||
There was a problem hiding this comment.
NIce regex in commit message
|
|
||
| expect_channel_ready_event(&nodes[0], &nodes[1].node.get_our_node_id()); | ||
| expect_channel_ready_event(&nodes[1], &nodes[0].node.get_our_node_id()); | ||
| expect_channel_ready_event(&nodes[0], &node_b_id); |
There was a problem hiding this comment.
Also after reviewing this PR, my opinion remains unchanged that changes like this aren't worth it. This PR is compact, so not a problem, but I'd be hesitant with future formatting changes that are anything more than what's the absolute undisputed minimum required for readability.
There was a problem hiding this comment.
I don't really agree that we should have a culture of only cleaning up code that is "undisputed minimum for readability", that seems like a weird bar to have for cleanup PRs? Unrelated to if we can do this in a separate PR or not, of course.
There was a problem hiding this comment.
If some people think the clean up makes it less readable (making it 'disputed'), I don't think we should do it and stick to the rustfmt output.
|
All trivial changes and there's two reviews so not worth waiting. |
This
rustfmts most of the remaining files inlnthat aren'tchannel{,manager}.rs,outbound_payment.rsand*_tests.rs. It avoidsonion_utilssince that may get more edits for trampoline,peer_handler.rs(since that's #3725), andchan_utils.rssince that may get more changes for custom commitments.