Repatriate residual stake and locks on lease termination#2709
Conversation
l0r1s
requested review from
JohnReedV,
evgeny-s,
gztensor,
open-junius and
sam0x17
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE LOW scrutiny: l0r1s has repo write permission, a 2018 account, substantial contribution history, matching commit authors, and no protected instruction/dependency/build-script changes; branch is fix-lease-handoff -> devnet-ready. No Findings
Prior-comment reconciliation
ConclusionThe PR still has a non-transactional lease termination path that can leave partial state committed on a later error, and it adds an under-accounted hotkey-swap path inside 📜 Previous run (superseded)
# 🔍 AI Review — Auditor (domain review) has not yet run on this PR. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
|
||
| // Move any lease coldkey lock to the beneficiary-controlled hotkey without | ||
| // assigning ownership of the generated lease hotkey to the beneficiary. | ||
| Self::move_lease_lock_to_beneficiary_hotkey(&lease, &hotkey)?; |
There was a problem hiding this comment.
[MEDIUM] Lease termination can still commit a partial handoff on lock conflict
terminate_lease is not wrapped in #[transactional] or with_transaction, but this call mutates lock rows via do_move_lock before later fallible calls. In the conflict case covered by the new test, repatriate_lease_coldkey_alpha can still return LockHotkeyMismatch after this mutation and after owner/aggregate updates, leaving the lease in storage while the subnet owner and lock state have already changed. Make the whole termination path storage-transactional, or preflight every fallible lock/proxy/currency operation before the first mutation.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| Self::move_lease_lock_to_beneficiary_hotkey(&lease, &hotkey)?; | ||
|
|
||
| // Transfer ownership to the beneficiary | ||
| SubnetOwner::<T>::insert(lease.netuid, lease.beneficiary.clone()); | ||
| // Set the owner hotkey before moving locks so the destination lock is | ||
| // accounted in the subnet-owner aggregate, not the regular hotkey aggregate. | ||
| Self::set_subnet_owner_hotkey(lease.netuid, &hotkey)?; | ||
| Self::reassign_subnet_owner_lock_aggregates(lease.netuid, &lease.hotkey, &hotkey); | ||
|
|
||
| Self::repatriate_lease_coldkey_alpha(&lease, &hotkey)?; |
There was a problem hiding this comment.
[MEDIUM] Lease termination can still commit a partial handoff on lock conflict
terminate_lease is still not wrapped in #[transactional] or with_transaction. A beneficiary that already has a lock on a different hotkey can make repatriate_lease_coldkey_alpha return LockHotkeyMismatch via transfer_stake_within_subnet after the earlier lock move, owner change, owner-hotkey set, and aggregate reassignment have already written storage. Dispatch errors do not roll those writes back, so the lease can be left partially handed off. Preflight the beneficiary lock compatibility before any mutation, or wrap the whole handoff in a storage transaction and rollback on every Err.
| let mut weight = Weight::zero(); | ||
| Self::perform_hotkey_swap_on_one_subnet( |
There was a problem hiding this comment.
[HIGH] Hotkey-swap work is discarded from terminate_lease weight
This creates a local Weight::zero() and passes it into perform_hotkey_swap_on_one_subnet, but the accumulated weight is never returned from terminate_lease or otherwise charged. That helper walks and rewrites dynamic storage such as PendingChildKeys::<T>::iter_prefix(netuid), Alpha::<T>::iter_prefix(old_hotkey), and AlphaV2::<T>::iter_prefix(old_hotkey). The dispatch weight for terminate_lease is still only keyed by contributor count, so a lease termination can execute hotkey-swap work that is not reflected in the block weight. Account for the helper's accumulated weight in the post-dispatch result and regenerate/update the lease termination weights with bounds for the dynamic hotkey-swap state, or avoid this helper in favor of a bounded lease-specific migration.
|
🔄 AI review updated — Skeptic: VULNERABLE |
3 participants
Repatriate residual stake and locks on lease termination
Closes #2663.
Problem
do_terminate_leasehanded subnet ownership to the beneficiary but left theaccumulated owner-cut alpha and lock state on the lease's derived coldkey
(
blake2_256("leasing/coldkey", lease_id)). That coldkey has no private key andthe beneficiary proxy can't extract stake, so every fixed-end lease termination
permanently stranded the owner emissions accrued during the lease.
Change
Before tearing down the lease coldkey, termination now:
conviction earned during the lease (
do_move_lock(..., preserve_conviction));(
reassign_subnet_owner_lock_aggregates, extracted so it's shared with theexisting owner-change path);
transfer_full_lock_to_coldkey);SubnetUidToLeaseId;dec_providers, then sweeps theunreserved proxy deposit to the beneficiary, so the keyless coldkey is reaped
cleanly with no dangling account, refcount, or stranded funds.
Post-condition: no alpha, lock, or ownership rows remain under the lease coldkey.