Eiger pallet final#559
Conversation
|
I have enough context now. Let me give you a focused review of PR #559 (Eiger pallet final). OverviewSmall (≈109 / -38), focused finalization PR responding to remaining Eiger audit items across five pallets. Most of the diff is doc improvements; the substantive logic changes are localized and defensible.
What looks good
Issues to consider1. Scheduler
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Now I have everything needed. Here is my comprehensive review: PR #559 Review — Security & SoundnessOverall this is a careful "audit-cleanup" PR — mostly docs, defensive logging, and an extra weight re-check. Most of it is solid hardening. But there's one critical issue and a few minor ones. 🔴 Critical — Missing storage migration for
|
| Severity | Issue |
|---|---|
| Critical | Removing ProposalData::call_weight + STORAGE_VERSION bump without a migration silently bricks any existing proposal and locks deposits forever |
| Medium | cancel_retry_named reads Agenda twice |
| Low | cancel_named failure in recover_funds still emits TransactionCancelled and only log::warn! — consider a distinct event or defensive! |
| Nit | README, stale comment in propose, missing tests for the new failure-path events |
The execute-time hardening, CEI ordering, and observability improvements are clean wins. The blocker before merge (or at least before a runtime upgrade that touches a chain with any existing multisig proposals) is the multisig storage migration. Happy to draft the v0→v1 migration if you'd like — it's straightforward.
|
Follow up on migration - we don't have any proposals... isn't that good enough? Mostly yes — that removes the immediate "stuck deposits" scenario, but a few subtler concerns remain. Let me walk through them. What zero proposals actually mitigatesIf
So the fund-loss vector goes away. That's the critical part. What's still a problem even with zero proposals1. Race window between "we checked" and "upgrade applied"The treasury multisig is in every preset ( The fix is to make the assumption enforced rather than assumed — see below. 2. In-code vs on-chain storage version will permanently desync
This bites you next time you add a v2 migration. The natural pattern is 3. It normalizes "bump STORAGE_VERSION without a migration"This is a fragile precedent for a chain that explicitly lists "fail loudly" as a core principle. The next person who removes a field from Minimum hygiene I'd ask for before mergeA trivial
Something like: pub struct MigrateV0ToV1<T>(PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for MigrateV0ToV1<T> {
fn on_runtime_upgrade() -> Weight {
let on_chain = Pallet::<T>::on_chain_storage_version();
if on_chain != 0 {
return T::DbWeight::get().reads(1);
}
let cleared = Proposals::<T>::clear(u32::MAX, None);
if cleared.unique > 0 {
log::error!(
target: "multisig",
"v0->v1 migration cleared {} proposal(s); deposits remain reserved",
cleared.unique,
);
}
STORAGE_VERSION.put::<Pallet<T>>();
T::DbWeight::get().reads_writes(
cleared.unique as u64 + 1,
cleared.unique as u64 + 1,
)
}
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
ensure!(
Proposals::<T>::iter().next().is_none(),
"v0->v1 assumption violated: Proposals is non-empty"
);
Ok(Vec::new())
}
}And wire it into If clearing-on-mismatch feels too aggressive, swap the One thing to double-check on your endYou said "zero proposals on testnet" — single chain or all of them? Worth a 1-minute So: yes, the empty-state observation downgrades this from "critical, do not merge" to "merge-able with a small no-op migration + a try-runtime guard." The migration is ~30 lines, doesn't add code that won't run (it runs once, on the v0→v1 transition, and is a noop afterward), and keeps you out of the version-drift trap. |
|
I think this looks good after we make sure we don't bork the testnet msig with this |
2 participants
Addressing some final issues:
REVERSIBLE-F-01 — set_high_security
No exit path has been added. recover_funds still does not clear HighSecurityAccounts or GuardianIndex. The team documented this permanence as intentional, so we understand that the risk is accepted.
Added documentation that recover_funds should be callable in perpetuity
REVERSIBLE-F-03 — orphaned holds
recover_funds now skips failed releases instead of aborting, which prevents new orphaned holds from being created. However, holds that were already detached from PendingTransfer metadata still have no repair path. No force-release extrinsic was added. One possible improvement would be to add a root-gated force_release_hold(account, asset, amount) extrinsic that can release holds directly.
This seems to only be possible due to migration, which we are skipping for testnet, as it would add code that wouldn't run on mainnet.
The following informational findings remain unaddressed: TREASURY-O-03, TREASURY-O-04, RUNTIME-O-01
Addressed the first two, the third was already addressed in a previous PR.
Note
High Risk
Touches core runtime pallets for multisig execution, high-security fund recovery, and block reward minting; logic/weight changes can affect funds movement, fee burning, and proposal execution constraints if misconfigured.
Overview
Strengthens runtime safety around multisig proposals/execution, high-security recovery, and block reward minting.
pallet-multisignow validates and decodes proposed calls up front, enforces a configurableMaxInnerCallWeight, recalculates/refunds weights based on actual call size and HS/non-HS path, and re-checks high-security whitelists and weight limits again atexecutetime (including after HS is enabled or limits change). It also revises proposal accounting/limits and benchmarking to reflect the updated behaviors.pallet-reversible-transfersupdatesrecover_fundsto skip transfers whose hold release fails (emittingTransferRecoveryFailed) instead of aborting, preserving metadata for later manual cancellation; it also adds explicit docs thatset_high_securityis intentionally permanent and recovery should remain callable indefinitely.pallet-mining-rewardsadjusts end-of-block reward distribution to account for burned fees in supply calculations, introduces a minting “from” account for proof recording, and adds explicit fallback/error events when minting to the miner/treasury fails (including redirect-to-treasury and a criticalTreasuryMintFailedsignal).Reviewed by Cursor Bugbot for commit 5866a1f. Bugbot is set up for automated code reviews on this repo. Configure here.