feat: Add Brainpool and NIST elliptic curve support for PACE (issue #11)#12
Conversation
📝 WalkthroughWalkthroughECDH-PACE expands from P-256-only handling to NIST P-384/P-521 and Brainpool curves. Curve dependencies, domain-parameter support, macro-generated engines, enum dispatch, and ChangesECDH-PACE curve expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PaceSession
participant ECDHPace
participant CurveEngine
participant PublicKeyPace
PaceSession->>ECDHPace: map_and_generate_ephemeral(...)
ECDHPace->>CurveEngine: map public key and generate ephemeral key
CurveEngine-->>PaceSession: ephemeral public key
PaceSession->>ECDHPace: get_ephemeral_shared_seed(...)
ECDHPace->>PublicKeyPace: decode peer ephemeral key
CurveEngine-->>ECDHPace: derive shared seed
ECDHPace-->>PaceSession: shared seed bytes
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request expands the ECDH PACE engine to support multiple NIST and Brainpool curves (including NIST P-384, NIST P-521, Brainpool P256, and Brainpool P384) by refactoring the implementation into a macro-based architecture. Feedback focuses on optimizing performance within the generated macro code, specifically by avoiding redundant heap allocations (such as moving Vec allocations out of loops and using stack-allocated arrays for coordinate buffers) and caching the parsed curve order using OnceLock instead of parsing it on every scalar reduction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn scalar_from_bytes(&self, bytes: &[u8]) -> $scalar { | ||
| let n = BigUint::parse_bytes($order_str, 10).unwrap(); | ||
| let val = BigUint::from_bytes_be(bytes); | ||
| let reduced = val % &n; | ||
| let r_bytes = reduced.to_bytes_be(); | ||
| let mut buf = vec![0u8; $coord_len]; | ||
| if r_bytes.len() <= $coord_len { | ||
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | ||
| } else { | ||
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | ||
| } | ||
| let field_bytes = *<$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::from_slice(&buf); | ||
| <$scalar as $curve_crate::elliptic_curve::ff::PrimeField>::from_repr(field_bytes).unwrap() | ||
| } |
There was a problem hiding this comment.
Parsing the decimal string $order_str into a BigUint on every invocation of scalar_from_bytes is highly inefficient and causes unnecessary heap allocations. Since the curve order is constant, we can parse it once using std::sync::OnceLock. Additionally, allocating buf as a Vec on the heap is unnecessary since $coord_len is a constant expression; we can use a stack-allocated array instead.
| fn scalar_from_bytes(&self, bytes: &[u8]) -> $scalar { | |
| let n = BigUint::parse_bytes($order_str, 10).unwrap(); | |
| let val = BigUint::from_bytes_be(bytes); | |
| let reduced = val % &n; | |
| let r_bytes = reduced.to_bytes_be(); | |
| let mut buf = vec![0u8; $coord_len]; | |
| if r_bytes.len() <= $coord_len { | |
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | |
| } | |
| let field_bytes = *<$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::from_slice(&buf); | |
| <$scalar as $curve_crate::elliptic_curve::ff::PrimeField>::from_repr(field_bytes).unwrap() | |
| } | |
| fn scalar_from_bytes(&self, bytes: &[u8]) -> $scalar { | |
| static N: std::sync::OnceLock<BigUint> = std::sync::OnceLock::new(); | |
| let n = N.get_or_init(|| BigUint::parse_bytes($order_str, 10).unwrap()); | |
| let val = BigUint::from_bytes_be(bytes); | |
| let reduced = val % n; | |
| let r_bytes = reduced.to_bytes_be(); | |
| let mut buf = [0u8; $coord_len]; | |
| if r_bytes.len() <= $coord_len { | |
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | |
| } | |
| let field_bytes = *<$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::from_slice(&buf); | |
| <$scalar as $curve_crate::elliptic_curve::ff::PrimeField>::from_repr(field_bytes).unwrap() | |
| } |
| fn scalar_from_bytes(&self, bytes: &[u8]) -> $scalar { | ||
| let n = BigUint::parse_bytes($order_str, 10).unwrap(); | ||
| let val = BigUint::from_bytes_be(bytes); | ||
| let reduced = val % &n; | ||
| let r_bytes = reduced.to_bytes_be(); | ||
| let mut buf = vec![0u8; $coord_len]; | ||
| if r_bytes.len() <= $coord_len { | ||
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | ||
| } else { | ||
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | ||
| } | ||
| let field_bytes = *<$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::from_slice(&buf); | ||
| <$scalar as $curve_crate::elliptic_curve::ff::PrimeField>::from_repr(field_bytes).unwrap() | ||
| } |
There was a problem hiding this comment.
Parsing the decimal string $order_str into a BigUint on every invocation of scalar_from_bytes is highly inefficient and causes unnecessary heap allocations. Since the curve order is constant, we can parse it once using std::sync::OnceLock. Additionally, allocating buf as a Vec on the heap is unnecessary since $coord_len is a constant expression; we can use a stack-allocated array instead.
| fn scalar_from_bytes(&self, bytes: &[u8]) -> $scalar { | |
| let n = BigUint::parse_bytes($order_str, 10).unwrap(); | |
| let val = BigUint::from_bytes_be(bytes); | |
| let reduced = val % &n; | |
| let r_bytes = reduced.to_bytes_be(); | |
| let mut buf = vec![0u8; $coord_len]; | |
| if r_bytes.len() <= $coord_len { | |
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | |
| } | |
| let field_bytes = *<$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::from_slice(&buf); | |
| <$scalar as $curve_crate::elliptic_curve::ff::PrimeField>::from_repr(field_bytes).unwrap() | |
| } | |
| fn scalar_from_bytes(&self, bytes: &[u8]) -> $scalar { | |
| static N: std::sync::OnceLock<BigUint> = std::sync::OnceLock::new(); | |
| let n = N.get_or_init(|| BigUint::parse_bytes($order_str, 10).unwrap()); | |
| let val = BigUint::from_bytes_be(bytes); | |
| let reduced = val % n; | |
| let r_bytes = reduced.to_bytes_be(); | |
| let mut buf = [0u8; $coord_len]; | |
| if r_bytes.len() <= $coord_len { | |
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | |
| } | |
| let field_bytes = *<$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::from_slice(&buf); | |
| <$scalar as $curve_crate::elliptic_curve::ff::PrimeField>::from_repr(field_bytes).unwrap() | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break sk; | ||
| } | ||
| } |
There was a problem hiding this comment.
Allocating bytes as a Vec inside the loop causes unnecessary heap allocations on every iteration. This is especially problematic for curves like P-521 where the loop can run multiple times on average. Moving the allocation outside the loop avoids this overhead.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } | |
| let mut bytes = vec![0u8; $coord_len]; | |
| loop { | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break self.scalar_from_bytes(&sk.to_bytes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Allocating bytes as a Vec inside the loop causes unnecessary heap allocations on every iteration. Moving the allocation outside the loop avoids this overhead.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break self.scalar_from_bytes(&sk.to_bytes()); | |
| } | |
| } | |
| let mut bytes = vec![0u8; $coord_len]; | |
| loop { | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break self.scalar_from_bytes(&sk.to_bytes()); | |
| } | |
| } |
| let mut x_bytes = vec![0u8; $coord_len]; | ||
| let mut y_bytes = vec![0u8; $coord_len]; | ||
| let x_be = x.to_bytes_be(); | ||
| let y_be = y.to_bytes_be(); | ||
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | ||
| return Err(ECDHPaceError::InvalidEncoding); | ||
| } | ||
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | ||
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); |
There was a problem hiding this comment.
Allocating x_bytes and y_bytes as heap-allocated Vecs is unnecessary since $coord_len is a constant expression. Using stack-allocated arrays avoids heap allocations and improves performance.
| let mut x_bytes = vec![0u8; $coord_len]; | |
| let mut y_bytes = vec![0u8; $coord_len]; | |
| let x_be = x.to_bytes_be(); | |
| let y_be = y.to_bytes_be(); | |
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | |
| return Err(ECDHPaceError::InvalidEncoding); | |
| } | |
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | |
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); | |
| let mut x_bytes = [0u8; $coord_len]; | |
| let mut y_bytes = [0u8; $coord_len]; | |
| let x_be = x.to_bytes_be(); | |
| let y_be = y.to_bytes_be(); | |
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | |
| return Err(ECDHPaceError::InvalidEncoding); | |
| } | |
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | |
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break sk; | ||
| } | ||
| } |
There was a problem hiding this comment.
Allocating bytes as a Vec inside the loop causes unnecessary heap allocations on every iteration. Moving the allocation outside the loop avoids this overhead.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } | |
| let mut bytes = vec![0u8; $coord_len]; | |
| loop { | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break self.scalar_from_bytes(&sk.to_bytes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Allocating bytes as a Vec inside the loop causes unnecessary heap allocations on every iteration. Moving the allocation outside the loop avoids this overhead.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break self.scalar_from_bytes(&sk.to_bytes()); | |
| } | |
| } | |
| let mut bytes = vec![0u8; $coord_len]; | |
| loop { | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break self.scalar_from_bytes(&sk.to_bytes()); | |
| } | |
| } |
| let mut x_bytes = vec![0u8; $coord_len]; | ||
| let mut y_bytes = vec![0u8; $coord_len]; | ||
| let x_be = x.to_bytes_be(); | ||
| let y_be = y.to_bytes_be(); | ||
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | ||
| return Err(ECDHPaceError::InvalidEncoding); | ||
| } | ||
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | ||
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); |
There was a problem hiding this comment.
Allocating x_bytes and y_bytes as heap-allocated Vecs is unnecessary since $coord_len is a constant expression. Using stack-allocated arrays avoids heap allocations and improves performance.
| let mut x_bytes = vec![0u8; $coord_len]; | |
| let mut y_bytes = vec![0u8; $coord_len]; | |
| let x_be = x.to_bytes_be(); | |
| let y_be = y.to_bytes_be(); | |
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | |
| return Err(ECDHPaceError::InvalidEncoding); | |
| } | |
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | |
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); | |
| let mut x_bytes = [0u8; $coord_len]; | |
| let mut y_bytes = [0u8; $coord_len]; | |
| let x_be = x.to_bytes_be(); | |
| let y_be = y.to_bytes_be(); | |
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | |
| return Err(ECDHPaceError::InvalidEncoding); | |
| } | |
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | |
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/dmrtd/src/proto/ecdh_pace.rs (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
rand::rand_core::RnginRngBridge
Cargo.tomlalready pinsrand = "0.10", soSysRng/UnwrapErrare fine. Incrates/dmrtd/src/proto/ecdh_pace.rs:293-311, switch theRngBridgebound fromrand::rand_core::RngCoretorand::rand_core::Rngto avoid the deprecation warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dmrtd/src/proto/ecdh_pace.rs` around lines 11 - 14, Update the RngBridge generic bound around the implementation at lines 293–311 from rand::rand_core::RngCore to rand::rand_core::Rng, preserving the existing bridge behavior and imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dmrtd/src/proto/ecdh_pace.rs`:
- Around line 47-254: Refactor the impl_curve_ops macro so the duplicated shared
methods—scalar_from_bytes, get_pub_key, get_pub_key_ephemeral,
compute_shared_point, point_to_pubkey_pace, and transform_public—are defined
once and reused by both modern and legacy arms, leaving only RNG-specific key
generation logic distinct. Hoist the legacy RngBridge implementation into one
private reusable helper and update both generate_key_pair and
map_and_generate_ephemeral to use it, eliminating the per-call-site and
per-curve duplicates.
- Around line 69-82: Update scalar handling in scalar_from_bytes,
compute_shared_point, and the ephemeral-scalar branches to avoid variable-time
BigUint reduction for secret scalars: use SecretKey::to_nonzero_scalar()
directly for private-key paths, while preserving the existing byte-reduction
path for public nonce inputs. Replace the hand-transcribed decimal curve-order
literals with the corresponding elliptic-curve Curve::ORDER constants.
---
Nitpick comments:
In `@crates/dmrtd/src/proto/ecdh_pace.rs`:
- Around line 11-14: Update the RngBridge generic bound around the
implementation at lines 293–311 from rand::rand_core::RngCore to
rand::rand_core::Rng, preserving the existing bridge behavior and imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7f1046b-2c1d-4d5a-ab6d-ec10d3fd69ca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlcrates/dmrtd/Cargo.tomlcrates/dmrtd/src/proto/domain_parameter.rscrates/dmrtd/src/proto/ecdh_pace.rscrates/dmrtd/src/proto/pace_session.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the dmrtd crate to support additional PACE curves, including NIST P-224, P-384, P-521, and Brainpool P-256 and P-384, refactoring the ECDHPace engine to use a macro-based implementation for curve operations. The review feedback focuses on performance optimizations within these macro-generated operations, specifically recommending the use of stack-allocated FieldBytes and pre-allocated vectors to avoid unnecessary heap allocations during scalar reduction, key pair generation, and public key serialization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let mut buf = vec![0u8; $coord_len]; | ||
| if r_bytes.len() <= $coord_len { | ||
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | ||
| } else { | ||
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | ||
| } | ||
| let mut field_bytes = | ||
| <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | ||
| field_bytes.copy_from_slice(buf.as_slice()); |
There was a problem hiding this comment.
Avoid allocating a temporary Vec on the heap (buf) during scalar reduction. We can write the bytes directly into field_bytes to improve performance and reduce memory overhead.
| let mut buf = vec![0u8; $coord_len]; | |
| if r_bytes.len() <= $coord_len { | |
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | |
| } | |
| let mut field_bytes = | |
| <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| field_bytes.copy_from_slice(buf.as_slice()); | |
| let mut field_bytes = | |
| <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| if r_bytes.len() <= field_bytes.len() { | |
| field_bytes[field_bytes.len() - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| field_bytes.copy_from_slice(&r_bytes[r_bytes.len() - field_bytes.len()..]); | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break sk; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid allocating a new Vec on the heap (bytes) inside the loop. We can allocate FieldBytes on the stack once and reuse it across iterations.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } | |
| let mut bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| loop { | |
| rng.fill_bytes(bytes.as_mut_slice()); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break *sk.to_nonzero_scalar(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid allocating a new Vec on the heap (bytes) inside the loop. We can allocate FieldBytes on the stack once and reuse it across iterations.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break *sk.to_nonzero_scalar(); | |
| } | |
| } | |
| let mut bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| loop { | |
| rng.fill_bytes(bytes.as_mut_slice()); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break *sk.to_nonzero_scalar(); | |
| } | |
| } |
| let mut x_bytes = vec![0u8; $coord_len]; | ||
| let mut y_bytes = vec![0u8; $coord_len]; | ||
| let x_be = x.to_bytes_be(); | ||
| let y_be = y.to_bytes_be(); | ||
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | ||
| return Err(ECDHPaceError::InvalidEncoding); | ||
| } | ||
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | ||
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); | ||
|
|
||
| let mut bytes = vec![0x04]; | ||
| bytes.extend_from_slice(&x_bytes); | ||
| bytes.extend_from_slice(&y_bytes); |
There was a problem hiding this comment.
Avoid heap allocations for x_bytes and y_bytes by using stack-allocated FieldBytes. Additionally, pre-allocate bytes with capacity to prevent multiple reallocations during serialization.
let mut x_bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default();
let mut y_bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default();
let x_be = x.to_bytes_be();
let y_be = y.to_bytes_be();
if x_be.len() > $coord_len || y_be.len() > $coord_len {
return Err(ECDHPaceError::InvalidEncoding);
}
x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be);
y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be);
let mut bytes = Vec::with_capacity(1 + 2 * $coord_len);
bytes.push(0x04);
bytes.extend_from_slice(&x_bytes);
bytes.extend_from_slice(&y_bytes);| let mut buf = vec![0u8; $coord_len]; | ||
| if r_bytes.len() <= $coord_len { | ||
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | ||
| } else { | ||
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | ||
| } | ||
| let mut field_bytes = | ||
| <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | ||
| field_bytes.copy_from_slice(buf.as_slice()); |
There was a problem hiding this comment.
Avoid allocating a temporary Vec on the heap (buf) during scalar reduction. We can write the bytes directly into field_bytes to improve performance and reduce memory overhead.
| let mut buf = vec![0u8; $coord_len]; | |
| if r_bytes.len() <= $coord_len { | |
| buf[$coord_len - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| buf.copy_from_slice(&r_bytes[r_bytes.len() - $coord_len..]); | |
| } | |
| let mut field_bytes = | |
| <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| field_bytes.copy_from_slice(buf.as_slice()); | |
| let mut field_bytes = | |
| <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| if r_bytes.len() <= field_bytes.len() { | |
| field_bytes[field_bytes.len() - r_bytes.len()..].copy_from_slice(&r_bytes); | |
| } else { | |
| field_bytes.copy_from_slice(&r_bytes[r_bytes.len() - field_bytes.len()..]); | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break sk; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid allocating a new Vec on the heap (bytes) inside the loop. We can allocate FieldBytes on the stack once and reuse it across iterations.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } | |
| let mut bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| loop { | |
| rng.fill_bytes(bytes.as_mut_slice()); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break sk; | |
| } | |
| } |
| loop { | ||
| let mut bytes = vec![0u8; $coord_len]; | ||
| rng.fill_bytes(&mut bytes); | ||
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | ||
| break *sk.to_nonzero_scalar(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid allocating a new Vec on the heap (bytes) inside the loop. We can allocate FieldBytes on the stack once and reuse it across iterations.
| loop { | |
| let mut bytes = vec![0u8; $coord_len]; | |
| rng.fill_bytes(&mut bytes); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break *sk.to_nonzero_scalar(); | |
| } | |
| } | |
| let mut bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default(); | |
| loop { | |
| rng.fill_bytes(bytes.as_mut_slice()); | |
| if let Ok(sk) = <$secret_key>::from_slice(&bytes) { | |
| break *sk.to_nonzero_scalar(); | |
| } | |
| } |
| let mut x_bytes = vec![0u8; $coord_len]; | ||
| let mut y_bytes = vec![0u8; $coord_len]; | ||
| let x_be = x.to_bytes_be(); | ||
| let y_be = y.to_bytes_be(); | ||
| if x_be.len() > $coord_len || y_be.len() > $coord_len { | ||
| return Err(ECDHPaceError::InvalidEncoding); | ||
| } | ||
| x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be); | ||
| y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be); | ||
|
|
||
| let mut bytes = vec![0x04]; | ||
| bytes.extend_from_slice(&x_bytes); | ||
| bytes.extend_from_slice(&y_bytes); |
There was a problem hiding this comment.
Avoid heap allocations for x_bytes and y_bytes by using stack-allocated FieldBytes. Additionally, pre-allocate bytes with capacity to prevent multiple reallocations during serialization.
let mut x_bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default();
let mut y_bytes = <$curve_crate::elliptic_curve::FieldBytes<$curve_type>>::default();
let x_be = x.to_bytes_be();
let y_be = y.to_bytes_be();
if x_be.len() > $coord_len || y_be.len() > $coord_len {
return Err(ECDHPaceError::InvalidEncoding);
}
x_bytes[$coord_len - x_be.len()..].copy_from_slice(&x_be);
y_bytes[$coord_len - y_be.len()..].copy_from_slice(&y_be);
let mut bytes = Vec::with_capacity(1 + 2 * $coord_len);
bytes.push(0x04);
bytes.extend_from_slice(&x_bytes);
bytes.extend_from_slice(&y_bytes);
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
This PR implements issue #11, expanding PACE support beyond P-256 to include NIST and Brainpool elliptic curves. It adds support for the following curves in the dmrtd reader core library:
All unit tests, integration tests, and doc-tests pass successfully.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds full ECDH PACE support for NIST P-224/256/384/521 and Brainpool P-256/384 (r1), expanding beyond P-256 to meet issue #11. Also streamlines the ECDH API and improves performance in the PACE flow.
New Features
bp256,bp384; enabled ECDH onp384; addedp521,p224. Bumped workspace/crates to 0.3.0 and updated READMEs.Refactors
rand::Rngand resolved remaining strict-warning lints.Written for commit f3b71ea. Summary will update on new commits.