fix(wallets): make WalletDB.deleteAccount atomic - #25219
Conversation
Same defect as storeAccount (audit finding 12 vs 5): four parallel account entry deletes plus a separate alias cleanup, with no transaction. A failure partway through leaves the account entries gone but the alias behind, or any other partial mix. Wrap the whole body in store.transactionAsync() so the deletes commit all-or-nothing; the alias scan reads through the pending write transaction so its semantics are unchanged. Extends the crashingStore test helper to also inject failures on delete, and pins rollback: a failed alias delete must leave the account fully intact. Also documents that deleteAccount does not deregister the account from the PXE, per the audit's recommendation.
vezenovm
changed the title
The PXE exposes no account removal API, so telling callers to deregister separately prescribed an impossible action. State the true contract instead: deletion is local to this store and PXE state is unaffected.
…:AztecProtocol/aztec-packages into mv/f-548-wallet-db-atomic-delete-account
mverzilli
left a comment
mverzilli
left a comment
There was a problem hiding this comment.
Approved but note that test coverage only is assured for LMDB. I'd consider adding something to AztecKit's browser suite if we want to make sure this will behave well on browsers.
| } | ||
|
|
||
| /** | ||
| * Deletes an account's stored data and its alias atomically. Deletion is local to this store; |
There was a problem hiding this comment.
Not introduced here, but since the doc now promises we delete "its alias": #readAccountAliases is keyed by address, so if an account ever got a second alias we'd only delete one of them and the other would keep pointing at a deleted address. Is one-alias-per-account an invariant somewhere? If not, then maybe we could fix it or add a TODO? Just checking
|
|
||
| it('deletes no account data when a write fails midway through deleteAccount', async () => { | ||
| const store = await openTmpStore('wallet-db-atomicity-test'); | ||
| const db = new WalletDB(store, () => {}); |
There was a problem hiding this comment.
Tiny thing: this db shadows the one from the outer describe. Could we name it seedDb or similar? Otherwise if the const ever gets dropped we'd seed the beforeEach store instead and the failure would point somewhere confusing.
| this.accounts.delete(accountKey('signingKey', address)), | ||
| ]); | ||
| // Clean up alias if one exists | ||
| const aliasesByAddress = await this.#readAccountAliases(); |
There was a problem hiding this comment.
Small thing: this scan is now inside the transaction, so we hold the write lock while iterating every alias to find one. Could we just resolve the alias before opening the transaction? Not sure how much it matters in practice
3 participants
Follow-up to #25216, fixing the same defect in
WalletDB.deleteAccount: four parallel account entry deletes plus a separate alias scan and delete, with no transaction. A failure partway through could leave any partial mix, e.g. the account entries gone with a stale alias left behind.The whole body now runs inside
store.transactionAsync(), so the deletes commit all-or-nothing.The red/green test reuses the
crashingStorehelper from #25216, generalized to inject failures ondeleteas well asset. It fails the alias delete, which runs after the four account entry deletes, then asserts the account is fully intact. Pre-fix, the account entries were deleted while the stale alias survived (retrieveAccountthrew "does not exist").deleteAccountnow carries a jsdoc note that deletion is local to this store and leaves PXE state unaffected.