|
| 1 | +# Bittensor PR Review Guidelines |
| 2 | + |
| 3 | +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. |
| 4 | + |
| 5 | +## Branch Strategy |
| 6 | +* Unless this is a hotfix or deployment PR (`devnet-ready` => `devnet`, `devnet` => `testnet`, or `testnet` => `main`), all PRs must target `devnet-ready` |
| 7 | +* Flag PRs targeting `main` directly unless they are hotfixes |
| 8 | +* `devnet` and `testnet` branches must only receive merges from their respective `-ready` branches |
| 9 | + |
| 10 | +## CRITICAL: Runtime Safety (Chain-Bricking Prevention) |
| 11 | +The runtime CANNOT panic under any circumstances. A single panic can brick the entire chain. |
| 12 | + |
| 13 | +**Panic Sources to Flag:** |
| 14 | +* Direct indexing: `vec[i]`, `arr[3]` → Must use `.get()` returning `Option` |
| 15 | +* `.unwrap()`, `.expect()` → Must handle `Result`/`Option` properly |
| 16 | +* `.unwrap_or()` is acceptable only with safe defaults |
| 17 | +* Unchecked arithmetic: `a + b`, `a - b`, `a * b`, `a / b` → Must use `checked_*` or `saturating_*` |
| 18 | +* Division without zero checks |
| 19 | +* Type conversions: `.try_into()` without handling, casting that could truncate |
| 20 | +* Iterator operations that assume non-empty collections: `.first().unwrap()`, `.last().unwrap()` |
| 21 | +* String operations: slicing without bounds checking |
| 22 | +* `unsafe` blocks (absolutely prohibited in runtime) |
| 23 | + |
| 24 | +## Substrate-Specific Vulnerabilities |
| 25 | + |
| 26 | +### Storage Safety |
| 27 | +* Unbounded storage iterations (DoS vector) - check for loops over storage maps without limits |
| 28 | +* Missing storage deposits/bonds for user-created entries (state bloat attack) |
| 29 | +* Storage migrations without proper version checks or error handling |
| 30 | +* Direct storage manipulation without proper weight accounting |
| 31 | +* `kill_storage()` or storage removals without cleanup of dependent data |
| 32 | + |
| 33 | +### Weight & Resource Exhaustion |
| 34 | +* Missing or incorrect `#[pallet::weight]` annotations |
| 35 | +* Computational complexity not reflected in weight calculations |
| 36 | +* Database reads/writes not accounted for in weights |
| 37 | +* Potential for weight exhaustion attacks through parameter manipulation |
| 38 | +* Loops with user-controlled bounds in extrinsics |
| 39 | + |
| 40 | +### Origin & Permission Checks |
| 41 | +* Missing `ensure_signed`, `ensure_root`, or `ensure_none` checks |
| 42 | +* Origin checks that can be bypassed |
| 43 | +* Privilege escalation paths |
| 44 | +* Missing checks before state-modifying operations |
| 45 | +* Incorrect origin forwarding in cross-pallet calls |
| 46 | + |
| 47 | +### Economic & Cryptoeconomic Exploits |
| 48 | +* Integer overflow/underflow in token/balance calculations |
| 49 | +* Rounding errors that can be exploited (especially in repeated operations) |
| 50 | +* MEV/front-running vulnerabilities in auction/pricing mechanisms |
| 51 | +* Flash loan-style attacks or single-block exploits |
| 52 | +* Reward calculation errors or manipulation vectors |
| 53 | +* Slashing logic vulnerabilities |
| 54 | +* Economic denial of service (forcing expensive operations on others) |
| 55 | + |
| 56 | +### Migration Safety |
| 57 | +* Migrations without try-state checks or validation |
| 58 | +* Missing version guards (checking current vs new version) |
| 59 | +* Unbounded migrations that could time out |
| 60 | +* Data loss risks during migration |
| 61 | +* Missing rollback handling for failed migrations |
| 62 | + |
| 63 | +### Consensus & Chain State |
| 64 | +* Anything that could cause non-deterministic behavior (randomness sources, timestamps without validation) |
| 65 | +* Fork-causing conditions due to different execution paths |
| 66 | +* Block production or finalization blockers |
| 67 | +*Validator set manipulation vulnerabilities |
| 68 | + |
| 69 | +### Cross-Pallet Interactions |
| 70 | +* Reentrancy-like patterns when calling other pallets |
| 71 | +* Circular dependencies between pallets |
| 72 | +* Assumptions about other pallet state that could be violated |
| 73 | +* Missing error handling from pallet calls |
| 74 | + |
| 75 | +## Supply Chain & Dependency Security |
| 76 | + |
| 77 | +**Flag any PR that:** |
| 78 | +* Adds new dependencies (require justification and thorough vetting) |
| 79 | +* Updates cryptographic or core dependencies |
| 80 | +* Uses dependencies with known vulnerabilities (check advisories) |
| 81 | +* Depends on unmaintained or obscure crates |
| 82 | +* Introduces git dependencies or path dependencies pointing outside the repo |
| 83 | +* Uses pre-release versions of critical dependencies |
| 84 | +* Includes large dependency version jumps without explanation |
| 85 | + |
| 86 | +**For dependency changes, verify:** |
| 87 | +* Changelog review for security fixes or breaking changes |
| 88 | +* Maintainer reputation and project activity |
| 89 | +* Number of reverse dependencies (more = more scrutiny) |
| 90 | +* Whether it introduces new transitive dependencies |
| 91 | + |
| 92 | +## Code Quality & Maintainability |
| 93 | + |
| 94 | +* Code duplication that could lead to inconsistent bug fixes |
| 95 | +* Overly complex logic that obscures security issues |
| 96 | +* Missing error messages or unclear panic contexts in tests |
| 97 | +* Insufficient test coverage for new extrinsics or storage operations |
| 98 | +* Missing or inadequate documentation for complex algorithms |
| 99 | +* Magic numbers without explanation |
| 100 | +* TODO/FIXME comments introducing technical debt in critical paths |
| 101 | + |
| 102 | +## External Contributor Scrutiny |
| 103 | +For contributors without "Nucleus" role, apply **maximum scrutiny**: |
| 104 | +* Verify the PR solves a real, documented issue |
| 105 | +* Check for hidden backdoors or logic bombs |
| 106 | +* Review commit history for suspicious patterns |
| 107 | +* Validate that changes match the stated purpose |
| 108 | +* Question any unusual patterns or overcomplicated solutions |
| 109 | +* Require clear explanations for non-obvious changes |
| 110 | + |
| 111 | +## Build & Tooling |
| 112 | +* If lints fail (clippy, rustfmt, cargo check), suggest running `./scripts/fix_rust.sh` |
| 113 | +* Uncommitted `Cargo.lock` changes should be included in commits |
| 114 | +* Ensure CI passes before deep review |
| 115 | + |
| 116 | +## Review Style |
| 117 | +* Be **concise** - report only legitimate issues, no nitpicks |
| 118 | +* Provide **specific line numbers** and **concrete examples** |
| 119 | +* Suggest **fixes** when possible, not just problems |
| 120 | +* **Severity levels**: Use [CRITICAL], [HIGH], [MEDIUM], [LOW] tags |
| 121 | +* Block PRs on [CRITICAL] and [HIGH] issues |
| 122 | +* For security issues, consider discussing privately before commenting publicly |
| 123 | + |
| 124 | +## Final Check |
| 125 | +Before approving, ask yourself: |
| 126 | +1. Could this brick the chain? (panic, consensus break) |
| 127 | +2. Could this lose or steal funds? (arithmetic, logic errors) |
| 128 | +3. Could this DOS the network? (unbounded operations, weight issues) |
| 129 | +4. Could this introduce a backdoor? (especially for external contributors) |
| 130 | +5. Is this change necessary and minimal? |
| 131 | + |
| 132 | +**Remember: $4B market cap. Err on the side of caution. When in doubt, escalate.** |
0 commit comments