feat: add Pedersen hash #627

bidzyyys · 2025-04-10T17:41:55Z

Resolves #269

PR Checklist

Tests
Documentation
Changelog

netlify · 2025-04-10T17:42:15Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`de31feb`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/67f802e63cb4050008284342

netlify · 2025-04-10T17:42:35Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`a34d610`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/6808e660d124f60008ecb383

codecov · 2025-04-10T17:45:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.3%. Comparing base (16cc1ff) to head (2492e86).

✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/crypto/src/pedersen/instance/starknet.rs	`99.9% <ø> (ø)`
lib/crypto/src/pedersen/mod.rs	`100.0% <100.0%> (ø)`

... and 2 files with indirect coverage changes

qalisander

Good job!
First pass

lib/crypto/src/pedersen/mod.rs

qalisander · 2025-04-23T09:36:59Z

lib/crypto/src/pedersen/mod.rs

+    type Output = Affine<P>;
+
+    /// Update the hash state with a new element.
+    fn update(&mut self, input: impl AsRef<[u8]>) {


I don't think it is a good API for this kind of hash. Since after update is called you recreate U256 from variable size of bytes.
And e.g. calling update() two times with inputs [42, 42] and [43,43]. Or calling it with inputs [42, 42, 0, 0] and [43, 43] should produce the same hash, since bytes are not padded. And [42, 42] and [42, 42, 0, 0] will be parsed to the same integer. So we can easily peek different set of bytes, that will produce the same hash. It is an actual vulnerability to preimage attack. I don't think we should implement Hasher in this case (more simple) or redesign the trait and make it more generic (able to accept U256 as an argument)

I see that starknet-rs has only api where you can pass 256-bit integer.

Fair point, I removed Hasher trait implementation.

lib/crypto/src/pedersen/mod.rs

lib/crypto/src/pedersen/instance/starknet.rs

Co-authored-by: Alisander Qoshqosh <[email protected]>

0xNeshi · 2025-04-23T15:37:49Z

examples/pedersen/src/lib.rs

+};
+use stylus_sdk::prelude::*;
+#[entrypoint]
+#[storage]


Suggested change

};

use stylus_sdk::prelude::*;

#[entrypoint]

#[storage]

};

use stylus_sdk::prelude::*;

#[entrypoint]

#[storage]

0xNeshi · 2025-04-23T15:40:37Z

lib/crypto/proptest-regressions/pedersen/instance/starknet.txt

+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc 9f552914238459525586fa596ededdce984599cffe1af7515181e7f8890fabf6 # shrinks to input = [3618502788666131219974424518481750869458896638539263116075447500599906533376]


Is this proptest regression important enough to have in repo? If this was the result on some mishap in the impl, we can remove it (this is the PR that first introduces the Pedersen hash after all); if this is a specific edge case, we can leave it in

0xNeshi · 2025-04-23T15:41:39Z

lib/crypto/src/pedersen/instance/mod.rs

+//! This module contains pedersen hash instances for some popular curve
+//! instances.


Suggested change

//! This module contains pedersen hash instances for some popular curve

//! instances.

//! This module contains pedersen hash instances for some popular curves.

0xNeshi · 2025-04-23T15:42:17Z

lib/crypto/src/pedersen/instance/starknet.rs

+};
+#[derive(Clone, Default, PartialEq, Eq)]
+/// Starknet's Curve Details.
+pub struct StarknetCurveConfig;


Suggested change

};

#[derive(Clone, Default, PartialEq, Eq)]

/// Starknet's Curve Details.

pub struct StarknetCurveConfig;

};

/// Starknet's Curve Details.

#[derive(Clone, Default, PartialEq, Eq)]

pub struct StarknetCurveConfig;

0xNeshi · 2025-04-23T15:42:58Z

lib/crypto/src/pedersen/instance/starknet.rs

+    // TODO: confirm this
+    const COFACTOR: &'static [u64] = &[0x1];


0xNeshi · 2025-04-23T16:05:00Z

lib/crypto/src/pedersen/mod.rs

+    params: core::marker::PhantomData<F>,
+    curve: core::marker::PhantomData<P>,


Suggested change

params: core::marker::PhantomData<F>,

curve: core::marker::PhantomData<P>,

params: PhantomData<F>,

curve: PhantomData<P>,

nit: import PhantomData directly instead of using absolute path repeatedly

0xNeshi · 2025-04-23T16:05:50Z

lib/crypto/src/pedersen/mod.rs

+pub mod instance;
+pub mod params;
+use alloc::{vec, vec::Vec};


nit: place pub mods below imports for clarity, I almost didn't realize these modules were declared here (confused them with additional imports)

0xNeshi · 2025-04-23T16:07:02Z

lib/crypto/src/pedersen/instance/starknet.rs

+/// Constant points for Starknet Curve.
+static CONSTANT_POINTS: Lazy<Vec<Affine<StarknetCurveConfig>>> = Lazy::new(


A link to where these constant points were taken from would be useful, helps maintainability and review

0xNeshi · 2025-04-23T16:07:48Z

lib/crypto/src/pedersen/instance/starknet.rs

+    // Starknet's base field modulus.
+    const MODULUS: U256 = from_num!("3618502788666131213697322783095070105623107215331596699973092056135872020481");


Link to source of this const, if not here then at the module-level docs //!

0xNeshi · 2025-04-23T16:11:44Z

lib/crypto/src/pedersen/mod.rs

+            assert!(point_list.len() == F::N_ELEMENT_BITS_HASH);
+
+            for pt in point_list {
+                assert!(pt.x != point.x, "Unhashable input.");


We should assert! things that must always hold for our code to be valid, the "invariants" of our code.
Otherwise returning a Result::Err might be clearer and more in line with Rust's way of doing things.

Not saying this or any other assert should be converted to an error as I lack enough in-depth knowledge to determine whether it should be done, I'm just pointing this out so that you can determine the right approach and that we're on the same page.

0xNeshi

Approved by mistake

bidzyyys · 2025-05-06T10:50:37Z

Solved in #644.

bidzyyys linked an issue Apr 10, 2025 that may be closed by this pull request

[Feature]: Pedersen Hash #269

Closed

1 task

bidzyyys force-pushed the feat/pedersen-hash branch from 8b3af08 to 53e3f6f Compare April 22, 2025 18:14

bidzyyys marked this pull request as ready for review April 22, 2025 18:15

bidzyyys requested review from qalisander and 0xNeshi as code owners April 22, 2025 18:15

bidzyyys added 3 commits April 22, 2025 20:18

feat: add Pedersen hash

ef16e2a

test: add unit tests

94412ed

docs: update CHANGELOG

eadb81f

bidzyyys force-pushed the feat/pedersen-hash branch from 53e3f6f to eadb81f Compare April 22, 2025 18:21

qalisander reviewed Apr 23, 2025

View reviewed changes

bidzyyys and others added 3 commits April 23, 2025 12:52

ref: apply comments

9ba23c2

Co-authored-by: Alisander Qoshqosh <[email protected]>

ref: get rid of long running tasks

48489de

fix: apply clippy warnings

a34d610

bidzyyys changed the base branch from main to v0.2 April 23, 2025 13:25

bidzyyys added 4 commits April 23, 2025 15:31

fix: nonexistent link in docs

47d7c56

chore: remove Default impl from Pedersen hash

9818641

test: add E2E test

a1051fa

feat: add Pedersen bench

fc3cf27

bidzyyys requested a review from qalisander April 23, 2025 16:01

0xNeshi approved these changes Apr 23, 2025

View reviewed changes

0xNeshi self-requested a review April 23, 2025 16:12

0xNeshi reviewed Apr 23, 2025

View reviewed changes

Merge branch 'v0.2' into feat/pedersen-hash

3f27af7

0xNeshi self-requested a review April 24, 2025 11:31

Merge branch 'v0.2' into feat/pedersen-hash

2492e86

bidzyyys closed this May 6, 2025

		//! This module contains pedersen hash instances for some popular curve
		//! instances.

		// TODO: confirm this
		const COFACTOR: &'static [u64] = &[0x1];

		params: core::marker::PhantomData<F>,
		curve: core::marker::PhantomData<P>,

		/// Constant points for Starknet Curve.
		static CONSTANT_POINTS: Lazy<Vec<Affine<StarknetCurveConfig>>> = Lazy::new(

		// Starknet's base field modulus.
		const MODULUS: U256 = from_num!("3618502788666131213697322783095070105623107215331596699973092056135872020481");

feat: add Pedersen hash #627

feat: add Pedersen hash #627

Uh oh!

Conversation

bidzyyys commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

netlify bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for contracts-stylus canceled.

Uh oh!

netlify bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for contracts-stylus canceled.

Uh oh!

codecov bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

qalisander left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qalisander Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xNeshi Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xNeshi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bidzyyys commented May 6, 2025

Uh oh!

Uh oh!

bidzyyys commented Apr 10, 2025 •

edited

Loading

netlify bot commented Apr 10, 2025 •

edited

Loading

netlify bot commented Apr 10, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading

qalisander left a comment •

edited

Loading

qalisander Apr 23, 2025 •

edited

Loading

0xNeshi Apr 23, 2025 •

edited

Loading

0xNeshi left a comment •

edited

Loading