Skip to content

Conversation

@0xKarl98
Copy link
Contributor

This one resolves issue #3 , cc @tcoratger @b-wagn

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we removed this here ea9fc1d so I don't think this is a good idea to put this back

Copy link
Contributor

@tcoratger tcoratger left a comment

Choose a reason for hiding this comment

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

I think that the goal of this was just to reactivate

/*#[test]
pub fn test_large_base_sha() {
// Note: do not use these parameters, they are just for testing
type PRF = ShaPRF<24, 8>;
type TH = ShaTweak192192;
// use chunk size 8
type MH = ShaMessageHash<24, 8, 32, 8>;
const TARGET_SUM: usize = 1 << 12;
type IE = TargetSumEncoding<MH, TARGET_SUM>;
const LOG_LIFETIME: usize = 10;
type Sig = GeneralizedXMSSSignatureScheme<PRF, IE, TH, LOG_LIFETIME>;
Sig::internal_consistency_check();
test_signature_scheme_correctness::<Sig>(0, 0, Sig::LIFETIME as usize);
test_signature_scheme_correctness::<Sig>(11, 0, Sig::LIFETIME as usize);
}
#[test]
pub fn test_large_dimension_sha() {
// Note: do not use these parameters, they are just for testing
type PRF = ShaPRF<24, 8>;
type TH = ShaTweak192192;
// use 256 chunks
type MH = ShaMessageHash<24, 8, 256, 1>;
const TARGET_SUM: usize = 128;
type IE = TargetSumEncoding<MH, TARGET_SUM>;
const LOG_LIFETIME: usize = 10;
type Sig = GeneralizedXMSSSignatureScheme<PRF, IE, TH, LOG_LIFETIME>;
Sig::internal_consistency_check();
test_signature_scheme_correctness::<Sig>(2, 0, Sig::LIFETIME as usize);
test_signature_scheme_correctness::<Sig>(19, 0, Sig::LIFETIME as usize);
}*/
with Poseidon instead of SHA no?

I'm not sure about the content of this PR, which seems to have deviated significantly from this goal.

@0xKarl98
Copy link
Contributor Author

0xKarl98 commented Nov 27, 2025

pub fn test_large_base_sha() {

Yeah I have removed this commented-out SHA part and just keep the poseidon

Comment on lines 46 to 153

#[cfg(test)]
/// Splits a list of bytes into smaller fixed-size bit chunks.
///
/// Each byte in the input slice is divided into `chunk_size`-bit chunks,
/// starting from the least significant bits. The `chunk_size` must divide 8 exactly
/// (i.e., valid values are 1, 2, 4, or 8), since each byte contains 8 bits.
#[must_use]
#[inline]
pub fn bytes_to_chunks(bytes: &[u8], chunk_size: usize) -> Vec<u8> {
// Only the chunk sizes 1, 2, 4, or 8 are valid.
assert!(
matches!(chunk_size, 1 | 2 | 4 | 8),
"chunk_size must be 1, 2, 4, or 8"
);

// Calculate how many chunks each byte will produce and preallocate exactly.
let chunks_per_byte = 8 / chunk_size;
let mut out = Vec::with_capacity(bytes.len() * chunks_per_byte);

match chunk_size {
8 => {
out.extend_from_slice(bytes);
}
4 => {
for &b in bytes {
out.push(b & 0x0F);
out.push(b >> 4);
}
}
2 => {
for &b in bytes {
out.push(b & 0b11);
out.push((b >> 2) & 0b11);
out.push((b >> 4) & 0b11);
out.push((b >> 6) & 0b11);
}
}
1 => {
for &b in bytes {
out.push(b & 1);
out.push((b >> 1) & 1);
out.push((b >> 2) & 1);
out.push((b >> 3) & 1);
out.push((b >> 4) & 1);
out.push((b >> 5) & 1);
out.push((b >> 6) & 1);
out.push((b >> 7) & 1);
}
}
_ => unreachable!(),
}

out
}

#[cfg(test)]
mod tests {
use super::bytes_to_chunks;
use proptest::prelude::*;

#[test]
fn test_bytes_to_chunks() {
let byte_a: u8 = 0b0110_1100;
let byte_b: u8 = 0b1010_0110;

let bytes = [byte_a, byte_b];
let expected_chunks = [0b00, 0b11, 0b10, 0b01, 0b10, 0b01, 0b10, 0b10];

let chunks = bytes_to_chunks(&bytes, 2);

assert_eq!(chunks.len(), 8);

for i in 0..chunks.len() {
assert_eq!(chunks[i], expected_chunks[i]);
}

// now test chunk size 8
let chunks = bytes_to_chunks(&bytes, 8);

assert_eq!(chunks.len(), 2);
assert_eq!(chunks[0], byte_a);
assert_eq!(chunks[1], byte_b);
}

proptest! {
#[test]
fn prop_bytes_to_chunks_matches_manual_bit_extraction(
bytes in proptest::collection::vec(any::<u8>(), 0..32),
chunk_size in prop_oneof![Just(1usize), Just(2), Just(4), Just(8)],
) {
let chunks = bytes_to_chunks(&bytes, chunk_size);

let chunks_per_byte = 8 / chunk_size;
let mut expected = Vec::with_capacity(bytes.len() * chunks_per_byte);

for &b in &bytes {
for i in 0..chunks_per_byte {
let shifted = b >> (i * chunk_size);
let mask = if chunk_size == 8 { 0xFF } else { (1u8 << chunk_size) - 1 };
expected.push(shifted & mask);
}
}

prop_assert_eq!(chunks.as_slice(), expected.as_slice());
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we need all this neither no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i was thought too much , have removed them

Comment on lines 707 to 708
test_signature_scheme_correctness::<Sig>(19, 0, Sig::LIFETIME as usize);
}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the old commented tests

const TARGET_SUM: usize = 128;
type IE = TargetSumEncoding<MH, TARGET_SUM>;
const LOG_LIFETIME: usize = 10;
const LOG_LIFETIME: usize = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use this as before?

Suggested change
const LOG_LIFETIME: usize = 6;
const LOG_LIFETIME: usize = 10;

const TARGET_SUM: usize = 8 * (256 - 1) / 2;
type IE = TargetSumEncoding<MH, TARGET_SUM>;
const LOG_LIFETIME: usize = 10;
const LOG_LIFETIME: usize = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use this?

Suggested change
const LOG_LIFETIME: usize = 6;
const LOG_LIFETIME: usize = 10;

// use chunk size 8
type MH = ShaMessageHash<24, 8, 32, 8>;
const TARGET_SUM: usize = 1 << 12;
type PRF = ShakePRFtoF<4, 4>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use this? We want to be as close as possible to the old test configuration I think

Suggested change
type PRF = ShakePRFtoF<4, 4>;
type PRF = ShakePRFtoF<24, 8>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood , will revert them back

@0xKarl98 0xKarl98 requested a review from tcoratger November 27, 2025 12:07
Comment on lines 608 to 632
fn compress_prf_output<const OUT_LEN: usize>(value: [F; 24]) -> FieldArray<OUT_LEN> {
assert!(
OUT_LEN <= 24,
"Poseidon compression output length must be <= 24"
);
let hash = crate::symmetric::tweak_hash::poseidon::poseidon_compress::<F, _, 24, OUT_LEN>(
&crate::poseidon2_24(),
value.as_slice(),
);
FieldArray(hash)
}

// Compress a wide PRF output down to the four-field-element domain used by PoseidonTweakHash.
impl From<[F; 24]> for FieldArray<4> {
fn from(value: [F; 24]) -> Self {
compress_prf_output::<4>(value)
}
}

// Compress a wide PRF output down to the eight-field-element domain used by PoseidonTweakHash.
impl From<[F; 24]> for FieldArray<8> {
fn from(value: [F; 24]) -> Self {
compress_prf_output::<8>(value)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this

type PRF = ShakePRFtoF<24, 8>;
type TH = PoseidonTweakHash<4, 4, 2, 8, 8>;
type MH = PoseidonMessageHash<4, 8, 2, 8, 256, 2, 9>;
const TARGET_SUM: usize = 8 * (256 - 1) / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we can't have

Suggested change
const TARGET_SUM: usize = 8 * (256 - 1) / 2;
const TARGET_SUM: usize = 1 << 12;

@0xKarl98 0xKarl98 requested a review from tcoratger November 27, 2025 14:30
Copy link
Contributor

@tcoratger tcoratger left a comment

Choose a reason for hiding this comment

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

Looks good, @b-wagn can you confirm this solves the issue you had in mind?

Copy link
Contributor

@b-wagn b-wagn left a comment

Choose a reason for hiding this comment

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

Yes, it does.

@b-wagn b-wagn merged commit adad238 into leanEthereum:main Dec 3, 2025
2 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.

3 participants