Skip to content

Add lock-aware stake transfer extrinsic#2717

Open
gztensor wants to merge 3 commits into
devnet-readyfrom
feat/transfer-free-locked-alpha
Open

Add lock-aware stake transfer extrinsic#2717
gztensor wants to merge 3 commits into
devnet-readyfrom
feat/transfer-free-locked-alpha

Conversation

@gztensor
Copy link
Copy Markdown
Contributor

@gztensor gztensor commented Jun 4, 2026

Description

This change adds transfer_stake_lock_aware extrinsic, a variant of transfer_stake that lets callers choose whether to transfer from the locked or unlocked portion of stake via a new locked boolean. The transition path now caps the requested amount to the selected bucket, so it transfers the maximum available amount without erroring when the request exceeds locked or unlocked alpha. For locked transfers, the existing lock state follows the moved stake and preserves the lock hotkey and proportional conviction; for unlocked transfers, only free stake moves and lock state remains untouched.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have run ./scripts/fix_rust.sh to ensure my code is formatted and linted correctly
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@gztensor gztensor added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Jun 4, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +371 to +379
let alpha_amount = lock_aware_transfer
.map(|locked| {
alpha_amount.min(Self::lock_aware_transferable_alpha(
origin_coldkey,
origin_netuid,
locked,
))
})
.unwrap_or(alpha_amount);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Capped lock-aware amount can mutate stake before failing minimum check

For same-subnet lock-aware transfers, this cap can reduce a caller-supplied alpha_amount to a dust-sized selected bucket after any transaction-extension or input-level minimum checks have seen the original amount. The capped value then reaches transfer_stake_within_subnet, which calls transfer_lock and decreases/increases stake before computing tao_equivalent and returning AmountTooLow for sub-minimum transfers. A caller can submit a large alpha_amount, have it capped to a below-minimum locked/unlocked remainder, and still move stake/lock state through a dispatch that reports failure. Validate the capped move_amount against DefaultMinStake before any lock/stake mutation, or move the same-netuid minimum check ahead of transfer_lock and the stake balance updates.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

BASELINE scrutiny: author has write permission, substantial prior activity, matching commit author, and no Gittensor allowlist hit; branch feat/transfer-free-locked-alpha -> devnet-ready.

No .github, dependency, lockfile, or build-script changes were present. Static review focused on the new runtime staking path; the previously flagged mutation-before-minimum-check issue has been addressed by moving the same-subnet minimum check before lock/stake mutation and adding a regression test.

Findings

No findings.

Prior-comment reconciliation

  • 8e7ad664: addressedtransfer_stake_within_subnet now checks the TAO-equivalent minimum before calling transfer_lock or mutating stake, and the PR adds a regression test for the capped sub-minimum lock-aware transfer path.

Conclusion

No malicious intent or security vulnerability found in the current diff. The prior HIGH finding is addressed.


📜 Previous run (superseded)
Sev File Finding Status
HIGH pallets/subtensor/src/staking/move_stake.rs:379 Capped lock-aware amount can mutate stake before failing minimum check ✅ Addressed
transfer_stake_within_subnet now checks the TAO-equivalent minimum before calling transfer_lock or mutating stake, and the PR adds a regression test for the capped sub-minimum lock-aware transfer path.

🔍 AI Review — Auditor (domain review)

VERDICT: 👎

Gittensor: LIKELY by recent subtensor-focused PR history; established repo contributor with write permission, but this new runtime extrinsic still needs strict behavior review.

PR description is substantive. I did not run builds/tests; the blocking issue is visible statically. I also attempted the devnet spec-version RPC for an auto-fix, but DNS resolution failed in this environment, so I left runtime/src/lib.rs unchanged.

Findings

Sev File Finding
HIGH pallets/subtensor/src/staking/move_stake.rs:195 Cross-subnet locked transfers do not move lock state inline

Conclusion

Blocking because transfer_stake_lock_aware does not actually move locked stake correctly across subnets despite exposing and documenting cross-subnet support. The implementation should either reject cross-subnet lock-aware transfers or implement lock migration for that path with coverage.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🔄 AI review updated — Skeptic: VULNERABLE

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +183 to +195
let tao_moved = Self::transition_stake_internal(
&coldkey,
&destination_coldkey,
&hotkey,
&hotkey,
origin_netuid,
destination_netuid,
alpha_amount,
None,
None,
true,
false,
Some(locked),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Cross-subnet locked transfers do not move lock state

Some(locked) is passed into the generic transition path even when origin_netuid != destination_netuid, but the cross-subnet branch later uses unstake_from_subnet and stake_into_subnet rather than transfer_lock. That means no lock state is moved to the destination subnet/coldkey. Worse, validate_stake_transition still enforces ensure_available_to_unstake for cross-subnet moves, so a fully locked position fails with StakeUnavailable, while a partially locked position with enough unlocked alpha can succeed by moving unlocked stake and leaving the source lock behind. This contradicts the new extrinsic docs and the PR body claim that locked transfers preserve/move the lock state. Either reject origin_netuid != destination_netuid for this extrinsic, or implement explicit cross-subnet lock migration and add a test that locked=true moves lock state across netuids.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-cargo-audit This PR fails cargo audit but needs to be merged anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant