Skip to content

Conviction enabled constant#2693

Closed
gztensor wants to merge 1 commit into
devnet-readyfrom
feat/conviction-disable
Closed

Conviction enabled constant#2693
gztensor wants to merge 1 commit into
devnet-readyfrom
feat/conviction-disable

Conversation

@gztensor
Copy link
Copy Markdown
Contributor

Description

Add a global constant to enable/disable conviction in one-line change. Makes code maintenance while turning off conviction on testnet more convenient and non-breaking.

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 May 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

ghost commented May 25, 2026

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

BASELINE scrutiny: gztensor has write permission, substantial prior subtensor history, matching PR/commit author, and no trusted Gittensor allowlist hit; branch feat/conviction-disable -> devnet-ready.

Findings

No findings.

Conclusion

No malicious behavior or security vulnerability found in the static diff. The PR does not touch .github, dependencies, build scripts, or lockfiles, and the runtime changes add a direct conviction-disable gate without introducing new panic/unsafe paths.


🔍 AI Review — Auditor (domain review)

VERDICT: 👎

Gittensor: LIKELY by repository-history heuristic; author has write permission and substantial recent subtensor contribution history.

The Auditor proposed a replacement PR description, but the current body is non-trivial; not overwriting. Maintainers: ask the Auditor to regenerate if you want it.

The implementation matches the stated intent at a high level: CONVICTION_ENABLED is set false, new lock creation becomes a no-op, existing rolled locks collapse to zero, and owner reassignment is bypassed. I did not find a better duplicate candidate among the overlapping open PR metadata; #2687 appears to be a broader conviction-v2 branch rather than a replacement for this targeted disable toggle.

Spec-version auto-fix was not applied because the devnet RPC lookup failed in this environment (Could not resolve host: dev.chain.opentensor.ai), so I could not confirm whether the devnet-ready spec gate would require a bump.

Findings

Sev File Finding
MEDIUM pallets/subtensor/src/tests/locks.rs:21 Disabled conviction mode is not tested inline

Conclusion

Blocking on the test gap: this PR disables the conviction path and also turns most existing conviction tests into no-ops, but it does not add assertions for the new disabled-mode contract.

Copy link
Copy Markdown
Contributor

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

use crate::staking::lock::{CONVICTION_ENABLED, LockState};
use crate::*;

macro_rules! skip_if_conviction_disabled {
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.

[MEDIUM] Disabled conviction mode is not tested

With CONVICTION_ENABLED set to false, this macro makes most conviction/lock tests return before executing their assertions. The PR changes runtime staking behavior, but it does not add positive coverage for the new disabled-mode contract: do_lock_stake should leave Lock/aggregate storage empty, rolling an existing lock should produce zero locked mass and zero conviction, and change_subnet_owner_if_needed should not reassign ownership. Add explicit tests for those disabled behaviors instead of only skipping the enabled-mode tests.

@github-actions
Copy link
Copy Markdown
Contributor

ghost commented May 25, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👎

@gztensor gztensor closed this May 25, 2026
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