refactor(nns): add neuron slot reservation mechanism for create_neuron#9213
refactor(nns): add neuron slot reservation mechanism for create_neuron#9213jasonz-dfinity wants to merge 9 commits intomasterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
Introduce NeuronSlotReservation in NeuronStore — an RAII guard that reserves a spot under the neuron limit without creating a placeholder neuron. This allows create_neuron to reserve a slot, perform the async ledger transfer, and only create the neuron on success. On failure, the reservation drops automatically — no explicit cleanup needed. For claim_neuron, the reservation is used for consistent limit checking but the empty-neuron-before-async pattern is preserved because its deterministic subaccount must be claimed to prevent concurrent duplicate claims.
…a parameter Move the neuron limit from a parameter on try_reserve_neuron_slot to a field on NeuronStore, set by Governance via set_max_neurons. The constant MAX_NUMBER_OF_NEURONS remains in governance.rs. Tests use a helper to construct a NeuronStore with a custom limit.
jasonz-dfinity
force-pushed
the
jason/reserve-neuron-limit
branch
from
9bf5773 to
4206b78
Compare
NeuronStore::new and new_restored now use MAX_NUMBER_OF_NEURONS as the default limit, removing the need for callers to call set_max_neurons. The setter is replaced with a #[cfg(test)] builder method with_max_neurons for tests that need a custom limit.
There was a problem hiding this comment.
Pull request overview
This PR introduces an RAII-based “neuron slot” reservation mechanism to enforce the max-neuron limit across async boundaries without creating placeholder neurons, and refactors create_neuron / claim_neuron to use it.
Changes:
- Add
NeuronSlotReservationtoNeuronStorewithtry_reserve_neuron_slot()and reservation counting. - Refactor
create_neuronto reserve a slot, perform the ledger transfer, and only then create/add the neuron on success. - Update neuron-add paths to account for outstanding reservations, and add
add_neuron_with_reservationto bypass redundant limit checks when already reserved.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rs/nns/governance/src/neuron_store/neuron_store_tests.rs | Adds unit tests validating reservation creation, drop behavior, and limit enforcement. |
| rs/nns/governance/src/neuron_store.rs | Implements reservation guard + counter in NeuronStore and a new NeuronLimitReached error. |
| rs/nns/governance/src/governance/create_neuron.rs | Refactors create_neuron to use reservations and avoid placeholder neurons. |
| rs/nns/governance/src/governance.rs | Accounts for reservations in neuron-limit checks and adds add_neuron_with_reservation; updates claim_neuron to reserve a slot. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge 5 overlapping tests into 2 that cover all the same edge cases: - test_try_reserve_neuron_slot_respects_limit (success, failure, neurons + reservations toward limit) - test_neuron_slot_reservation_released_on_drop (RAII drop frees slot, re-reservation succeeds)
Restructure claim_neuron to create the neuron with the correct stake after the async ledger balance check, rather than creating an empty neuron before and updating/removing it after. Uses the neuron slot reservation to hold a spot under the limit across the async boundary, and checks subaccount uniqueness both before and after the await to prevent concurrent claims of the same subaccount.
- Add check_heap_can_grow() before async ledger calls in create_neuron and claim_neuron, so the heap check fails before funds move - Rename add_neuron -> add_neuron_without_reservation for clarity, consolidate with add_neuron_with_reservation via shared add_neuron
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Why
In
create_neuronandclaim_neuron, an empty placeholder neuron is created before the async ledger call to hold a spot under the neuron limit. If the call fails, the neuron must be explicitly removed in every error path — fragile and easy to get wrong.What
NeuronSlotReservationtoNeuronStore— an RAII guard (Arc<AtomicUsize>counter +Dropimpl) that reserves a spot under the neuron limit without creating a placeholder neurontry_reserve_neuron_slot(&self)takes&self(not&mut self) using interior mutability, so the reservation can be held across async boundaries without borrow conflictscreate_neuronto: reserve slot → transfer ICP → create neuron with correct stake only on success. On failure, the reservation drops automaticallyclaim_neuronsimilarly: reserve slot → check subaccount uniqueness → await ledger balance → re-check subaccount uniqueness → create neuron with correct stake. No empty placeholder neuron is created before the async call; duplicate claims are prevented by pre/post-await subaccount checksadd_neuronto account for outstanding reservations in its limit checkadd_neuron_with_reservationthat skips the limit check (already reserved)Testing
Added unit tests for the reservation mechanism: creation, drop behavior, limit enforcement with combinations of neurons and reservations.