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 The latest commits address part of the prior review by adding Findings
Prior-comment reconciliation
ConclusionThe PR still under-accounts the hotkey-swap work in successful 📜 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 |
| 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 |
| Self::repatriate_lease_coldkey_alpha(&lease, &hotkey)?; | ||
| Self::transfer_full_lock_to_coldkey( |
There was a problem hiding this comment.
[MEDIUM] Lease termination can still commit a partial handoff on lock conflict
This fallible handoff still runs after earlier storage mutations: move_lease_lock_to_beneficiary_hotkey, SubnetOwner::insert, set_subnet_owner_hotkey, reassign_subnet_owner_lock_aggregates, and repatriate_lease_coldkey_alpha have already modified state before transfer_full_lock_to_coldkey can return LockHotkeyMismatch for a beneficiary that already has a lock on the same subnet under a different hotkey. I do not see a with_storage_layer / with_transaction wrapper around do_terminate_lease, so that error path can leave ownership, lock aggregates, and residual stake partially moved while the lease remains live. Add a preflight check for the beneficiary's existing lock hotkey before any mutation, or wrap the whole termination mutation sequence in a storage transaction that rolls back on any later error.
There was a problem hiding this comment.
Error rollbacks storage
|
🔄 AI review updated — Skeptic: VULNERABLE |
l0r1s
added
the
skip-cargo-audit
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.