Handle dust in order_swap.rs and leasing.rs to sync TI#2754
Conversation
gztensor
added
the
skip-cargo-audit
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: gztensor has repo write permission, matching PR author/commit author identity, substantial contribution history, and no trusted Gittensor allowlist hit; branch fix/dust-handling -> devnet-ready. Static review of the pre-fetched diff found no changes to FindingsNo findings. ConclusionNo malicious behavior or Skeptic-level security vulnerability was found in the diff. The bounded leasing weight/accounting calibration concern remains domain-review territory rather than a maliciousness or direct security finding here. 🔍 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. No auto-fixes were applied. Duplicate-work check: PR #2707 overlaps on the same dust/issuance problem with a broader runtime-level Findings
Prior-comment reconciliation
ConclusionBlocking because the leasing contributor refund path still adds TAO-transfer issuance accounting inside a contributor-scaled loop without updating the extrinsic weight. The unrelated hotkey-swap test change should also be restored so it keeps covering the error path named by the test. 📜 Previous run (superseded)
|
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| contributor_refund.into(), | ||
| Preservation::Expendable, | ||
| )?; | ||
| Self::transfer_tao(&lease_coldkey, &contributor, contributor_refund.into())?; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
Bad review. Direct Currency::transfer does not update subtensor pallet TotalIssuance if it burns dust.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| contributor_refund.into(), | ||
| Preservation::Expendable, | ||
| )?; | ||
| Self::transfer_tao(&lease_coldkey, &contributor, contributor_refund.into())?; |
There was a problem hiding this comment.
[HIGH] Update leasing weight for contributor refund issuance accounting
This still routes each contributor refund through transfer_tao, but the PR does not update the register_leased_network(k) benchmark/weight. Compared with the old raw Currency::transfer, transfer_tao performs additional total-issuance accounting on every refund and can write Subtensor TotalIssuance when dust is reaped. Because this call sits inside the contributor-scaled loop, the extrinsic weight needs to be regenerated or manually adjusted for the new worst-case storage work.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| contributor_refund.into(), | ||
| Preservation::Expendable, | ||
| )?; | ||
| Self::transfer_tao(&lease_coldkey, &contributor, contributor_refund.into())?; |
There was a problem hiding this comment.
[HIGH] Update leasing weight for contributor refund issuance accounting
register_leased_network is still charged from the existing generated weight, but this line now calls Self::transfer_tao once per refunded contributor. That helper adds balance/issuance reads and can mutate TotalIssuance when dust is burned; none of that extra work is reflected in pallets/subtensor/src/weights.rs. Because this loop scales up to T::MaxContributors, the extrinsic can be undercharged on the worst-case path. Re-run the benchmark or explicitly update the generated register_leased_network(k) weight/proof comments to include the new per-contributor accounting.
| @@ -864,7 +865,7 @@ fn test_swap_owner_new_hotkey_already_exists() { | |||
| Some(netuid), | |||
| false | |||
| ), | |||
| Error::<Test>::HotKeyAlreadyRegisteredInSubNet | |||
| Error::<Test>::NonAssociatedColdKey | |||
There was a problem hiding this comment.
[LOW] Restore the already-registered hotkey test case
This test is named and documented as covering HotKeyAlreadyRegisteredInSubNet, but the new setup no longer registers new_hotkey on netuid and now asserts NonAssociatedColdKey instead. That silently removes coverage for the already-registered branch while leaving the old test name/comment in place. Restore the original setup so new_hotkey is registered on the target subnet and old_hotkey is merely owned by the caller.
| let netuid = add_dynamic_network(&new_hotkey, &coldkey); | |
| add_balance_to_coldkey_account(&coldkey, 1_000_000_000_000_u64.into()); | |
| // old_hotkey is owned by coldkey; new_hotkey was already registered on `netuid` | |
| // by add_dynamic_network (the condition under test). Do NOT reassign new_hotkey to | |
| // a foreign coldkey — the new_hotkey-ownership check (NonAssociatedColdKey) would | |
| // then fire before the already-registered-in-subnet check this test targets. | |
| Owner::<Test>::insert(old_hotkey, coldkey); | |
| // Perform the swap | |
| System::set_block_number(System::block_number() + HotkeySwapOnSubnetInterval::get()); | |
| assert_err!( | |
| SubtensorModule::do_swap_hotkey( | |
| RuntimeOrigin::signed(coldkey), | |
| &old_hotkey, | |
| &new_hotkey, | |
| Some(netuid), | |
| false | |
| ), | |
| Error::<Test>::HotKeyAlreadyRegisteredInSubNet |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
3 participants
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)
OrderSwapInterface::transfer_taoburns dust at origin without decrementing subtensor::TotalIssuance #2738Type of Change
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly