Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 59 additions & 6 deletions packages/miden-multisig-client/src/multisig.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)],
Expand All @@ -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,
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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);
});
});

Expand Down
47 changes: 38 additions & 9 deletions packages/miden-multisig-client/src/multisig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<AccountState> {
const state = await this.fetchState();
Expand All @@ -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);
Expand Down Expand Up @@ -345,25 +352,45 @@ 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<void> {
): Promise<boolean> {
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}`
);
}
}

const accountId = AccountId.fromHex(this._accountId);
const onChainCommitment = await this.getOnChainCommitment(accountId);
if (!onChainCommitment) {
return;
return true;
}

const incomingCommitment = normalizeHexWord(incomingAccount.to_commitment().toHex());
Expand All @@ -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<string | null> {
Expand Down
Loading