-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust docs noise_sv2
#1111
base: main
Are you sure you want to change the base?
Rust docs noise_sv2
#1111
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
just two minor non blocking things for me:
|
noted, will push these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some typos and grammer corrections and some general questions for my curiousity.
Should generate_key be documented for handshake.rs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my first pass, mainly focusing on the initiator, responder, and handshake trait. I will leave another review for the remaining modules, but this review is already quite large. Everything you are doing is great! Most of this are just my nits.
- We should add some top level docs in
lib.rs
that follow the same format as this which is from my PR Rust docs forprotocols::v2::codec-sv2
#1040. Please use the sv2-spec/04-Protocol-Security.md for reference when describing what the crate does in this section. - Each module (file) should have the appropriate top level docs.
lib.rs
needs to have doc comments on thePARITY
constant and theNoiseCodec
struct and its implemented methods.- All documentation should span the full 100 character line length instead of 80 characters.
- Avoid including language like "This struct performs ABC" or "This is a trait defining XYZ", just say "Performs ABC" or "Defines XYZ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
which generates this warning:
we could give this link some neat markdown formatting, something like:
|
0c25e92
to
bea943a
Compare
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
4b7be45
to
5144028
Compare
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
5596727
to
5027e63
Compare
5027e63
to
424f8c8
Compare
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I added a few commits to add type linking supported by cargo docs
and also added the module comments in lib.rs
. Let's get a couple other team members approval, then we can implement any feedback and merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fi3 I cannot find any instance of the use of the into_aesg
and into_chacha
methods in the CipherState<Cipher_: AeadCipher>
implementation. Should these be removed? If they need to stay, can you help me understand when/how they should be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another into_aesg
method in the impl GenericCipher
block. It has the [allow(dead_code)]
decorator, should this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. The documentation should belong to the trait itself, not the implementation. Again reiterating, that there may be exceptions to this rule, however this is not an exception case.
@@ -17,22 +79,47 @@ use secp256k1::{ellswift::ElligatorSwift, Keypair, Secp256k1, SecretKey}; | |||
|
|||
const VERSION: u16 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const VERSION: u16 = 0; | |
/// Version of the certificate format. | |
const VERSION: u16 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to change the var name to CERTIFICATE_FORMAT_VERSION
and not add docs. wdyt @rrybarczyk ?
protocols/v2/noise-sv2/src/error.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there are several error variants here that I cannot find uses of in the codebase. I did my best to make comments for them for the sake completeness and listed them in this comment in our #1130 tech debt tracker issue.
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very promising. Added few comments as I was able to review just a couple of files, will deep dive into the rest when I have a minute.
@@ -1,3 +1,70 @@ | |||
//! # Noise NX Protocol Initiator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is NX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For noise protocol, we have a number of different handshake patterns. NX is the one we are using.
//! # Noise NX Protocol Initiator | ||
//! | ||
//! This module manages the state and operations of the initiator within the Noise NX protocol. | ||
//! This struct plays a crucial role in the cryptographic handshake, establishing a secure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to which struct
this is referring to. Maybe we should have the initial documentation general and not specific to any struct in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not pointing to any struct, just a general module level overview.
@@ -274,6 +379,8 @@ impl Initiator { | |||
} | |||
} | |||
impl Drop for Initiator { | |||
/// Ensures that sensitive data is securely erased when the [`Initiator`] instance is dropped, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove this. The docs on the erase
function and on the Drop
trait are enough IMO
@@ -252,6 +351,12 @@ impl Initiator { | |||
} | |||
} | |||
|
|||
/// Securely erases sensitive data in the initiator's memory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Securely erases sensitive data in the initiator's memory. | |
/// Securely erases sensitive data from the [`Initiator`] memory. |
/// | ||
/// Clears all sensitive cryptographic material within the [`Initiator`] to prevent any | ||
/// accidental leakage or misuse. It overwrites the stored keys, chaining key, handshake hash, | ||
/// and session ciphers with zeros or other non-sensitive values. This method is typically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is or other non-sensitive-values
?
handshake_cipher: Option<ChaCha20Poly1305>, | ||
/// Optional static key used in the handshake. This key may be derived from the pre-shared key | ||
/// (PSK) or generated during the handshake. | ||
k: Option<[u8; 32]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and in the initiator
(very) short names are used for the variables. wdyt about using full names here and else-where, even if its too long? that should be a task for the refactor phase though if we agree on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going by the noise protocol naming convention.
/// | ||
/// The [`AeadCipher`] trait provides a standard interface for initializing AEAD ciphers, and for | ||
/// performing encryption and decryption operations with additional AAD. This trait is implemented | ||
/// by either the [`ChaCha20Poly1305`] or [`Aes256Gcm`] specific cipher types, allowing them to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// by either the [`ChaCha20Poly1305`] or [`Aes256Gcm`] specific cipher types, allowing them to be | |
/// by both the ChaCha20Poly1305 and Aes256Gcm, allowing them to be |
I think both of them implement the trait right? also I dont think we should link to crates outside of our project, unless maybe if its top of the file documentation or something else that is more general
/// Defines the interface for AEAD ciphers. | ||
/// | ||
/// The [`AeadCipher`] trait provides a standard interface for initializing AEAD ciphers, and for | ||
/// performing encryption and decryption operations with additional AAD. This trait is implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is AAD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authenticated Associated data
/// used interchangeably in cryptographic protocols. It is utilized by the | ||
/// [`crate::handshake::HandshakeOp`] trait to secure the handshake process. | ||
/// | ||
/// The `T: Buffer` represents the data buffer to be encrypted or decrypted. The buffer must |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is decrypt/encrypt
functions specific and not in trait level, so maybe it should be moved(duplicated) above each function
/// | ||
/// The `T: Buffer` represents the data buffer to be encrypted or decrypted. The buffer must | ||
/// implement the [`Buffer`] trait, which provides necessary operations for in-place encryption and | ||
/// decryption. | ||
pub trait AeadCipher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fi3 whats the goal behind this trait and then implementing it for ChaChaPoly1305
if the ChaCha..
already have some AEAD
traits implemented for it https://docs.rs/chacha20poly1305/latest/src/chacha20poly1305/lib.rs.html#241 which is used in our implementation without any changes https://github.com/stratum-mining/stratum/pull/1111/files#diff-af2c2f10de68656c4249abc945c50c903ccfcd35bf169ce3a4779f1c4e5245e1L33?
/// Creates a new [`Initiator`] without specifying a responder's public key. | ||
/// This function initializes the [`Initiator`] with a default empty state. | ||
pub fn without_pk() -> Result<Box<Self>, Error> { | ||
Ok(Self::new(None)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should elaborate more here to let the user know under which circumstances this could be useful
according to @Fi3 via #916 (comment):
we do not check the certificate. So you still have an encrypted connection but you don't authenticate the upstream
For example when you connect to a TP that you run locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are used when we want encrypted connection but without any authentication.
closes #1013