fix(tbtc): anchor the emergency rekey, and correct what the certified floor bounds - #4226
fix(tbtc): anchor the emergency rekey, and correct what the certified floor bounds#4226mswilkison wants to merge 7 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Two further commits, both from the analysis in #4222 and both required under every candidate design there.
The rotation warning gets its own gauge. It was computed but reachable only via readiness snapshots and the activation handshake — a hand-assembled loopback challenge that hard-errors once headroom reaches zero, which is exactly when an operator needs it. It can't be derived at scrape time from the two headroom gauges, because the workload term needs the node's largest local seat count, so the seat count now travels with the pair; that also lets the commit path recompute the warning without an inventory of its own. A partial computation leaves the previous reading standing rather than publishing half a pair — the two dimensions are only meaningful together, and a stale pair is more honest than a half-fresh one.
Verified: |
The engine has always exported frost_tbtc_trigger_emergency_rekey, but nothing in Go called it. With no Go caller the only way to arm the wallet kill switch was to stop the node and mutate its durable store out of band, which produces an uncertified local write: on restart the anchor sees local and remote agreeing, so an operator who restores the pre-rekey state file erases the kill switch aimed at them with no evidence anywhere in the anchor stream. Wire the call through callBuildTaggedTBTCSignerOperation so the durable write is compare-and-swapped onto the anchor stream before the call returns, which is what makes a later erasure detectable. Two properties worth stating, because both look like omissions: The path is deliberately not admission-gated. The operation wrapper takes no capacity reservation, and admission refuses all work once headroom reaches the rotation floor while the barrier keeps admitting until the certified window is genuinely exhausted. A kill switch that capacity accounting can veto is not a kill switch, so it runs unreserved in that band. The barrier, by contrast, stays mandatory: a poisoned anchor or an exhausted window fails the trigger. Every barrier refusal predicate is operation-independent, so a state in which this trigger is refused is one in which the node already refuses every signature-producing call - the switch is redundant there, not defeated, and its residual is availability rather than authority. No Rust change: the export exists at the pinned ref, so the ABI and ci/frost-signer-pin.env are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rotation runbook said the certified floor is "checked at startup against pins that the running node cannot influence." That holds against a compromised signer process, which cannot forge the offline authority's signature. It does not hold against the operator: every artifact naming the floor - certificate chain, init config, activation manifest - is a local file the operator owns, and the certificate sequence and digest are only ever compared against the init config. Replace the overclaim, and add a section stating plainly what does bound an operator-side downgrade: the artifact bundle moves in lockstep (each rotation must change the manifest hash and advance the manifest sequence and service epoch by one), the durable trust journal is append-only so a downgrade also needs a pre-rotation store snapshot, and the anchor service can retire the previous binding. Only the third is a real control, and it lives outside this repository. Two checklist items follow from that: the anchor-service binding-retirement rule, and a decision on where a monotonic floor is published if operator rollback is in scope. Nothing in keep-core can close that gap - every candidate pin is downstream of an operator-owned file - so the honest deliverable is the requirement, not a local check. The signed activation handshake already exports the values an external monitor needs to detect a downgrade today. Also name one condition that already failed closed obliquely: a durable trust head ahead of the configured pin now reports that the artifacts are older than the store, instead of surfacing as an endpoint identity mismatch. That is diagnosability only - reverting the bundle and the store together leaves the two equal and fires nothing - and the comment says so to stop a later reader mistaking it for a fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Go and Rust descendant validators disagreed about which references are admissible, in both directions. Rust alone refused a checkpoint store fingerprint change across generations; Go alone required the certified floor to be revision 1 of its service epoch. Neither gap is reachable today, since certificate endpoints force a revision-1 To reference and the fingerprint is pinned per certificate. But a divergence between the trees is not a latent nicety: the more permissive tree would accept a chain the other refuses on every store open, with no truncation or rebase path back. That is fail-closed, which is the safe direction, and still a dead store. Take the union rather than the intersection. This adds Rust's rule here; the mirrored genesis-floor check lands on the Rust side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion warning The headroom mirror refreshed only when a full readiness reconciliation succeeded. That left two blind spots. A node signing steadily and a node that had stalled looked identical at the scrape, because nothing republished between reconciliations. And pre-sign authorization caches the readiness snapshot per finalized point, taking a branch that deliberately publishes nothing, so an entire authorized batch could burn window inside one finality window with the gauge unchanged throughout. Publish from the acknowledgement install instead. Every successful compare-and-swap already holds an authenticated tip, which is exactly where these numbers are both fresh and free; the recorder's own comment says it was built for a caller holding the anchor mutex, which is this one. Both install paths funnel through the same readback, so the ordinary commit, the startup local-ahead commit, and read-recovery are all covered. Also export the rotation warning as its own gauge. It was computed but reachable only through readiness snapshots and the activation handshake - a hand-assembled loopback challenge that refuses to answer once headroom hits zero, which is when an operator most needs it. It cannot be derived at scrape time from the two headroom gauges, because the workload term needs the node's largest local seat count, so the seat count now travels with the pair. That also lets the commit path recompute the warning without an inventory: seat count only changes on DKG and retirement, both of which force a reconciliation that republishes it. Failure to compute either half leaves the previous reading standing rather than publishing a partial pair - the two are only meaningful together, and a stale pair is more honest than a half-fresh one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…raming The rotation runbook's cadence table and its lifetime warning were built from a single burn figure per input. Real fault-free burn is a range - k+3 to 3k+2 generations - because a call whose sweep prologue mutates state advances more than one generation, and the document's own note already retracted an earlier set of figures for a related reason without correcting the table itself. Any capacity number taken from a point value understates the window by roughly a factor of two, and these are the figures that feed anchor-window sizing decisions. Rebuild the table from the range, and correct the lifetime warning: at the expected seat count a store admits roughly 630-1400 full-size sweeps over its 63 rotations, not the 64-128 previously stated. Also record that the 64-certificate cap is a size artifact - each certificate must fit a bounded journal record - rather than a security property, so a reader does not mistake the ceiling for something the chain proves. Signers are equal-weighted, so seats per operator is groupSize divided by operatorCount and the expected holder is around five. The high-seat rows are sensitivity checks on operator-set size, not a stake concentration tail, and two comments that motivated their reasoning from a fifty- or hundred-seat holder now cite the real distribution. The admission comment also read better than the truth: the superseded four-seat ceiling excluded every operator, not just the larger ones. Documentation and comments only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every anchor-window sizing and rotation-cadence figure this project has is a code reading, not an observation. Fault-free burn is only bounded - between k+3 and 3k+2 durable generations per signed input, because a call whose sweep prologue mutates state advances more than one generation - and a factor of two decides how much window a node actually has and how often it must stop for an offline ceremony. Nothing published today closes that. The headroom gauges report a level, and that level resets at every rotation, so no rate can be taken across one; and no counter records how much work produced the consumption. Add both halves. In pkg/frost/signing, monotonic totals for generations and revisions consumed, incremented at the barrier only after an acknowledgement is validated and durably read back - so they count work the anchor witnessed rather than work attempted. An operation that advanced no generation spends no revision either, since the barrier skips the compare-and-swap when the tip is unchanged, and is counted as neither. In pkg/tbtc, a counter of admitted workflows. Every other admission counter records a refusal; this one records work allowed to proceed, which is what gives the consumption totals a unit. For pre-sign it ticks once per input, so consumption divided by admissions is generations and revisions per signed input directly. The measurement matters beyond capacity planning: the runbook now documents cadence as a range pending live-testnet measurement, and the threshold for whether the anchor design needs to change at all is a burn rate nobody has yet observed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
69a24d0 to
f200d71
Compare
Resolves all 20 confirmed findings from the multi-agent review of #4226. Documentation (docs/development/frost-anchor-rotation.adoc): - Propagate the corrected cadence figures from c07b475 to four stale sites that still cited the retracted 588-generation worst case at 4 seats. - Replace the equal-weighted sizing directive with stake-weighted framing and consistent stake-weighted wording throughout (also fixed in wallet.go and frost_native_signer_anchor_admission.go comments). - Drop the false no-rotation-warning claim from the Gaps section and mark the pre-activation checklist item done, naming the three existing gauges. - Add a subsection for the anchored emergency-rekey kill switch, recording its admission bypass, immutability, and absent operator-facing trigger. - Remove the stray bare + line and correct the bindingHash attribution in the pre-activation checklist parenthetical. Engine (native_frost_engine_tbtc_signer_registration_frost_native.go): - Qualify the TriggerNativeTBTCSignerEmergencyRekey doc comment to state that the CAS-before-return property holds only on the success path, documenting the invoke-then-commit window and the recovery condition. - Add ErrNativeTBTCSignerEmergencyRekeyArmedButUnverified sentinel for post-commit decode failures so the caller can distinguish 'switch armed but unverifiable' from 'did not run'. - Builder now returns the normalized reason string so the echo comparison in the entrypoint consumes a single source of truth. Admission (frost_native_signer_anchor_admission.go): - Split the single admissions counter into three workflow-specific counters (presign input, presign relay gate, native DKG) and remove the shared reserve() increment so each ratio has a defined unit. Update the reset helper, var-block, and registration docs to match. Inline: - recoverAcknowledgementLocked now publishes the refreshed headroom mirror before returning, matching the install path. - Fix the burn-rate numerator/denominator vocabulary mismatch at the skip-CAS branch comment. Tests: - New cgo-tagged test for the trigger wiring and barrier gating (success advances the anchor tip once; poisoned and exhausted-window cases refuse). - Extended barrier-commit tests to assert consumption advances on success and stays flat on the skip-CAS path; added zero-counter assertions for the three new admission counters across admission tests. - Added healthy and near-floor rotation-warning assertions to the headroom publication test. - Extended the readiness-observer test to thread rotationWarning and largestLocalSeatCount through readinessSnapshotWithHeadroom and assert the accessors. PR description updated to describe all six commits and soften the flock argument for the deferred trigger surface.
2 participants
Six fixes for the FROST native signer anchor path. Stacked on #4199 rather than folded into it, so that PR stays mergeable as-is.
None of these change the Rust signer:
frost_tbtc_trigger_emergency_rekeyalready exists at the pinned ref, so the ABI andci/frost-signer-pin.envare untouched.1. Route the emergency rekey through the state anchor (fdbfdd2)
The engine has always exported the kill-switch trigger, but nothing in Go called it — a repo-wide grep for the symbol returns nothing. With no Go caller, the only way to arm the switch is to stop the node and mutate its durable store directly. That write never passes through the anchored operation path, so it is never compare-and-swapped onto the anchor stream. On restart the anchor sees local and remote agreeing, and an operator who restores the pre-rekey state file first has erased a kill switch aimed at them with no evidence anywhere in the stream.
The fix routes the call through
callBuildTaggedTBTCSignerOperation, so the durable write is CAS-ed onto the anchor stream before the call returns. That is what makes a later erasure detectable.Two properties that look like omissions but are deliberate, and are documented at the entrypoint:
The exported call is single-flight by contract: the engine treats an armed event as immutable and no export clears it.
No runtime behaviour changes from this PR. The kill switch remains unreachable from any operator-facing CLI; the anchored path is landed so any trigger built on it will be certified. The remaining options for that trigger surface each need a decision the code cannot supply: what names the target session, whether the switch is per-wallet or node-wide, and what the recovery path is for an accidental trigger given the event is immutable. The #4222 exposure stays open until those answers exist. Note that an out-of-process CLI is specifically not a viable path here: with the running node holding the store's exclusive
flockit cannot write, and with the node stopped the write is unanchored — the same bug restated — but that constraint is one input to the design, not the whole reason the surface is deferred.Quarantine, fault scores, and
finalize_request_fingerprinthave the same missing-wiring shape but no runtime writers in the Rust crate at all — only test-support writers. A Go call into a nonexistent writer is not a fix, so those need a Rust-side writer first and are filed separately.2. Correct what the anchor certified floor bounds (eb6692c)
docs/development/frost-anchor-rotation.adocsaid the floor is "checked at startup against pins that the running node cannot influence." That holds against a compromised signer process, which cannot forge the offline authority's signature. It does not hold against the operator: every artifact naming the floor — certificate chain, init config, activation manifest — is a local file the operator owns, and the certificate sequence and digest are only ever compared against the init config. The activation manifest is a signed envelope read from disk, not fetched from chain.The doc now says that, and adds what genuinely bounds an operator-side downgrade:
bindingHashandactivationManifestSequence.Only (3) is a real control, and it lives outside this repository, so it becomes a pre-activation checklist item alongside a decision on where a monotonic floor is published if operator-side rollback is in scope.
No code change closes this gap, and the PR does not pretend otherwise: every candidate in-repo pin is downstream of an operator-owned file. Putting the floor reference into the activation manifest is not merely awkward but unsatisfiable —
manifestHashfeedsbindingHashfeeds the event root, which is the floor, so the manifest would have to commit to a hash of itself. A plain minimum-sequence integer would dodge that circularity but is security-null here, because an old certificate is only presentable together with its own old manifest, which would carry the old minimum.Worth noting for whoever picks up the checklist item: the signed activation-handshake attestation already exports
trustCertificateSequence,anchorServiceEpochand the certified floor revision/generation, so an external monitor retaining the maximum ever seen per operator detects a downgrade today with no protocol change.3. Diagnose a durable trust head ahead of the configured pin (eaae1fc)
A durable trust head ahead of the configured pin now says the artifacts are older than the store, rather than surfacing later as an opaque endpoint-identity mismatch. The comment states explicitly that this is not a bound on operator rollback — reverting the bundle and the store together leaves the two equal and fires nothing — so a later reader does not mistake it for a fix.
4. Publish restartable headroom on every acknowledged commit (cd83062)
The restartable headroom mirror and the rotation warning were previously published only on a full readiness reconciliation, so a steadily signing node and a stalled node looked identical at the scrape until the next reconciliation. Since pre-sign authorization caches readiness per finality window, an entire authorized batch could burn window invisibly inside one window. Every successful compare-and-swap already holds an authenticated tip, so the publication moves onto the commit path and the rotation warning follows the seat count that travelled with the previous publication (seat count only changes on DKG and retirement, both of which force a full reconciliation).
5. Correct the runbook cadence and seat-scaling framing (c07b475)
docs/development/frost-anchor-rotation.adoccarried stale workload numbers in several places. The corrected table derives each cell from the fault-free rangek+3..3k+2generations per input: at 4 seats the worst case is 294 generations (was 588 — the retracted figure), the range at the 5-seat expected holder is 13–26 sweeps per epoch (was 6–13), and at 20 seats 2–7 sweeps (was 1–2). The seat-sizing directive was also rewritten: the original framing assumed equal-weighted signers and recommended sizing against the 5-seat average; this PR removes that assumption, names the sortition as stake-weighted with replacement, and reframes the high-seat rows as the upper tail of the distribution reachable by operators with concentrated stake.6. Measure real anchor burn rate (69a24d0)
Every workflow that reaches the shared
reserve()path now increments its own counter, so the published burn-rate ratio has a single well-defined unit. The singleadmitted_totalcounter is replaced by three:admitted_presign_input_total,admitted_presign_relay_gate_total, andadmitted_dkg_total. The pre-sign reservation counts the per-input work; the relay-gate reservation counts the up-front authorization; DKG counts the per-local-seat reservation. The reset helper, var-block comment, and registration function doc were updated to match.Verification
New tests cover the payload builder (empty/whitespace reason, over-long and malformed session IDs), the response decoder (six rejection cases, including the engine retargeting a per-signing session to its wallet session), the default-build stub failing closed, the rotation warning gauge value in both healthy and near-floor scenarios, the observer threading warning and seat count through to the accessors, the consumption and admission counters advancing on commit and staying flat on the skip-CAS path, the new cgo-tagged wiring and barrier-gating for the trigger, and both the honest and rejected paths of the new trust-head assertion.