rustfmt: Format sign module
#2910
Conversation
|
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe updates primarily enhance the signing functionalities within the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai pause |
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- lightning/src/sign/ecdsa.rs (8 hunks)
- lightning/src/sign/mod.rs (55 hunks)
- lightning/src/sign/taproot.rs (8 hunks)
- lightning/src/sign/type_resolver.rs (2 hunks)
- rustfmt_excluded_files (1 hunks)
Files not summarized due to errors (1)
- lightning/src/sign/mod.rs: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- lightning/src/sign/ecdsa.rs
- lightning/src/sign/taproot.rs
Additional comments: 13
lightning/src/sign/type_resolver.rs (6)
- 4-7: The restructuring of the
ChannelSignerTypeenum with clearer trait bounds is a positive change for readability and maintainability. It makes the relationship betweenChannelSignerTypeandSignerProviderexplicit, which is beneficial for developers working with this code.- 14-17: The implementation block for
ChannelSignerTypecorrectly specifies the trait bounds forSP, ensuring thatSP::TargetimplementsSignerProvider. This consistency with the enum declaration is crucial for maintaining the integrity of the type system.- 1-20: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [18-26]
The
as_refmethod provides a safe way to obtain a reference to the underlyingChannelSigner, leveraging Rust's powerful match syntax and type system. This method enhances the usability ofChannelSignerTypeby allowing easy access to the signer's functionality.
- 35-51: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-35]
Similar to
as_ref, theas_mutmethod improves theChannelSignerType's flexibility by providing mutable access to the underlyingChannelSigner. This is essential for scenarios where the signer needs to be modified.
- 36-38: The
as_ecdsamethod introduces an option type for safely handling the case where theChannelSignerTypeis not an ECDSA signer. This approach prevents runtime errors and aligns with Rust's emphasis on safety and explicit error handling.- 43-48: The
as_mut_ecdsamethod complementsas_ecdsaby providing mutable access to the ECDSA signer if present. This method is crucial for operations that require altering the state of the ECDSA signer.rustfmt_excluded_files (1)
- 234-239: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of
ecdsa.rs,mod.rs,taproot.rs, andtype_resolver.rsfrom therustfmt_excluded_fileslist is a significant step towards ensuring consistent code formatting across the project. This change aligns with the PR's objectives of improving code readability and maintainability by adhering to coding standards.lightning/src/sign/mod.rs (6)
- 15-74: The reordering of imports and the addition of conditional compilation flags (
#[cfg(taproot)]) improve the code's readability and maintainability by clearly separating features and dependencies. This change aligns with best practices for managing feature-dependent code in Rust.- 116-123: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-122]
The
DelayedPaymentOutputDescriptorstruct and its implementation are well-structured, providing clear documentation and a straightforward implementation. The use of constants likeMAX_WITNESS_LENGTHfor defining the maximum length of a well-formed witness is a good practice, ensuring that magic numbers are avoided and making the code more maintainable.
- 162-187: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [123-172]
The implementation of
StaticPaymentOutputDescriptorand its methodwitness_scriptdemonstrates good use of Rust's option handling and conditional logic to support different channel types. However, it's essential to ensure comprehensive testing around these conditional paths, especially since they impact the script generation for spending outputs.
- 179-184: The method
max_witness_lengthinStaticPaymentOutputDescriptorcorrectly calculates the maximum witness length based on whether the channel supports anchor outputs. This is a critical piece of logic for ensuring that transactions are constructed correctly. It's recommended to add explicit unit tests for this method to verify the calculations under different conditions.Verification successful
The search for explicit unit tests targeting the
StaticPaymentOutputDescriptor::max_witness_lengthmethod did not yield any results, indicating that such tests likely do not exist in the codebase. This supports the original suggestion to add explicit unit tests for this method to verify its calculations under different conditions.* 236-236: The use of `Recipient::PhantomNode` in the `NodeSigner` trait implementation for `PhantomKeysManager` introduces a new variant to handle phantom node payments. This is a significant addition that enhances the flexibility of the signing process to accommodate different types of nodes. Ensure that all potential security implications of introducing phantom node payments are thoroughly reviewed, especially regarding the reuse of the `cross_node_seed` across multiple nodes. * 2313-2337: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2297-2328]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Suggest adding explicit unit tests for StaticPaymentOutputDescriptor::max_witness_length. echo "Suggest adding explicit unit tests for StaticPaymentOutputDescriptor::max_witness_length."Length of output: 188
Script:
#!/bin/bash # Search for unit tests targeting StaticPaymentOutputDescriptor::max_witness_length rg "StaticPaymentOutputDescriptor::max_witness_length" --type rustLength of output: 66
The method
PhantomKeysManager::spend_spendable_outputsdelegates toKeysManager::spend_spendable_outputs, maintaining a clear separation of concerns and reusing existing functionality effectively. This delegation is a good example of code reuse. However, ensure that the unique aspects of phantom node payments are adequately handled in the broader context of transaction signing and output spending.Verification successful
Given the need to verify the handling of phantom node payments in transaction signing and output spending, and the lack of information from the previous script execution, we will proceed to gather the necessary information by searching the codebase for relevant implementations or references. This will help us understand how
PhantomKeysManagerinteracts with transaction signing and output spending functionalities, and whether there are any unique aspects of phantom node payments that require special handling.
The search results provide extensive references to
PhantomKeysManagerand its usage across various parts of the codebase, including transaction signing, output spending, and handling phantom node payments. The references inlightning/src/sign/mod.rsand related files indicate that thePhantomKeysManageris designed to support phantom node payments by managing keys and signatures specific to phantom nodes. This includes generating phantom node IDs, handling secret keys for phantom nodes, and ensuring that phantom node payments are processed correctly.The code snippets and references found in the search results show that the
PhantomKeysManagerplays a crucial role in handling phantom node payments, including aspects related to transaction signing and output spending. The implementation details, such as the generation of phantom node IDs and the handling of secret keys, suggest that the unique aspects of phantom node payments are indeed considered and adequately handled within the broader context of transaction signing and output spending.Based on the information gathered from the search results, it can be concluded that the handling of phantom node payments, including their unique aspects in transaction signing and output spending, is implemented and integrated within the codebase through the
PhantomKeysManagerand related functionalities.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure that the unique aspects of phantom node payments are adequately handled in transaction signing and output spending. echo "Verify handling of phantom node payments in transaction signing."Length of output: 138
Script:
#!/bin/bash # Search for references to PhantomKeysManager and its interaction with transaction signing and output spending echo "Searching for PhantomKeysManager usage..." rg "PhantomKeysManager" --vimgrep echo "Searching for special handling of phantom node payments in transaction signing and output spending..." rg "phantom" --vimgrepLength of output: 39541
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/util/hash_tables.rs (7 hunks)
Additional comments: 9
lightning/src/util/hash_tables.rs (9)
- 19-19: The import of
HashMapfromstd::collectionsremains unchanged and is correctly placed for thestd_hashtablesmodule.- 21-21: The use of
std::collections::{hash_map, HashSet}is appropriate for thestd_hashtablesmodule, facilitating access to bothHashMapandHashSettypes and their related functionalities.- 29-31: The function
new_hash_mapcorrectly initializes a newHashMapwith the default constructor. This is a straightforward and expected implementation for creating a new hash map.- 36-42: The function
hash_map_from_iteris implemented correctly, utilizingHashMap::from_iterto create a newHashMapfrom an iterator. This is a clean and efficient way to initialize a hash map with pre-defined key-value pairs.- 46-54: The functions
new_hash_setandhash_set_from_iterfor creating newHashSetinstances and initializing them from an iterator are correctly implemented. These methods follow the expected patterns for set creation and initialization in Rust.- 76-92: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [79-112]
The
RandomStatestruct and its implementation in thehashbrown_tablesmodule introduce a significant change. The logic for potentially randomizing the hasher state usinggetrandom(if available) is a crucial security enhancement. However, it's important to ensure that thepossiblyrandomfeature is correctly configured in the project settings to make use of this functionality effectively.
- 124-124: The re-export of the
hashermodule's contents in thehashbrown_tablesmodule is a good practice, ensuring that all necessary hasher-related types and functions are accessible where needed.- 143-149: The updated
hash_map_from_iterfunction in thehashbrown_tablesmodule now includes logic to pre-allocate the hash map with a capacity based on the iterator's size hint. This is a performance optimization that can reduce the number of re-allocations when filling the map from an iterator with a known or estimated size.- 163-165: Similarly, the
hash_set_from_iterfunction's update to pre-allocate capacity based on the iterator's size hint is a performance optimization forHashSetcreation. This approach is consistent with best practices for initializing collections from iterators with known sizes.
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 #2910 +/- ##
==========================================
+ Coverage 89.36% 89.40% +0.03%
==========================================
Files 117 117
Lines 95622 96016 +394
Branches 95622 96016 +394
==========================================
+ Hits 85457 85840 +383
- Misses 7930 7959 +29
+ Partials 2235 2217 -18 ☔ View full report in Codecov by Sentry. |
tnull
force-pushed
the
2024-02-rustfmt-sign-mod
branch
from
4989371
to
a8717a2
Compare
|
Kicked CI. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- msrv-no-dev-deps-check/src/lib.rs (1 hunks)
- rustfmt_excluded_files (2 hunks)
Files skipped from review due to trivial changes (1)
- msrv-no-dev-deps-check/src/lib.rs
Files skipped from review as they are similar to previous changes (1)
- rustfmt_excluded_files
|
@coderabbitai pause, pretty please. |
|
@tnull: Sure, I'll pause the review process as requested. If you need anything else, feel free to let me know. Actions performedReviews paused. |
|
Lets wait until #2761. |
Alright, then I'll move the hash tables fix to its own PR to unbreak CI. Also considering then prioritizing other modules to keep making progress. |
Added here: #2919 |
|
Lets let the author of #2761 decide whether he wants to rebase with the fmt changes or without. |
tnull
force-pushed
the
2024-02-rustfmt-sign-mod
branch
from
a8717a2
to
230961f
Compare
|
Rebased, addressed comments, and cleaned up commit history. |
tnull
force-pushed
the
2024-02-rustfmt-sign-mod
branch
from
230961f
to
d0043d9
Compare
|
Force-pushed including the following fixups: > git diff-tree -U3 230961f3e d0043d972
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 18c4d8340..b8fdbb9f5 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -850,6 +850,11 @@ impl ChannelTransactionParameters {
self.counterparty_parameters.is_some() && self.funding_outpoint.is_some()
}
+ /// Whether the channel supports zero-fee HTLC transaction anchors.
+ pub fn supports_anchors(&self) -> bool {
+ self.channel_type_features.supports_anchors_zero_fee_htlc_tx()
+ }
+
/// Convert the holder/counterparty parameters to broadcaster/countersignatory-organized parameters,
/// given that the holder is the broadcaster.
///
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index dd9075db6..22a5fff83 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -163,7 +163,7 @@ impl StaticPaymentOutputDescriptor {
/// originated from an anchor outputs channel, as they take the form of a P2WSH script.
pub fn witness_script(&self) -> Option<ScriptBuf> {
self.channel_transaction_parameters.as_ref().and_then(|channel_params| {
- if params_support_anchors(channel_params) {
+ if channel_params.supports_anchors() {
let payment_point = channel_params.holder_pubkeys.payment_point;
Some(chan_utils::get_to_countersignatory_with_anchors_redeemscript(&payment_point))
} else {
@@ -176,8 +176,7 @@ impl StaticPaymentOutputDescriptor {
/// Note: If you have the grind_signatures feature enabled, this will be at least 1 byte
/// shorter.
pub fn max_witness_length(&self) -> u64 {
- if self.channel_transaction_parameters.as_ref().map(params_support_anchors).unwrap_or(false)
- {
+ if self.channel_transaction_parameters.as_ref().map_or(false, |p| p.supports_anchors()) {
let witness_script_weight = 1 /* pubkey push */ + 33 /* pubkey */ +
1 /* OP_CHECKSIGVERIFY */ + 1 /* OP_1 */ + 1 /* OP_CHECKSEQUENCEVERIFY */;
1 /* num witness items */ + 1 /* sig push */ + 73 /* sig including sighash flag */ +
@@ -365,8 +364,7 @@ impl SpendableOutputDescriptor {
let sequence = if descriptor
.channel_transaction_parameters
.as_ref()
- .map(params_support_anchors)
- .unwrap_or(false)
+ .map_or(false, |p| p.supports_anchors())
{
Sequence::from_consensus(1)
} else {
@@ -381,8 +379,9 @@ impl SpendableOutputDescriptor {
witness_weight += descriptor.max_witness_length();
#[cfg(feature = "grind_signatures")]
{
+ // Guarantees a low R signature
witness_weight -= 1;
- } // Guarantees a low R signature
+ }
input_value += descriptor.output.value;
},
SpendableOutputDescriptor::DelayedPaymentOutput(descriptor) => {
@@ -2348,10 +2347,6 @@ impl EntropySource for RandomBytes {
}
}
-fn params_support_anchors(channel_params: &ChannelTransactionParameters) -> bool {
- channel_params.channel_type_features.supports_anchors_zero_fee_htlc_tx()
-}
-
// Ensure that EcdsaChannelSigner can have a vtable
#[test]
pub fn dyn_sign() { |
tnull
force-pushed
the
2024-02-rustfmt-sign-mod
branch
from
d0043d9
to
5b33d32
Compare
|
Force-pushed including the following changes: > git diff-tree -U2 d0043d972 5b33d3264
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index b8fdbb9f5..f8d15d2a8 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -852,5 +852,5 @@ impl ChannelTransactionParameters {
/// Whether the channel supports zero-fee HTLC transaction anchors.
- pub fn supports_anchors(&self) -> bool {
+ pub(crate) fn supports_anchors(&self) -> bool {
self.channel_type_features.supports_anchors_zero_fee_htlc_tx()
} |
|
Lets maybe wait for #2845? |
.. now continously checking its formatting in CI.
.. now continously checking its formatting in CI.
.. now continously checking its formatting in CI.
.. now continously checking its formatting in CI.
.. now continously checking its formatting in CI.
.. now continously checking its formatting in CI.
tnull
force-pushed
the
2024-02-rustfmt-sign-mod
branch
from
5b33d32
to
3a199c6
Compare
|
Rebased to resolve minor conflict. |
Chose this one next as it's straightforward and AFAICT there should be ~nothing currently in flight conflicting with this.
Also added a commit to formatutil/hash_tables.rsto fix the silent rebase conflict (i.e., it was introduced after the base of #2877) and a few trivial ones for helper crates.