Edmund/continue poseidon #1313

edmundnoble · 2023-10-25T22:41:09Z

This PR supersedes #1274.

Remaining work: check test vectors, check gas.
Edit: both done.

The performance improvements in this PR go from this:

defaultMain [bench "poseidon" $ whnf poseidon [1,2,3,4,5,6,7,8]]
benchmarking poseidon
time                 1.696 ms   (1.686 ms .. 1.710 ms)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 1.704 ms   (1.695 ms .. 1.722 ms)
std dev              41.47 μs   (25.65 μs .. 74.70 μs)
variance introduced by outliers: 13% (moderately inflated)

to this:

defaultMain [bench "poseidon" $ whnf poseidon [1,2,3,4,5,6,7,8]]
benchmarking poseidon
time                 1.222 ms   (1.215 ms .. 1.230 ms)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 1.222 ms   (1.212 ms .. 1.244 ms)
std dev              49.37 μs   (21.97 μs .. 99.33 μs)

and from this:

defaultMain [bench "poseidon" $ whnf poseidon [1,2]]
benchmarking poseidon
time                 182.7 μs   (182.1 μs .. 183.3 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 182.0 μs   (181.2 μs .. 182.7 μs)
std dev              2.628 μs   (2.202 μs .. 3.368 μs)

to this:

defaultMain [bench "poseidon" $ whnf poseidon [1,2]]
benchmarking poseidon
time                 159.5 μs   (155.2 μs .. 164.8 μs)
                     0.996 R²   (0.993 R² .. 0.999 R²)
mean                 155.5 μs   (153.7 μs .. 157.8 μs)
std dev              6.763 μs   (5.227 μs .. 10.27 μs)
variance introduced by outliers: 43% (moderately inflated)

PR checklist:

Test coverage for the proposed changes
PR description contains example output from repl interaction or a snippet from unit test output
Documentation has been updated if new natives or FV properties have been added. To generate new documentation, issue cabal run tests. If they pass locally, docs are generated.
Any changes that could be relevant to users have been recorded in the changelog
In case of changes to the Pact trace output (pact -t), make sure pact-lsp is in sync.

Additionally, please justify why you should or should not do the following:

Confirm replay/back compat
Benchmark regressions
(For Kadena engineers) Run integration-tests against a Chainweb built with this version of Pact

Co-authored-by: chessai <chessai@kadena.io>

edmundnoble · 2023-10-26T21:25:59Z

Gas graph:

The actual inputs don't affect the time taken to compute the hash from what I can tell, only the number of them, so that's the X axis. The Y axis is micros * 2.5, i.e. gas as a unit of time.

emilypi

Looking good so far

src/Crypto/Hash/PoseidonNative.hs

emilypi · 2023-10-27T00:35:40Z

src/Pact/Native.hs

+    , "The input consists of 1 to 8 integers."
+    , "The output is the hash result as an integer."
+    , "Note: This is a reference version of the Poseidon hash function used by Hack-a-Chain."
+    ])


This is non-idiomatic of the way we write native docs. There's no need to intro this now.

Let me know if the new version is idiomatic.

tests/PactTests.hs

0xd34df00d · 2023-10-30T19:39:50Z

src/Crypto/Hash/PoseidonNative.hs

+  let in2 = mulmod inVal inVal
+      in4 = mulmod in2 in2
+  in mulmod in4 inVal


My group theory is rusty, but can you do mod just once after all the multiplications?

I tried some basic tests and that seems to hold true:

module Main where modulus :: Integer modulus = 53 mulmod :: Integer -> Integer -> Integer mulmod a b = (a * b) `mod` modulus sig :: Integer -> Integer sig inVal = let in2 = mulmod inVal inVal in4 = mulmod in2 in2 in mulmod in4 inVal mulmod2:: Integer -> Integer -> Integer mulmod2 a b = (a * b) sig2 :: Integer -> Integer sig2 inVal = let in2 = mulmod2 inVal inVal in4 = mulmod2 in2 in2 in mulmod2 in4 inVal `mod` modulus main :: IO () main = do print $ sig 123 print $ sig 456 print $ sig 789 print $ sig2 123 print $ sig2 456 print $ sig2 789

Alright, I grabbed a pen and paper today! So the question effectively is whether ab mod n = (a mod n) × (b mod n) mod n. Let's use brute force and assume a = k₁n + r₁, b = k₂n + r₂ (with the usual conditions of r₁, r₂ < n). Then:

ab mod n = (...) × n + r₁r₂ mod n = r₁r₂ mod n

while for the rhs

(a mod n) × (b mod n) mod n = r₁r₂ mod n

so that clearly holds, and mod can happen just once. That is assuming nothing funny happens wrt negative signs here and haskell mod behaves like math mod (which IIRC it does).

Thanks Georg, worth a try.

Just tried it, it's not any faster according to my benchmark to do

(inVal ^ 5) `mod` modulus

than to do what we're doing now. I think that's what you were suggesting.

Yep, that was it.

Well, perhaps it's not that much of a hotspot then! Up to you what expression to keep, although I'd probably vote for inVal ^ 5 — this way it's more obvious what's happening.

I think sig is just strangely named (due a the paper reference impl maybe?).

That said (inVal ^ 5) if the ^ implementation isn't applying modulo may be significantly slower because of the size the integers can grow. The modulus is quite a large prime, so there are numbers quite large there.

edmundnoble · 2023-10-31T23:04:21Z

Here's where exactly the constants came from: https://github.com/iden3/circomlibjs/blob/main/src/poseidon_constants.js. I've patched the PR to use the exact same constants (in hex, just like they do it) so it's easier to compare. I have verified by hand that the output for these test vectors matches the output from https://github.com/iden3/circomlibjs/blob/main/src/poseidon_reference.js.

src/Pact/Native/Pairing.hs

imalsogreg

Looks fantastic!

Ran it locally in repl, outputs look reasonable
Rejects some bad arguments as expected
Unit tests look good
Gas tests look good
Gas analysis matches the benchmarks
Repl complains when DisablePact410

jmcardon · 2023-11-06T16:47:26Z

src/Crypto/Hash/PoseidonNative.hs

+import qualified Data.Primitive.SmallArray as SmallArray
+
+modulus :: Integer
+modulus = 21888242871839275222246405745257275088548364400416034343698204186575808495617


This constant is in our ZK impl as well. I wonder whether we want a common place for it.

jmcardon · 2023-11-06T17:03:30Z

src/Crypto/Hash/PoseidonNative.hs

+summod :: Integer -> Integer -> Integer
+summod a b = (a + b) `mod` modulus
+
+sig :: Integer -> Integer


sig is just a^5 mod n, is the name due to the reference impl?

Yes I think.

jmcardon · 2023-11-06T17:04:35Z

src/Crypto/Hash/PoseidonNative.hs

+  in mulmod in4 inVal
+
+ark :: Integer -> Integer -> Integer
+ark inVal cc = summod inVal cc


ark = summod ?

Yeah I kept the names just in case this makes it easier to compare to the ref. impl.

jmcardon · 2023-11-06T17:07:35Z

src/Crypto/Hash/PoseidonNative.hs

+  let in2 = mulmod inVal inVal
+      in4 = mulmod in2 in2
+  in mulmod in4 inVal


I think sig is just strangely named (due a the paper reference impl maybe?).

That said (inVal ^ 5) if the ^ implementation isn't applying modulo may be significantly slower because of the size the integers can grow. The modulus is quite a large prime, so there are numbers quite large there.

jmcardon · 2023-11-06T17:08:06Z

src/Crypto/Hash/PoseidonNative.hs

+ark inVal cc = summod inVal cc
+
+data NineRings =
+  NineRings !Integer !Integer !Integer !Integer !Integer !Integer !Integer !Integer !Integer


Same amount Kobe and Shaq have combined.

jmcardon · 2023-11-06T17:11:30Z

src/Crypto/Hash/PoseidonNative.hs

+poseidon :: [Integer] -> Integer
+poseidon inputs =
+  if nInputs > 8 || nInputs == 0
+  then error $ "poseidon: number of inputs not between 0 and 8: " <> show nInputs


error here worries me ever so slightly, only since the function spits out package hashes.

We never get to this point anyway, because of the case in poseidonDef. But I can replace this with a bespoke exception type if you'd prefer.

Should be okay 👍

davibauer and others added 5 commits August 10, 2023 16:15

add poseidon-hash

bef9ca4

add poseidon-hash

be9f818

add poseidon-hash

0de681f

Merge branch 'master' into edmund/continue-poseidon

d6dac6f

Performance and readability

aa7aa48

Co-authored-by: chessai <chessai@kadena.io>

edmundnoble requested review from sirlensalot, jwiegley, emilypi and jmcardon as code owners October 25, 2023 22:41

edmundnoble added 3 commits October 26, 2023 15:08

reformat

2b86387

Renames

6847692

Complete gas model

e3c2dd6

edmundnoble added 2 commits October 26, 2023 17:41

Fix tests

7bf5cbf

disable poseidon hash before pact 4.10

3b7aeb2

emilypi reviewed Oct 27, 2023

View reviewed changes

edmundnoble added 2 commits October 27, 2023 05:32

Address review

0b7046f

remove redundant TODO

f511232

emilypi approved these changes Oct 27, 2023

View reviewed changes

0xd34df00d approved these changes Oct 30, 2023

View reviewed changes

Comment constants and hexify them to match the original

62ac69f

imalsogreg reviewed Oct 31, 2023

View reviewed changes

src/Pact/Native/Pairing.hs Show resolved Hide resolved

imalsogreg approved these changes Oct 31, 2023

View reviewed changes

jmcardon reviewed Nov 8, 2023

View reviewed changes

jmcardon approved these changes Nov 10, 2023

View reviewed changes

edmundnoble merged commit 422cec4 into master Nov 13, 2023
8 checks passed

edmundnoble mentioned this pull request Nov 14, 2023

feat/poseidon-hash #1274

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edmund/continue poseidon #1313

Edmund/continue poseidon #1313

edmundnoble commented Oct 25, 2023 •

edited

edmundnoble commented Oct 26, 2023

emilypi left a comment

emilypi Oct 27, 2023

edmundnoble Oct 27, 2023

0xd34df00d Oct 30, 2023

jwiegley Oct 31, 2023

0xd34df00d Oct 31, 2023

edmundnoble Oct 31, 2023

0xd34df00d Nov 1, 2023 •

edited

jmcardon Nov 6, 2023

edmundnoble commented Oct 31, 2023

imalsogreg left a comment

jmcardon Nov 6, 2023

jmcardon Nov 6, 2023

edmundnoble Nov 8, 2023

jmcardon Nov 6, 2023

edmundnoble Nov 8, 2023

jmcardon Nov 6, 2023

jmcardon Nov 6, 2023

jmcardon Nov 6, 2023

edmundnoble Nov 8, 2023

jmcardon Nov 9, 2023

Edmund/continue poseidon #1313

Edmund/continue poseidon #1313

Conversation

edmundnoble commented Oct 25, 2023 • edited

edmundnoble commented Oct 26, 2023

emilypi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xd34df00d Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundnoble commented Oct 31, 2023

imalsogreg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundnoble commented Oct 25, 2023 •

edited

0xd34df00d Nov 1, 2023 •

edited