refactor(be): one write path for account reference lists - #4232
refactor(be): one write path for account reference lists#4232sea-snake wants to merge 2 commits into
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 22f5c00. Security Overview
Detected Code Changes
|
78329c3 to
28029ff
Compare
sea-snake
added
the
feature:tracked-default-accounts
Every counter derived from a reference list is now diffed inside `write_reference_list` instead of being incremented at the six call sites that store one. `update_counters` and `AccountType` are gone; the anchor, application and global counters follow from the list itself, the way `rebuild_identity_account_counters` already defines them. The application row is resolved before anything is written, so a write against a reaped or missing application writes nothing at all rather than storing the row and leaving the counters half applied. Writing an empty list is rejected. `with_account_mut` and `set_account_last_used` return `Result` so those faults are not collapsed into "account not found". Implements docs/ongoing/tracked-default-accounts.md §4 (D3, D4) and §8.1 (D19). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
28029ff to
9d290c1
Compare
| stored_accounts: self | ||
| .stored_accounts | ||
| .checked_add(1) | ||
| .expect("overflow in stored_accounts"), |
There was a problem hiding this comment.
Would it be easy to making the code panic-free in this PR?
There was a problem hiding this comment.
The field stored_accounts is a u64 value, it actually overflowing should be practically unreachable and cannot be considered a user error.
Plumbing this unreachable error through to the candid response would introduce an error that the frontend can't handle and should never expect. Similar to the salt not being set.
There's no handling of the error either in the canister, a genuine overflow would not be resolvable since it would lead to an invalid count e.g. stop counting or resetting the count.
I'm open to suggestions though :D
There was a problem hiding this comment.
Update, seems like I can plumb it through to the generic StorageError which is already intended for these edge cases.
There was a problem hiding this comment.
Done, both writes to the counter return now instead of trapping.
The overflow check moved into allocate_account_number, which already returns Result<_, StorageError>, with a new AccountsCounterOverflow variant. That let increment_accounts go away entirely, so StorableAccountsCounter is a plain storable type again like everything else under storable/, and the check sits on the line that produces the account number instead of one indirection away.
Worth being explicit about why it refuses rather than saturating, since saturating looks like the tidier way to be panic-free: allocate_account_number takes the account number from the counter, so at the ceiling saturating would hand out the same number forever, and since a named account's principal is calculate_account_seed(account_number, origin), two accounts at one origin would collide on a principal. Refusing is the only safe option there, and there's a comment saying so.
One more I'd missed, which matters more than the overflow: apply_reference_counter_deltas wrote the global counter with an expect. That's a regression in this PR rather than existing behaviour, main maps the same write to StorageError::ErrorUpdatingAccountCounter in update_counters, so it does that again and the function returns Result.
There's now a test that sets stored_accounts to u64::MAX and asserts the error, so the "unreachable in practice" claim above is at least pinned by something. I couldn't find a way to make the stable cell's set() fail from a unit test, so that second path is unverified beyond compiling.
Two writes to the accounts counter could trap. Neither can now. `allocate_account_number` takes the account number from the counter, so the increment must not wrap or saturate: either re-issues a number already in use, and two accounts at one origin would then derive the same principal. The check moves to the allocator, where `StorageError` already is, which lets `StorableAccountsCounter` go back to being a plain storable type with no logic on it, and puts the check on the line that makes the number. `AccountsCounterOverflow` is the refusal. `apply_reference_counter_deltas` wrote the global counter with an `expect`. It returns `Result` and maps that write to `ErrorUpdatingAccountCounter`, which is what the write path this replaced already did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // Update last used timestamp | ||
| storage_borrow_mut(|storage| { | ||
| storage.set_account_last_used(anchor_number, origin.clone(), account_number, time()); | ||
| let _ = |
There was a problem hiding this comment.
Should we propagate this error instead of discarding it? If updating last_used fails, the sign-in still succeeds but II may not record the account use correctly. What would this cause downstream?
|
|
||
| let deltas = ReferenceListDeltas::between(&previous, ¤t); | ||
|
|
||
| self.stable_account_reference_list_memory |
There was a problem hiding this comment.
The reference list is saved before the counters are updated. If the counter update fails, the list is saved but the counters are not. Can we make sure these changes succeed or fail together?
| let value = StorableAccountReferenceList::from_vec(account_references); | ||
|
|
||
| self.stable_account_reference_list_memory.insert(key, value); | ||
| self.write_reference_list( |
There was a problem hiding this comment.
| self.write_reference_list( | |
| let Some(result) = result else { | |
| return Ok(None); | |
| }; | |
| self.write_reference_list( |
If no matching reference was found, account_references is unchanged here. Can we return Ok(None) instead of passing the unchanged list to write_reference_list?
| self.accounts == 0 && self.references == 0 | ||
| } | ||
|
|
||
| fn apply(&self, accounts: u64, references: u64) -> (u64, u64) { |
There was a problem hiding this comment.
| fn apply(&self, accounts: u64, references: u64) -> (u64, u64) { | |
| fn apply( | |
| &self, | |
| accounts: u64, | |
| references: u64, | |
| ) -> Result<(u64, u64), StorageError> { | |
| let accounts = accounts | |
| .checked_add_signed(self.accounts) | |
| .ok_or(StorageError::AccountCounterOutOfBounds)?; | |
| let references = references | |
| .checked_add_signed(self.references) | |
| .ok_or(StorageError::AccountCounterOutOfBounds)?; | |
| Ok((accounts, references)) | |
| } | |
| // and on the call sites something like | |
| let (stored_accounts, stored_account_references) = | |
| deltas.apply(existing_accounts, existing_references)?; |
Maybe it would be better to return an error here instead of clamping the count to zero. If eviction removes a reference while the counter is already at zero, clamping hides the fact that the counter and stored references no longer match
Design: #4222. Overview: #4230.
The account reference list is written at six sites, and five counters plus a global gauge are derived from it at each one. Keeping them correct is a convention rather than a guarantee, and the PRs above this add two more write sites and a reverse index.
write_reference_listbecomes the only place a reference list is stored. It reads the previous row, diffs it against the new one, and derives every counter from that diff.update_countersandAccountTypeare gone.rebuild_identity_account_countersdefines them, so a write cannot disagree with a rebuild.with_account_mutandset_account_last_usedreturnResult, so a bookkeeping fault is not collapsed into "account not found".The derived counters equal what the six call sites computed, site by site; those two error paths and the empty-list refusal are the behaviour that does change.
Tests:
reference_list_write_path_tests(7), including written counters equalling a rebuild over a mixed workload, and a write against an unknown application writing nothing at all even on a zero-delta write.