Skip to content

feat(formal): Clean/Lean model of BITWISE, LT, EQ and SHIFT with proo… - #1027

Draft
ColoCarletti wants to merge 10 commits into
mainfrom
formal/clean-model
Draft

ColoCarletti wants to merge 10 commits into
mainfrom
formal/clean-model

Conversation

@ColoCarletti

Copy link
Copy Markdown
Collaborator

…fs and a differential check against the prover

@MauroToscano

Copy link
Copy Markdown
Contributor

/review-ai

@MauroToscano

Copy link
Copy Markdown
Contributor

/ai-review

let (prog, _) = cb.finish(num_base_from_meta(&meta));

// Fingerprints over random rows.
let rows = random_rows(num_cols, 0xC1EA_0000 ^ label.len() as u64);

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.

Low (readability): label.len() gives the same seed for "LT" and "EQ" (both are length 2), so salting by label length does nothing here. It's harmless because the tables differ, but a fixed seed (or one passed in per table) would be clearer.

path: |
formal/.lake
!formal/.lake/packages/mathlib
key: lake-${{ hashFiles('formal/lean-toolchain', 'formal/lake-manifest.json') }}-${{ github.sha }}

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.

Low (CI): Because the key ends in github.sha, every push saves a new .lake cache entry. That cache includes the Clean and CompPoly builds, so entries will pile up and push older caches (including the Rust ones) out of the repo's 10 GB cache quota. Consider keying only on toolchain + manifest, plus a hash of formal/**/*.lean if incremental builds matter.

@claude

claude Bot commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Review summary

No critical, high or medium issues. The Rust changes are test-only, apart from making Multiplicity::evaluate_with public. The differential harness in lean_diff.rs is sound:

  • eval_program_base asserts that a constraint reads only the current row, so a constraint that also reads the next row fails loudly rather than being skipped.
  • Unknown export nodes or channels panic.
  • Column counts are asserted to match.
  • Honest traces include a padding row.
  • The expected LT differences (μ·(1−μ) vs. ARE_BYTES[μ,0]) are pinned exactly, so any further drift fails the test.

Minor notes (two are posted inline):

  • Low: formal.yml cache key ends in github.sha, which saves a new cache entry on every run and adds pressure on the repo's cache quota.
  • Low: the seed 0xC1EA_0000 ^ label.len() is the same for LT and EQ, so the label salt has no effect.
  • Low: git diff --exit-code -- export does not catch untracked new export files. If a chip is added to Export.lean without committing its JSON, the check still passes. git status --porcelain -- export with an empty-output check would cover that.
  • Low: Multiplicity::evaluate_with became pub only for this test. That's fine, but #[doc(hidden)] or a short note would make the intent clear.
  • Note: BITWISE is modeled in Lean but is not exported or diff-checked like LT, EQ and SHIFT. Worth saying so in the README so readers don't assume it's covered.

🤖 Generated with Claude Code

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