Eiger pallet final

[illuzen]

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 multiple runtime pallets with changes to minting (mining-rewards), multisig execution/weight accounting, and high-security fund recovery; bugs here can impact token supply, proposal safety, or stuck funds. Logic is non-trivial (new limits, re-checks, and fallback paths) and should be reviewed with extra care and tested on-chain.

Overview
Adds a new mining-rewards pallet that accumulates transaction fees and mints dynamic block rewards on on_finalize, splitting rewards between miner and treasury, recording wormhole transfer proofs, and falling back to treasury (with explicit events) if minting to the miner fails.

Strengthens pallet-multisig by validating/decoding calls at propose time for all multisigs, enforcing a new MaxInnerCallWeight bound (and re-checking it at execute), re-checking high-security whitelisting at execute, and changing execution semantics to remove proposals/return deposits before dispatch while still succeeding even if the inner call fails (signal via event). Also adds/adjusts filibuster limits and explicit cleanup flows (remove_expired, claim_deposits), corrects dynamic fee calculation precision, normalizes signer sets (sort+dedup), and updates benchmarks/tests to cover the new paths.

Improves pallet-reversible-transfers high-security UX and recovery safety by documenting set_high_security as permanent and recover_funds as callable indefinitely, and by changing recover_funds to skip failed hold releases (emitting TransferRecoveryFailed) instead of aborting, preventing new orphaned holds while preserving metadata for later manual cancel retries.

^{Reviewed by Cursor Bugbot for commit 0fe5101. Bugbot is set up for automated code reviews on this repo. Configure here.}

[n13]

I have enough context now. Let me give you a focused review of PR #559 (Eiger pallet final).

Overview

Small (≈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.

File	Type of change
pallets/mining-rewards/src/lib.rs	New TreasuryMintFailed event in two existing error paths
pallets/multisig/src/lib.rs	Re-check inner call weight at execute time (belt-and-suspenders)
pallets/reversible-transfers/src/lib.rs	Docs + defensive invariant check + remove 2 unused errors
pallets/scheduler/src/lib.rs	ok_or → defensive_ok_or in do_cancel_retry
pallets/treasury/src/lib.rs	Documentation only
Two test files	Stale comment cleanup

What looks good

• mining-rewards: TreasuryMintFailed { reward } is deposited in exactly the two spots where the existing log::error! already fired (catastrophic redirect failure and no-miner treasury mint failure). On-chain visibility for a fund-loss scenario that was previously only logged. Clean change, fail-loud compatible.
• multisig execute re-check: The pre-dispatch weight annotation is WeightInfo::execute(MaxCallSize) + MaxInnerCallWeight. The new check at pallets/multisig/src/lib.rs:1071-1077 enforces current_call_weight <= MaxInnerCallWeight so the post-dispatch fallback info.actual_weight.unwrap_or(current_call_weight) is provably bounded by what was reserved. The scenario this guards — MaxInnerCallWeight lowered via runtime upgrade between propose and execute — is real and worth defending.
• reversible-transfers recover_funds: The let-else continue + defensive! at pallets/reversible-transfers/src/lib.rs:572-579 is the right shape: storage inconsistency between PendingTransfersBySender and PendingTransfers is a true invariant, not a user error, and emitting defensive! then skipping (rather than aborting the whole call) preserves the "skip-failed, continue-others" property the PR is trying to maintain. Removing RecovererCannotBeSelf / CallDecodingFailed is safe — git grep across the PR branch shows no remaining references.
• Docs: The added paragraphs on permanence of HS mode + repeat-callable recover_funds (pallets/reversible-transfers/src/lib.rs:360-364, 532-548) and the expanded treasury module docs are a meaningful improvement and directly justify the F-01 risk acceptance.

Issues to consider

1. Scheduler defensive_ok_or covers a user-input path (minor correctness)

	fn do_cancel_retry(
		origin: &T::PalletsOrigin,
		(when, index): TaskAddressOf<T>,
	) -> Result<(), DispatchError> {
		let agenda = Agenda::<T>::get(when);
		// When called via cancel_retry_named, the task address comes from Lookup, so
		// the Agenda entry should exist. Use defensive check for Lookup/Agenda sync.
		let scheduled = agenda
			.get(index as usize)
			.and_then(Option::as_ref)
			.defensive_ok_or(Error::<T>::NotFound)?;

The comment's premise only holds for one of the two callers. cancel_retry_named looks up the address from Lookup (premise valid) — but cancel_retry at pallets/scheduler/src/lib.rs:617 takes a TaskAddressOf<T> directly from the caller. ScheduleOrigin (root/fellowship) supplying a stale or wrong (when, index) is user error, not an invariant violation; defensive_ok_or will produce a misleading "INVARIANT VIOLATION" log line and panic in debug / try-runtime builds for what's a legitimate NotFound.

Suggested fix: either keep .ok_or(Error::<T>::NotFound)? here and move the defensive check inside cancel_retry_named (after Lookup hit, before calling do_cancel_retry), or split into two helpers. The on-chain result is unchanged either way; this is purely about not crying wolf.

2. No tests for the new failure-path events / re-check

• TreasuryMintFailed, TransferRecoveryFailed, and the CallWeightExceedsLimit-at-execute branch are not exercised by tests (git grep finds them only in the implementing pallets). The multisig case is the most worth covering, since the explicit motivation is a runtime-upgrade scenario — a tiny mock with two different MaxInnerCallWeight values would pin the behavior down.

3. Stale comment in multisig (cosmetic)

			// Calculate actual weight: bookkeeping + inner call's actual weight
			// Use current_call_weight (computed at execute time) as fallback when post-dispatch
			// info is unavailable. This is more accurate than the stored weight since
			// dispatch_info could have changed via runtime upgrade.

If proposal.call_weight is no longer consulted on the execute path, consider noting (here or in ProposalData) that the stored call_weight is now only used at propose-time validation. Not blocking, just keeps reader expectations aligned.

On the audit findings the PR explicitly does not fully close

• REVERSIBLE-F-01 (no HS exit): permanence is now justified in code docs as protection for future inbound funds — good narrative, but the absence of a force_disable_high_security extrinsic still implies any account that turns this on is permanently restricted. Accepted risk; docs make it visible to users.
• REVERSIBLE-F-03 (pre-existing orphan holds): no force_release_hold. The PR correctly stops creating new orphans by skipping-not-aborting in recover_funds. Leftover orphans from prior versions remain unreachable — accepted since no testnet→mainnet migration is planned.

Verdict

Approve with two small asks:

1. Narrow the defensive_ok_or in scheduler::do_cancel_retry to the cancel_retry_named path only.
2. Add a minimal test for the multisig execute-time CallWeightExceedsLimit branch, since the whole justification for the change is a future runtime upgrade.

Everything else is doc/cleanup and looks fine. Nothing in here changes the funds-critical happy paths; the new code is purely defensive logging and clearer error surfaces.