Repository navigation
Integrate dev hardening set into master - #37
Draft
piotr-roslaniec wants to merge 167 commits into
Draft
piotr-roslaniec wants to merge 167 commits into
piotr-roslaniec wants to merge 167 commits into
Conversation
Port the upstream constant-time framework (common/constant_time.go, built on filippo.io/bigmod) and wire it, behind a default-off runtime toggle, into the secret-exponent modular exponentiations in Paillier Decrypt/Proof and the DLN, factor, and Paillier-Blum modulus proofs. Mitigates the timing side-channel gap (CVE-2023-26557 class) flagged in the threshold-ECDSA variant analysis. - common: EnableConstantTimeOps / DisableConstantTimeOps / IsConstantTimeEnabled toggle + CTModInt (ExpCT / ModInverseCT / MulCT) over filippo.io/bigmod. - Wired ops use odd moduli only; phi(N)-even inverses stay on math/big. - Default off: zero behavior/perf change unless EnableConstantTimeOps() is called. - Bump go directive 1.16 -> 1.23 (filippo.io/bigmod floor). - Tests: framework equivalence, per-proof byte-identical/verifies equivalence, and full constant-time keygen + signing end-to-end.
Close the secret-exponent coverage gap and fix correctness/robustness issues in the opt-in constant-time path: - Harden the remaining secret-exponent modexps: MtA ProveBobWC (h1^x, h1^y), ProveRangeAlice (h1^m), the ring-Pedersen trapdoor setup in keygen (h1i^alpha), and Paillier Encrypt (gamma^m) / HomoMult (c1^m). - Pad the exponent to a fixed width in ExpCT so its running time no longer leaks the secret exponent's magnitude. - ModInverseCT returns nil for non-coprime inputs (matching math/big.ModInverse) instead of a silent wrong value. - reduceToPaddedBytes reduces unconditionally (no secret-dependent branch). - Assert odd modulus at CTModInt construction (fail at build, not at Exp). - Remove the unused TimingProtection / ConstantTimeCompare / Mod() API (the jitter helper also discarded a rand error -> nil-deref panic path). - Document the hardening coverage scope in the package doc. - Add equivalence/regression tests for every new site.
Follow-up to the go.mod merge conflict resolution when rebasing the constant-time-hardening branch onto current master: go mod tidy moves the indirect dependencies (previously listed inline from the branch's older base) into their own require block with versions resolved against master's current module graph. No functional change.
The constant-time-hardening branch's tests were written against an older base, before this fork's own independent hardening added: (1) the 2048-bit floor on dlnproof.Verify's modulus (verifyMinModulusBitLen), (2) the mandatory per-ceremony SetSessionNonce on ECDSA keygen/signing Start(), and (3) the mandatory positive fullBytesLen argument on ECDSA signing constructors. Without these adaptations the ported tests fail/panic against current master even though the underlying constant-time crypto is unaffected: - crypto/dlnproof/constant_time_equiv_test.go: use 1024-bit safe primes (NTilde ~2048 bits) instead of 512-bit, matching the verifyMinModulusBitLen=2048 floor and this fork's own convention (ecdsa/keygen/prepare.go safePrimeBitLen=1024). - ecdsa/keygen/constant_time_e2e_test.go: call params.SetSessionNonce(big.NewInt(1)) before Start(), as required by ecdsa/keygen/round_1.go's fail-closed session-nonce check. - ecdsa/signing/constant_time_e2e_test.go: same SetSessionNonce call, plus pass the now-mandatory fullBytesLen=32 argument to NewLocalParty (ecdsa/signing/local_party.go's validateFullBytesLen).
Two small zero-risk backports bundled together (same file, adjacent sections): - Size errCh to concurrency instead of concurrency*numPrimes (backport of upstream bnb-chain/tss-lib commit 4c83ace). At most 'concurrency' goroutines run at once, each sending at most one error, so the larger buffer was wasted allocation. No functional change. - Fix p=2q+1 typo in GetRandomSafePrimesConcurrent doc comment (backport of upstream commit 27922e0). Doc-only; no functional change.
…_vss.Create Backport of upstream bnb-chain/tss-lib commit b7b73a0. samplePolynomial already sets v[0] = secret (crypto/vss/feldman_vss.go), so this assignment was a no-op. Verified against fork's current samplePolynomial before applying.
…g style consistent Backport of upstream bnb-chain/tss-lib commit 0629cff. Behaviorally identical (skip nil errs vs. include non-nil errs); pure style.
Adapt the generator check from bnb-chain/tss-lib commit d3c1af5, pinning v1.30.0 to match the four generated files in this fork.
Adapt the result handoff in common/safe_prime.go from public upstream commit 24bb7d3. Add bounded worker lifecycle tests for cancellation with no receiver and a full result buffer, plus ordinary result delivery. Test cleanup drains pending results and joins workers.
Adapt Xi and ShareID copying from public upstream commit 04bb840 and the roster-content guard from 1693884, limited to ecdsa/keygen/save_data.go. Preserve nil local secrets and existing pre-parameter and per-party pointer sharing. The nil saved-key diagnostic and focused ownership, subset-selection, and input-guard unit tests are additions for this branch.
Adapt the common prime-size and unit-domain guards from public upstream dc9b957. Adapt the Paillier sampling changes from public upstream b64213a to this base. Use an 18-bit minimum for the local safe-prime candidate range and separation condition. Propagate empty sampler results through direct callers while preserving function signatures and the ordinary 2048-bit challenge output. These are source-level adaptations, not cherry-picks. This base has no quadratic-non-residue sampling helper, and its ModProof is outside the scope of this change. Add bounded domain checks, modulus-width controls, and a fixed 2048-bit challenge digest captured from base 86bd1a3.
Validate helper inputs before indexed reads and coordinate-buffer allocation, require a commitment payload, and propagate nil signing point results through the existing error paths. Preserve base proof arities, optional-mode proof serialization, nil-key parameter skips, and valid routing and point encodings. Adapted from upstream commits: f1588ae (WC decoder arity) 8deab46 (sender metadata guards) 1a8e277 (embedded PartyID guards) 8069c6c (SortedPartyIDs.Keys guard) 6e0bcd4 (shared nullable-input guards) 58a131c (commitment minimum parts) cbb8fe4 (commitment boundary coverage) e65fb36 (Gob coordinate allocation bounds) 8acae9a (signing nil-result guards) 1693884 (PartyID.String content guard)
Include the message integer and fixed byte width in security-v2 signing SSIDs. Preserve legacy transcript selection and the 32-byte SSID encoding. Snapshot the caller's message when constructing a signing party so its context remains stable. Add context separation, matching-peer, fixed-width, message ownership, and legacy tests. SSID binding adapted from the ECDSA signing changes in upstream commits 71dad22 and b73eea7.
Validate ECDSA keygen decommitment part counts before hashing and point decoding. Adapted from public upstream commit 1693884.
Port the zero-exponent path, public exponent bounds, and toggle snapshot corrections from public PR #10 commits 3169e56 and cf15377. Adapt the default-on behavior from upstream PR bnb-chain#332 commit 44a95b2 on top of public PR #11 at 68686af. Keep the existing exported method and toggle signatures, add an explicit public exponent bound, and snapshot the mode across each multi-step operation. Use LambdaN for decryption's inverse so keys without PhiN remain supported. Restore prior toggle state in tests and select disabled baselines explicitly. Document the limited coverage of the bigmod path.
Raise the minimum Go version to 1.17 as required by the patched runtime. Preserve generated schemas and add binary encoding controls captured on the previous runtime.
Select x/crypto v0.52.0 and its required x/sys v0.45.0. Align the minimum Go version to 1.25.7 and development and CI toolchain to 1.26.8 while preserving runtime source and existing compatibility vectors.
…t v1.4.0 notes NOT ready to merge/push. Exploratory work pending reconciliation with the live #8/#10/#11/#17 stack (mswilkison) which already rebases+hardens the CT backport more rigorously than this commit (bounded exponent widths, overflow rejection, mode snapshotting) and already flips constantTimeEnabled's default directly. This commit's own contribution -- CT coverage for crypto/schnorr/schnorr_proof.go and ecdsa/signing/round_3-5.go, which none of #8/#10/#11/#17 touch -- is real and not yet duplicated elsewhere, but should land as an addition on top of that stack, not a competing branch. Kept locally, unpushed, for reference pending user/maintainer coordination.
…tion gap - Remove common/constant_time_init.go: PR #17 (mswilkison, stacked on #11) already flips constantTimeEnabled's default to 1 directly in common/constant_time.go, making a separate init() redundant. It was also actively wrong: dlnproof/constant_time_equiv_test.go (and this commit's own new schnorr test) generate their 'non-CT baseline' proof before calling EnableConstantTimeOps(), assuming an ambient disabled default -- an unconditional init() would silently make that baseline vacuous instead of failing loudly. - Update the COVERAGE doc comment in common/constant_time.go to list the Schnorr proof responses and ECDSA signing rounds 3-5 now covered, and to record crypto/mta.AliceEnd/AliceEndWC's Paillier-decrypt timing protection (upstream BNB 3709c25) as a known, deliberately-NOT-ported gap: upstream uses a sleep-based response-time normalizer (NewTimingProtection, ~200ms target + jitter), a different mechanism entirely from the bigmod constant-time path used everywhere else in this file. That primitive exists nowhere in this fork's lineage (checked master, both CT branches, and PRs #12-17). Porting it would inject a fixed ~200ms delay into every MtA share round -- a real latency cost that needs its own sign-off, not a mechanical extension of the existing pattern. - Fix CHANGELOG.md's now-stale 'has not yet published its own tagged release' line (contradicted by the new [1.4.0] section added earlier).
…_init.go Two spots still described CT-enablement as happening via a package init() in common/constant_time_init.go. That file was removed before this branch was ever pushed (superseded by PR #17's direct default-value change, constantTimeEnabled = 1 in common/constant_time.go) -- these two CHANGELOG lines were never updated to match and got carried forward by the cherry-pick onto this branch. Correct the mechanism description in both places.
Address review findings on PR #23: - The equivalence tests built their non-CT baseline without disabling constant-time mode. Since this branch defaults constantTimeEnabled to 1, the baseline actually exercised the CT path, and the bare deferred DisableConstantTimeOps leaked disabled state into subsequent Schnorr tests, making coverage order-dependent and dropping default-path coverage. Save, set, and restore the ambient mode via a withCTMode helper, and assert the baseline really is running with CT off. - Restore the changelog hardening entries to [Unreleased]. This fork has no tags or published releases, so a dated [1.4.0] heading advertised the security work as obtainable, and both comparison links pointed at a tag that does not exist.
Both workflows filtered pull_request events to base branch master. This fork stacks pull requests on each other's branches, so every stacked PR and every PR targeting dev ran with no checks at all: #17 and #23 currently report no checks, and the whole in-flight batch targeting dev is covered only by locally-run tests. Drop the pull_request branch filter so any PR gets CI regardless of base, and add dev to the push trigger so the integration branch is covered on merge.
Addresses review findings on PR #23's constant-time hardening entry: - CHANGELOG.md: tag Breaking Change #8 and the Added CT-symbols entry with PR #17/#23, extend the Composing PRs list (F2) - CHANGELOG.md: soften 'closing the timing side-channel' framing to scope it to the operations covered, cross-reference the mta AliceEnd/AliceEndWC gap instead of implying full closure (F6) - CHANGELOG.md: note the coverage broadening from secret-exponent-only to secret-operand operations (F7) - CHANGELOG.md: restore a 'Not ported / deferred' bullet for the mta gap so the dangling '(see below)' cross-reference resolves again (F8) - CHANGELOG.md: fix the benchmark command to actually run both BenchmarkExpCT and BenchmarkExpStandard, correct the mismatched sample-count claim (F10) - common/constant_time.go: note in the reduceToPaddedBytes NOTE that round_5's rx (a field-prime coordinate) is the one operand reduced into group-order space, and why that's still safe (F3) - common/constant_time_test.go: add BenchmarkMulCT/BenchmarkModInverseCT on a 256-bit-class modulus so the CHANGELOG's performance claim can cite the operations this PR's stack actually added, not just the 2048-bit ExpCT benchmark (F5)
Addresses review findings on PR #23's constant-time hardening in ecdsa/signing: - round_5.go: correct the SECURITY comment's operand list -- m is the public message hash, not secret; name rx = R.X() instead, the operand that actually needed flagging (F9) - constant_time_equiv_test.go (new): add targeted equivalence tests for round_3's thelta/sigma, round_4's thetaInverse, and round_5's si terms -- the five new MulCT/ModInverseCT call sites this PR's stack added had only e2e coverage before this (F13) - constant_time_e2e_test.go: expand TestE2EConcurrentConstantTime's doc comment to name the Schnorr and round 3/4/5 CT paths it now also exercises, not just the pre-existing Paillier/MtA path (F14)
Author
Review record and decisions (2026-10-02)The review of this PR was not posted on GitHub. Its fixes were pushed directly to Fixes pushed to
|
| Commit | Fix |
|---|---|
873b8ad |
ModProof encodes its invariant secret exponents once per proof and wipes them on completion; canonical-operand constant-time helpers |
3b609ae |
ProveBobWC / ProveRangeAlice reject out-of-domain witnesses with an error instead of panicking; compatibility-enabled Bob verification checks nil inputs first |
20d66f8 |
Paillier Encrypt / HomoMult / Decrypt validate keys and return errors instead of panicking; per-key constant-time state cache |
c0978ef |
Mixed-binary harness requires both peers to reach round 8 and checks each accept-run proof at both bounds |
98f642d |
Regression test for mixed-curve ZKV constructor rejection |
fa6ef4d |
go.mod minimum restored to the documented Go 1.25.7 (PR #33 had lowered it to 1.25.6) |
Verified on fa6ef4d: CI Test, Vet, Keygen units and Go-fmt pass. Local race and shuffle tests pass on common, crypto/... and tss. The mixed-binary harness passes in both directions.
Decisions
- Paillier private-key cache: removed in fix(paillier): stop caching private-key decryption state; review follow-ups #38. It gave no measured
Decryptgain (70.7 → 70.6 ms) and kept unzeroed copies ofLambdaN-derived secrets in package-global state. Key validation and the public-key cache stay. - Changes reach
devonly through PRs. Branch protection is now on: PR required, the Test / Vet / Keygen units / Go fmt project checks must pass, and it applies to admins too. The changelog now attributes these fixes to their commits instead of "(PR Integrate dev hardening set into master #37)". - keep-core's first release hardcodes
ProtocolModeLegacy. Historical Bob compatibility is enabled through node config. - The compatibility flag logs a startup warning while enabled and is removed in the next keep-core release.
- Latency gate: at most a 2x slowdown. Measured as full keep-core signing time on
devvsmaster, at the same group size on the same hardware.
…ow-ups (#38) fix(paillier): stop caching private-key decryption state; review follow-ups
common.SignatureData is a protobuf message and embeds a mutex, so delivering it by value over the signing end channel made every receiver copy a lock (go vet copylocks). The end channel is now chan<- *common.SignatureData and the party stores its data by pointer, matching upstream BNB fbb0ef7. finalize sends a proto.Clone so the receiver owns a deep copy.
feat(signing)!: deliver signature results by pointer
List every composing PR, add breaking changes for the btcec/v2 curve type, keygen decoder signatures and the Go 1.25.7 minimum, record the new exported API, correct the AliceEnd constant-time gap description and the Paillier cache entry, and fix the README release test commands and audit scope.
docs: complete changelog and README for the dev integration
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M003 [P3] common/constant_time.go:292: GetCTModInt keeps an exported, unbounded, never-evicted global cache keyed by modulus - M004 [P3] common/constant_time.go:310: NewCTModIntWithPhi duplicates NewCTModInt's body and has one caller - M007 [P3] common/validation.go:21-30,39-41: The 65,536-bit modulus ceiling does not bound verifier CPU as its comment claims (ProbablyPrime is ... - M008 [P3] common/validation.go:33-48: Modulus-policy check is a two-call protocol (ceiling before primality) hand-ordered at each of 6 verifiers - M010 [P3] common/constant_time.go:13-14,34-38: common/constant_time.go header: garbled COVERAGE sentence, change-narration with no issue link, stale Encrypt ... - M014 [P3] common/constant_time_test.go:431-555: TestMulCTTimingConsistency, TestModInverseCTTimingConsistency and TestExpCTTimingConsistency can never fail - M017 [P3] common/random.go:80,98,123-130: New nil-return contracts are undocumented - M019 [P3] common/validation.go:13-18,24-26,33-38: common/validation.go modulus-policy comments: illogical Min rationale and false O(bitLen) claims - M059 [P2] crypto/dlnproof/proof.go:92-97: The #34 modulus policy (2048..65536 bits, odd, composite) is tested at one verifier and only for the ceiling - M093 [P3] ecdsa/signing/round_5.go:77: CHANGELOG, constant_time.go and round_5.go disagree on whether rx is secret; it is derived from public values ...
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M002 [P3] common/constant_time.go:207-215: The secret-wipe behavior advertised by 873b8ad has no assertion - M020 [P3] crypto/paillier/mod_proof.go:96: ModProof exponent wipe is not deferred, so a panic skips it; its doc comment says it also runs on abort - M021 [P2] crypto/paillier/paillier.go:194: Constant-time exponents are padded to N.BitLen() (2048) instead of public protocol bounds: HomoMult and the ... - M023 [P3] crypto/paillier/key_reuse_regression_test.go:81-119: TestKeyReuseAfterKeyValuesReplaced and TestByValueKeyCopyIsIndependent use the public-key N² cache only for ... - M026 [P3] crypto/paillier/mod_proof.go:368-405: Four standalone QR helpers are test-only copies of the context methods - M027 [P3] crypto/paillier/paillier.go:121-123,170-180,233-239: No test references ErrMessageTooLong or ErrMessageMalFormed - M035 [P3] crypto/paillier/mod_proof.go:65-70,232-236: mod_proof.go comments narrate the change history instead of describing the current design - M037 [P3] crypto/paillier/paillier.go:157,193,196: runtime.KeepAlive(publicKey) appears three times with no comment - M038 [P3] crypto/paillier/paillier.go:200-212: HomoAdd lacks the checkPaillierModulus guard the other Paillier operations gained - M039 [P3] crypto/paillier/paillier.go:259-271: The ErrMalformedKey doc says a non-invertible decryption coefficient is reported "consistently by both" modes - M040 [P3] crypto/paillier/paillier.go:284-285: The Proof doc runs two sentences together with no break: "...square-free from 3.1" then "It panics if..." - M050 [P2] crypto/mta/share_protocol_test.go:183-203: "Overwide ... must fail before exponentiation" MtA tests cannot tell the cheap bound check from the later ... - M059 [P2] crypto/dlnproof/proof.go:92-97: The #34 modulus policy (2048..65536 bits, odd, composite) is tested at one verifier and only for the ceiling
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M021 [P2] crypto/paillier/paillier.go:194: Constant-time exponents are padded to N.BitLen() (2048) instead of public protocol bounds: HomoMult and the ... - M042 [P2] crypto/mta/proof_bob_verifier.go:74-79: Nothing tests the upper edge of the opt-in historical-compat bound T1 < (q+1)*N - M043 [P2] crypto/mta/legacy_bob_compatibility_test.go:119-138: The security-v2 Bob T1 bound (q^7 + 1) is never exercised by a test - M044 [P3] crypto/mta/proof_bob_verifier.go:44-60: Bob and RangeAlice verifiers duplicate the same modulus/generator/ciphertext preamble - M045 [P3] crypto/mta/proof_bob_verifier.go:84-95: The default tight Bob bound is justified with the wrong prover, and "legacy" names two different provers - M046 [P3] crypto/mta/proofs.go:40-52,61-62,76-78,116: Constant-time path panics on even/nil/degenerate moduli in exported provers; 3b609ae no-panic claim overstated - M048 [P3] crypto/mta/share_protocol.go:105-185: Four near-identical "verify, then decrypt, then reduce mod q" functions - M049 [P3] crypto/mta/share_protocol.go:144-148: AliceEndLegacy doc comment has a truncated sentence about the historicalBobCompat=false case - M050 [P2] crypto/mta/share_protocol_test.go:183-203: "Overwide ... must fail before exponentiation" MtA tests cannot tell the cheap bound check from the later ... - M056 [P3] crypto/mta/proofs.go:92: Review-process labels appear with no definition in production comments, CHANGELOG, READMEs and CI: "PRIOR", ... - M058 [P3] crypto/mta/share_protocol.go:149-185: AliceEndLegacy and AliceEndWCLegacy with historicalBobCompat=false rejecting a historical-witness proof, and ... - M059 [P2] crypto/dlnproof/proof.go:92-97: The #34 modulus policy (2048..65536 bits, odd, composite) is tested at one verifier and only for the ceiling
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M059 [P2] crypto/dlnproof/proof.go:92-97: The #34 modulus policy (2048..65536 bits, odd, composite) is tested at one verifier and only for the ceiling - M060 [P2] crypto/dlnproof/proof_test.go:37-95: DLN Verify reject tests use N=23, so the 2048-bit floor rejects first and the named T/Alpha/nil checks are ... - M062 [P3] crypto/ckd/child_key_derivation.go:108-123: ckd curve type switch returns a nil-coordinate key with nil error for non-v2 secp256k1 curve objects
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M075 [P2] ecdsa/keygen/local_party.go:71: No test shows the keygen/signing constructors enforce the explicit protocol-mode rule (the headline runtime ... - M077 [P2] ecdsa/keygen/rounds.go:104-121: Legacy keygen transcript selection (proofSession/proofContext return nil) is not pinned by any test - M079 [P3] ecdsa/keygen/prepare.go:129: Keygen ring-Pedersen trapdoor inverse (beta = alpha^-1 mod pq) stays variable-time, contrary to Breaking ... - M083 [P3] ecdsa/keygen/rounds.go:125-131: The keygen getSSID doc still says "Callers must invoke this exactly once, in round 1", but legacy mode never ... - M088 [P3] ecdsa/signing/local_party.go:~110-122: The constructor docs omit the new caller obligation, which is the main compatibility break - M097 [P3] ecdsa/signing/signing_boundaries_test.go:32: Mixed-mode ceremonies (a legacy party among security-v2 peers, or the reverse) are not tested
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M075 [P2] ecdsa/keygen/local_party.go:71: No test shows the keygen/signing constructors enforce the explicit protocol-mode rule (the headline runtime ... - M084 [P2] ecdsa/signing/finalize.go:82: No test inspects the delivered *common.SignatureData (breaking change 9): R/S/recovery id/M width and the ... - M085 [P2] ecdsa/signing/round_5.go:102-111: The #36 store-before-emit ordering in signing rounds 5 and 7 has no regression test - M086 [P3] ecdsa/signing/local_party.go:156: The constructor copies msg (lines 157-159) to stop caller mutation changing the signing context, but ... - M088 [P3] ecdsa/signing/local_party.go:~110-122: The constructor docs omit the new caller obligation, which is the main compatibility break - M093 [P3] ecdsa/signing/round_5.go:77: CHANGELOG, constant_time.go and round_5.go disagree on whether rx is secret; it is derived from public values ... - M094 [P2] ecdsa/signing/round_ct_wiring_test.go:27: The signing "CT wiring" tests cannot fail if the constant-time branch is removed - M096 [P3] ecdsa/signing/signing_boundaries_test.go:297-376: The m = q / q-1 boundaries are covered only in legacy mode and not for the SSID or full-ceremony effect - M097 [P3] ecdsa/signing/signing_boundaries_test.go:32: Mixed-mode ceremonies (a legacy party among security-v2 peers, or the reverse) are not tested - M104 [P3] ecdsa/signing/signing_boundaries_test.go:361-367: The m = q-1 accept subtest receives from out with select { case m := <-out: } and no timeout
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M056 [P3] crypto/mta/proofs.go:92: Review-process labels appear with no definition in production comments, CHANGELOG, READMEs and CI: "PRIOR", ... - M122 [P2] testdata/legacy_transcript/verify.sh:22-37: The CI "bidirectional" oracle never checks that HEAD legacy provers produce proofs the historical verifier ... - M123 [P3] testdata/legacy_transcript/README.md: Mixed-binary evidence is narrower than claimed: no live mixed keygen, stops at round 8 - M124 [P3] testdata/legacy_transcript/README.md:30-46: The README describes the proof-source digest as SHA-256 over the listed repository files, which implies a ... - M125 [P3] testdata/legacy_transcript/oracle/main.go:225-227: The oracle vector holds no wide-witness historical Bob/BobWC proof, the shape the compat toggle exists for - M127 [P3] testdata/legacy_transcript/README.md:93,145,160: The fixture shape is called "2-of-20" in some places and "2-of-2" in others - M128 [P3] testdata/legacy_transcript/verify_mixed_interop.sh:16-17: The script header says the compat-on party accepts "the identical exchange" - M129 [P2] .github/workflows/test.yml:99-103: The live mixed-binary harness (verify_mixed_interop.sh) and its own tests never run in CI - M130 [P3] .github/workflows/test.yml:42,74,96: Documented Go 1.25.7 minimum is never built in CI
Fix the findings below from the review of PR #37 (ID, severity, review location: issue). - M001 [P2] common/constant_time.go:14-40: CHANGELOG and PR risk text overclaim constant-time coverage: MtA proof blinds and secret EC scalar ... - M015 [P3] common/int.go:104: CHANGELOG "Added" says IsInInterval and tss.SameCurve back the hardened range checks; IsInInterval has no ... - M024 [P2] crypto/paillier/mod_proof.go:184-199: CHANGELOG Breaking Change 4 says ModProof/FactorProof use HashToNTagged; no production code calls it - M056 [P3] crypto/mta/proofs.go:92: Review-process labels appear with no definition in production comments, CHANGELOG, READMEs and CI: "PRIOR", ... - M062 [P3] crypto/ckd/child_key_derivation.go:108-123: ckd curve type switch returns a nil-coordinate key with nil error for non-v2 secp256k1 curve objects - M067 [P3] crypto/commitments/commitment.go:55-71: CHANGELOG says canonical-generator checks were added in crypto/commitments; that package has none - M088 [P3] ecdsa/signing/local_party.go:~110-122: The constructor docs omit the new caller obligation, which is the main compatibility break - M093 [P3] ecdsa/signing/round_5.go:77: CHANGELOG, constant_time.go and round_5.go disagree on whether rx is secret; it is derived from public values ... - M113 [P3] tss/params.go:160-166: SetLegacyHistoricalBobCompatibility(false) panics outside legacy mode, contrary to the documented contract - M116 [P3] tss/params.go:226-227: SetSessionNonce godoc says keygen/signing fail closed without a nonce; true only in security-v2 - M123 [P3] testdata/legacy_transcript/README.md: Mixed-binary evidence is narrower than claimed: no live mixed keygen, stops at round 8 - M132 [P3] go.mod:1: Proposed release tag v2.0.0-threshold.1 is not consumable as a Go module version: module path has no /v2 ... - M136 [P2] README.md:63-98,145-150: README example reuses one Parameters (and session nonce) for keygen and signing, and never says Parameters is ... - M138 [P3] CHANGELOG.md:322-328: Breaking Changes 4 and 5 were not updated for dual-mode, and they contradict #3 (lines 335-350) and the ... - M140 [P3] CHANGELOG.md:478-480: CHANGELOG break-ledger blockquote: wrong verification baseline and incomplete list - M141 [P3] CHANGELOG.md:91-99: CHANGELOG claims four caller obligations are enforced at runtime; obligation 4 (and parts of 2/3) are not - M149 [P3] README.md:83: README keygen example passes *LocalPreParams where NewLocalParty takes LocalPreParams by value
- Samplers in common return nil instead of panicking when the limit is wider than the 5000-bit cap; MtA provers return an error then. The provers' own width pre-check and its mirrored constant are removed. - Replace paillier HomoMultWithBitLen with HomoMultBounded(m, c1, bound), which takes a public bound and rejects m outside [0, bound). New ErrInvalidBound sentinel. BobMid/BobMidWC pass q. - Signing round 2 checks local gamma and w are in [0, q) before the per-peer fan-out and blames no peer on failure. - Drop the six redundant ceiling pre-checks; IsUsableUnknownOrderModulus already checks width before ProbablyPrime. Unexport the helper. Modulus errors print the bit length, not the full value. - ckd decodes non-secp256k1 keys with elliptic.UnmarshalCompressed. - ModProof test hook proves the deferred exponent wipe runs on panic.
…y CI and harness comments
- DLN, Bob/Alice bound, verifier-gate and ModVerify policy tests use equation-valid inputs, so only the named check can reject them. - Exact T1 edges for default, compat and session bounds; S2/T2 shift tests; q-1 witness edges in constant-time mode. - Signing delivery check mutates all results before comparing; errors before the expected round fail the test; E2E helper has a deadline; constant-time E2E variants become one mode x CT table; KDD delta copy test. - Rename review-named test files after their behavior; replace hand- written panic helpers with testify; drop review labels.
fix: address PR #37 review findings
6 of 12 tasks
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.
Tracking PR for everything integrated on
devsincemaster(1cd3f0b). Draft: do not merge until the merge gates below are cleared.masteris protected: merging needs one approving review (not the author's) and passing Test, Vet, Keygen units and Go fmt project checks.Scope at
9ff573c: 152 commits, 137 files, +14,583 / −602. Already onmasterand not repeated here: #2, #4, #5, #6, #7 (BNB hardening base stack) and #22 (release workflow). Per-change detail, compatibility notes, and provenance live inCHANGELOG.md.Breaking changes for callers (introduced on
dev)Numbers refer to
CHANGELOG.md→ Breaking changes. keep-core currently pins2e712689, so it also absorbs the earlier breaks already onmaster(Breaking changes 1–7).fullBytesLen*common.SignatureData(9)tss.S256()concrete curve type is btcec/v2 / Decred (10)UnmarshalFactorProof,UnmarshalFactorProofTilde,UnmarshalProofIntsreturn errors (11)PRs merged into
devTooling and dependencies
masterbtcec/v2Constant-time hardening
masterbigmodconstant-time operations enabled by default (supersedes #8 and #10, both closed unmerged)(N+1)^m, exponent width narrowed to publicN.BitLen(),sync.Poolboxing allocation removedInput validation and lifecycle
ModProofreuses constant-time contexts across its 80 iterationsProtocol transcript
ProtocolModeLegacy/ProtocolModeSecurityV2transcript selection; exact historical legacy compatibility; default-off, rollout-only historical Bob compatibility switchfullBytesLen)*common.SignatureData(protobuf message with an embedded mutex) instead of a by-value copy; matches upstreamBNB fbb0ef7, with a deep copy on sendgodirective matches the documented minimumAssurance and docs
m = q/q−1, out-of-order round readiness2e712689binaryReview fixes pushed directly to
devMade in response to the review of this PR; the review record and decisions are in the PR comments.
devhas since been protected, so future changes arrive by PR only.873b8adModProofencodes its invariant secret exponents once per proof and wipes them on completion3b609ae20d66f8c0978ef98f642dfa6ef4dVerification on
devf8bbaff(plus #39 at6056170, #40 at9ff573c)go test -race -shuffle=on:common,crypto/...,tss,ecdsa/signingandecdsa/keygen(561 s) pass.govulncheck: 0 reachable vulnerabilities. 5 advisories exist in required modules, but their code isn't called.6056170): CI pass;ecdsa/signingrace tests, mixed-binary harness and transcript oracle pass.9ff573c): documentation only; CI pass.6056170:pkg/tecdsa/...(full DKG and signing protocol tests),pkg/tbtc/...,pkg/tbtcpg/...,cmd/...pass.Earlier verification on `e7be9a0` (before the review fixes)
go test -race -shuffle=on ./...: pass (keygen 2058 s, signing 397 s).govulncheck: 0 reachable vulnerabilities. 5 advisories exist in required modules, but their code isn't called.pkg/tecdsa/...compiled againstdev; its runtime tests failed closed until the migration in feat(tecdsa): adopt hardened tss-lib in legacy mode keep-core#4349.Merge gates
1. keep-core migration: PR open, tests pass
threshold-network/keep-core#4349 (draft) pins tss-lib
6056170and meets the caller obligations:ProtocolModeLegacyfor its first release.fullBytesLento every signing constructor: the 32-byte sighash width.tbtc.legacyHistoricalBobCompatibilityflag, off by default.It also receives signing results as
*common.SignatureData(#39) and setsGOTOOLCHAIN=autoin its Docker build stages for the Go 1.25.7 minimum. Its DKG, signing,tbtc,tbtcpgandcmdtests pass. The migration also uncovered and fixed a keep-core issue caused by tss-lib's btcec/v2 move: wallet keys from tss-lib and from keep-core's own parser used different secp256k1 curve objects, so identical keys compared unequal. Remaining: review and merge threshold-network/keep-core#4349, then move its pin to a tagged tss-lib release after this PR merges.2. Rollout plan: decided for the first release; security-v2 cutover deferred
ProtocolModeLegacy. It speaks the exact historical transcript, so upgraded and old nodes can still sign together.tbtc.legacyHistoricalBobCompatibility. The node logs a startup warning while it is on.ProtocolModeSecurityV2is a wire break that needs every signer to switch together. Deferred; options tracked in Design: coordinated cutover to tss-lib ProtocolModeSecurityV2 keep-core#4350.3. Latency: within budget at 10 members; production size not yet measured
Agreed budget: full keep-core signing on
devat most 2x slower than onmaster. Measured in keep-core with 10 members and threshold 6, 10 signatures per side:2e712689)devf8bbaffThat's 1.30x. DKG time is unchanged (about 33 s). The constant-time cost grows with group size, so at 100 members the ratio could move toward the per-operation Paillier decryption ratio (about 70 ms vs 33 ms, roughly 2.1x). Before release: re-measure at production size on dedicated hardware.
Next steps
In order. Steps 1–3 can run in parallel; 4 follows the merge; 5 is required before mainnet.
masterprotection requires one approval from someone other than the author.AGENTS.md/CONTRIBUTING.md; #4349 follows its existing style and Conventional Commits.dev. Pass: median ratio at most 2x (merge gate 3). Measured so far: 1.30x at 10 members; the ratio may approach the ~2.1x Paillier decryption ratio at 100.v1.4.0).CHANGELOG.md[Unreleased]under the tag; the release workflow publishes from it.pkg/tecdsa/...tests.Known risks and residual gaps
y = N − Twrap family). It is default-off, legacy-only, and frozen at party construction. keep-core bounds its lifetime: a node flag, a startup warning while enabled, and removal in the following release.bigmodpath. Surroundingmath/bigconversion and reduction remain variable-time; known instances are the round-5 reduction ofrxand the lack of response-time normalization around Paillier decryption. An independent side-channel review is advisable before mainnet.Follow-ups (non-blocking)
golang.org/x/cryptofrom v0.52.0 to v0.56.0 or later (GO-2026-6354/6355/6303). Replacegogo/protobufv1.2.1 (GO-2021-0053): bump to v1.3.2, or better, migrate to thegoogle.golang.org/protobufruntime the repo already uses.devstays inactive until this PR merges.staticcheckfindings are deprecatedelliptic.Curve.ScalarBaseMultcalls in tests and one ST1005 error-string style warning.