Noise-sv2 hardening - #706
Conversation
GitGab19
left a comment
There was a problem hiding this comment.
Clanker review:
Review findings
try_write_frame() can desynchronize the Noise transport
try_write_frame() encrypts the frame before calling try_write().
Encryption advances the Noise nonce. If the socket returns WouldBlock, the method returns Ok(false) but discards both the ciphertext and the consumed frame. The next frame then uses a later nonce while the peer is still expecting the discarded one, permanently desynchronizing the connection.
Partial writes have a similar problem: a ciphertext prefix may reach the socket while the remainder is discarded.
This behavior predates this PR, but it is directly relevant to the transport-state hardening. The write half should retain pending ciphertext and its write offset until the complete encrypted frame has been sent.
Add direct coverage for the split transport transition
The migration to separate TransportEncryptState and TransportDecryptState appears correct, but there is no focused test covering it.
A bidirectional client/server test should complete the handshake and exchange multiple frames concurrently. This would protect against swapped transport states, incorrect nonce progression, and regressions in future refactors.
For first, as its predates this PR, I would open an issue for that and for second one, I might wanna change the current network helpers setup a bit once the changes to codec-sv2, maybe then I will add the test. |
companion stratum-mining/stratum#2283