fix(frost): refuse scaffold key-group by default; tighten witness and message hygiene#3959
Merged
mswilkison merged 1 commit intoMay 22, 2026
Conversation
…iene Four findings from the independent PR #3866 review, bundled because they all sit in the same code seam (frost_native scaffold path + receive loops). 1. H1+H4 — scaffold key-group must be opt-in (was silently accepted) `pkg/tbtc/signer_material_resolver_build_frost_native_tbtc_signer.go` previously built signer material whose `KeyGroupSource` was the string `"legacy-wallet-pubkey"` — a placeholder derived from a sha256 of the legacy wallet public key rather than the output of a real FROST DKG run — and the FFI primitive at `pkg/frost/signing/native_ffi_primitive_transitional_frost_native.go` silently substituted the Rust signer's RunDKG key group when the payload's source was that placeholder. Together that meant a production deployment with placeholder material would have routed signing through whatever key group the Rust side happened to return without any operator-facing signal. Add an explicit, refuse-by-default opt-in: `KEEP_CORE_FROST_TBTC_SIGNER_ACCEPT_SCAFFOLD_KEY_GROUP=1`. The new `signing.AcceptScaffoldKeyGroupEnabled` helper is per-call (not cached), so flipping the env back to unset recovers fail-closed behavior without a restart. Both the resolver and the FFI primitive check the flag; both refuse with a clear error message that names the env var and the placeholder source. Existing scaffold-using tests (`TestBuildTaggedTBTCSignerRoundKeyGroup`, `..._LegacyKeyGroupSourceUsesRunDKGResult`, `TestRegisterSignerMaterialResolverForBuild_UsesDefaultProvider`, the `signingExecutor` suite in `pkg/tbtc`) opt in via `t.Setenv` to continue exercising the scaffold path; a new `TestRegisterSignerMaterialResolverForBuild_DefaultProviderRefusesScaffoldWithoutOptIn` pins the refuse-by-default behavior, and a new `TestBuildTaggedTBTCSignerRoundKeyGroup/legacy_source_mismatch_refused_without_opt_in` covers the FFI side. 2. M2+M3 — Bitcoin witness restoration refuses unsupported shapes `pkg/bitcoin/transaction_builder.go`'s `ReplaceUnsignedTransaction` restoration path handled only `len(previousInput.Witness) == 1` (the P2WSH-style single redeem script). Multi-element previous witnesses — what a P2TR script-path spend would carry — were silently dropped, leaving the replaced input with an empty witness that signing later couldn't recover. Out-of-scope for the current P2TR key-path FROST migration but a footgun the next person to touch this code would hit. Switch to an explicit `switch` over previous witness length: 0 leaves the replacement empty, 1 restores the redeem script as before, anything else fails loudly with a clear "only zero- or single-element pre-signing witnesses are currently supported" error. Lifting this to support multi-element witnesses needs a per-input policy (the replacement could legitimately differ in witness shape from the previous), so failing loudly is the safer shape today. Also remove the tautological inner `len(replacedInput.X) == 0` checks that the two outer refusals already guarantee. New regression test `TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsMultiElementPreviousWitness`. 3. M5 — first-write-wins on peer messages The three round-message receive loops (`native_ffi_primitive_transitional_frost_native.go` tbtc-signer contribution, `native_frost_protocol_frost_native.go` round one and round two) all did `receivedMessages[message.SenderID()] = message`, last-write-wins. That let a peer mutate its own contribution after the first send. ROAST evidence semantics call for first-write-wins, with bit-identical retransmissions being idempotent and conflicting retransmissions being dropped with a structured log entry. Each receive loop now checks `receivedMessages[senderID]` first. If present and the new message is byte-equal on the relevant payload fields (`Contribution{Identifier,Data}` for tbtc-signer, `Commitment{Identifier,Data}` for round one, `SignatureShare{Identifier,Data}` for round two), the duplicate is ignored; if different, the new message is dropped with a `protocolLogger.Warnf` line that names the sender. Three equality helpers (`buildTaggedTBTCSignerRoundContributionMessagesEqual`, `nativeFROSTRoundOneCommitmentMessagesEqual`, `nativeFROSTRoundTwoSignatureShareMessagesEqual`) plus a new package-level `protocolLogger` log channel. Verification (local, GOCACHE under /private/tmp): go test ./pkg/frost/... ./pkg/bitcoin go test -tags 'frost_native frost_tbtc_signer' ./pkg/frost/... ./pkg/bitcoin go test -tags 'frost_native frost_tbtc_signer' ./pkg/tbtc -run \ 'TestConfigureFrostSigningBackend|TestNewNode_ConfiguresFrostSigningBackend|TestSigningExecutor_Sign|TestRegisterSignerMaterialResolverForBuild|TestBuildTaggedTBTCSignerRoundKeyGroup|TestBuildTaggedLegacyCompatibleNativeExecutionFFISigningPrimitive|TestTransactionBuilder_ReplaceUnsignedTransaction' All pass. This is the safe-by-default tier of the PR #3866 review remediation; the M4 (ROAST bounded transition evidence) and M7 (ROAST-aware retry replacing the byte-identical tECDSA shuffle) tracks are separate multi-PR efforts, and L5 (FFI status-code semantics) is paired with a forthcoming tbtc-signer change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Base automatically changed from
fix/frost-readiness-ffi-safety-2026-05-22
to
feat/frost-schnorr-migration-scaffold
May 22, 2026 22:47
mswilkison
merged commit df53906
into
feat/frost-schnorr-migration-scaffold
15 checks passed
mswilkison
added a commit
that referenced
this pull request
May 22, 2026
…3960) ## Summary Closes the persistence-vs-execution gap that #3959 left open. Stacked on #3959. ### The gap #3959 fenced *creation* (resolver refuses to BUILD \`legacy-wallet-pubkey\` signer material without \`KEEP_CORE_FROST_TBTC_SIGNER_ACCEPT_SCAFFOLD_KEY_GROUP=1\`) and *result acceptance* (FFI primitive refuses to substitute the Rust-returned key group for the placeholder). What it left open: scaffold material persisted from a previous opted-in session can still drive the FFI signing path on later runs after the operator has unset the flag. The signing path feeds \`buildTaggedTBTCSignerDKGPlaceholderPublicKeyHex(identifier)\` — a non-curve-point 3-byte string like \`\"020001\"\` — into the Rust signer's \`RunDKG\` for every included participant. ### Fix Refuse to enter \`signWithTBTCSignerCoarseEngine\` when the payload's \`KeyGroupSource == \"legacy-wallet-pubkey\"\` and the env opt-in is not set. The fence sits immediately after the signer material is decoded, before the engine availability check, before member-set determination, before \`buildTaggedTBTCSignerRunDKGInputsForIncludedMembers\` runs — so placeholder participant pubkeys are never built when the flag is unset. - Per-call (not cached) — matches the contract on \`AcceptScaffoldKeyGroupEnabled\`. - Wraps \`ErrNativeCryptographyUnavailable\` so callers that already handle that condition treat this the same way. - Error message references both the env var and the placeholder source for actionable operator diagnostics. ### Tests - **New regression**: \`TestBuildTaggedLegacyCompatibleNativeExecutionFFISigningPrimitive_Sign_TBTCSignerPath_RefusesScaffoldMaterialWithoutOptIn\` registers a working mock engine, omits the env var, calls \`Sign\`, asserts the refusal references both the env var and the placeholder source, and confirms \`RunDKG\`/\`StartSignRound\`/\`FinalizeSignRound\` were never called. - **Opt-in additions** to existing scaffold-path tests so they continue to exercise the path past the fence: \`..._BootstrapVersion_InvalidCoarseSignatureFallsBack\`, \`..._NoEngineNoLegacyShare\`, \`..._AttemptVariationRunDKGConflictFallsBack\`, \`..._BootstrapVersion_AttemptVariationStartSignRoundConflictFallsBack\`, \`..._InvalidAttemptPolicy_DoesNotFallback\`, \`..._ConsumedAttemptReplay_DoesNotFallback\`. ## Verification Local (GOCACHE under \`/private/tmp\`): - \`go test -tags 'frost_native frost_tbtc_signer' ./pkg/frost/... ./pkg/bitcoin\` — PASS - \`go test -tags 'frost_native frost_tbtc_signer' ./pkg/tbtc -run 'TestConfigureFrostSigningBackend|TestNewNode_ConfiguresFrostSigningBackend|TestSigningExecutor_Sign|TestRegisterSignerMaterialResolverForBuild'\` — PASS ## Out of scope L5 (FFI status-code semantics) and M4 / M7 (ROAST bounded transition evidence + ROAST-aware retry) are tracked as separate follow-ups.
Merged
3 tasks
mswilkison
added a commit
that referenced
this pull request
May 22, 2026
…ield Extends the three FROST/tbtc-signer protocol message types with an optional 32-byte AttemptContextHash field that binds the message to a specific RFC-21 AttemptContext (introduced in Phase 1A). * nativeFROSTRoundOneCommitmentMessage * nativeFROSTRoundTwoSignatureShareMessage * buildTaggedTBTCSignerRoundContributionMessage Migration contract (Phase 1B intentionally limited): * Field uses omitempty -- absent on the wire when the sender has not bound the message to a context. Old peers continue to interop. * Receiver-side Unmarshal validates length-when-present (must be exactly AttemptContextHashFieldLength = 32) but does not yet match against the locally-computed context. Higher-level acceptance lands in a later RFC-21 phase behind a build tag. * Shared helpers in attempt_context_binding.go convert between the on-wire []byte form and the canonical [32]byte hash form. Senders use SetAttemptContextHash; receivers use GetAttemptContextHash to get the hash + presence flag. Equal-or-reject is extended to compare AttemptContextHash bytewise, so a peer that retransmits the same contribution but mutates the binding mid-stream triggers the existing first-write-wins reject path (introduced in PR #3959). 17 new tests cover: length validation; array<->slice round-trip without caller aliasing; per-message marshal/unmarshal round-trip with field absent and present; backward compatibility with pre-Phase-1B JSON; wrong-length rejection; equal-or-reject sensitivity to the new field. All pass under `go test -tags 'frost_native frost_tbtc_signer' ./pkg/frost/...` plus the pkg/tbtc regression subset. Refs RFC-21 (docs/rfc/rfc-21-*); stacked on Phase 1A (#3963).
mswilkison
added a commit
that referenced
this pull request
May 22, 2026
…n protocol messages (#3964) ## Summary RFC-21 Phase 1B: extends the three FROST/tbtc-signer protocol message types with an optional 32-byte `AttemptContextHash` field that binds each message to the deterministic `AttemptContext` introduced in Phase 1A (#3963). **No protocol behaviour change yet.** Receivers do not match the hash against a locally-computed context in this phase; that lands later behind a build tag. Phase 1B is purely the wire-format extension. Stacked on #3963. ## Affected messages - `nativeFROSTRoundOneCommitmentMessage` - `nativeFROSTRoundTwoSignatureShareMessage` - `buildTaggedTBTCSignerRoundContributionMessage` ## Migration contract | Property | Behaviour | |---|---| | Field tag | `json:\"attemptContextHash,omitempty\"` -- absent on the wire when the sender has not bound the message. | | Old peer compatibility | Pre-Phase-1B JSON unmarshals cleanly; field reports as absent via `GetAttemptContextHash`. | | New peer compatibility | New JSON with a 32-byte hash field unmarshals; `GetAttemptContextHash` returns `(hash, true)`. | | Validation | Unmarshal rejects wrong-length hash fields; len must be exactly 32 or absent. | | Equal-or-reject | The existing first-write-wins helper (`buildTaggedTBTCSignerRoundContributionMessagesEqual` from #3959) now compares `AttemptContextHash` bytewise, so a peer mutating the binding mid-stream is reported as a conflict. | ## Why split from Phase 1A Phase 1A (#3963) added the `AttemptContext` type with no consumers. Phase 1B (this PR) adds the wire-format extension. Splitting keeps the type definition reviewable independently of the wire change, since the wire change touches existing message structs in production code paths. ## Test coverage 17 new tests under `pkg/frost/signing/attempt_context_binding_test.go`: - `TestValidateAttemptContextHashField_AcceptsAbsentOrCorrectLength` (3 sub-tests) - `TestValidateAttemptContextHashField_RejectsWrongLength` (3 sub-tests) - `TestAttemptContextHashField_ArrayRoundTrip` - `TestAttemptContextHashField_ArrayToArrayAbsent` - `TestAttemptContextHashField_FromArrayDoesNotAliasCaller` - `TestRoundOneCommitmentMessage_OptionalFieldRoundTrip` (2 sub-tests) - `TestRoundOneCommitmentMessage_BackwardCompatWithOldJSON` - `TestRoundOneCommitmentMessage_RejectsWrongLengthHashField` - `TestRoundTwoSignatureShareMessage_OptionalFieldRoundTrip` - `TestRoundTwoSignatureShareMessage_BackwardCompatWithOldJSON` - `TestRoundTwoSignatureShareMessage_RejectsWrongLengthHashField` - `TestBuildTaggedTBTCSignerRoundContributionMessage_OptionalFieldRoundTrip` - `TestBuildTaggedTBTCSignerRoundContributionMessage_BackwardCompatWithOldJSON` - `TestBuildTaggedTBTCSignerRoundContributionMessage_RejectsWrongLengthHashField` - `TestBuildTaggedTBTCSignerRoundContributionMessagesEqual_HashFieldDifferentiates` - `TestRoundOneCommitmentMessage_JSONEncoderOmitsAbsentField` All pass under: - `go test -tags 'frost_native frost_tbtc_signer' ./pkg/frost/...` - `pkg/tbtc` regression subset: `go test -tags 'frost_native frost_tbtc_signer' -run 'TestConfigureFrostSigningBackend|TestNewNode_ConfiguresFrostSigningBackend|TestSigningExecutor_Sign|TestRegisterSignerMaterialResolverForBuild' ./pkg/tbtc/` ## Test plan - [ ] CI green. - [ ] Reviewer confirms the `omitempty`/optional contract is what we want as the *Phase 1* default (i.e., we are intentionally not rejecting messages that lack the field). - [ ] Reviewer confirms it's OK for the equal-or-reject helper to now treat \"contribution with hash\" and \"contribution without hash\" as unequal during the migration window (sketched in Migration contract table above).
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
1 participant
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.
Summary
Bundles four findings from the independent PR #3866 review that all sit in the same code seam (frost_native scaffold path + receive loops). Stacked on #3958.
H1+H4 — scaffold key-group must be opt-in (was silently accepted)
`signer_material_resolver_build_frost_native_tbtc_signer.go` built signer material with `KeyGroupSource: "legacy-wallet-pubkey"` (a sha256 placeholder, not a DKG output) and the FFI primitive in `native_ffi_primitive_transitional_frost_native.go` silently substituted the Rust signer's RunDKG key group when the source was that placeholder. Production deployments with placeholder material would have signed through whatever key group the Rust side returned without operator-facing signal.
Add a refuse-by-default opt-in: `KEEP_CORE_FROST_TBTC_SIGNER_ACCEPT_SCAFFOLD_KEY_GROUP=1`. The new `signing.AcceptScaffoldKeyGroupEnabled` helper is per-call (not cached), so flipping the env unset recovers fail-closed behavior without restart. Both the resolver and the FFI primitive check the flag; both refuse with a clear error that names the env var and the placeholder source. New regression test pins the refuse-by-default path; existing scaffold-using tests opt in via `t.Setenv`.
M2+M3 — Bitcoin witness restoration refuses unsupported shapes
`ReplaceUnsignedTransaction`'s restoration path handled only single-element previous witnesses (P2WSH redeem script). Multi-element witnesses (P2TR script-path) were silently dropped. Replace with an explicit switch: 0 elements → leave empty, 1 → restore as before, ≥2 → fail loudly. Removes the tautological inner `len(replacedInput.X) == 0` checks that the outer refusals already guarantee. New regression test `TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsMultiElementPreviousWitness`.
M5 — first-write-wins on peer messages
Three round-message receive loops (tbtc-signer contribution, FROST round one, FROST round two) did last-write-wins, letting a peer mutate its own contribution after first send. Switch to first-write-wins with byte-equal retransmissions idempotent and conflicting retransmissions logged via a new `protocolLogger` channel. Three message-equality helpers cover the three message types.
Out of scope (deferred to separate PRs)
Verification
Local (GOCACHE under `/private/tmp`):