Fix and prove the Bernstein-Yang word-by-word Montgomery specs (scrutineer #2522, #2523, #2524) - #2413
Draft
JasonGross wants to merge 1 commit into
Draft
Fix and prove the Bernstein-Yang word-by-word Montgomery specs (scrutineer #2522, #2523, #2524)#2413JasonGross wants to merge 1 commit into
JasonGross wants to merge 1 commit into
Conversation
The three Bernstein-Yang operations of the word-by-word Montgomery synthesis (msat, divstep_precomp, divstep) were the only operations in src/COperationSpecifications.v with no discharged `Lemma <op>_correct` in src/PushButtonSynthesis/WordByWordMontgomery.v, and their `Prop`s are pretty-printed verbatim into the `Postconditions:` doc-comment of every generated C/Rust/Go/Zig/Java file. Nothing ever checked them, and all three were false (scrutineer findings #2522, #2523, #2524): * #2524 msat_correct asserted `twos_complement_eval msat = m /\ valid msat`, but `valid` includes `eval msat < m`, so the conjunction is unconditionally false: the function returns exactly m. The generated comment therefore claimed `0 <= eval out1 < m`, which is verbatim the precondition of every Montgomery-domain function in the same file. * #2523 divstep_precomp_correct equated a field element with the unreduced power `((m - 1) / 2) ^ i` (no `mod m`), and with the wrong base: the emitted constant is `((m + 1) / 2) ^ i mod m`, i.e. 2^-i mod m, which is what src/PushButtonSynthesis/WordByWordMontgomery.v computes in `divstep_precompmod` and what Bernstein-Yang needs. * #2522 divstep_correct wrote the fifth output of the odd branch as `(eval (from_montgomery v) - eval (from_montgomery v)) mod m` (identically 0) instead of `r - v`, and its bounds line read `valid r1 /\ valid r1 /\ valid f1 /\ valid g1`, duplicating r1 and omitting v1. Verifying the claims against the definitions turned up further defects in the same specs that the report did not mention: * `twos_complement_eval` was `eval_twos_complement bitwidth n`, over the n Montgomery limbs, while msat, f and g carry sat_limbs = n + 1 limbs. For p256 (m >= 2^255) that reads m as a negative 256-bit number, so even `twos_complement_eval msat = m` was false, and the header line defining `twos_complement_eval z` in every generated file described a 4-limb evaluation applied to 5-limb arrays. * `valid f1 /\ valid g1` cannot hold: f1 and g1 have sat_limbs limbs and f may equal m. What holds is the saturated limb bound, which the bounds pipeline establishes. * `d1 = 1 - d` ignores that d is a two's-complement machine word; the Arithmetic theorem (BYInv.divstep_correct_full) is stated in terms of `Z.twos_complement bitwidth d`. * divstep needs the Bernstein-Yang preconditions (f odd, |f|, |g| and |d| bounded away from overflow), which were absent. The specs are now stated so that they are provable from the existing Arithmetic development, and are proved: * src/COperationSpecifications.v: msat_correct and divstep_correct take `sat_limbs`; `twos_complement_eval` is over sat_limbs limbs; the word d is interpreted with `Z.twos_complement`; msat gets the saturated limb bounds instead of `valid`; divstep_precomp gets `mod m` on both sides and the base `(m + 1) / 2`; divstep gets `r - v`, `valid v1`, bounds on d1/f1/g1 and the required preconditions. * src/Arithmetic/BYInv.v: new lemmas `divstep_valid` (v1 and r1 are valid), `select_valid`, `length_valid` and `eval_twos_complement_partition`. * src/PushButtonSynthesis/WordByWordMontgomery.v: discharged `msat_correct`, `divstep_precomp_correct`, `divstep_correct` and the corresponding `Wf_*` lemmas, following the other 16 operations, so the printed contracts are now theorems. The doc-comment context maps `twos_complement_eval` to the sat_limbs evaluation and adds `twos_complement` for the single word; the "Computed values" header defines both at the right width. `use_curve_good` additionally exports `1 < machine_wordsize` (already checked by check_args). * src/PushButtonSynthesis/Primitives.v: the correctness printer now strips leading `list_Z_bounded_by`/word-bound conjuncts and prints `Z.odd x = true` as "x is odd", so the new specs can be rendered. The generated files (21 curve/word-size combinations for C, Rust, Zig, Go, bedrock2 and 10 for Java) are regenerated with the rebuilt synthesis binaries; only the doc-comments and the "Computed values" header change, the code is unchanged. Scrutineer findings #2522, #2523, #2524. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes scrutineer findings #2522, #2523 and #2524 (one shared root cause).
The bug
The three Bernstein-Yang operations of the word-by-word Montgomery synthesis (
msat,divstep_precomp,divstep) were the only word-by-word Montgomery operations whoseDefinition <op>_correctinsrc/COperationSpecifications.vhad no dischargedLemma <op>_correctinsrc/PushButtonSynthesis/WordByWordMontgomery.v. ThoseProps are pretty-printed verbatim into thePostconditions:doc-comment of every generated C/Rust/Go/Zig/Java file, so the generated files shipped contracts that nothing ever checked, and all three were false:msat_correctassertedtwos_complement_eval msat = m /\ valid msat;validincludeseval msat < m, so this is unconditionally false (the output is exactlym). The generated comment claimed0 ≤ eval out1 < m, which is verbatim the precondition of every Montgomery-domain function in the same file.divstep_precomp_correctequated a field element with the unreduced power((m - 1) / 2) ^ i(nomod m) and used the wrong base: the emitted constant is((m + 1) / 2) ^ i mod m(i.e.2^-i mod m, the inverse of 2 to the iteration count), which is whatdivstep_precompmodcomputes. I confirmed this numerically against the shipped constants ofp256_64,p256_32andsecp256k1_montgomery_64.divstep_correctwrote the fifth output of the odd branch as(eval (from_montgomery v) - eval (from_montgomery v)) mod m(identically 0) instead ofr - v, and its bounds line readvalid r1 /\ valid r1 /\ valid f1 /\ valid g1.Verifying the report against the definitions turned up more defects in the same specs that the report did not mention:
twos_complement_evalwaseval_twos_complement bitwidth n(over thenMontgomery limbs) whilemsat,fandgcarrysat_limbs = n + 1limbs. For p256 (m ≥ 2^255) that readsmas a negative 256-bit integer, so eventwos_complement_eval msat = mwas false, and theComputed valuesheader definingtwos_complement_eval zin every generated file described a 4-limb evaluation applied to 5-limb arrays.valid f1 /\ valid g1can never hold (f1,g1havesat_limbslimbs andfmay equalm); the true statement is the saturated limb bound established by the bounds pipeline.d1 = 1 - dignores thatdis a two's-complement machine word;BYInv.divstep_correct_fullis stated withZ.twos_complement bitwidth d.divstep(fodd;|f|,|g|,|d|bounded away from overflow) were missing, so the spec was unprovable as stated.The fix
src/COperationSpecifications.v: the three specs are restated so that they are true and provable from the existing Arithmetic development.msat_correctanddivstep_correcttakesat_limbs;twos_complement_evalis oversat_limbslimbs;dis interpreted withZ.twos_complement;msatgets the saturated limb bounds instead ofvalid;divstep_precompgetsmod mon both sides and the base(m + 1) / 2;divstepgetsr - v,valid v1, the bounds ond1/f1/g1and the required preconditions.src/Arithmetic/BYInv.v: new lemmasdivstep_valid(the Montgomery-domain outputsv1,r1arevalid),select_valid,length_valid,eval_twos_complement_partition.src/PushButtonSynthesis/WordByWordMontgomery.v: dischargedLemma msat_correct,Lemma divstep_precomp_correct,Lemma divstep_correctplusWf_msat/Wf_divstep_precomp/Wf_divstep(added to thewf_op_cachehints), following the pattern of the other 16 operations, so that the printed contracts are theorems from now on. The doc-comment context mapstwos_complement_evalto thesat_limbsevaluation and addstwos_complementfor the single wordd; theComputed valuesheader defines both at the right width.use_curve_goodadditionally exports1 < machine_wordsize(already enforced bycheck_args; appended after the existing conjuncts so positional destructs elsewhere are unaffected).src/PushButtonSynthesis/Primitives.v: the correctness-stringification printer strips leadinglist_Z_bounded_by/ word-bound conjuncts (previously only a single trailing one was supported) and printsZ.odd x = trueasx is odd. Existing outputs are unaffected (no other generated file changes).word_by_word_montgomeryandbedrock2_word_by_word_montgomerybinaries. Only thePostconditions:/Preconditions:doc-comments ofmsat,divstep,divstep_precompand theComputed valuesheader change; the generated code is byte-for-byte unchanged.Example (p256_64, C)
Alternatives considered
n-limbtwos_complement_evaland only fixing the three reported lines: the resulting specs would still be false (see above) and unprovable.(2^i * eval (from_montgomery out1)) mod m = 1: equivalent, but the chosen form mirrorsdivstep_precompmodexactly and matches the existing style.f oddprecondition could not be rendered otherwise.Verification
All verification was done with the overlay build described in the team notes (local
coqc -q -R src Crypto ...against the installedcoq-fiat-crypto-with-bedrock, Rocq 9.4+alpha), not a fullmake.Numeric check of the report's claims (Python, against the shipped constants in
fiat-c/src/{p256_64,p256_32,secp256k1_montgomery_64}.c):eval(msat) == min all three;((m+1)/2)^i mod mequals the emitted precomp constant in all three and((m-1)/2)^i mod mdoes not (i = 741for the 256-bit moduli).Coq (all compiled successfully, in dependency order):
src/Arithmetic/BYInv.v,src/COperationSpecifications.v,src/PushButtonSynthesis/Primitives.v,src/PushButtonSynthesis/BYInversionReificationCache.vsrc/PushButtonSynthesis/{BaseConversion,DettmanMultiplication,SaturatedSolinas,SolinasReduction,UnsaturatedSolinas,WordByWordMontgomery}.v(the last one carries the three new_correctlemmas and the doc-comment generation)src/CLI.v,src/StandaloneOCamlMain.v,src/ExtractionOCaml/word_by_word_montgomery.vsrc/Bedrock/Field/Common/{Util,Tactics}.v,Translation/Proofs/{UsedVarnames,EquivalenceProperties,Flatten,LoadStoreList}.v,Common/Arrays/MakeAccessSizes.v,Common/Names/MakeNames.v,Translation/Parameters/Defaults.v,Stringification/Stringification.v,src/Bedrock/Standalone/StandaloneOCamlMain.v) andsrc/ExtractionOCaml/bedrock2_word_by_word_montgomery.vsrc/Bedrock/Field/Synthesis/New/{WordByWordMontgomery,UnsaturatedSolinas}.v(these consumeuse_curve_goodand the specs), compiled with the HEADsrc/Bedrock/Field/Common/Types.vBinaries and generated files: both
src/ExtractionOCaml/word_by_word_montgomeryandsrc/ExtractionOCaml/bedrock2_word_by_word_montgomerywere rebuilt from the extracted OCaml with the Makefile'socamlfind ocamloptinvocation, and all 136 generated files that requestmsat/divstep/divstep_precompwere regenerated viamake -f Makefile.examples <targets> WORD_BY_WORD_MONTGOMERY=... BEDROCK2_WORD_BY_WORD_MONTGOMERY=.... The 21fiat-jsonfiles came out byte-identical (JSON carries no doc-comments); the other 115 changed. A check overgit diffconfirms every added/removed line in the generated files is a comment line (/* */,//,///,//!).Compile checks of the regenerated files:
cc -Wall -Wno-unused-function -Wpedantic -Werror -con all 21 touchedfiat-c/src/*.c(thetest-c-filesflags) andcc -Wall -Wno-unused-function -Werror -Wno-error=unused-but-set-variable -con all 21 touchedfiat-bedrock2/src/*.c: all pass.go build ./...infiat-go: passes.zig build testinfiat-zig: passes (its test driver only exercises a subset of functions).Not verified (please rely on CI):
cargo/rustcon this machine, sofiat-rustwas not built. The Rust changes are//!////comment lines only.javac, sofiat-javawas not compiled (comment lines only).makeof the repository, includingsrc/Bedrock/End2End/**, the Haskell/JS extractions and CI's regeneration check.🤖 Generated with Claude Code
https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD