Skip to content

Conversation

@nstilt1
Copy link
Contributor

@nstilt1 nstilt1 commented Sep 4, 2025

  • remove unnecessary endian conversions
  • update docs
  • consolidate StreamId and BlockPos with type aliases
  • update counter_overflow_1 test to be more descriptive when the 64-bit counter is not implemented correctly for the 4th parblock - may be unnecessary, but it provides a better error message than the other test that would fail in the same scenario

fixes #453

@nstilt1
Copy link
Contributor Author

nstilt1 commented Sep 5, 2025

So I was able to consolidate the StreamId and BlockPos structs/impls with a U32x2 struct and a pair of type aliases.

One thing we could do is switch pub struct U32x2([u32; Self::LEN]); to pub struct U32x2([u32; 2]); or maybe even call the struct U32Array with a const generic argument usize for the length, and then make the type aliases specify 2 as the length. We wouldn't need to use this type anywhere else though, as it would likely be a little less secure to use this generic type for the seed.

Also, by using a more generic type for the U32Array, we would probably be limited to u32 arrays of length 1 or 2, unless we add another generic arg for the From<{unsigned integer type}> impl, which could be used to have up to a [u32; 4], but it would still not be enough for the seed either unless there was a way to only impl From<unsigned integer> when BYTES <= 16

@nstilt1 nstilt1 marked this pull request as ready for review September 5, 2025 15:33
@nstilt1 nstilt1 changed the title chacha20 fixes regarding #453 chacha20 - minor improvements Sep 5, 2025
@nstilt1
Copy link
Contributor Author

nstilt1 commented Sep 6, 2025

I added some better diagnostics in the event that a new backend will be developed. I mutated neon.rs to see what the failing tests look like, and here they are:

thread 'rng::tests::counter_overflow_and_diagnostics' panicked at chacha20/src/rng.rs:1242:13:
PARBLOCK #2 uses incorrect counter addition
Diagnostic = When the state was added to the results/res buffer at the end of fn rounds, the counter was probably incremented in 32-bit fashion for this parblock
num_incorrect_bytes = 1
index_of_first_incorrect_word = Some(13)

and

thread 'rng::tests::counter_overflow_and_diagnostics' panicked at chacha20/src/rng.rs:1242:13:
PARBLOCK #4 uses incorrect counter addition
Diagnostic = Most of the block was incorrect, indicating an issue with the counter using 32-bit addition towards the beginning of fn rounds()
num_incorrect_bytes = 64
index_of_first_incorrect_word = Some(0)

and if PARBLOCK number 1 is off, it says that ChaCha was implemented incorrectly, suggesting to look in fn rounds or the functions it calls. I know this probably isn't necessary, but if anyone tweaks with the backends again, they'll at least get some diagnostic error messages about the counter

Copy link
Contributor

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Test may be a little over-complex (that it tests counter overflow twice or fills output in two steps isn't clearly signposted; block_pos constant isn't needed). Still acceptable.

/// The arrays should be in little endian order. You should not need to use
/// this directly, as the methods in this crate that use this type call
/// `.into()` for you, so you only need to supply any of the above types.
pub struct U32x2([u32; Self::LEN]);
Copy link
Member

@tarcieri tarcieri Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a bit of an odd name with something with a specific purpose that's only used in a single part of the API, particularly since it's public. I'll admit "stream ID" also seemed like an odd name for a nonce to me, but seemed OK in the context of the RNG API, particularly given the legacy of rand_chacha.

I guess this is because it now has a dual role as both a nonce and a block position identifier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; this was discussed in #453. No comments on the name.

@tarcieri tarcieri merged commit 8f63e23 into RustCrypto:master Sep 8, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chacha set_stream issues

3 participants