From 60ee6e61a2c03ada88b67ea6fe4f4a527d9b5c51 Mon Sep 17 00:00:00 2001 From: Sam Johnson Date: Fri, 10 Oct 2025 16:19:46 -0400 Subject: [PATCH 1/3] tweak template to mention fix_rust.sh --- .github/pull_request_template.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From f1e30513a42e33410458a56592a2d5a9ee866501 Mon Sep 17 00:00:00 2001 From: Sam Johnson Date: Fri, 10 Oct 2025 17:19:26 -0400 Subject: [PATCH 2/3] add copilot instructions --- .github/copilot-instructions.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..fde965284a --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,29 @@ +When reviewing pull requests, please adhere to and keep in mind the following: +* Unless this PR is a hotfix or a deployment from `devnet-ready` => `devnet`, `devnet` => + `testnet`, or `testnet` => `main`, all new PRs should be opened against the `devnet-ready` + branch. The `devnet` and `testnet` branches should only be updated via merges from + `devnet-ready` and `testnet-ready`, respectively, and are designed to track the current state + of the `devnet` and `testnet` networks. Feel free in your review to encourage people to + change the target to `devnet-ready` if they are targeting `main` and this is not a hotfix PR. +* Bittensor is a substrate-based blockchain. It is critical that there are never panics in the + runtime, as this can brick the chain. You should be flagging any runtime/pallet code + (extrinsics, code that could be called by extrinsics, code in migrations, etc) that could + panic because of changes introduced by the PR. We do have clippy lints in place to detect + this, but these do not always catch everything. We CANNOT have panics in the runtime. +* Remember that things like direct indexing (i.e. `arr[3]`) can panic and are dangerous when + used in a substrate runtime. Everything should be using `get` / `set` and `Result` / `Option` + wherever possible. +* Similarly you should look out for unsafe math, such as scenarios where a value might + overflow, potential divide by zero errors, etc., programmers should always be using checked + or saturating math ops. +* If one of the lints is failing (clippy / cargo fmt / cargo fix / etc), you should suggest + that they run `./scripts/fix_rust.sh` as this will typically auto-resolve the issue. This + includes issues like uncommitted `Cargo.lock`. +* There should never be unsafe code in the runtime. +* You should be extra careful with contributions from external contributors (users without the + "Nucleus" role) +* Be extra vigilent for PRs that introduce a security vulnerability or exploit. Always call out + potential security vulnerabilities. +* Code should be performant and maintainable, with maximum safety and security. $4B market cap + is on the line. +* You should be concise in your review, only report legitimate issues, no fluff. \ No newline at end of file From 2a2141078c3cb6e09330c3912b267d0764b05eaf Mon Sep 17 00:00:00 2001 From: Sam Johnson Date: Thu, 16 Oct 2025 11:44:06 -0400 Subject: [PATCH 3/3] use loris's modifications --- .github/copilot-instructions.md | 161 ++++++++++++++++++++++++++------ 1 file changed, 132 insertions(+), 29 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index fde965284a..42b2da2827 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,29 +1,132 @@ -When reviewing pull requests, please adhere to and keep in mind the following: -* Unless this PR is a hotfix or a deployment from `devnet-ready` => `devnet`, `devnet` => - `testnet`, or `testnet` => `main`, all new PRs should be opened against the `devnet-ready` - branch. The `devnet` and `testnet` branches should only be updated via merges from - `devnet-ready` and `testnet-ready`, respectively, and are designed to track the current state - of the `devnet` and `testnet` networks. Feel free in your review to encourage people to - change the target to `devnet-ready` if they are targeting `main` and this is not a hotfix PR. -* Bittensor is a substrate-based blockchain. It is critical that there are never panics in the - runtime, as this can brick the chain. You should be flagging any runtime/pallet code - (extrinsics, code that could be called by extrinsics, code in migrations, etc) that could - panic because of changes introduced by the PR. We do have clippy lints in place to detect - this, but these do not always catch everything. We CANNOT have panics in the runtime. -* Remember that things like direct indexing (i.e. `arr[3]`) can panic and are dangerous when - used in a substrate runtime. Everything should be using `get` / `set` and `Result` / `Option` - wherever possible. -* Similarly you should look out for unsafe math, such as scenarios where a value might - overflow, potential divide by zero errors, etc., programmers should always be using checked - or saturating math ops. -* If one of the lints is failing (clippy / cargo fmt / cargo fix / etc), you should suggest - that they run `./scripts/fix_rust.sh` as this will typically auto-resolve the issue. This - includes issues like uncommitted `Cargo.lock`. -* There should never be unsafe code in the runtime. -* You should be extra careful with contributions from external contributors (users without the - "Nucleus" role) -* Be extra vigilent for PRs that introduce a security vulnerability or exploit. Always call out - potential security vulnerabilities. -* Code should be performant and maintainable, with maximum safety and security. $4B market cap - is on the line. -* You should be concise in your review, only report legitimate issues, no fluff. \ No newline at end of file +# 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