From d589cb147c14b2ed314dc9675990ac89b2c41176 Mon Sep 17 00:00:00 2001 From: maclane Date: Wed, 1 Jul 2026 16:26:28 -0500 Subject: [PATCH 1/2] fix(pr971): resolve CodeRabbit review follow-ups (CI pinning, watchtower, docs, NatSpec) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups on PR #971 (extraction/frost-mirror-2026-05-26). Per-item status: 1. .github/workflows/ci-pr.yml (SHA-pin actions + persist-credentials): SKIPPED — file does not exist in PR #971 or on branch extraction/frost-mirror-2026-05-26; nothing to pin. 2. .github/workflows/nightly-formal-invariants.yml (same treatment): SKIPPED — file does not exist in PR #971 or on the branch. 3. docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md: FIXED — normalized non-canonical repo paths in the scope summary (and the L32 plan-doc reference) to the canonical layout (solidity/contracts/..., solidity/test/..., docs/rfc/frost-migration/). 4. docs/rfc/frost-migration/b1-implementation-plan.md: FIXED — deploy-script filename made consistent with on-disk solidity/deploy/48_deploy_frost_wallet_registry.ts (46_ -> 48_). 5. services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts: FIXED — saveChallengeRecords now maps through the existing normalizeSerializedRecord validator before JSON.stringify, mirroring the load path; atomic-write behavior preserved. 6. solidity/contracts/bridge/BridgeGovernance.sol: FIXED — NatSpec on the migrateLegacyFraudChallenges forwarder now states the target Bridge helper is intentionally stubbed and always reverts (MigrateLegacyFraudChallengesNotImplemented) until a future upgrade; function retained. 7. solidity/contracts/bridge/IBridgeLifecycleRouter.sol: FIXED — interface docs aligned with the shipped Bridge ABI: point implementers to frostLifecycleContext(walletPubKeyHash); note the absence of separate lifecycleRouter/frostWalletRegistry/ walletIDByWalletPubKeyHash getters. One-shot setLifecycleRouter note was already present and accurate. 8. services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts: FIXED — abortableDelay stores the handler in const onAbort and removes the abort listener in the timeout path too, preventing listener leaks across loop iterations; clearTimeout kept in the abort path. Co-Authored-By: Claude Fable 5 --- .../audit-prep-findings-2026-05-25.md | 8 +++---- .../frost-migration/b1-implementation-plan.md | 2 +- ...2TRWatchtowerChallengeRecordPersistence.ts | 3 ++- .../src/P2TRSignatureFraudWatchtowerLoop.ts | 21 ++++++++++--------- .../contracts/bridge/BridgeGovernance.sol | 8 +++++++ .../bridge/IBridgeLifecycleRouter.sol | 21 ++++++++++++------- 6 files changed, 39 insertions(+), 24 deletions(-) diff --git a/docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md b/docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md index daba79f8f..1a344b437 100644 --- a/docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md +++ b/docs/rfc/frost-migration/audit-prep-findings-2026-05-25.md @@ -1,9 +1,9 @@ # FROST migration audit-prep findings (2026-05-25) Sweep of the merged FROST migration surface -(`contracts/frost-registry/`, `contracts/bridge/`, -`test/frost-registry/`, `test/integration/utils/`, -`docs/frost-migration/`) for residual hygiene issues prior to +(`solidity/contracts/frost-registry/`, `solidity/contracts/bridge/`, +`solidity/test/frost-registry/`, `solidity/test/integration/utils/`, +`docs/rfc/frost-migration/`) for residual hygiene issues prior to external audit. Categorized by load-bearingness. ## Methodology @@ -29,7 +29,7 @@ external audit. Categorized by load-bearingness. ### MUST-FIX — stale plan-doc claims that mislead readers -All four are in `docs/frost-migration/d2-ecdsa-hard-retirement-plan.md` +All four are in `docs/rfc/frost-migration/d2-ecdsa-hard-retirement-plan.md` (the D-2.1 plan doc). After D-2.2 slice 1 (PR #447), several claims in this doc are factually wrong about runtime behavior: diff --git a/docs/rfc/frost-migration/b1-implementation-plan.md b/docs/rfc/frost-migration/b1-implementation-plan.md index c95449f17..aaf79b89d 100644 --- a/docs/rfc/frost-migration/b1-implementation-plan.md +++ b/docs/rfc/frost-migration/b1-implementation-plan.md @@ -109,7 +109,7 @@ re-checked in `submitDkgResult` to fail fast. ### B-1.4: Deploy script + Bridge fixture wiring - New deploy script at - `contracts/tbtc-v2/deploy/46_deploy_frost_wallet_registry.ts` + `contracts/tbtc-v2/deploy/48_deploy_frost_wallet_registry.ts` (depends on `Bridge`, `SortitionPool`, `RandomBeacon`; tags `["FrostWalletRegistry"]`). - Extend `test/fixtures/bridge.ts` to deploy the registry and diff --git a/services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts b/services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts index e9aae967e..a7a620d49 100644 --- a/services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts +++ b/services/watchtower/src/FileBackedP2TRWatchtowerChallengeRecordPersistence.ts @@ -49,7 +49,8 @@ export class FileBackedP2TRWatchtowerChallengeRecordPersistence async saveChallengeRecords( records: P2TRWatchtowerChallengeRecordJSON[] ): Promise { - const serializedRecords = `${JSON.stringify(records, null, 2)}\n` + const normalizedRecords = records.map(normalizeSerializedRecord) + const serializedRecords = `${JSON.stringify(normalizedRecords, null, 2)}\n` await this.assertStateFileUnchangedSinceLoad() await writeFileAtomically(this.filePath, serializedRecords) diff --git a/services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts b/services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts index 402777fde..18195b24a 100644 --- a/services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts +++ b/services/watchtower/src/P2TRSignatureFraudWatchtowerLoop.ts @@ -92,16 +92,17 @@ export function abortableDelay( } return new Promise((resolve) => { - const timeout = setTimeout(resolve, milliseconds) - - signal?.addEventListener( - "abort", - () => { - clearTimeout(timeout) - resolve() - }, - { once: true } - ) + const onAbort = () => { + clearTimeout(timeout) + resolve() + } + + const timeout = setTimeout(() => { + signal?.removeEventListener("abort", onAbort) + resolve() + }, milliseconds) + + signal?.addEventListener("abort", onAbort, { once: true }) }) } diff --git a/solidity/contracts/bridge/BridgeGovernance.sol b/solidity/contracts/bridge/BridgeGovernance.sol index 01e74af0d..388dd950b 100644 --- a/solidity/contracts/bridge/BridgeGovernance.sol +++ b/solidity/contracts/bridge/BridgeGovernance.sol @@ -1854,6 +1854,14 @@ contract BridgeGovernance is Ownable { /// classification is required since the legacy Bridge /// mapping was shared between the two lifecycles. /// @param challengeKeys Legacy challenge keys to migrate. + /// @dev The target `Bridge.migrateLegacyFraudChallenges` helper is + /// intentionally non-operational in the current Bridge + /// implementation: its body is stubbed out and it always + /// reverts with `MigrateLegacyFraudChallengesNotImplemented`. + /// Calls through this forwarder therefore revert until a + /// future Bridge upgrade swaps in the migration body. The + /// forwarder is kept in place so the governance ABI stays + /// stable across that upgrade (a body swap, not an ABI change). function migrateLegacyFraudChallenges( uint8 routerKind, uint256[] calldata challengeKeys diff --git a/solidity/contracts/bridge/IBridgeLifecycleRouter.sol b/solidity/contracts/bridge/IBridgeLifecycleRouter.sol index 49ae26cb6..c1327869f 100644 --- a/solidity/contracts/bridge/IBridgeLifecycleRouter.sol +++ b/solidity/contracts/bridge/IBridgeLifecycleRouter.sol @@ -7,20 +7,25 @@ pragma solidity 0.8.17; /// /// The Bridge dispatches FROST-scheme wallet lifecycle operations /// (closeWallet, seize, isWalletMember) to a router implementing this -/// interface. The router resolves the wallet's canonical walletID from -/// the Bridge's `walletIDByWalletPubKeyHash` mapping and forwards the -/// call to the configured `frostWalletRegistry`. ECDSA-scheme lifecycle +/// interface. The router resolves the configured FROST wallet registry +/// address and the wallet's canonical walletID from the Bridge's +/// `frostLifecycleContext(walletPubKeyHash)` view and forwards the call +/// to that registry. ECDSA-scheme lifecycle /// operations bypass the router entirely and continue to call /// `ecdsaWalletRegistry` directly, preserving the existing /// ownership/callback model for ECDSA wallets. /// /// The Bridge passes only the 20-byte wallet public key hash to the /// router; the router reads the rest of the lifecycle state from the -/// Bridge via its public view functions (lifecycleRouter, -/// frostWalletRegistry, walletIDByWalletPubKeyHash). This keeps the -/// per-call-site Bridge bytecode footprint minimal (one external call -/// with one argument) at the cost of one cross-contract view per -/// dispatch. +/// Bridge's single `frostLifecycleContext(walletPubKeyHash)` view, +/// which returns the configured `frostWalletRegistry` address and the +/// canonical walletID in one call. The Bridge does not expose separate +/// `lifecycleRouter`, `frostWalletRegistry`, or +/// `walletIDByWalletPubKeyHash` getters; folding them into +/// `frostLifecycleContext` keeps the Bridge implementation under the +/// EIP-170 deploy limit. This keeps the per-call-site Bridge bytecode +/// footprint minimal (one external call with one argument) at the cost +/// of one cross-contract view per dispatch. /// /// The router is stateless and immutable. `Bridge.setLifecycleRouter` /// is a one-time setter that reverts once a router has been set, so the From a60b49b36c87975399cfd153d6009744cd9e9e1b Mon Sep 17 00:00:00 2001 From: maclane Date: Wed, 1 Jul 2026 16:31:14 -0500 Subject: [PATCH 2/2] ci(watchtower): pin actions to SHAs + disable checkout credential persistence Addresses CodeRabbit findings 1 & 2 on the real workflow file introduced by PR #971 (services-watchtower.yml), rather than the non-existent ci-pr.yml / nightly-formal-invariants.yml the findings named. - Add `with: persist-credentials: false` to both `actions/checkout` steps. - Pin every `uses:` to a full 40-hex commit SHA (major version unchanged): - actions/checkout@v3 -> f43a0e5ff2bd294095638e18286ca9a3d1956744 # v3.6.0 - dorny/paths-filter@v2 -> 4512585405083f25c027a35db413c2b3b9006d50 # v2.11.1 - pnpm/action-setup@v4 -> b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4.3.0 - actions/setup-node@v3 -> 3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 SHAs resolved via `git ls-remote` against each action repo's tags (the annotated pnpm/action-setup tag pinned to its peeled commit). Scope limited to services-watchtower.yml (the workflow this PR introduces); pre-existing repo workflows are left untouched. Co-Authored-By: Claude Fable 5 --- .github/workflows/services-watchtower.yml | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/services-watchtower.yml b/.github/workflows/services-watchtower.yml index 130825133..4b34ecef3 100644 --- a/.github/workflows/services-watchtower.yml +++ b/.github/workflows/services-watchtower.yml @@ -24,10 +24,12 @@ jobs: outputs: path-filter: ${{ steps.filter.outputs.path-filter }} steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@f43a0e5ff2bd294095638e18286ca9a3d1956744 # v3.6.0 if: github.event_name == 'pull_request' + with: + persist-credentials: false - - uses: dorny/paths-filter@v2 + - uses: dorny/paths-filter@4512585405083f25c027a35db413c2b3b9006d50 # v2.11.1 if: github.event_name == 'pull_request' id: filter with: @@ -49,17 +51,19 @@ jobs: || needs.watchtower-detect-changes.outputs.path-filter == 'true' runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@f43a0e5ff2bd294095638e18286ca9a3d1956744 # v3.6.0 + with: + persist-credentials: false # The watchtower is a pnpm workspace (`pnpm-workspace.yaml`: typescript + # services/watchtower) and uses the `workspace:*` protocol, which yarn # classic cannot resolve -- so this service is installed with pnpm, not # the yarn flow used by `typescript.yml`. - - uses: pnpm/action-setup@v4 + - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4.3.0 with: version: "10.17.1" - - uses: actions/setup-node@v3 + - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: node-version: "22.x"