diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..42b2da2827 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,132 @@ +# Bittensor PR Review Guidelines + +You are reviewing code for a Substrate-based blockchain with a $4B market cap. Lives and livelihoods depend on the security and correctness of this code. Be thorough, precise, and uncompromising on safety. + +## Branch Strategy +* Unless this is a hotfix or deployment PR (`devnet-ready` => `devnet`, `devnet` => `testnet`, or `testnet` => `main`), all PRs must target `devnet-ready` +* Flag PRs targeting `main` directly unless they are hotfixes +* `devnet` and `testnet` branches must only receive merges from their respective `-ready` branches + +## CRITICAL: Runtime Safety (Chain-Bricking Prevention) +The runtime CANNOT panic under any circumstances. A single panic can brick the entire chain. + +**Panic Sources to Flag:** +* Direct indexing: `vec[i]`, `arr[3]` → Must use `.get()` returning `Option` +* `.unwrap()`, `.expect()` → Must handle `Result`/`Option` properly +* `.unwrap_or()` is acceptable only with safe defaults +* Unchecked arithmetic: `a + b`, `a - b`, `a * b`, `a / b` → Must use `checked_*` or `saturating_*` +* Division without zero checks +* Type conversions: `.try_into()` without handling, casting that could truncate +* Iterator operations that assume non-empty collections: `.first().unwrap()`, `.last().unwrap()` +* String operations: slicing without bounds checking +* `unsafe` blocks (absolutely prohibited in runtime) + +## Substrate-Specific Vulnerabilities + +### Storage Safety +* Unbounded storage iterations (DoS vector) - check for loops over storage maps without limits +* Missing storage deposits/bonds for user-created entries (state bloat attack) +* Storage migrations without proper version checks or error handling +* Direct storage manipulation without proper weight accounting +* `kill_storage()` or storage removals without cleanup of dependent data + +### Weight & Resource Exhaustion +* Missing or incorrect `#[pallet::weight]` annotations +* Computational complexity not reflected in weight calculations +* Database reads/writes not accounted for in weights +* Potential for weight exhaustion attacks through parameter manipulation +* Loops with user-controlled bounds in extrinsics + +### Origin & Permission Checks +* Missing `ensure_signed`, `ensure_root`, or `ensure_none` checks +* Origin checks that can be bypassed +* Privilege escalation paths +* Missing checks before state-modifying operations +* Incorrect origin forwarding in cross-pallet calls + +### Economic & Cryptoeconomic Exploits +* Integer overflow/underflow in token/balance calculations +* Rounding errors that can be exploited (especially in repeated operations) +* MEV/front-running vulnerabilities in auction/pricing mechanisms +* Flash loan-style attacks or single-block exploits +* Reward calculation errors or manipulation vectors +* Slashing logic vulnerabilities +* Economic denial of service (forcing expensive operations on others) + +### Migration Safety +* Migrations without try-state checks or validation +* Missing version guards (checking current vs new version) +* Unbounded migrations that could time out +* Data loss risks during migration +* Missing rollback handling for failed migrations + +### Consensus & Chain State +* Anything that could cause non-deterministic behavior (randomness sources, timestamps without validation) +* Fork-causing conditions due to different execution paths +* Block production or finalization blockers +*Validator set manipulation vulnerabilities + +### Cross-Pallet Interactions +* Reentrancy-like patterns when calling other pallets +* Circular dependencies between pallets +* Assumptions about other pallet state that could be violated +* Missing error handling from pallet calls + +## Supply Chain & Dependency Security + +**Flag any PR that:** +* Adds new dependencies (require justification and thorough vetting) +* Updates cryptographic or core dependencies +* Uses dependencies with known vulnerabilities (check advisories) +* Depends on unmaintained or obscure crates +* Introduces git dependencies or path dependencies pointing outside the repo +* Uses pre-release versions of critical dependencies +* Includes large dependency version jumps without explanation + +**For dependency changes, verify:** +* Changelog review for security fixes or breaking changes +* Maintainer reputation and project activity +* Number of reverse dependencies (more = more scrutiny) +* Whether it introduces new transitive dependencies + +## Code Quality & Maintainability + +* Code duplication that could lead to inconsistent bug fixes +* Overly complex logic that obscures security issues +* Missing error messages or unclear panic contexts in tests +* Insufficient test coverage for new extrinsics or storage operations +* Missing or inadequate documentation for complex algorithms +* Magic numbers without explanation +* TODO/FIXME comments introducing technical debt in critical paths + +## External Contributor Scrutiny +For contributors without "Nucleus" role, apply **maximum scrutiny**: +* Verify the PR solves a real, documented issue +* Check for hidden backdoors or logic bombs +* Review commit history for suspicious patterns +* Validate that changes match the stated purpose +* Question any unusual patterns or overcomplicated solutions +* Require clear explanations for non-obvious changes + +## Build & Tooling +* If lints fail (clippy, rustfmt, cargo check), suggest running `./scripts/fix_rust.sh` +* Uncommitted `Cargo.lock` changes should be included in commits +* Ensure CI passes before deep review + +## Review Style +* Be **concise** - report only legitimate issues, no nitpicks +* Provide **specific line numbers** and **concrete examples** +* Suggest **fixes** when possible, not just problems +* **Severity levels**: Use [CRITICAL], [HIGH], [MEDIUM], [LOW] tags +* Block PRs on [CRITICAL] and [HIGH] issues +* For security issues, consider discussing privately before commenting publicly + +## Final Check +Before approving, ask yourself: +1. Could this brick the chain? (panic, consensus break) +2. Could this lose or steal funds? (arithmetic, logic errors) +3. Could this DOS the network? (unbounded operations, weight issues) +4. Could this introduce a backdoor? (especially for external contributors) +5. Is this change necessary and minimal? + +**Remember: $4B market cap. Err on the side of caution. When in doubt, escalate.** \ No newline at end of file diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 1e279b2ebe..f2413b444b 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -31,7 +31,7 @@ Please ensure the following tasks are completed before requesting a review: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas -- [ ] I have run `cargo fmt` and `cargo clippy` to ensure my code is formatted and linted correctly +- [ ] 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