Skip to content

Eliminate dependencies#77

Merged
n13 merged 7 commits into
mainfrom
illuzen/reduce-dependencies
Jun 4, 2026
Merged

Eliminate dependencies#77
n13 merged 7 commits into
mainfrom
illuzen/reduce-dependencies

Conversation

@illuzen

@illuzen illuzen commented Jun 3, 2026

Copy link
Copy Markdown
Contributor
  • flatten the repo
  • inline all relevant p3 code
  • make ct_bench an example instead of binary

@cursor

cursor Bot commented Jun 3, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@n13

n13 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Findings

  • Medium: CONSTANT_TIME_TESTING.md still documents the old layout: cd core plus cargo run --release --features ct-testing --bin ct_bench. This PR removes core/, removes ct-testing, and makes ct_bench an example, so the documented command now fails with package ... does not contain this feature: ct-testing. Update it to the new root/example command and refresh the stale test names.

  • Medium: The public API now uses the crate’s own Goldilocks type from src/goldilocks.rs instead of the previous p3_goldilocks::Goldilocks exposed through function signatures in src/lib.rs. That is a downstream breaking change; if this crate is published, it should be treated as a major/versioned migration or clearly documented.

  • Low: README.md still says the crate is built on Plonky3 field arithmetic and labels qp-poseidon-core as core/, which contradicts the flattening/removal of dependencies.

Checks

Ran in a throwaway worktree for PR #77:

  • cargo test passed
  • cargo clippy --all-targets --all-features passed
  • cargo build --no-default-features --locked passed
  • cargo doc --workspace --locked --no-deps --all-features passed with one bare URL warning
  • GitHub PR checks are green

@n13

n13 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review: PR #77 — "Eliminate dependencies"

I reviewed this in an isolated worktree (now cleaned up). Overall this is high-quality, well-tested work that I'd approve with a couple of minor doc/process nits. Here's the full picture.

What the PR does

  • Flattens the core/ workspace member into a single root crate (qp-poseidon-core v2.0.2).
  • Inlines all needed Plonky3 (p3) code, dropping p3-field, p3-goldilocks, p3-poseidon2, p3-symmetric, and qp-poseidon-constants. The crate now has zero runtime dependencies.
  • Adds src/goldilocks.rs (Goldilocks field) and src/poseidon2.rs (Poseidon2 permutation, WIDTH=12).
  • Converts ct_bench from a feature-gated bin to an example.

Correctness — the important part

This is a reimplementation of audited cryptographic code, so functional equivalence is everything. The strongest evidence is that the Known Answer Test vectors in lib.rs are unchanged from main (the diff only removes p3 imports) and still pass. Those vectors were generated by the real p3 library, so passing them proves the inlined hash is byte-for-byte identical to the previous published version.

I verified the inlined code line-by-line:

  • goldilocks.rs is a faithful copy of p3-goldilocks: correct prime (2^64−2^32+1), reduce128, branchless halve, S-box x^7, and the x86_64 inline-asm fast-add with a portable fallback (I tested the portable path on ARM; x86_64 is covered by Ubuntu CI + KAT vectors).
  • poseidon2.rs matches the canonical Poseidon2 structure (initial external linear layer → 4 external → 22 internal → 4 external). MATRIX_DIAG is the well-known MATRIX_DIAG_12_GOLDILOCKS_U64 constant set, and apply_mat4 correctly implements the [2 3 1 1] circulant (verified by hand and by test_apply_mat4).
  • The serialization.rs swap from p3's canonicalizing from_int to the non-canonicalizing from_u64 is safe: every value is immediately consumed by field arithmetic that reduces mod P, and test_rehash_to_bytes_vectors (all-0xFF input, which exceeds P) locks this in.

Verification (all green locally, toolchain 1.93.0)

  • cargo test55 tests + 2 doctests pass, including test_known_value_hashes, test_poseidon2_matches_p3_zero_input, and test_poseidon2_matches_p3_sequential_input.
  • cargo clippy --workspace --all-targets --all-features --locked --release -- -D warnings → clean (--workspace still works on a single-package repo; Cargo.lock is in sync).
  • cargo +nightly fmt --all --check and taplo format --check → clean.
  • cargo build --target wasm32v1-noneno_std build succeeds.
  • cargo doc → succeeds (one pre-existing bare-URL warning, non-fatal).

So CI should be green.

Minor issues (non-blocking)

  1. Stale docs left by the restructure (these are real and should be fixed):

    • CONSTANT_TIME_TESTING.md:10-11 still says cd core + cargo run --release --features ct-testing --bin ct_bench. Both the core/ dir and ct-testing feature are gone; it should be cargo run --release --example ct_bench.
    • README.md:7 still references ### qp-poseidon-core (core/).
  2. cargo doc bare-URL warning in poseidon2.rs:4 — wrap the eprint URL in <...>.

  3. Process/security note: the security review (audits/, ConstantTiming.pdf) was performed against the p3-based version. The team now maintains a hand-inlined fork of p3 0.3.0 crypto code, so it's worth confirming the audit scope covers the inlined copy and noting how it'll be kept in sync with upstream p3.

  4. Possible breaking API change / versioning: public functions now return the local qp_poseidon_core::Goldilocks instead of p3_goldilocks::Goldilocks. That's source-breaking for downstream users, yet the version stays 2.0.2. Worth a major bump when published (your release workflow may handle this).

  5. ct_bench input generation changed from rng.next_u64() % ORDER_U64 to from_u64(rng.next_u64()) (now feeds non-canonical, full-range felts). This is arguably a stricter constant-time test, but it does change what the baseline exercises — fine, just flagging intent.

Net: correct, dependency-free, fully tested, CI-green. I'd merge after fixing the stale CONSTANT_TIME_TESTING.md/README.md references (and ideally addressing the audit-scope and versioning notes).

Want me to push doc fixes to the branch, or draft review comments? I won't post anything to GitHub without your go-ahead.

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, I think it's good to go, probably worth having AI clean it up a bit as per the above nits

@n13 n13 merged commit 90426d5 into main Jun 4, 2026
8 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.

2 participants