feat(frost/roast): attribute lost-sync triggers; document permissioned-set acceptance#4119
Merged
mswilkison merged 2 commits intoJun 27, 2026
Conversation
…d-set acceptance When a seat enters lost-sync from a transition bundle for an attempt it never observed, the triggering bundle is operator-authenticated -- log the sending seat and the claimed attempt-context hash once per lost-sync episode, so the operational runbook can attribute and remove/slash a member spamming bogus-attempt bundles. (This is an authenticated-insider liveness halt the blame bridge does NOT close, because a never-observed attempt yields no evidence to attribute it.) markLostSync now uses CompareAndSwap and returns whether it transitioned, so the attribution is logged exactly once even while the listener keeps receiving such bundles. No change to the fail-closed semantics. Also document that this residual is accepted under the PERMISSIONED operator set (attributable + liveness-only + governance-removable) and MUST be revisited before any move to a permissionless set, where the f+1 snapshot-corroboration / resync fix would be warranted instead of accepting it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…ostsync-attribution-logging
mswilkison
merged commit f823ad1
into
feat/frost-schnorr-migration-scaffold
17 checks passed
mswilkison
added a commit
that referenced
this pull request
Jun 27, 2026
…ror type (#4122) ## Summary Addresses a Codex finding (relayed via the #4119 review): `TbtcChain.GetWallet` derived a **legacy** wallet ID on **any** error from the canonical `walletID` accessor. For a FROST wallet on a canonical Bridge, a transient call failure would silently yield the left-padded legacy ID — and callers use `WalletChainData.WalletID` to choose **P2TR (FROST)** vs **P2WPKH (legacy)** scripts, so the node would build or search the **wrong wallet script**. ## Why route by scheme (revised after Codex P1) The first revision distinguished by error type (a sentinel for the missing accessor, surface everything else). Codex correctly flagged a **P1 regression**: a *legacy on-chain Bridge* built with the *current* generated binding still satisfies the accessor interface, so its missing `walletID` function returns a normal RPC/ABI error — not the sentinel — and that revision would surface it and **break `GetWallet` on exactly the legacy deployments the fallback exists for**. Error type cannot reliably separate "function absent on-chain" from "transient." So this routes by **scheme**, using the wallet's `EcdsaWalletID` (which `GetWallet` already reads, and which the codebase already uses to infer scheme — zero ⇒ FROST): - **Legacy ECDSA wallet** (`EcdsaWalletID != 0`): its canonical wallet ID *equals* its legacy derivation, so fall back on **any** accessor error — and it's the only option on a legacy Bridge lacking the accessor. - **FROST wallet** (`EcdsaWalletID == 0`): requires the canonical ID; **surface** the error rather than return a wrong legacy ID. A FROST wallet only exists on a canonical-ID Bridge, so such an error is genuinely transient. Logic is extracted into `resolveWalletID(bridge, walletPublicKeyHash, ecdsaWalletID)`. ## Tests `TestResolveWalletID` covers all four cases: accessor success → canonical; FROST + accessor error → surfaced; **legacy + accessor error → legacy fallback** (the P1 regression guard — verified to fail if the routing surfaces errors for legacy wallets); legacy + missing-accessor binding → legacy fallback. gofmt + `go vet` clean; full `pkg/chain/ethereum` suite passes. _Found during the Codex review batch on #4115–#4120; revised per the Codex P1 re-review._ Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
Follow-up to #3866. A small, behavior-preserving hardening + decision record for the ROAST transition-exchange lost-sync path (review finding #2).
Background. In
onBundle, a seat that receives a transition bundle for an attempt it never observed tripsmarkLostSync()— failing the wallet's signing retry loop closed — before any verification (a behind-seat lacks the observe handle fullVerifyBundleneeds). So an authenticated group member can broadcast a structurally-valid bundle with a bogus, never-committed attempt hash and halt every honest seat's signing. The blame bridge (PR2b-2) does not close this: a never-committed attempt produces no evidence to attribute the sender.Decision (after Codex review + threat analysis). Under the permissioned operator set this residual is accepted: it's liveness-only (fail-closed — never an unsafe/divergent signature), the triggering seat is operator-authenticated (so attribution is immediate), and a misbehaving operator is governance-removable + economically deterred. The proper fix (f+1 snapshot-corroboration, or a resync state) is a real ROAST protocol change whose simple form can fracture the group on legitimate sparse-failure bundles — disproportionate while the set stays permissioned.
Change
markLostSyncnow usesCompareAndSwapand returns whether it transitioned, so the attribution is logged exactly once even while the listener keeps receiving such bundles. No change to the fail-closed semantics.Verification
gofmt+go vetclean; builds under-tags 'frost_native frost_roast_retry cgo frost_tbtc_signer'; the transition-exchange / lost-sync / bundle tests pass. The only caller ofmarkLostSyncis the updated site.Found during review of #3866.
🤖 Generated with Claude Code