Skip to content

Fix silent cap on transferred alpha amount by lock-available#2829

Closed
lorismoulin wants to merge 2 commits into
mainfrom
no-cap-alpha-transfers
Closed

Fix silent cap on transferred alpha amount by lock-available#2829
lorismoulin wants to merge 2 commits into
mainfrom
no-cap-alpha-transfers

Conversation

@lorismoulin

@lorismoulin lorismoulin commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Description

Note: This is a good PR for the community to discuss and possibly vote.

When transferring alpha between subnets, the extrinsics currently silently cap transfer amount by available alpha: If more alpha is requested to transfer, the extrinsic transfers only what's available and succeeds. There are two concurrent and conflicting issues:

  1. If the chain silently caps (current behavior): The extrinsic reports success and mis-accounting may occur because less alpha was transferred than was requested yet the extrinsic result is Ok.
  2. If the chain does not silently cap (new behavior implemented in this PR): When the user requests exactly available alpha to transfer and has no TAO on the signing coldkey, the tx fees are paid in alpha before the transfer amount is checked, which results in transaction error (and loss of fees). The client software needs to account for alpha fees to prevent this.

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):

Breaking Change

Some clients may be broken if they request transfer amounts above the unlocked alpha.

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

@github-actions github-actions Bot added the hotfix This PR needs to be merged very quickly and will likely skip testing on devnet and testnet label Jul 6, 2026
@lorismoulin lorismoulin force-pushed the no-cap-alpha-transfers branch from 18cfe63 to f14085b Compare July 6, 2026 17:59
@github-actions

github-actions Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

Baseline scrutiny: established write-permission contributor with extensive substantive subtensor history; no Gittensor association found. Branch no-cap-alpha-transfers -> main.

Static review found no malicious code, supply-chain changes, trusted review-file modifications, runtime panic source, authorization bypass, or exploitable economic inconsistency. The removed cap is followed by validate_stake_transition, which rejects requests exceeding the caller’s current origin stake before movement.

Findings

No findings.

Prior-comment reconciliation

  • ad05bc81: no longer applies — The prior finding misapplied branch policy: main is the normal target for feature PRs. The special hotfix-justification rule applies only when base_ref == mainnet, which is not the case here.

Conclusion

The change removes silent partial fulfillment while preserving stake-availability validation, and the added tests cover insufficient stake and alpha-fee depletion. No security vulnerability was found.

📜 Previous run (superseded)
Sev File Finding Status
HIGH PR body Direct-to-main hotfix lacks explicit PR-body justification ⏭️ No longer applies
The prior finding misapplied branch policy: main is the normal target for feature PRs. The special hotfix-justification rule applies only when base_ref == mainnet, which is not the case here.

🔍 AI Review — Auditor (domain review)

VERDICT: 👍

Gittensor association: UNKNOWN. Established contributor with extensive Subtensor contribution and review history; full domain scrutiny applied.

The change is narrowly scoped: it removes silent capping while retaining validate_stake_transition’s explicit insufficient-stake rejection. Tests cover overdraw attempts, unchanged balances on failure, alpha-fee depletion, and the fee-adjusted successful transfer path.

PR #2891 overlaps only in the transaction-fee test module and addresses a different extrinsic, so it is not duplicate work.

Findings

No findings.

Conclusion

The implementation matches the substantive PR description and provides appropriate regression coverage. No blocking domain issues were found.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@UnArbosSix UnArbosSix added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Jul 6, 2026
@vercel

vercel Bot commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
subtensor Ready Ready Preview, Comment Jul 14, 2026 4:48pm

Request Review

@UnArbosFour UnArbosFour removed the hotfix This PR needs to be merged very quickly and will likely skip testing on devnet and testnet label Jul 14, 2026
@RaoFoundation RaoFoundation deleted a comment from github-actions Bot Jul 14, 2026
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions github-actions Bot mentioned this pull request Jul 14, 2026
Closed
7 tasks
@IntiTechnologies

IntiTechnologies commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Superseded by #2898 (v431 release): these changes were reviewed and integrated there.

@IntiTechnologies IntiTechnologies deleted the no-cap-alpha-transfers branch July 14, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UnArbosFour UnArbosFour UnArbosFour approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lorismoulin @IntiTechnologies @UnArbosFour @UnArbosSix @unarbos