Skip to content

Handle dust in order_swap.rs and leasing.rs to sync TI#2754

Open
gztensor wants to merge 8 commits into
devnet-readyfrom
fix/dust-handling
Open

Handle dust in order_swap.rs and leasing.rs to sync TI#2754
gztensor wants to merge 8 commits into
devnet-readyfrom
fix/dust-handling

Conversation

@gztensor

Copy link
Copy Markdown
Contributor

Description

When Currency:: is directly used to transfer or withdraw TAO, it may burn dust and not decrease subtensor pallet total issuance. Fixing all across codebase.

Related Issue(s)

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 self-assigned this Jun 12, 2026
@gztensor gztensor added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Jun 12, 2026
@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

BASELINE scrutiny: gztensor has write permission, an established account, and substantial prior subtensor history; branch fix/dust-handling -> devnet-ready.

Reviewed the full pre-fetched diff statically. The PR does not modify .github/ai-review/* or .github/copilot-instructions.md, adds no dependencies, and the pallet changes route direct TAO transfers through the existing transfer_tao helper so Balances-side dust burns are mirrored in Subtensor TotalIssuance. The workflow hunks only retarget existing self-hosted jobs to Fireactions runner labels from an already-present runner-migration commit; I did not find added executable steps, permissions expansion, or untrusted checkout behavior.

Findings

No findings.

Conclusion

No malicious behavior or security vulnerability was found in the diff. The runtime-facing changes are narrow and use existing checked/saturating patterns without introducing new panic sources, origin-check bypasses, or supply-chain changes.


🔍 AI Review — Auditor (domain review)

VERDICT: 👎

Gittensor association: LIKELY by heuristic; author is not allowlisted, but has write permission and extensive recent subtensor PR history.

The dust-handling direction is correct: the changed call sites route TAO movement through transfer_tao, so Balances-side ED dust burns can be mirrored into Subtensor TotalIssuance.

Description discrepancies: the PR body only describes dust handling, but the branch also carries broad workflow runner-label changes and a swap_hotkey behavior change from other commits. I did not find a separate blocking issue in those unrelated changes, but the PR body should name them or the branch should be narrowed.

No auto-fixes were applied. I attempted the devnet specVersion RPC required for spec-version calibration, but DNS resolution is unavailable in this environment; CI remains the source of truth for that check.

Findings

Sev File Finding
HIGH pallets/subtensor/src/subnets/leasing.rs:149 Update leasing weight for per-contributor issuance accounting inline

Conclusion

Blocking because the leasing path now performs extra storage work per contributor without updating the extrinsic weight. That needs to be reweighted or explicitly accounted for before merge.

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions github-actions Bot left a comment

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.

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

contributor_refund.into(),
Preservation::Expendable,
)?;
Self::transfer_tao(&lease_coldkey, &contributor, contributor_refund.into())?;

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] Update leasing weight for per-contributor issuance accounting

This refund loop now calls Self::transfer_tao, which does more storage work than the previous direct Currency::transfer: it checks the source balance, reads Balances total issuance before and after, and can mutate Subtensor TotalIssuance when dust is burned. Because this runs once per contributor, register_leased_network(k) gained O(k) additional reads and possible writes, but SubnetLeasingWeightInfo::do_register_leased_network and the generated benchmark weight were not updated. Re-benchmark/update the weight or manually add the extra DB reads/writes so the extrinsic is not underweighted at high MaxContributors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Bad review. Direct Currency::transfer does not update subtensor pallet TotalIssuance if it burns dust.

@github-actions

Copy link
Copy Markdown
Contributor

🔄 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.

3 participants