Skip to content

fix(tbtc/signer): reject empty attestation statuses#4156

Merged
mswilkison merged 1 commit into
extraction/frost-signer-mirror-2026-05-26from
fix/signer-admission-attestation-2026-07-13
Jul 13, 2026
Merged

fix(tbtc/signer): reject empty attestation statuses#4156
mswilkison merged 1 commit into
extraction/frost-signer-mirror-2026-05-26from
fix/signer-admission-attestation-2026-07-13

Conversation

@mswilkison

Copy link
Copy Markdown
Contributor

Stack

Rust signer review-fix stack 2 of 2.

  • Depends on: #4155
  • Ultimate base: extraction/frost-signer-mirror-2026-05-26 (#4005)

What changed

  • Reject an empty or whitespace-only policy required_attestation_status with required_attestation_status_missing.
  • Reject an empty or whitespace-only candidate attestation_status with attestation_status_missing.
  • Compare statuses only after both normalized values are non-empty, avoiding a misleading mismatch reason for missing data.
  • Preserve case-insensitive, whitespace-normalized matching for legitimate non-empty values.

Review finding addressed

  • P2: reject empty attestation statuses before comparing them — an empty policy and empty candidate can no longer normalize to equality and pass admission.

Verification

  • cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --check
  • cargo check --locked --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets
  • cargo clippy --locked --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets -- -D warnings
  • cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml
  • cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml --bin admission_checker attestation
  • git diff --check

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ef72493e-cf60-46ea-a861-9aa00f404fc9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/signer-admission-attestation-2026-07-13

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mswilkison mswilkison force-pushed the fix/signer-admission-attestation-2026-07-13 branch from 3e800c8 to 92ecab4 Compare July 13, 2026 19:25
@mswilkison mswilkison force-pushed the fix/signer-admission-attestation-2026-07-13 branch from 92ecab4 to 07b4190 Compare July 13, 2026 19:41
mswilkison added a commit that referenced this pull request Jul 13, 2026
## Stack

Rust signer review-fix stack 1 of 2.

- Base: `extraction/frost-signer-mirror-2026-05-26`
([#4005](#4005))
- Follow-up:
[#4156](#4156) —
admission-attestation validation

## What changed

- Retain the `RefreshShares` symbol but fail closed with terminal code
`cryptographic_refresh_not_supported` until a multi-round, zero-constant
FROST refresh protocol exists.
- Bump the FFI contract from ABI 3.3 to 4.0 so ABI-3 consumers reject
the incompatible success-to-error response change during startup
negotiation.
- Remove the SHA-256 pseudo-refresh path, its process-local
pending-operation machinery, and tests that encoded invalid
refresh-success semantics.
- Prove rejected refresh requests leave DKG key packages, the public
package, lifecycle state, and persisted bytes unchanged.
- Anchor the first refresh deadline and overdue metric to durable DKG
creation time.
- Treat metadata from the retired synthetic refresh stub as
non-authoritative: it cannot postpone cadence, increment valid refresh
counts, or claim key continuity.
- Mark persisted synthetic refresh-only sessions without a DKG anchor
immediately overdue in both status and telemetry, including when the
system clock saturates at the Unix epoch.

## Review findings addressed

- P1: implement cryptographic refresh instead of hashing shares — the
unsafe one-shot operation now rejects and returns no replacement
material.
- P2: anchor the first refresh deadline to DKG creation — status and
telemetry share the same durable deadline helper.
- P2: bump the ABI for the incompatible refresh response — the endpoint
now reports ABI 4.0 with its minor version reset to zero.
- P1: mark unanchored legacy refresh sessions overdue — untrusted
refresh artifacts now resolve to an explicit, unconditionally overdue
sentinel instead of a sliding deadline.

The C symbol, request shape, response envelope, and reserved response
type remain present; the major version makes the changed status and JSON
semantics explicit.

## Verification

- `cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --check`
- `cargo check --locked --manifest-path pkg/tbtc/signer/Cargo.toml
--all-targets`
- `cargo clippy --locked --manifest-path pkg/tbtc/signer/Cargo.toml
--all-targets -- -D warnings`
- `cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml`
- `cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml
formal_verification_`
- `pkg/tbtc/signer/scripts/run_phase5_chaos_suite.sh`
- `git diff --check`

The TLA+ runner was not available locally because this machine has no
Java runtime; CI remains the authoritative model-check gate.
Base automatically changed from fix/signer-refresh-safety-2026-07-13 to extraction/frost-signer-mirror-2026-05-26 July 13, 2026 21:41
@mswilkison mswilkison merged commit 1679a60 into extraction/frost-signer-mirror-2026-05-26 Jul 13, 2026
20 checks passed
@mswilkison mswilkison deleted the fix/signer-admission-attestation-2026-07-13 branch July 13, 2026 21:41
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.

1 participant