Refactor: `noise-sv2` crate #1130

rrybarczyk · 2024-08-21T19:57:16Z

Background

This task is an outcome of the protocols Rust docs issues tracked in #845.

While documenting protocols::v2::noise-sv2 in #1013, areas of potential code debt were identified. This issue servers as a place to list out these items to then be addressed in an organized manner. The initial Rust documentation effort was an immediate action, while a refactoring (which implies breaking API changes) is not so urgent priority, so for now we should leave this in the backlog for an appropriate moment in the future.

Identified Potential Code Debt

Consider movingnoise_sv2::cipher_state::GenericCipher enum + associated logic to its own noise_sv2::generic_cipher module. Or, perhaps given the comment in the #916 discussion and the removal of AES256GCM in this sv2-spec PR, we do not need the GenericCipher at all? Do we plan so support anything other than ChaCha20Poly1305 in the future?
Consider moving noise_sv2::cipher_state::Cipher struct and associated logic into its own noise_sv2::cipher module.
unwraps in the impl From<[u8; 74]> for SignatureNoiseMessage from method.
There is no custom Result type alias around our custom Error enum. Should add and use this everywhere we have a std::result::Result type.
Consider making handshake::HandshakeOp trait public so users can call Responder::generate_key().

The text was updated successfully, but these errors were encountered:

Shourya742 · 2024-08-22T09:04:37Z

I noticed a few unwrap calls in the initiator, responder, and signature modules. It's generally not advisable to use unwrap in a library crate since it can cause the program to panic if an error occurs. Below are the locations where unwrap is being used:

initiator.rs

let plaintext: [u8; SIGNATURE_NOISE_MESSAGE_SIZE] = to_decrypt.try_into().unwrap();
let signature_message: SignatureNoiseMessage = plaintext.into();
let rs_pub_key = PublicKey::from_ellswift(elligatorswift_theirs_static)
    .x_only_public_key()
    .0
    .serialize();
let rs_pk_xonly = XOnlyPublicKey::from_slice(&rs_pub_key).unwrap();

responder.rs

// 7. appends `EncryptAndHash(SIGNATURE_NOISE_MESSAGE)` to the buffer
let valid_from = std::time::SystemTime::now()
    .duration_since(std::time::UNIX_EPOCH)
    .unwrap()
    .as_secs();

signature_message.rs

let now = SystemTime::now()
    .duration_since(SystemTime::UNIX_EPOCH)
    .unwrap()
    .as_secs() as u32;

let version = u16::from_le_bytes(value[0..2].try_into().unwrap());
        let valid_from = u32::from_le_bytes(value[2..6].try_into().unwrap());
        let not_valid_after = u32::from_le_bytes(value[6..10].try_into().unwrap());
        let signature = value[10..74].try_into().unwrap();

We should propagate these errors instead of using unwrap and allow the implementing binary to handle them appropriately.

Fi3 · 2024-08-22T16:51:45Z

they are inffalible, if you prefer you can use expect. I strongly suggest to not propagate error for impossible cases as we would only make the code harder to understand.

Shourya742 · 2024-08-23T14:34:52Z

Based on the discussion about using the multi-cipher (referenced here: #916 (comment)), we need to remove AES_256_GCM from our codebase and update the specs accordingly.

Shourya742 · 2024-08-23T14:44:48Z

Considering the comment by @plebhash here: #916 (comment), and the removal of aes_256_gcm, we will now use chacha20poly1305::Error instead of aes_gcm::Error. Although we could have used aead::Error, since we've finalized using only chacha20poly1305, it makes sense to use chacha20poly1305::Error now.

rrybarczyk · 2024-08-27T16:09:03Z

Considering the comment by @plebhash here: #916 (comment), and the removal of aes_256_gcm, we will now use chacha20poly1305::Error instead of aes_gcm::Error. Although we could have used aead::Error, since we've finalized using only chacha20poly1305, it makes sense to use chacha20poly1305::Error now.

Just so we know, the aes-gcm lib appears in the util/buffer/Cargo.toml as well and should be removed.

%rg aes_gcm -i -l

protocols/v2/noise-sv2/src/cipher_state.rs
protocols/v2/noise-sv2/src/aed_cipher.rs
protocols/v2/noise-sv2/src/responder.rs
protocols/v2/noise-sv2/src/handshake.rs
protocols/v2/noise-sv2/src/lib.rs
protocols/v2/noise-sv2/src/initiator.rs
protocols/v2/noise-sv2/src/error.rs
protocols/v2/noise-sv2/tmp.rs
utils/buffer/src/lib.rs
utils/buffer/src/buffer.rs
utils/buffer/src/buffer_pool/mod.rs

Shourya742 · 2024-08-27T16:11:32Z

Based on the discussion about using the multi-cipher (referenced here: #916 (comment)), we need to remove AES_256_GCM from our codebase and update the specs accordingly.

The specs have been updated with this PR.

plebhash · 2024-08-29T22:26:49Z

potentially relevant here: stratum-mining/sv2-spec#65

Shourya742 · 2024-09-02T06:22:17Z

Reminder: Add a unit test to the noise-sv2 module. See discussion: #1111 (comment)

Shourya742 · 2024-09-02T09:47:34Z

remove redundant unused code in module: #1111 (comment)

rrybarczyk · 2024-09-12T14:45:40Z

The following may be unused imports in the handshake.rs test module:

secp256k1::ecdh::SharedSecret
secp256k1::ecdh::XOnlyPublicKey

rrybarczyk · 2024-09-12T14:48:18Z

Perhaps test.rs is a case where we should have a proper integration test using the tests/ directory.

rrybarczyk · 2024-09-12T19:05:04Z

Reminder that all imports should be at the top of the file, meaning we should never be calling crate::<Type> inside actual logic. Once case of this is calling crate::NoiseCodec in the Initiator::step_2 method.

rrybarczyk · 2024-09-12T19:15:13Z

Because docs on private modules are not rendered by cargo docs by default (will render if the --document-private-items flag is present when generating), we should really think about what documentation will be included or excluded. For example, the initiator and responder modules are private, but the Initiator and Responder structs are public. So the documentation at the top of these files are not accessible by default.

Perhaps we make these modules public? Or perhaps when we publish the final documentation, we publish the private logic docs as well (if this is an option)?

rrybarczyk · 2024-09-12T19:31:21Z

In error::Error, what is the difference between the UnsupportedCiphers(Vec<u8>) and theInvalidCipherChosed(Vec<u8>)? If there is no difference, we should remove InvalidCipherChosed(Vec<u8>) in support of UnsupportedCiphers(Vec<u8>).

Otherwise, if both are needed, we need to fix the grammatical error: InvalidCipherChosed(Vec<u8>) -> InvalidCipherChosen(Vec<u8>).

rrybarczyk · 2024-09-12T19:35:51Z

In the error module, we should not import use aes_gcm::Error as AesGcm;. Instead we should define the variant as AesGcm(aes_gcm::Error). Also the impl From will need to updated with this change.

rrybarczyk · 2024-09-12T20:21:45Z

Error variants that I cannot find uses of in the entire repo:

CipherListMustBeNonEmpty
UnsupportedCiphers
InvalidCipherList
InvalidCipherChosed (see related comment)
InvalidCipherState

Fi3 · 2024-09-13T08:40:17Z

I noticed a few unwrap calls in the initiator, responder, and signature modules. It's generally not advisable to use unwrap in a library crate since it can cause the program to panic if an error occurs. Below are the locations where unwrap is being used:

* **`initiator.rs`**
  ```rust
  let plaintext: [u8; SIGNATURE_NOISE_MESSAGE_SIZE] = to_decrypt.try_into().unwrap();
  let signature_message: SignatureNoiseMessage = plaintext.into();
  let rs_pub_key = PublicKey::from_ellswift(elligatorswift_theirs_static)
      .x_only_public_key()
      .0
      .serialize();
  let rs_pk_xonly = XOnlyPublicKey::from_slice(&rs_pub_key).unwrap();
  ```

* **`responder.rs`**
  ```rust
  // 7. appends `EncryptAndHash(SIGNATURE_NOISE_MESSAGE)` to the buffer
  let valid_from = std::time::SystemTime::now()
      .duration_since(std::time::UNIX_EPOCH)
      .unwrap()
      .as_secs();
  ```

* **`signature_message.rs`**
  ```rust
  let now = SystemTime::now()
      .duration_since(SystemTime::UNIX_EPOCH)
      .unwrap()
      .as_secs() as u32;
  
  let version = u16::from_le_bytes(value[0..2].try_into().unwrap());
          let valid_from = u32::from_le_bytes(value[2..6].try_into().unwrap());
          let not_valid_after = u32::from_le_bytes(value[6..10].try_into().unwrap());
          let signature = value[10..74].try_into().unwrap();
  ```

We should propagate these errors instead of using unwrap and allow the implementing binary to handle them appropriately.

not agree if the code is infallibe unwrap or expect is a fare better solution that propoagate the error. We can put some assertion to be extrasafe.

Fi3 · 2024-09-13T08:42:33Z

Reminder that all imports should be at the top of the file, meaning we should never be calling crate::<Type> inside actual logic. Once case of this is calling crate::NoiseCodec in the Initiator::step_2 method.

why?

Fi3 · 2024-09-13T08:46:28Z

Error variants that I cannot find uses of in the entire repo:

* `CipherListMustBeNonEmpty`

* `UnsupportedCiphers`

* `InvalidCipherList`

* `InvalidCipherChosed` (see related [comment](https://github.com/stratum-mining/stratum/issues/1130#issuecomment-2347084401))

* `InvalidCipherState`

if not used better to remove them, remember to bump minor version if you do it, otherwise we will get compilation error on importers

jbesraa · 2024-09-13T12:44:09Z

Reminder that all imports should be at the top of the file, meaning we should never be calling crate::<Type> inside actual logic. Once case of this is calling crate::NoiseCodec in the Initiator::step_2 method.

In general I agree with this approach but I think there are some exceptions. If you are using a module from tokio that also exist in the standard library, for example tokio::time::sleep and std::thread::sleep, it would be better to write the full path where ever it is used to make it easier/quicker to understand where the module is imported from.

jbesraa · 2024-09-13T12:49:52Z

I noticed a few unwrap calls in the initiator, responder, and signature modules. It's generally not advisable to use unwrap in a library crate since it can cause the program to panic if an error occurs. Below are the locations where unwrap is being used:

* **`initiator.rs`**
  ```rust
  let plaintext: [u8; SIGNATURE_NOISE_MESSAGE_SIZE] = to_decrypt.try_into().unwrap();
  let signature_message: SignatureNoiseMessage = plaintext.into();
  let rs_pub_key = PublicKey::from_ellswift(elligatorswift_theirs_static)
      .x_only_public_key()
      .0
      .serialize();
  let rs_pk_xonly = XOnlyPublicKey::from_slice(&rs_pub_key).unwrap();

responder.rs

// 7. appends `EncryptAndHash(SIGNATURE_NOISE_MESSAGE)` to the buffer
let valid_from = std::time::SystemTime::now()
    .duration_since(std::time::UNIX_EPOCH)
    .unwrap()
    .as_secs();

signature_message.rs

let now = SystemTime::now()
    .duration_since(SystemTime::UNIX_EPOCH)
    .unwrap()
    .as_secs() as u32;

let version = u16::from_le_bytes(value[0..2].try_into().unwrap());
        let valid_from = u32::from_le_bytes(value[2..6].try_into().unwrap());
        let not_valid_after = u32::from_le_bytes(value[6..10].try_into().unwrap());
        let signature = value[10..74].try_into().unwrap();



    
      
    

      
    

    
  
We should propagate these errors instead of using `unwrap` and allow the implementing binary to handle them appropriately.

not agree if the code is infallibe unwrap or expect is a fare better solution that propoagate the error. We can put some assertion to be extrasafe.

I think expect is generally better than unwrap because the bit of context it provides. Maybe we can also utilize https://doc.rust-lang.org/std/macro.debug_assert.html.

IMO though, it is best to add the error variants and keep the code consistent without panics.

plebhash · 2024-09-13T13:21:11Z

not agree if the code is infallibe unwrap or expect is a fare better solution that propoagate the error.

if the code is provably safe (it could never panic under any circumstances), then it makes sense to use expect with a message that makes it very clear that there will never be a panic there.

For example:

let valid_from = std::time::SystemTime::now()
    .duration_since(std::time::UNIX_EPOCH)
-   .unwrap()
+   .expect("std::time::SystemTime::now() should always be later than std::time::UNIX_EPOCH")
    .as_secs();

I'm against using unwrap, because it doesn't make this point explicitly clear.

We can put some assertion to be extrasafe.

I don't think this makes sense. The only thing an assertion would do is to panic, which is exactly what would happen with the unwrap, so it would be completely redundant.

overall, my strong stance on this discussion is:

we cannot ever have our libs doing panics for edge cases... a robust library has proper error handling

and if we're making exceptions for cases we are 100% sure there will never be a panic, we should make that explicitly clear.

Shourya742 · 2024-09-14T04:24:29Z

Consistent Implementation and Naming that Adheres to the Noise Protocol Specification

Noise protocol reference link.

We have HandshakeOp and TestHandShake, we need to make sure we standardize on Handshake with a lowercase "s".

@Shourya742 may also have identified more instances that pertain to more protocol details.

@Fi3 ,
For any Noise protocol, we have three key objects for a handshake:

CipherState Object: Responsible for defining the key (k), nonce, and the AEAD algorithm (in our case, ChaCha20Poly1305).
SymmetricState Object: Manages the chaining key (ck), handshake hash (h), and includes the cipher state.
Handshake Object: Contains the SymmetricState, along with s, rs, e, and re (static and ephemeral key pairs).

However, in our implementation, the Initiator and Responder structs are combined with the Handshake Object, and the SymmetricState Object is referred to as HandshakeOps. Should we consider changing this to align more closely with the Noise protocol, using the correct names? This would make it less confusing for others reviewing our code alongside the Noise framework specification.

Fi3 · 2024-09-16T12:01:48Z

Consistent Implementation and Naming that Adheres to the Noise Protocol Specification

Noise protocol reference link.

We have HandshakeOp and TestHandShake, we need to make sure we standardize on Handshake with a lowercase "s".

@Shourya742 may also have identified more instances that pertain to more protocol details.

@Fi3 , For any Noise protocol, we have three key objects for a handshake:
1. **CipherState Object**: Responsible for defining the key (`k`), nonce, and the AEAD algorithm (in our case, ChaCha20Poly1305).

2. **SymmetricState Object**: Manages the chaining key (`ck`), handshake hash (`h`), and includes the cipher state.

3. **Handshake Object**: Contains the SymmetricState, along with `s`, `rs`, `e`, and `re` (static and ephemeral key pairs).
However, in our implementation, the Initiator and Responder structs are combined with the Handshake Object, and the SymmetricState Object is referred to as HandshakeOps. Should we consider changing this to align more closely with the Noise protocol, using the correct names? This would make it less confusing for others reviewing our code alongside the Noise framework specification.

Personally I find HandshakeOp a better name for the trait that define the handshake operations. Which names do you want to change and with what?

Shourya742 · 2024-09-16T15:13:45Z

I would personally prefer the implementation to align more closely with the Noise NX protocol. We already have the CipherState, but I would also introduce a SymmetricState object, removing the handshakeOps. The SymmetricState will handle key derivation and encryption for the handshake. I would also create a HandshakeState object that contains the client and server keys along with the SymmetricState. I plan to redefine the Initiator and Responder structs to incorporate the final cipher keys, allowing them to encode and decode messages after the handshake is completed. While the current implementation functions well, I think if we all agree, we can move forward with this change to bring it closer to Noise NX. However, we also have the option to stick with the current implementation.
cc: @Fi3 @rrybarczyk

Fi3 · 2024-09-16T15:16:24Z

I would personally prefer the implementation to align more closely with the Noise NX protocol. We already have the CipherState, but I would also introduce a SymmetricState object, removing the handshakeOps. The SymmetricState will handle key derivation and encryption for the handshake. I would also create a HandshakeState object that contains the client and server keys along with the SymmetricState. I plan to redefine the Initiator and Responder structs to incorporate the final cipher keys, allowing them to encode and decode messages after the handshake is completed. While the current implementation functions well, I think if we all agree, we can move forward with this change to bring it closer to Noise NX. However, we also have the option to stick with the current implementation. cc: @Fi3 @rrybarczyk

nack for me is working well and I don't think that the code lack clarity.

rrybarczyk · 2024-09-16T22:02:31Z

I would personally prefer the implementation to align more closely with the Noise NX protocol. We already have the CipherState, but I would also introduce a SymmetricState object, removing the handshakeOps. The SymmetricState will handle key derivation and encryption for the handshake. I would also create a HandshakeState object that contains the client and server keys along with the SymmetricState. I plan to redefine the Initiator and Responder structs to incorporate the final cipher keys, allowing them to encode and decode messages after the handshake is completed. While the current implementation functions well, I think if we all agree, we can move forward with this change to bring it closer to Noise NX. However, we also have the option to stick with the current implementation. cc: @Fi3 @rrybarczyk

I think it is worth while to identify the difference between the Noise protocol specification and our current implementation. We should adhere as close to the specification as possible as this is a very important crate.

@Shourya742 continue to identify the differences, then we can discuss these areas for potential refactor.

plebhash · 2024-09-17T19:26:32Z

I think NoiseCodec is a bad name, because it implies encoding and decoding, while the actual idea is encrypting and decrypting.

stratum/protocols/v2/noise-sv2/src/lib.rs

Lines 22 to 25 in ab0ddd6

    
           pub struct NoiseCodec { 
        
               encryptor: GenericCipher, 
        
               decryptor: GenericCipher, 
        
           }

edit:

after we drop support for AES-GCM cipher, we will be able to further simplify things because enum GenericCipher will not be needed anymore.

So I would suggest something like this:

-pub struct NoiseCodec { 
+pub struct NoiseEngine { 
-    encryptor: GenericCipher, 
+    encryptor: Cipher<ChaCha20Poly1305>, 
-    decryptor: GenericCipher, 
+    decryptor: Cipher<ChaCha20Poly1305>,
}

plebhash · 2024-09-18T21:10:15Z

I would personally prefer the implementation to align more closely with the Noise NX protocol. We already have the CipherState, but I would also introduce a SymmetricState object, removing the handshakeOps. The SymmetricState will handle key derivation and encryption for the handshake. I would also create a HandshakeState object that contains the client and server keys along with the SymmetricState. I plan to redefine the Initiator and Responder structs to incorporate the final cipher keys, allowing them to encode and decode messages after the handshake is completed. While the current implementation functions well, I think if we all agree, we can move forward with this change to bring it closer to Noise NX. However, we also have the option to stick with the current implementation. cc: @Fi3 @rrybarczyk

I think it is worth while to identify the difference between the Noise protocol specification and our current implementation. We should adhere as close to the specification as possible as this is a very important crate.

@Shourya742 continue to identify the differences, then we can discuss these areas for potential refactor.

based on my learnings around codec_sv2 + framing_sv2, I would also agree with @Shourya742 and @rrybarczyk

HandshakeOps sounds like a convenience that would be better fit for codec_sv2 and framing_sv2 (which operate on a higher level of abstraction)... and we already have something like this there, for example:

codec_sv2::State::HandShake(HandshakeRole)
framing_sv2::framing::HandShakeFrame

for noise_sv2, I would prefer if this crate follows the spec as closely as possible, which would mean the introduction of SymmetricState and HandshakeState into noise_sv2

then this complexity can be abstracted away via codec_sv2 and framing_sv2

plebhash · 2024-09-18T21:39:28Z

replying to 3 comments by @rrybarczyk

#1130 (comment)

Error variants that I cannot find uses of in the entire repo:

CipherListMustBeNonEmpty

UnsupportedCiphers

InvalidCipherList

InvalidCipherChosed (see related comment)

InvalidCipherState

#1130 (comment)

In error::Error, what is the difference between the UnsupportedCiphers(Vec<u8>) and theInvalidCipherChosed(Vec<u8>)? If there is no difference, we should remove InvalidCipherChosed(Vec<u8>) in support of UnsupportedCiphers(Vec<u8>).

Otherwise, if both are needed, we need to fix the grammatical error: InvalidCipherChosed(Vec<u8>) -> InvalidCipherChosen(Vec<u8>).

#1130 (comment)

In the error module, we should not import use aes_gcm::Error as AesGcm;. Instead we should define the variant as AesGcm(aes_gcm::Error). Also the impl From will need to updated with this change.

if we remember we will drop support for AES-GCM cipher, all these errors will be greatly simplified as a result of only one cipher being supported

Fi3 · 2024-09-19T06:55:45Z

I would personally prefer the implementation to align more closely with the Noise NX protocol. We already have the CipherState, but I would also introduce a SymmetricState object, removing the handshakeOps. The SymmetricState will handle key derivation and encryption for the handshake. I would also create a HandshakeState object that contains the client and server keys along with the SymmetricState. I plan to redefine the Initiator and Responder structs to incorporate the final cipher keys, allowing them to encode and decode messages after the handshake is completed. While the current implementation functions well, I think if we all agree, we can move forward with this change to bring it closer to Noise NX. However, we also have the option to stick with the current implementation. cc: @Fi3 @rrybarczyk

I think it is worth while to identify the difference between the Noise protocol specification and our current implementation. We should adhere as close to the specification as possible as this is a very important crate.
@Shourya742 continue to identify the differences, then we can discuss these areas for potential refactor.

based on my learnings around codec_sv2 while I review #1040, I would also agree with @Shourya742 and @rrybarczyk

HandshakeOps sounds like a convenience that would be better fit for codec_sv2 (which operates on a higher level of abstraction)... and we already have something like this there, which is codec_sv2::State::HandShake(HandshakeRole)

for noise_sv2, I would prefer if this crate follows the spec as closely as possible, which would mean the introduction of SymmetricState and HandshakeState into noise_sv2, and then this complexity can be abstracted away via codec_sv2::State::HandShake(HandshakeRole)

Handshake is part of Noise. The crate already "follow" the spec as close as possible since it implement them. HandshakeOp and CipherState are traits if you just want to change the trait name is ok with me. But before refactoring noise keep in mind that:

is easy to do mistakes
this implementation have been extensively tested with unit tests and against other implementation
this implementation have been benchmarked and it is pretty fast (I can't find anymore the benches, but it was significantly faster the the old one)

Fi3 · 2024-09-19T06:58:45Z

I think NoiseCodec is a bad name, because it implies encoding and decoding, while the actual idea is encrypting and decrypting.

stratum/protocols/v2/noise-sv2/src/lib.rs

Lines 22 to 25 in ab0ddd6

pub struct NoiseCodec {

encryptor: GenericCipher,

decryptor: GenericCipher,

}

edit:

after we drop support for AES-GCM cipher, we will be able to further simplify things because enum GenericCipher will not be needed anymore.

So I would suggest something like this:
-pub struct NoiseCodec { 
+pub struct NoiseEngine { 
-    encryptor: GenericCipher, 
+    encryptor: Cipher<ChaCha20Poly1305>, 
-    decryptor: GenericCipher, 
+    decryptor: Cipher<ChaCha20Poly1305>,
} 

for the reasons that I enumerated in my prev message I would be very careful here. Agree that NoiseCodec is bad name

plebhash · 2024-09-24T21:47:18Z

the AeadCipher trait is currently defined under a private module, which means only code inside noise_sv2 crate is able to implement it

and it seems it only exists under the assumption that more than one single cipher is supported

but once AES-GCM support is dropped, we will only have one single struct (ChaCha20Poly1305) implementing the AeadCipher trait

so we should probably eliminate the AeadCipher trait

if we choose to keep the AeadCipher trait after we drop AES-GCM, we should probably make the aed_cipher module public... this way, a consumer of noise_sv2 crate could choose to implement their own cipher

however I'm not sure how realistic this use-case is

plebhash · 2024-09-24T21:48:02Z

aed_cipher.rs seems to be a typo, this file/module should be called aead_cipher.rs

Fi3 · 2024-09-25T06:53:09Z

the AeadCipher trait is currently defined under a private module, which means only code inside noise_sv2 crate is able to implement it

and it seems it only exists under the assumption that more than one single cipher is supported

but once AES-GCM support is dropped, we will only have one single struct (ChaCha20Poly1305) implementing the AeadCipher trait

so we should probably eliminate the AeadCipher trait

if we choose to keep the AeadCipher trait after we drop AES-GCM, we should probably make the aed_cipher module public... this way, a consumer of noise_sv2 crate could choose to implement their own cipher

however I'm not sure how realistic this use-case is

maybe a future version of sv2? I'm not super happy to change code in this crate since is very hard to debug (and it works and it is fast), also leave it like it is is not really bad, probably would be better without the trait, but we still have a clean interface with the trait.

plebhash · 2024-09-26T16:15:27Z

this comment will probably be erased by #1111 but it is an indication that this function should probably be private, not public

stratum/protocols/v2/noise-sv2/src/cipher_state.rs

Lines 196 to 197 in 060131e

    
               /// Internal use only, we need k for handshake 
        
               pub fn from_key_and_cipher(k: [u8; 32], c: C) -> Self {

see #916 (comment) for more context

Shourya742 · 2024-10-07T12:58:23Z

Change VERSION to CERTIFICATE_FORMAT_VERSION in Responder.rs. See #1111 (comment) for more context

plebhash · 2024-10-11T21:51:24Z

we should evaluate visibility of all elements of the crate

jbesraa · 2024-10-14T13:40:26Z

#1111 (comment)

Shourya742 · 2024-10-14T13:51:32Z

#1111 (comment)

jbesraa · 2024-10-14T14:00:01Z

Interestingly, there is a src/test.rs file in the crate(https://github.com/stratum-mining/stratum/blob/main/protocols/v2/noise-sv2/src/test.rs). We should move the test to lib.rs and remove the extra file

rrybarczyk · 2024-10-15T15:35:56Z

Interestingly, there is a src/test.rs file in the crate(https://github.com/stratum-mining/stratum/blob/main/protocols/v2/noise-sv2/src/test.rs). We should move the test to lib.rs and remove the extra file

I posed a question somewhere in this PR's conversation suggesting we remove test.rs completely as we have the examples now. We just need to consider: do we want to remove it in this PR, or in the technical debt issue?

cc: @plebhash, @GitGab19

GitGab19 · 2024-10-16T09:09:55Z

Interestingly, there is a src/test.rs file in the crate(https://github.com/stratum-mining/stratum/blob/main/protocols/v2/noise-sv2/src/test.rs). We should move the test to lib.rs and remove the extra file

I posed a question somewhere in this PR's conversation suggesting we remove test.rs completely as we have the examples now. We just need to consider: do we want to remove it in this PR, or in the technical debt issue?

cc: @plebhash, @GitGab19

I would remove it in the refactoring PRs, for now we should take note of that in the specific issue

rrybarczyk · 2024-10-16T18:34:46Z

Interestingly, there is a src/test.rs file in the crate(https://github.com/stratum-mining/stratum/blob/main/protocols/v2/noise-sv2/src/test.rs). We should move the test to lib.rs and remove the extra file

I posed a question somewhere in this PR's conversation suggesting we remove test.rs completely as we have the examples now. We just need to consider: do we want to remove it in this PR, or in the technical debt issue?
cc: @plebhash, @GitGab19

I would remove it in the refactoring PRs, for now we should take note of that in the specific issue

Agree.

jbesraa · 2024-10-18T06:34:30Z

Why remove it entirely and not having tests in lib.rs ? Examples should provide a direction for library users on how to use it, while tests should guarantee the library is correctly implemented

rrybarczyk · 2024-10-18T18:00:18Z

I think we should consider implementing noise_sv2 in a way that does not require std, such that it can be used on various OSs (like for firmware).

plebhash · 2024-10-25T13:07:01Z

I think we should consider implementing noise_sv2 in a way that does not require std, such that it can be used on various OSs (like for firmware).

that is a very good catch!

adding some extra context that will be relevant when we tackle this:

most protocols crates already have support for no_std (although we should probably make a general assessment to check if there's any other crate that also requires this change)

for the crates that already support no_std, it originally was mandatory (meaning that every build would force no_std)

but then we noticed that was becoming a blocker for #985 (comment) so we made no_std into something to be opt-in (as a no_std cargo feature flag) via #932

now we have #1230 that is proposing to invert this logic, making the opt-in feature flag to be std

Georges760 · 2024-10-25T19:27:19Z

I didn't had the historical point of view. Thanks to explaining it to me.

#1230 is not proposing to have std by default, just to change the logic polarity, keeping the actual default as it is.

I run into lot of difficulties becasue many crate, even if having a no_std feature doesn't enforce the no_std nature for the crate by adding

#![cfg_attr(feature = "no_std", no_std)]

For instance :

const_sv2: have feature and well enforced : OK
buffer_sv2 : no explicit feature commented out
binary_codec_sv2 : have feature not enforced
derive_codec_sv2 : no feature not enforced
serde_sv2: have feature and well enforced : OK
binary_sv2: have a core feature and not enforcement
framing_sv2: have feature and well enforced : OK
codec_sv2 : have feature and well enforced : OK
and of course noise_sv2: no feature / no enforcement, which is the subject of this issue...

So before doing noise_sv2, I wanted to propose an uniform way of handling no_std is all these crates above.

rrybarczyk added refactor Implies refactoring code protocols Lowest level protocol logic noise-sv2 labels Aug 21, 2024

rrybarczyk assigned Shourya742 Aug 21, 2024

rrybarczyk changed the title ~~Refactor: noise-sv2~~ Refactor: noise-sv2 crate Aug 21, 2024

rrybarczyk mentioned this issue Aug 21, 2024

Rust Docs + refactor: protocols crates #845

Open

78 tasks

rrybarczyk mentioned this issue Aug 27, 2024

Rust docs noise_sv2 #1111

Merged

rrybarczyk mentioned this issue Sep 12, 2024

Rust docs for protocols::v2::codec-sv2 #1040

Merged

Shourya742 mentioned this issue Oct 21, 2024

doc binary-sv2 #1231

Merged

This was referenced Oct 26, 2024

const_sv2: remove unused dep (secp256k1) #1237

Merged

enforce no_std for all? protocol/v2 crates #1230

Merged

Propose an implementation of noise_sv2 with optional no_std #1238

Open

Refactor: noise-sv2 crate #1130

Refactor: noise-sv2 crate #1130

Comments

rrybarczyk commented Aug 21, 2024 • edited Loading

Background

Identified Potential Code Debt

Shourya742 commented Aug 22, 2024 • edited Loading

Fi3 commented Aug 22, 2024 • edited Loading

Shourya742 commented Aug 23, 2024

Shourya742 commented Aug 23, 2024

rrybarczyk commented Aug 27, 2024 • edited Loading

Shourya742 commented Aug 27, 2024

plebhash commented Aug 29, 2024

Shourya742 commented Sep 2, 2024

Shourya742 commented Sep 2, 2024

rrybarczyk commented Sep 12, 2024

rrybarczyk commented Sep 12, 2024

rrybarczyk commented Sep 12, 2024

rrybarczyk commented Sep 12, 2024 • edited Loading

rrybarczyk commented Sep 12, 2024

rrybarczyk commented Sep 12, 2024 • edited Loading

rrybarczyk commented Sep 12, 2024

Fi3 commented Sep 13, 2024

Fi3 commented Sep 13, 2024

Fi3 commented Sep 13, 2024

jbesraa commented Sep 13, 2024

jbesraa commented Sep 13, 2024

plebhash commented Sep 13, 2024

Shourya742 commented Sep 14, 2024

Consistent Implementation and Naming that Adheres to the Noise Protocol Specification

Fi3 commented Sep 16, 2024

Consistent Implementation and Naming that Adheres to the Noise Protocol Specification

Shourya742 commented Sep 16, 2024

Fi3 commented Sep 16, 2024

rrybarczyk commented Sep 16, 2024 • edited Loading

plebhash commented Sep 17, 2024 • edited Loading

plebhash commented Sep 18, 2024 • edited Loading

plebhash commented Sep 18, 2024 • edited Loading

Fi3 commented Sep 19, 2024

Fi3 commented Sep 19, 2024 • edited Loading

plebhash commented Sep 24, 2024 • edited Loading

plebhash commented Sep 24, 2024

Fi3 commented Sep 25, 2024 • edited Loading

plebhash commented Sep 26, 2024 • edited Loading

Shourya742 commented Oct 7, 2024

plebhash commented Oct 11, 2024

jbesraa commented Oct 14, 2024

Shourya742 commented Oct 14, 2024

jbesraa commented Oct 14, 2024

rrybarczyk commented Oct 15, 2024

GitGab19 commented Oct 16, 2024

rrybarczyk commented Oct 16, 2024

jbesraa commented Oct 18, 2024

rrybarczyk commented Oct 18, 2024

plebhash commented Oct 25, 2024 • edited Loading

Georges760 commented Oct 25, 2024

Refactor: `noise-sv2` crate #1130

Refactor: `noise-sv2` crate #1130

rrybarczyk commented Aug 21, 2024 •

edited

Loading

Shourya742 commented Aug 22, 2024 •

edited

Loading

Fi3 commented Aug 22, 2024 •

edited

Loading

rrybarczyk commented Aug 27, 2024 •

edited

Loading

rrybarczyk commented Sep 12, 2024 •

edited

Loading

rrybarczyk commented Sep 12, 2024 •

edited

Loading

rrybarczyk commented Sep 16, 2024 •

edited

Loading

plebhash commented Sep 17, 2024 •

edited

Loading

plebhash commented Sep 18, 2024 •

edited

Loading

plebhash commented Sep 18, 2024 •

edited

Loading

Fi3 commented Sep 19, 2024 •

edited

Loading

plebhash commented Sep 24, 2024 •

edited

Loading

Fi3 commented Sep 25, 2024 •

edited

Loading

plebhash commented Sep 26, 2024 •

edited

Loading

plebhash commented Oct 25, 2024 •

edited

Loading