Refactor: noise-sv2 crate

[rrybarczyk]

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.
• TODO

[Shourya742]

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]

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]

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]

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]

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]

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]

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

[Shourya742]

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

[Shourya742]

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

[rrybarczyk]

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

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

[rrybarczyk]

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

[rrybarczyk]

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]

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]

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]

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]

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

• CipherListMustBeNonEmpty
• UnsupportedCiphers
• InvalidCipherList
• InvalidCipherChosed (see related comment)
• InvalidCipherState

[Fi3]

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]

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]

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]

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]

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]

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.

[Fi3]

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.

agree that using expect rather then unwrap is always better. Completely disagree with adding error for things that can not fail, otherwise we should do that anytime that we allocate (allocate can actually fail code above not) or when we index a vector. There are obvious cases where adding errors is only a price that do not make sense to pay.

[rrybarczyk]

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?

Because when I open a module, I want to immediately see everything that is being imported at the top. I think it is more readable, compared to scrolling through the code and then all of a sudden seeing there are more data types being imported further down. I believe it is much cleaner with all imports listed immediately at the top.

[rrybarczyk]

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.

[Shourya742]

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.

[Fi3]

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]

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]

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]

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]

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]

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]

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]

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:

1. is easy to do mistakes
2. this implementation have been extensively tested with unit tests and against other implementation
3. 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]

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