diff --git a/yarn-project/end-to-end/src/automine/accounts/multiple_accounts_1_enc_key.test.ts b/yarn-project/end-to-end/src/automine/accounts/multiple_accounts_1_enc_key.test.ts index a867a12c4772..f768a7a75d51 100644 --- a/yarn-project/end-to-end/src/automine/accounts/multiple_accounts_1_enc_key.test.ts +++ b/yarn-project/end-to-end/src/automine/accounts/multiple_accounts_1_enc_key.test.ts @@ -11,7 +11,9 @@ import { AutomineTestContext } from '../automine_test_context.js'; // Verifies that the PXE correctly handles multiple Schnorr accounts sharing the same encryption // key (different signing keys). Checks that note discovery and balance tracking remain accurate // across three accounts. Uses AUTOMINE_E2E_OPTS with 3 custom accounts sharing one secret. -describe('automine/accounts/multiple_accounts_1_enc_key', () => { +// TODO(F-835): unskip once the aztec-nr fix for #24821 is deployed and the PXE restriction on accounts sharing +// privacy keys is removed. Until then the key store rejects the shared-secret accounts this suite creates. +describe.skip('automine/accounts/multiple_accounts_1_enc_key', () => { let wallet: TestWallet; let accounts: AztecAddress[] = []; let logger: Logger; diff --git a/yarn-project/end-to-end/src/automine/token/crowdfunding_and_claim.test.ts b/yarn-project/end-to-end/src/automine/token/crowdfunding_and_claim.test.ts index c5ed4e47108f..ceaa3e7d4708 100644 --- a/yarn-project/end-to-end/src/automine/token/crowdfunding_and_claim.test.ts +++ b/yarn-project/end-to-end/src/automine/token/crowdfunding_and_claim.test.ts @@ -243,16 +243,22 @@ describe('automine/token/crowdfunding_and_claim', () => { // 1) Deploy another instance of the crowdfunding contract let otherCrowdfundingContract: CrowdfundingContract; { + const otherCrowdfundingSecretKey = Fr.random(); + const otherCrowdfundingPublicKeys = (await deriveKeys(otherCrowdfundingSecretKey)).publicKeys; const otherCrowdfundingDeployment = CrowdfundingContract.deploy( wallet, donationToken.address, operatorAddress, deadline, - { publicKeys: crowdfundingPublicKeys, deployer: operatorAddress }, + { publicKeys: otherCrowdfundingPublicKeys, deployer: operatorAddress }, ); const otherCrowdfundingInstance = await otherCrowdfundingDeployment.getInstance(); - await wallet.registerContract(otherCrowdfundingInstance, CrowdfundingContract.artifact, crowdfundingSecretKey); + await wallet.registerContract( + otherCrowdfundingInstance, + CrowdfundingContract.artifact, + otherCrowdfundingSecretKey, + ); ({ contract: otherCrowdfundingContract } = await otherCrowdfundingDeployment.send({ from: operatorAddress, // The contract constructor initializes private storage vars that need the contract's own nullifier key. diff --git a/yarn-project/end-to-end/src/guides/writing_an_account_contract.test.ts b/yarn-project/end-to-end/src/guides/writing_an_account_contract.test.ts index 8eae8db18827..c472e641cd63 100644 --- a/yarn-project/end-to-end/src/guides/writing_an_account_contract.test.ts +++ b/yarn-project/end-to-end/src/guides/writing_an_account_contract.test.ts @@ -92,7 +92,7 @@ describe('guides/writing_an_account_contract', () => { const wrongKey = GrumpkinScalar.random(); const wrongAccountContract = new SchnorrHardcodedKeyAccountContract(wrongKey); const wrongAccount = await (wallet as TestWallet).createAccount({ - secret: secretKey, + secret: Fr.random(), contract: wrongAccountContract, salt: Fr.random(), }); diff --git a/yarn-project/end-to-end/src/single-node/bot/bot.test.ts b/yarn-project/end-to-end/src/single-node/bot/bot.test.ts index 75a54e5b92f2..d06ee60ece52 100644 --- a/yarn-project/end-to-end/src/single-node/bot/bot.test.ts +++ b/yarn-project/end-to-end/src/single-node/bot/bot.test.ts @@ -188,8 +188,8 @@ describe('single-node/bot/bot', () => { } }); - // Changes the sender salt between attempts; asserts a new bridge claim is triggered even though - // the prior claim is in the store. + // Changes the sender private key between attempts; asserts a new bridge claim is triggered even + // though the prior claim is in the store. it('does not reuse prior bridge claims if recipient address changes', async () => { using saveSpy = jest.spyOn(store, 'saveBridgeClaim'); const config: BotConfig = { @@ -199,7 +199,7 @@ describe('single-node/bot/bot', () => { botMode: 'transfer', // this bot has a well defined private key and salt - senderPrivateKey: new SecretValue(Fr.fromString('0xcafe')), + senderPrivateKey: new SecretValue(Fr.fromString('0xbeef')), senderSalt: Fr.random(), l1RpcUrls, @@ -224,9 +224,9 @@ describe('single-node/bot/bot', () => { { saveSpy.mockClear(); - // same private key, but different salt derives a different L2 address, so the persisted claim does - // not apply and a fresh claim is bridged and saved - config.senderSalt = config.senderSalt!.add(Fr.ONE); + // a different private key derives a different L2 address, so the persisted claim does not apply + // and a fresh claim is bridged and saved + config.senderPrivateKey = new SecretValue(Fr.fromString('0xf00d')); await expect(Bot.create(config, wallet, aztecNode, aztecNodeAdmin, store)).resolves.toBeDefined(); expect(saveSpy).toHaveBeenCalledOnce(); } diff --git a/yarn-project/key-store/src/key_store.test.ts b/yarn-project/key-store/src/key_store.test.ts index 8b91fd89ee85..9f7784e9f305 100644 --- a/yarn-project/key-store/src/key_store.test.ts +++ b/yarn-project/key-store/src/key_store.test.ts @@ -101,6 +101,28 @@ describe('KeyStore', () => { } }); + it('allows re-registering the same account', async () => { + const keyStore = new KeyStore(await openTmpStore('test')); + + const privacyKeys = await deriveKeys(new Fr(8923n)); + const { address } = await keyStore.addAccount(privacyKeys, new Fr(243523n)); + + const { address: reRegisteredAddress } = await keyStore.addAccount(privacyKeys, new Fr(243523n)); + expect(reRegisteredAddress.equals(address)).toBe(true); + }); + + it.each(PRIVACY_SECRET_KEY_NAMES)('rejects registering a second account sharing only %s', async keyName => { + const keyStore = new KeyStore(await openTmpStore('test')); + + const privacyKeys = await deriveKeys(new Fr(8923n)); + const { address } = await keyStore.addAccount(privacyKeys, new Fr(243523n)); + + const otherPrivacyKeys = await deriveKeys(new Fr(1234n)); + otherPrivacyKeys[keyName] = privacyKeys[keyName]; + + await expect(keyStore.addAccount(otherPrivacyKeys, new Fr(243523n))).rejects.toThrow(address.toString()); + }); + it('rejects registering an account with secret key resulting in infinity public keys', async () => { const keyStore = new KeyStore(await openTmpStore('test')); diff --git a/yarn-project/key-store/src/key_store.ts b/yarn-project/key-store/src/key_store.ts index b714e3af031a..9bfafa9f4f00 100644 --- a/yarn-project/key-store/src/key_store.ts +++ b/yarn-project/key-store/src/key_store.ts @@ -106,6 +106,7 @@ export class KeyStore { * @param partialAddress - The partial address of the account. * @returns The account's complete address. * @throws If any of the account's six master public keys would be the point at infinity. + * @throws If a different account sharing any of the four master privacy keys is already registered. */ public async addAccount(keys: AccountPrivacyKeys, partialAddress: PartialAddress): Promise { const accountKeys = await completeAccountKeys(keys); @@ -359,6 +360,13 @@ export class KeyStore { const masterIncomingViewingPublicKeyHash = await hashPublicKey(publicKeys.ivpkM); await this.#db.transactionAsync(async () => { + await this.#assertPrivacyKeysNotInUse(account, { + n: publicKeys.npkMHash, + iv: masterIncomingViewingPublicKeyHash, + ov: publicKeys.ovpkMHash, + t: publicKeys.tpkMHash, + }); + // Naming of keys is as follows ${account}-${n/iv/ov/t}${sk/pk}_m. // // The message-signing and fallback keys are not stored: their secret keys are withheld from the key store, and @@ -384,6 +392,42 @@ export class KeyStore { return completeAddress; } + /** + * Throws if an account other than `account` is already stored with any of the given master public key hashes, keyed + * by key prefix. Must be called within a db transaction so the check and the subsequent writes are atomic. + * + * Noir does not currently handle different addresses sharing privacy keys correctly: two accounts with the same + * nullifier-hiding key compute the same PrivateMutable and PrivateImmutable initialization nullifiers, since those + * are derived from the key and the storage slot alone, without the owner address. Until that is fixed, PXE refuses + * to register such accounts. Note that a user registering the two accounts in separate PXEs can still run into the + * issue, as this check only sees locally registered accounts. + * + * TODO(F-835): remove this restriction once the aztec-nr fix for #24821 is deployed. + */ + async #assertPrivacyKeysNotInUse(account: AztecAddress, pkMHashes: Record) { + const collidingKeyByStorageSuffix = new Map( + KEY_PREFIXES.map(prefix => [ + `${prefix}pk_m_hash`, + { name: MASTER_KEY_NAMES[prefix], pkMHashBuffer: pkMHashes[prefix].toBuffer() }, + ]), + ); + for await (const [key, value] of this.#keys.entriesAsync()) { + const [existingAccount, suffix] = key.split('-'); + const collidingKey = collidingKeyByStorageSuffix.get(suffix); + if ( + collidingKey && + existingAccount !== account.toString() && + Buffer.from(value).equals(collidingKey.pkMHashBuffer) + ) { + throw new Error( + `Cannot register account ${account.toString()}: account ${existingAccount} is already registered with ` + + `the same master ${collidingKey.name} key, and accounts must not share master keys. Derive the new ` + + `account from a different secret key.`, + ); + } + } + } + /** * Fetches a stored master key buffer for an account by its storage suffix (e.g. `npk_m`, `ivsk_m`). * @throws If the account does not exist in the key store. @@ -410,3 +454,11 @@ export class KeyStore { return buffers; } } + +/** Human-readable master key names by key prefix, for error messages. */ +const MASTER_KEY_NAMES: Record = { + n: 'nullifier-hiding', + iv: 'incoming-viewing', + ov: 'outgoing-viewing', + t: 'tagging', +};