[Governance 1/4] Introduce pallet-multi-collective#2805
Conversation
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: l0r1s has write permission, an older account, substantial prefetched contribution history, no Gittensor allowlist hit, and matching in-PR authorship; branch governance-multi-collective -> governance-umbrella. Static-only Skeptic review of the prefetched diff found no FindingsNo findings. Prior-comment reconciliation
ConclusionNo malicious pattern or security vulnerability was identified in the current PR diff. Prior findings no longer apply to this review because the files they referenced are absent from the updated diff. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor UNKNOWN; author has repo write permission and substantial prior Subtensor contribution history, so review is calibrated as established contributor work. No blocking domain findings from static review. This PR introduces a standalone The overlapping PRs are expected stacked governance/workspace work rather than better duplicate implementations of this pallet. One PR-body cleanup remains: the Detailed Reference link text names FindingsNo findings. ConclusionApprove on domain review: the new membership extrinsics have explicit origins, bounded storage, generated weights, integrity/try-state checks, and broad unit coverage for sorted membership, bounds, hooks, and rotation behavior. I did not run tests because no finding required runtime confirmation. |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Error::<T>::AlreadyFinalizing | ||
| ); | ||
|
|
||
| crowdloan.finalized = true; |
There was a problem hiding this comment.
[HIGH] Failing finalize can permanently lock crowdloan finalization
finalize now writes crowdloan.finalized = true before dispatching the stored call, but the extrinsic is not wrapped in a transactional rollback and CurrentCrowdloanId::<T>::kill() only runs after a successful nested dispatch. If stored_call.dispatch(...) returns Err, the ? exits before cleanup: CurrentCrowdloanId remains set, the new AlreadyFinalizing guard blocks every future crowdloan finalization, and this crowdloan is marked finalized so withdraw/refund are also blocked. Wrap the fallible finalize branch in with_transaction and roll back on any nested dispatch error, and ensure the temporary CurrentCrowdloanId marker is cleared on every exit path.
|
|
||
| - name: Run Try Runtime Checks | ||
| uses: "paritytech/try-runtime-gha@v0.1.0" | ||
| uses: ./.github/actions/try-runtime |
There was a problem hiding this comment.
[MEDIUM] pull_request workflow loads a PR-mutable local action on self-hosted runners
This workflow checks out the PR and then executes ./.github/actions/try-runtime from that checkout on self-hosted runners. After this merges, a future PR can modify the local composite action and run arbitrary shell on those runners once CI is approved, without changing this workflow file. Nucleus approval and human .github scrutiny reduce the risk, but the steady-state workflow should load this helper from a trusted ref/reusable workflow or otherwise ensure the action body comes from the base branch, not the PR worktree.
|
🔄 AI review updated — Skeptic: VULNERABLE |
l0r1s
added
the
skip-cargo-audit
l0r1s
requested review from
JohnReedV,
evgeny-s,
gztensor,
open-junius and
sam0x17
| Error::<T>::AlreadyFinalizing | ||
| ); | ||
|
|
||
| crowdloan.finalized = true; |
There was a problem hiding this comment.
[HIGH] Failing finalize can permanently lock crowdloan finalization
finalize writes crowdloan.finalized = true before the fallible preimage lookup and nested stored_call.dispatch(...), and CurrentCrowdloanId is only killed on the success path. FRAME dispatchables in this repo use explicit #[transactional] / with_transaction when earlier writes must roll back on error, so a missing preimage or failing nested call can leave this crowdloan marked finalized and/or leave CurrentCrowdloanId set. That blocks withdrawals/refunds/future finalization and can strand the funds. Move the state writes after all fallible work that can fail, explicitly clear the guard on every error path, or wrap the whole finalization body in a storage transaction that rolls back on Err.
|
|
||
| - name: Run Try Runtime Checks | ||
| uses: "paritytech/try-runtime-gha@v0.1.0" | ||
| uses: ./.github/actions/try-runtime |
There was a problem hiding this comment.
[MEDIUM] pull_request workflow loads a PR-mutable local action on self-hosted runners
This pull_request workflow now runs ./.github/actions/try-runtime from the checked-out PR contents on self-hosted fireactions-tryruntime* runners. The action contains arbitrary shell (curl, rustup, cargo build, try-runtime execution), so after this merges any future PR can alter that local action and have the modified shell run once a nucleus member approves CI. The approval gate lowers this from a direct secret-exfil path, but it is a persistent runner-execution footgun. Keep the workflow pinned to an immutable external action, or checkout the trusted base/default branch copy of the local action before invoking it.
|
🔄 AI review updated — Skeptic: VULNERABLE |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
1 participant
Summary
Part of #2804.
Adds
pallet-multi-collective, a generic membership pallet for storing multiple named collectives under a runtime-definedCollectiveId.This is a building block for governance membership only. It does not add voting, proposing, or runtime governance behavior by itself.
What Changed
pallet-multi-collective.add_member,remove_member,swap_member,set_members, andforce_rotate.OnMembersChangedand term-rotation hooks for downstream consumers.Detailed Reference
See
pallets/multi-collective/README.mdfor the full design reference. It covers the membership storage model, extrinsic behavior, origin configuration, member-change hooks, rotating terms, integrity checks, migration notes, and an example runtime configuration.Reviewers may want to focus on the boundary behavior:
swap_memberpreserves member count atomically,set_membersrejects duplicate input instead of silently deduplicating, and rotating collectives keep their previous membership if the runtime-supplied rotation cannot satisfy the configured minimum size.Behavioral Impact
No live runtime behavior changes in this PR. The pallet is added to the codebase but is not wired into the Subtensor runtime until the governance wiring PR.
Migration / Spec Version
The pallet defines storage, but this PR does not add it to the runtime. No live storage migration or spec-version bump is required here.
Testing
Suggested local verification:
cargo test -p pallet-multi-collective