Fix GRANDPA set ID handling - #2971
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | ||
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | ||
| // pallet-grandpa's session handler after it successfully schedules an authority change. | ||
| pallet_grandpa::CurrentSetId::<Runtime>::mutate(|set_id| *set_id += 1); |
There was a problem hiding this comment.
[HIGH] Delay set-ID advancement until the authority change takes effect
schedule_change accepts an arbitrary in_blocks, but this advances CurrentSetId immediately while Grandpa::grandpa_authorities() remains the old set until the scheduled block. During that window, a restarting node reads the new set ID through GrandpaApi::current_set_id() alongside the old authorities and can initialize GRANDPA with an invalid (set_id, authorities) pair, impairing finality. The session-handler pattern cited here is safe only when scheduling at the session transition; it cannot be copied unchanged to this delayed admin API. Advance the ID when the change is enacted, or constrain this path to an immediate change and reject nonzero delays. Add a restart/runtime-API regression test for the pending-change window.
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE VERY HIGH scrutiny: account has zero public repos and was under 90 days old when the PR opened, mitigated by repository-admin status, matching author/committer, substantial merged history, and no Gittensor association; fix/grandpa-setid → release-v446. The diff is scoped to checked GRANDPA set-ID bookkeeping, zero-delay authority rotations, a Finney-specific warp-sync correction, associated tests and weights, and generated documentation. No dependencies or AI-review trust-boundary files change. FindingsNo findings. ConclusionStatic analysis found no security vulnerability or malicious behavior. The changes preserve authority/set-ID consistency and apply Finney’s historical offset only at warp-sync initialization. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor association UNKNOWN; repository admin with substantial recent contributions, though the account is under 90 days old. The implementation matches the substantive PR description. Checked set-ID advancement occurs only after successful scheduling, the zero-delay constraint keeps authority and set-ID updates atomic at the block boundary, and regression coverage includes delayed changes, pending changes, overflow, and multi-fragment Finney warp sync. The custom FindingsNo findings. ConclusionNo domain-correctness issues were found. The PR is ready from the Auditor perspective. |
|
🔄 AI review updated — Skeptic: VULNERABLE |
3f84ce2 to
e354fbb
Compare
eco-tests changed — indexer review requiredThis PR modifies files under Changed files
|
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | ||
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | ||
| // pallet-grandpa's session handler after it successfully schedules an authority change. | ||
| pallet_grandpa::CurrentSetId::<Runtime>::mutate(|set_id| *set_id += 1); |
There was a problem hiding this comment.
[HIGH] Handle set-ID overflow before scheduling the change
SetId is a u64, so *set_id += 1 can panic when overflow checks are enabled or wrap to zero otherwise. Either outcome violates the monotonic GRANDPA set-ID invariant. Compute the checked successor before schedule_change; this also ensures an overflow error cannot occur after PendingChange has already been written.
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | |
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | |
| // pallet-grandpa's session handler after it successfully schedules an authority change. | |
| pallet_grandpa::CurrentSetId::<Runtime>::mutate(|set_id| *set_id += 1); | |
| let next_set_id = Grandpa::current_set_id() | |
| .checked_add(1) | |
| .ok_or(sp_runtime::ArithmeticError::Overflow)?; | |
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | |
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | |
| // pallet-grandpa's session handler after it successfully schedules an authority change. | |
| pallet_grandpa::CurrentSetId::<Runtime>::put(next_set_id); |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
e354fbb to
cd1fa4d
Compare
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | ||
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | ||
| // pallet-grandpa's session handler after it successfully schedules an authority change. | ||
| pallet_grandpa::CurrentSetId::<Runtime>::mutate(|set_id| *set_id += 1); | ||
| Ok(()) |
There was a problem hiding this comment.
[HIGH] Handle set-ID overflow before scheduling the change
CurrentSetId is a u64, so *set_id += 1 can panic in the runtime at exhaustion. It also runs after schedule_change, making a fallible increment at that point unsafe because the pending authority change has already been written. Compute the checked next ID first, schedule only after that succeeds, then persist the ID.
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | |
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | |
| // pallet-grandpa's session handler after it successfully schedules an authority change. | |
| pallet_grandpa::CurrentSetId::<Runtime>::mutate(|set_id| *set_id += 1); | |
| Ok(()) | |
| let next_set_id = Grandpa::current_set_id() | |
| .checked_add(1) | |
| .ok_or(sp_runtime::ArithmeticError::Overflow)?; | |
| Grandpa::schedule_change(next_authorities, in_blocks, forced)?; | |
| // This runtime does not use pallet-session, so mirror the bookkeeping performed by | |
| // pallet-grandpa's session handler after it successfully schedules an authority change. | |
| pallet_grandpa::CurrentSetId::<Runtime>::put(next_set_id); | |
| Ok(()) |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
cd1fa4d to
8fa0a70
Compare
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
8fa0a70 to
304fd24
Compare
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
304fd24 to
ed28a46
Compare
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
ed28a46 to
943f242
Compare
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
943f242 to
ed568d8
Compare
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
2 participants
Summary
Grandpa::CurrentSetIdwith a checked next ID after a successful admin-scheduled authority changeRoot cause
The admin scheduling path called
Grandpa::schedule_changedirectly and therefore bypassed the set-ID bookkeeping normally performed bypallet-grandpa's session integration. This runtime does not usepallet-session, so a successful scheduling path must perform that bookkeeping explicitly. Because advancing the ID before a delayed authority change activates would persist a mismatched ID/authority pair, the admin path now accepts only immediate changes. It also checks the next set ID before writing the pending change, so overflow cannot panic or leave state half-applied.Separately, the pinned SDK's legacy initial-set-ID mechanism adds its offset inside the proof-fragment loop. Finney needs the historical offset only at the beginning of verification; applying it to every fragment over-counts later authority-set transitions.
Impact
Admin-scheduled authority rotations now keep GRANDPA's authorities and set ID aligned in every persisted state. Finney warp-sync verification applies the historical correction once, then carries the proof's set ID forward normally across subsequent transitions.
Validation
CARGO_NET_OFFLINE=true cargo test -p pallet-admin-utils --lockedCARGO_NET_OFFLINE=true cargo check -p pallet-admin-utils --features runtime-benchmarks --lockedSKIP_WASM_BUILD=1 CARGO_NET_OFFLINE=true cargo test -p node-subtensor grandpa_warp_sync --locked -- --nocaptureSKIP_WASM_BUILD=1 CARGO_NET_OFFLINE=true cargo test -p node-subtensor-runtime --lib --lockedSKIP_WASM_BUILD=1 CARGO_NET_OFFLINE=true cargo clippy -p pallet-admin-utils -p node-subtensor-runtime -p node-subtensor --tests --no-deps --lockeduv run ruff check . && uv run ruff format --check . && uv run ty check --exit-zero-on-warning bittensor(fromsdk/python)uv run python -m codegen.check --coverage && uv run python -m codegen.check --names && uv run python -m codegen.check --namespaces(fromsdk/python)uv run pytest(fromsdk/python; 1089 passed and 1 skipped in-sandbox, with all 6 local-socket bridge tests passing outside the socket-restricted sandbox)uv run python ../../website/apps/bittensor-website/scripts/generate.py --check(fromsdk/python)cargo fmt --all -- --checkgit diff --checkDependency
This PR is intentionally stacked on #3078 and should merge after it.