Skip to content

Allow wnaf to represent dense values #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kilic wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow wnaf to represent dense values #46

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/wnaf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,12 @@ pub(crate) fn wnaf_form<S: AsRef<[u8]>>(wnaf: &mut Vec<i64>, c: S, window: usize
// Required by the NAF definition
debug_assert!(window >= 2);
// Required so that the NAF digits fit in i64
debug_assert!(window <= 64);
debug_assert!(window < 64);
Copy link
Member

@str4d str4d Jul 29, 2023

Choose a reason for hiding this comment

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

This seems like a separate issue, and fixing it in this PR is confusing. Rather than reducing the allowed values of window, we should fix the bug by making width a u128, and adjusting the rest of the code accordingly.


let bit_len = c.as_ref().len() * 8;

wnaf.truncate(0);
wnaf.reserve(bit_len);
wnaf.reserve(bit_len + 1);

// Initialise the current and next limb buffers.
let mut limbs = LimbBuffer::new(c.as_ref());
Expand All @@ -109,7 +109,7 @@ pub(crate) fn wnaf_form<S: AsRef<[u8]>>(wnaf: &mut Vec<i64>, c: S, window: usize

let mut pos = 0;
let mut carry = 0;
while pos < bit_len {
while pos <= bit_len {
// Construct a buffer of bits of the scalar, starting at bit `pos`
let u64_idx = pos / 64;
let bit_idx = pos % 64;
Expand Down Expand Up @@ -144,6 +144,7 @@ pub(crate) fn wnaf_form<S: AsRef<[u8]>>(wnaf: &mut Vec<i64>, c: S, window: usize
pos += window;
}
}
wnaf.truncate(wnaf.len().saturating_sub(window - 1));
Copy link
Member

@str4d str4d Jul 29, 2023

Choose a reason for hiding this comment

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

The purpose of this line is unclear, and it could accidentally lead to unintended truncation of meaningful limbs if a future refactor altered how the earlier logic works. Document this line.

}

/// Performs w-NAF exponentiation with the provided window table and w-NAF form scalar.
Expand Down Expand Up @@ -504,3 +505,28 @@ impl<G: Group, const WINDOW_SIZE: usize> Mul<&WnafScalar<G::Scalar, WINDOW_SIZE>
wnaf_exp(&self.table, &rhs.wnaf)
}
}

#[test]
fn test_wnaf_form() {
fn from_wnaf(wnaf: &Vec<i64>) -> u128 {
wnaf.iter().rev().fold(0, |acc, next| {
let mut acc = acc * 2;
acc += *next as u128;
Comment on lines +513 to +514
Copy link
Member

@str4d str4d Jul 29, 2023

Choose a reason for hiding this comment

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

These lines panic in debug mode, which forbids integer overflow. I've replaced them with explicit wrapping operations, and added documentation to explain why it works.

Suggested change
let mut acc = acc * 2;
acc += *next as u128;
// If one of the least-significant w-NAF limbs is negative, `acc` may be large
// (due to the result being represented as a `u128`). `wrapping_mul` wraps at
// the boundary of the type; in this case, it has the effect of doubling the
// equivalent signed value:
// acc = -x = u128::MAX + 1 - x
// 2 * acc = 2 * (u128::MAX + 1 - x)
// = 2 * (u128::MAX + 1) - 2x
// = -2x as u128
let acc = acc.wrapping_mul(2);
// Rust signed-to-unsigned casts add or subtract `T::MAX + 1` until the value
// fits into the new type. A wrapping addition of the new type is therefore
// equivalent to a wrapping subtraction of the magnitude of the original type.
acc.wrapping_add(*next as u128)

Copy link
Contributor

@tarcieri tarcieri Jul 29, 2023

Choose a reason for hiding this comment

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

@str4d sidebar: you might consider adding the clippy::arithmetic_side_effects lint

acc
})
}
for w in 2..64 {
for e in 0..=u16::MAX {
let mut wnaf = vec![];
wnaf_form(&mut wnaf, e.to_le_bytes(), w);
assert_eq!(e as u128, from_wnaf(&wnaf));
}
}
for w in 2..64 {
for e in u128::MAX - 10000..=u128::MAX {
let mut wnaf = vec![];
Comment on lines +517 to +527
Copy link
Member

@str4d str4d Jul 29, 2023

Choose a reason for hiding this comment

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

wnaf_form clears the given vector before using it, so this is faster (and also tests that clearing logic as a side-effect):

Suggested change
}
for w in 2..64 {
for e in 0..=u16::MAX {
let mut wnaf = vec![];
wnaf_form(&mut wnaf, e.to_le_bytes(), w);
assert_eq!(e as u128, from_wnaf(&wnaf));
}
}
for w in 2..64 {
for e in u128::MAX - 10000..=u128::MAX {
let mut wnaf = vec![];
}
let mut wnaf = Vec::with_capacity(129);
for w in 2..64 {
for e in 0..=u16::MAX {
wnaf_form(&mut wnaf, e.to_le_bytes(), w);
assert_eq!(e as u128, from_wnaf(&wnaf));
}
}
for w in 2..64 {
for e in u128::MAX - 10000..=u128::MAX {

wnaf_form(&mut wnaf, e.to_le_bytes(), w);
assert_eq!(e, from_wnaf(&wnaf));
}
}
}