diff --git a/packages/miden-multisig-client/src/multisig.test.ts b/packages/miden-multisig-client/src/multisig.test.ts index ca648b60..70e96c16 100644 --- a/packages/miden-multisig-client/src/multisig.test.ts +++ b/packages/miden-multisig-client/src/multisig.test.ts @@ -541,7 +541,7 @@ describe('Multisig', () => { expect(mockWebClient.newAccount).not.toHaveBeenCalled(); }); - it('should throw when incoming state nonce is lower than local nonce', async () => { + it('keeps local state and refreshes config from it when GUARDIAN nonce is behind local', async () => { const config = { threshold: 1, signerCommitments: ['0x' + 'a'.repeat(64)], @@ -558,7 +558,8 @@ describe('Multisig', () => { 'https://rpc.devnet.miden.io' ); - mockWebClient.getAccount.mockResolvedValueOnce(mockedAccount('0x' + 'a'.repeat(64), 3)); + const localAccount = mockedAccount('0x' + 'a'.repeat(64), 3); + mockWebClient.getAccount.mockResolvedValueOnce(localAccount); mockAccountDeserialize.mockReturnValueOnce(mockedAccount('0x' + 'b'.repeat(64), 2)); mockFetch.mockResolvedValueOnce({ ok: true, @@ -570,11 +571,24 @@ describe('Multisig', () => { updated_at: '2024-01-02T00:00:00Z', }), }); + mockDetectConfig.mockReturnValueOnce({ + threshold: 2, + numSigners: 2, + signerCommitments: ['0x' + '1'.repeat(64), '0x' + '2'.repeat(64)], + guardianEnabled: true, + guardianCommitment: '0x' + 'd'.repeat(64), + vaultBalances: [], + procedureThresholds: new Map(), + }); - await expect(multisig.syncState()).rejects.toThrow( - 'incoming nonce 2 is not greater than local nonce 3' - ); + // GUARDIAN behind local (nonce 2 < 3): no throw, local kept, no overwrite, + // and the decision needs no on-chain round-trip. + await expect(multisig.syncState()).resolves.toBeDefined(); expect(mockWebClient.newAccount).not.toHaveBeenCalled(); + expect(mockRpcGetAccountDetails).not.toHaveBeenCalled(); + // Config refreshed from the authoritative local account (UI unfreezes). + expect(multisig.account).toBe(localAccount); + expect(multisig.threshold).toBe(2); }); it('should throw when incoming state nonce equals local nonce but commitment differs', async () => { @@ -608,9 +622,48 @@ describe('Multisig', () => { }); await expect(multisig.syncState()).rejects.toThrow( - 'incoming nonce 2 is not greater than local nonce 2' + 'incoming nonce 2 equals local nonce 2 but commitments differ' + ); + expect(mockWebClient.newAccount).not.toHaveBeenCalled(); + }); + + it('unfreezes Multisig.account after execute when GUARDIAN still lags by one nonce (regression, #343)', async () => { + const config = { + threshold: 1, + signerCommitments: ['0x' + 'a'.repeat(64)], + guardianCommitment: '0x' + 'c'.repeat(64), + }; + + const multisig = new Multisig( + mockAccount, + config, + guardian, + mockSigner, + mockWebClient, + undefined, + 'https://rpc.devnet.miden.io' ); + + // Post-execute: local advanced to nonce 1, GUARDIAN still reports nonce 0 + // (candidate not canonicalized yet). Before the fix this threw and left + // Multisig.account frozen at the pre-execute snapshot. + const localAccount = mockedAccount('0x' + 'a'.repeat(64), 1); + mockWebClient.getAccount.mockResolvedValueOnce(localAccount); + mockAccountDeserialize.mockReturnValueOnce(mockedAccount('0x' + 'b'.repeat(64), 0)); + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + account_id: multisig.accountId, + commitment: '0x' + 'b'.repeat(64), + state_json: { data: 'AQID' }, + created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-02T00:00:00Z', + }), + }); + + await expect(multisig.syncState()).resolves.toBeDefined(); expect(mockWebClient.newAccount).not.toHaveBeenCalled(); + expect(multisig.account).toBe(localAccount); }); }); diff --git a/packages/miden-multisig-client/src/multisig.ts b/packages/miden-multisig-client/src/multisig.ts index 9f05ab8f..7e4c5f88 100644 --- a/packages/miden-multisig-client/src/multisig.ts +++ b/packages/miden-multisig-client/src/multisig.ts @@ -287,7 +287,13 @@ export class Multisig { * Sync account state from GUARDIAN into the local Miden client store. * * If the GUARDIAN commitment differs from the local commitment (or the account - * is missing locally), the local store is overwritten with the GUARDIAN state. + * is missing locally) and the GUARDIAN state is safe to import, the local store + * is overwritten with the GUARDIAN state. When the GUARDIAN is merely *behind* + * local — e.g. the pushed execution delta has not been canonicalized yet + * (see OpenZeppelin/guardian#316) — the local state is already ahead and + * on-chain-verifiable, so it is kept as authoritative. Either way, config is + * refreshed from the resulting account so callers reading `Multisig.account` + * (e.g. the UI) observe the current state instead of a stale snapshot. */ async syncState(): Promise { const state = await this.fetchState(); @@ -304,9 +310,10 @@ export class Multisig { if (!localAccount || localCommitment !== guardianCommitment) { const accountBytes = base64ToUint8Array(state.stateDataBase64); const incomingAccount = Account.deserialize(accountBytes); - await this.ensureSafeToOverwriteLocalState(incomingAccount, localAccount); - await webClient.newAccount(incomingAccount, true); - accountForConfigRefresh = incomingAccount; + if (await this.isSafeToOverwriteLocalState(incomingAccount, localAccount)) { + await webClient.newAccount(incomingAccount, true); + accountForConfigRefresh = incomingAccount; + } } this.refreshConfigFromAccount(accountForConfigRefresh); @@ -345,17 +352,37 @@ export class Multisig { }; } - private async ensureSafeToOverwriteLocalState( + /** + * Decide whether GUARDIAN-provided state may overwrite the local store. + * + * Returns `false` — rather than throwing — when the GUARDIAN state is simply + * *behind* local (lower nonce). That happens whenever the execution delta the + * client pushed has not been canonicalized by the GUARDIAN's background worker + * yet (see OpenZeppelin/guardian#316), or permanently if that candidate was + * discarded (#312 / #319). In that case the local account is already ahead and + * is independently verifiable against chain (`verifyStateCommitment`), so it is + * authoritative and must be kept, not clobbered; the caller keeps local and + * refreshes config from it. + * + * Still throws for genuine divergence: an incoming state at the *same* nonce as + * local but a different commitment, or an incoming state whose commitment does + * not match the on-chain commitment. + */ + private async isSafeToOverwriteLocalState( incomingAccount: Account, localAccount?: Account, - ): Promise { + ): Promise { if (localAccount) { const localNonce = localAccount.nonce().asInt(); const incomingNonce = incomingAccount.nonce().asInt(); - if (incomingNonce <= localNonce) { + if (incomingNonce < localNonce) { + return false; + } + + if (incomingNonce === localNonce) { throw new Error( - `Refusing to overwrite local state: incoming nonce ${incomingNonce.toString()} is not greater than local nonce ${localNonce.toString()} for account ${this._accountId}` + `Refusing to overwrite local state: incoming nonce ${incomingNonce.toString()} equals local nonce ${localNonce.toString()} but commitments differ for account ${this._accountId}` ); } } @@ -363,7 +390,7 @@ export class Multisig { const accountId = AccountId.fromHex(this._accountId); const onChainCommitment = await this.getOnChainCommitment(accountId); if (!onChainCommitment) { - return; + return true; } const incomingCommitment = normalizeHexWord(incomingAccount.to_commitment().toHex()); @@ -372,6 +399,8 @@ export class Multisig { `Refusing to overwrite local state: incoming commitment does not match on-chain commitment for account ${this._accountId}` ); } + + return true; } private async getOnChainCommitment(accountId: AccountId): Promise {