Skip to content

Handle balancer misbalance on injection#2758

Open
gztensor wants to merge 8 commits into
devnet-readyfrom
fix/handle-misbalance-on-injection
Open

Handle balancer misbalance on injection#2758
gztensor wants to merge 8 commits into
devnet-readyfrom
fix/handle-misbalance-on-injection

Conversation

@gztensor

@gztensor gztensor commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes balancer emission injection when a block's TAO/alpha liquidity would push swap balancer weights outside the allowed range. Instead of dropping the whole injection attempt, the swap pallet now stores non-price-active TAO or alpha in per-subnet reservoirs and retries those reservoirs on later injection updates.

What Changed

  • Updated SwapHandler::adjust_protocol_liquidity to return separate price-active and materialization amounts for TAO and alpha.
  • Added BalancerTaoReservoir and BalancerAlphaReservoir storage in pallets/swap.
  • Changed coinbase injection to materialize TAO before offering it to swap reservoir accounting, so failed TAO spending cannot activate unescrowed current-block TAO.
  • Updated subnet dissolve / swap cleanup tests to include the new reservoir storage.
  • Added focused swap and coinbase tests for TAO/alpha reservoiring, retrying reservoirs, failed TAO materialization, and cleanup.

Behavioral Impact

Out-of-range protocol liquidity is no longer silently skipped. TAO/alpha that cannot be made price-active immediately is retained for later balancer updates while price-active reserve changes remain bounded by balancer weight constraints.

Runtime / Migration Notes

This changes pallet storage and runtime behavior. A spec_version bump may be required by the devnet-ready spec-version check depending on the live devnet runtime version.

Testing

The PR adds unit coverage in pallets/swap/src/pallet/tests.rs, pallets/subtensor/src/tests/coinbase.rs, and pallets/subtensor/src/tests/networks.rs for the new reservoir behavior and cleanup paths.

@gztensor gztensor self-assigned this Jun 15, 2026
@gztensor gztensor added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Jun 15, 2026

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

BalancerTaoReservoir::<T>::insert(netuid, TaoBalance::ZERO);
BalancerAlphaReservoir::<T>::insert(netuid, AlphaBalance::ZERO);
SwapBalancer::<T>::insert(netuid, new_balancer);
return (pending_tao, pending_alpha);

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] TAO reservoir can inject unescrowed prior-block credit

pending_tao includes BalancerTaoReservoir from earlier blocks, but TAO credit is not escrowed when it is stored there. In inject_and_maybe_swap, the unspent TAO credit from a failed/withheld injection is recycled at the end of that block; a later call that returns pending_tao asks the caller to fund old TAO from the current block's remaining_credit. If that spend fails, this function has already cleared the reservoir and inserted the updated SwapBalancer; if it succeeds, this subnet can consume emission credit that belonged to the current block's other allocations. The TAO-only branch below has the same issue. Fix by either escrowing/reserving TAO alongside the reservoir and committing the balancer update only after funding succeeds, or by not carrying TAO reservoir amounts across blocks under this interface.

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

BASELINE scrutiny: established contributor with repo write permission; no Gittensor allowlist hit; branch fix/handle-misbalance-on-injection -> devnet-ready.

Static review used only the trusted instructions and pre-fetched context. The diff does not modify .github/, build scripts, Cargo manifests, lockfiles, or Rust dependencies. The prior reservoir-cleanup finding remains addressed in the current revision.

Findings

No findings.

Prior-comment reconciliation

  • 5b48401d: addressed — The current diff materializes nonzero BalancerTaoReservoir and BalancerAlphaReservoir into reserve accounting before dissolution/direct cleanup removes the reservoir storage.

Conclusion

No malicious intent or security vulnerability was found in the current diff. Reservoir balances are now materialized into reserve accounting before subnet dissolution/direct swap cleanup clears them, and the runtime storage change includes a spec_version bump.


🔍 AI Review — Auditor (domain review)

VERDICT: 👍

LIKELY Gittensor/ecosystem contributor: not in trusted allowlists, but has repo write permission and substantial recent subtensor contribution history; no better duplicate candidate found.

PR body is substantive and matches the implementation. The runtime spec_version is bumped from 423 to 424.

Validation: git diff --check 4ff1e30c174acc0d6c766e4139f7f5ccd29ea488...HEAD passed. I attempted the focused swap reservoir tests, but cargo/rustc could not start because rustup tried to write temp files under read-only /home/runner/.rustup/tmp; no workspace files were modified.

Duplicate-work check: overlapping open PRs touch shared runtime/pallet files, but their titles/scopes address different work, so I do not see a better duplicate candidate.

Findings

Sev File Finding
LOW contract-tests/.papi/ Unrelated generated contract-test metadata is included in the economic fix (off-diff)

Other findings

  • [LOW] Unrelated generated contract-test metadata is included in the economic fix (contract-tests/.papi/) — The PR is scoped to swap/subtensor reservoir accounting, but it also adds a new .papi contract metadata bundle and devnet SCALE blob under contract-tests/ with no corresponding test harness or PR-body explanation. If these artifacts are required for this fix, document why; otherwise remove them so this runtime accounting change does not carry unrelated generated files.

Prior-comment reconciliation

  • 5d36cf5e: not addressed — The unrelated .papi contract metadata bundle is still present in the diff.
  • d13dc776: addressedget_alpha_issuance now includes protocol_alpha_reservoir, and subnet dissolve materializes reservoirs into reserve accounting before destroying alpha state.

Conclusion

The reservoir accounting fix now covers alpha issuance and subnet dissolve cleanup, addressing the prior blocking concern. The only remaining issue is low-severity PR hygiene around unrelated generated contract-test metadata.


📜 Previous run (superseded)
Sev File Finding Status
LOW contract-tests/.papi/ Unrelated generated contract-test metadata is included in the economic fix ➡️ Carried forward to current findings
The unrelated .papi contract metadata bundle is still present in the diff.

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@gztensor gztensor marked this pull request as draft June 15, 2026 14:59
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@gztensor gztensor marked this pull request as ready for review June 17, 2026 14:11

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

Comment thread pallets/subtensor/src/coinbase/run_coinbase.rs Outdated
@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.

Comment thread pallets/swap/src/pallet/impls.rs Outdated
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

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

Comment thread pallets/swap/src/pallet/impls.rs Outdated
@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: VULNERABLE

@github-actions

Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@gztensor gztensor marked this pull request as draft June 25, 2026 14:51
@gztensor gztensor marked this pull request as ready for review June 25, 2026 18:46
@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.

2 participants