From af122ee39ba8b4d2d421ee7513b09cfdc774d0cd Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:38:43 +0800 Subject: [PATCH 1/7] V12 round 2: fix referenda alarm-lifecycle and queue stranding Confirmed findings 180560, 181329, 181347, 181395, 181418, 181327, 181394, 181396, 181393. ensure_alarm_at now schedules before cancelling; failed queue admission and evictions keep a timeout wake-up; submit/schedule_enactment no longer strand referenda or discard the approved call; cancel/kill free the track queue; nudges no longer orphan alarms. Co-Authored-By: Claude Opus 4.8 --- pallets/referenda/src/branch.rs | 21 ++- pallets/referenda/src/lib.rs | 313 +++++++++++++++++++++++--------- pallets/referenda/src/tests.rs | 200 +++++++++++++++++--- pallets/referenda/src/types.rs | 30 +-- 4 files changed, 441 insertions(+), 123 deletions(-) diff --git a/pallets/referenda/src/branch.rs b/pallets/referenda/src/branch.rs index 30b69360..e4265126 100644 --- a/pallets/referenda/src/branch.rs +++ b/pallets/referenda/src/branch.rs @@ -37,6 +37,16 @@ pub fn alarm_retry_weight, I: 'static>() -> Weight { .saturating_mul(Pallet::::MAX_ALARM_SCHEDULE_RETRIES as u64) } +/// Worst-case overhead of repairing a referendum displaced from a full `TrackQueue` (V12 audit +/// #161743): one `ReferendumInfoFor` read+write plus the re-armed evicted referendum's alarm +/// retry overhead. Charged on the branches that insert into a possibly-full queue (`Queued` / +/// `RequeuedInsertion`). +pub fn queue_eviction_repair_weight, I: 'static>() -> Weight { + T::DbWeight::get() + .reads_writes(1, 1) + .saturating_add(alarm_retry_weight::()) +} + /// Branches within the `begin_deciding` function. pub enum BeginDecidingBranch { Passing, @@ -110,8 +120,12 @@ impl ServiceBranch { ContinueNotConfirming | Approved | Rejected => base.saturating_add(alarm_retry_weight::()), + // Inserting into a possibly-full queue may evict and repair another referendum + // (#161743): one read/write plus its alarm-retry overhead. + Queued | RequeuedInsertion => + base.saturating_add(queue_eviction_repair_weight::()), // These branches leave the referendum without an alarm (or only cancel one). - Queued | RequeuedInsertion | RequeuedSlide | TimedOut | Fail => base, + RequeuedSlide | TimedOut | Fail => base, } } @@ -134,6 +148,7 @@ impl ServiceBranch { .max(T::WeightInfo::nudge_referendum_rejected()) .max(T::WeightInfo::nudge_referendum_timed_out()) .saturating_add(alarm_retry_weight::()) + .saturating_add(queue_eviction_repair_weight::()) } /// Return the weight of the `place_decision_deposit` function when it takes the branch denoted @@ -148,7 +163,8 @@ impl ServiceBranch { // the referendum alarm-less. Preparing => T::WeightInfo::place_decision_deposit_preparing() .saturating_add(alarm_retry_weight::()), - Queued => T::WeightInfo::place_decision_deposit_queued(), + Queued => T::WeightInfo::place_decision_deposit_queued() + .saturating_add(queue_eviction_repair_weight::()), NotQueued => T::WeightInfo::place_decision_deposit_not_queued() .saturating_add(alarm_retry_weight::()), BeginDecidingPassing => T::WeightInfo::place_decision_deposit_passing() @@ -180,6 +196,7 @@ impl ServiceBranch { .max(T::WeightInfo::place_decision_deposit_passing()) .max(T::WeightInfo::place_decision_deposit_failing()) .saturating_add(alarm_retry_weight::()) + .saturating_add(queue_eviction_repair_weight::()) } } diff --git a/pallets/referenda/src/lib.rs b/pallets/referenda/src/lib.rs index 93b4d5ef..9117cef8 100644 --- a/pallets/referenda/src/lib.rs +++ b/pallets/referenda/src/lib.rs @@ -444,6 +444,8 @@ pub mod pallet { PreimageNotExist, /// The preimage is stored with a different length than the one provided. PreimageStoredWithDifferentLength, + /// The referendum's wake-up alarm could not be scheduled. + AlarmSchedulingFailed, } #[pallet::hooks] @@ -505,11 +507,12 @@ pub mod pallet { let now = T::BlockNumberProvider::current_block_number(); let nudge_call = T::Preimages::bound(CallOf::::from(Call::nudge_referendum { index }))?; + // A referendum without its undeciding-timeout alarm is never serviced (#91210) and + // nothing here can recover from it, so fail the extrinsic. Being transactional, the + // `Err` rolls back the reserve, the `ReferendumCount` bump and any scheduler writes. let alarm = - Self::set_alarm(nudge_call, now.saturating_add(T::UndecidingTimeout::get())); - // A referendum without its undeciding-timeout alarm is not serviced (#91210) - // and nothing here can recover from that, so flag it loudly under debug. - debug_assert!(alarm.is_some(), "unable to schedule the undeciding-timeout alarm"); + Self::set_alarm(nudge_call, now.saturating_add(T::UndecidingTimeout::get())) + .ok_or(Error::::AlarmSchedulingFailed)?; let status = ReferendumStatus { track, origin: proposal_origin, @@ -521,7 +524,7 @@ pub mod pallet { deciding: None, tally: TallyOf::::new(track), in_queue: false, - alarm, + alarm: Some(alarm), }; ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); @@ -597,8 +600,10 @@ pub mod pallet { /// Emits `Cancelled`. #[pallet::call_index(3)] // May defer `one_fewer_deciding` via `set_alarm`, bearing its worst-case retry - // overhead. - #[pallet::weight(T::WeightInfo::cancel().saturating_add(alarm_retry_weight::()))] + // overhead; plus one read/write to drop a queued entry from `TrackQueue`. + #[pallet::weight(T::WeightInfo::cancel() + .saturating_add(alarm_retry_weight::()) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)))] pub fn cancel(origin: OriginFor, index: ReferendumIndex) -> DispatchResult { T::CancelOrigin::ensure_origin(origin)?; let status = Self::ensure_ongoing(index)?; @@ -612,6 +617,11 @@ pub mod pallet { if status.deciding.is_some() { Self::note_one_fewer_deciding(status.track); } + // A queued referendum still holds a `TrackQueue` slot; drop it so terminal entries + // do not occupy the bounded queue's capacity. + if status.in_queue { + Self::remove_from_track_queue(status.track, index); + } Self::deposit_event(Event::::Cancelled { index, tally: status.tally }); let info = ReferendumInfo::Cancelled( T::BlockNumberProvider::current_block_number(), @@ -630,8 +640,10 @@ pub mod pallet { /// Emits `Killed` and `DepositSlashed`. #[pallet::call_index(4)] // May defer `one_fewer_deciding` via `set_alarm`, bearing its worst-case retry - // overhead. - #[pallet::weight(T::WeightInfo::kill().saturating_add(alarm_retry_weight::()))] + // overhead; plus one read/write to drop a queued entry from `TrackQueue`. + #[pallet::weight(T::WeightInfo::kill() + .saturating_add(alarm_retry_weight::()) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)))] pub fn kill(origin: OriginFor, index: ReferendumIndex) -> DispatchResult { T::KillOrigin::ensure_origin(origin)?; let status = Self::ensure_ongoing(index)?; @@ -642,6 +654,10 @@ pub mod pallet { if status.deciding.is_some() { Self::note_one_fewer_deciding(status.track); } + // As in `cancel`, drop any stale `TrackQueue` entry this referendum still holds. + if status.in_queue { + Self::remove_from_track_queue(status.track, index); + } Self::deposit_event(Event::::Killed { index, tally: status.tally }); Self::slash_deposit(Some(status.submission_deposit.clone())); Self::slash_deposit(status.decision_deposit.clone()); @@ -664,8 +680,17 @@ pub mod pallet { ensure_root(origin)?; let now = T::BlockNumberProvider::current_block_number(); let mut status = Self::ensure_ongoing(index)?; - // This is our wake-up, so we can disregard the alarm. - status.alarm = None; + // Only treat the alarm as consumed when this nudge IS its scheduled wake-up (its + // task is the currently-executing agenda entry, so it must not be cancelled). A + // manual Root nudge at any other block must cancel the still-pending task instead + // of orphaning it, which would otherwise leave a self-perpetuating alarm chain. + if let Some((when, _)) = status.alarm { + if when == now { + status.alarm = None; + } else { + Self::ensure_no_alarm(&mut status); + } + } let (info, dirty, branch) = Self::service_referendum(now, index, status); if dirty { ReferendumInfoFor::::insert(index, info); @@ -690,22 +715,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let track_info = T::Tracks::info(track).ok_or(Error::::BadTrack)?; - let mut track_queue = TrackQueue::::get(track); - let branch = - if let Some((index, mut status)) = Self::next_for_deciding(&mut track_queue) { - let now = T::BlockNumberProvider::current_block_number(); - let (maybe_alarm, branch) = - Self::begin_deciding(&mut status, index, now, &track_info); - if let Some(set_alarm) = maybe_alarm { - Self::ensure_alarm_at(&mut status, index, set_alarm); - } - ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); - TrackQueue::::insert(track, track_queue); - branch.into() - } else { - DecidingCount::::mutate(track, |x| x.saturating_dec()); - OneFewerDecidingBranch::QueueEmpty - }; + let branch = Self::one_fewer_deciding_now(track, &track_info); Ok(Some(branch.weight::()).into()) } @@ -936,13 +946,18 @@ impl, I: 'static> Pallet { const MAX_ALARM_SCHEDULE_RETRIES: u32 = 16; // Enqueue a proposal from a referendum which has presumably passed. + // + // #160560: returns `Err(retry_at)` if the enactment call could not be placed on any of the + // candidate agendas. The caller MUST keep the referendum `Ongoing` in that case (retrying at + // `retry_at`) rather than committing `ReferendumInfo::Approved`, which would discard the + // call/origin and lose the enactment entirely. fn schedule_enactment( index: ReferendumIndex, track: &TrackInfoOf, desired: DispatchTime>, origin: PalletsOriginOf, call: BoundedCallOf, - ) { + ) -> Result<(), BlockNumberFor> { let now = T::BlockNumberProvider::current_block_number(); // Earliest allowed block is always at minimum the next block. let earliest_allowed = now.saturating_add(track.min_enactment_period.max(One::one())); @@ -959,7 +974,7 @@ impl, I: 'static> Pallet { origin.clone(), call.clone(), ) { - Ok(_) => return, + Ok(_) => return Ok(()), Err(e) => { last_err = Some(e); when = when.saturating_add(One::one()); @@ -967,16 +982,17 @@ impl, I: 'static> Pallet { } } - // #91210: never fail silently. Even after sliding forward, a full agenda means an - // approved referendum's enacted call is never scheduled. + // #91210: never fail silently. Even after sliding forward, a full agenda means the + // enactment could not be scheduled; the caller retries at `when` instead of losing it. log::error!( target: "runtime::referenda", - "referendum {:?} approved but enactment scheduling failed after {} retries: {:?}", + "referendum {:?} enactment scheduling failed after {} retries, will retry at #{:?}: {:?}", index, Self::MAX_ENACTMENT_SCHEDULE_RETRIES, + when, last_err, ); - debug_assert!(false, "LOGIC ERROR: bake_referendum/schedule_named failed after retries"); + Err(when) } /// Set an alarm to dispatch `call` at block number `when`. @@ -1093,17 +1109,56 @@ impl, I: 'static> Pallet { let r = Self::begin_deciding(status, index, now, track); (r.0, r.1.into()) } else { - // Add to queue. #91271: honor the bounded-insertion result. If the item sorts beyond - // the queue bound it is NOT inserted, so it must not be marked `in_queue` — - // otherwise it is skipped by timeout yet absent from `TrackQueue`, stranding it - // (ghost-queued). + // Add to queue. #91271: honor the bounded-insertion result (see arms below). let item = (index, status.tally.ayes(status.track)); - status.in_queue = - TrackQueue::::mutate(status.track, |q| q.insert_sorted_by_key(item, |x| x.1)); - (None, ServiceBranch::Queued) + match TrackQueue::::mutate(status.track, |q| { + q.insert_sorted_by_key(item, |x| x.1) + }) { + Ok(evicted) => { + status.in_queue = true; + // #161743: inserting into a full queue displaced the lowest-ayes entry. + // Repair the evicted referendum so it is not left ghost-queued (marked + // `in_queue` yet absent from `TrackQueue`, with no alarm). + if let Some((evicted_index, _)) = evicted { + Self::repair_evicted_referendum(evicted_index, now); + } + (None, ServiceBranch::Queued) + }, + Err(_) => { + // #91271: the item sorts beyond the queue bound and was NOT inserted, so it + // must not be marked `in_queue`. Keep a wake-up (mirroring `NotQueued`) so it + // can time out or retry admission instead of stranding. + status.in_queue = false; + ( + Some(status.submitted.saturating_add(T::UndecidingTimeout::get())), + ServiceBranch::NotQueued, + ) + }, + } } } + /// Repair a referendum displaced from a full `TrackQueue`: clear its stale `in_queue` flag + /// and re-arm its undeciding-timeout alarm so it stays serviceable (#161743). + fn repair_evicted_referendum(index: ReferendumIndex, now: BlockNumberFor) { + ReferendumInfoFor::::mutate(index, |maybe_info| { + if let Some(ReferendumInfo::Ongoing(status)) = maybe_info { + status.in_queue = false; + let timeout = status.submitted.saturating_add(T::UndecidingTimeout::get()); + Self::ensure_alarm_at(status, index, timeout.max(now.saturating_add(One::one()))); + } + }); + } + + /// Remove a referendum's entry from its track queue, if present. + fn remove_from_track_queue(track: TrackIdOf, index: ReferendumIndex) { + TrackQueue::::mutate(track, |q| { + if let Some(pos) = q.iter().position(|(x, _)| *x == index) { + q.remove(pos); + } + }); + } + /// Grab the index and status for the referendum which is the highest priority of those for the /// given track which are ready for being decided. fn next_for_deciding( @@ -1118,6 +1173,43 @@ impl, I: 'static> Pallet { } } + /// Promote the highest-priority queued referendum on `track` into deciding, or release the + /// deciding slot if the queue is empty. Factored out of the `one_fewer_deciding` dispatchable + /// so the `note_one_fewer_deciding` fallback preserves the same invariant (#162455). + fn one_fewer_deciding_now( + track: TrackIdOf, + track_info: &TrackInfoOf, + ) -> OneFewerDecidingBranch { + let mut track_queue = TrackQueue::::get(track); + if let Some((index, mut status)) = Self::next_for_deciding(&mut track_queue) { + let now = T::BlockNumberProvider::current_block_number(); + let (maybe_alarm, branch) = Self::begin_deciding(&mut status, index, now, track_info); + if let Some(set_alarm) = maybe_alarm { + Self::ensure_alarm_at(&mut status, index, set_alarm); + } + // #162455: if the promoted referendum could not be armed, do not consume the freed + // slot with a referendum nothing will service. Release it and drop the referendum + // back to a serviceable, timeout-armed non-deciding state to retry later. + if status.alarm.is_none() { + DecidingCount::::mutate(track, |x| x.saturating_dec()); + status.deciding = None; + status.in_queue = false; + let timeout = status.submitted.saturating_add(T::UndecidingTimeout::get()); + let _ = Self::ensure_alarm_at( + &mut status, + index, + timeout.max(now.saturating_add(One::one())), + ); + } + ReferendumInfoFor::::insert(index, ReferendumInfo::Ongoing(status)); + TrackQueue::::insert(track, track_queue); + branch.into() + } else { + DecidingCount::::mutate(track, |x| x.saturating_dec()); + OneFewerDecidingBranch::QueueEmpty + } + } + /// Schedule a call to `one_fewer_deciding` function via the dispatchable /// `defer_one_fewer_deciding`. We could theoretically call it immediately (and it would be /// overall more efficient), however the weights become rather less easy to measure. @@ -1135,14 +1227,24 @@ impl, I: 'static> Pallet { }, }; if Self::set_alarm(call, next_block).is_none() { - // V12 audit #162455: the deferred `one_fewer_deciding` could not be scheduled - // even after sliding forward. Never lose the accounting: release the deciding - // slot inline so the track is not recorded as full forever (with - // `max_deciding = 1` that would deadlock the whole lane). Queued referenda are - // not promoted here; they are pulled as usual by the `one_fewer_deciding` of - // the next referendum to finish deciding on this track, or by a direct - // (Root-dispatched) `one_fewer_deciding` call. - DecidingCount::::mutate(track, |x| x.saturating_dec()); + // V12 audit #162455/#161395: the deferred `one_fewer_deciding` could not be scheduled + // even after sliding forward. Never lose the accounting nor a freed slot: run the + // promotion inline so a queued referendum is begun (or the slot released when the + // queue is empty), rather than leaving freed capacity dormant until an unrelated + // referendum happens to enter and leave deciding. + match T::Tracks::info(track) { + Some(track_info) => { + let _ = Self::one_fewer_deciding_now(track, &track_info); + }, + None => { + log::error!( + target: "runtime::referenda", + "no track info for {:?}; releasing deciding slot without promotion", + track, + ); + DecidingCount::::mutate(track, |x| x.saturating_dec()); + }, + } } } @@ -1157,8 +1259,9 @@ impl, I: 'static> Pallet { alarm: BlockNumberFor, ) -> bool { if status.alarm.as_ref().map_or(true, |&(when, _)| when != alarm) { - // Either no alarm or one that was different - Self::ensure_no_alarm(status); + // Either no alarm or one that was different. #161347: schedule the replacement + // BEFORE cancelling the existing alarm, so a scheduling failure cannot strand the + // referendum by destroying a working alarm. On failure keep the existing alarm. let call = match T::Preimages::bound(CallOf::::from(Call::nudge_referendum { index })) { Ok(c) => c, @@ -1170,13 +1273,13 @@ impl, I: 'static> Pallet { return false }, }; - status.alarm = Self::set_alarm(call, alarm); - // A referendum with no alarm is not serviced (#91210) and nothing here can - // recover from that, so flag it loudly under debug. - debug_assert!( - status.alarm.is_some(), - "unable to schedule the referendum's service alarm", - ); + let Some(new_alarm) = Self::set_alarm(call, alarm) else { + // `set_alarm` already logged the failure; the existing alarm (if any) is kept. + return false + }; + if let Some((_, old_alarm)) = status.alarm.replace(new_alarm) { + let _ = T::Scheduler::cancel(old_alarm); + } true } else { false @@ -1218,6 +1321,8 @@ impl, I: 'static> Pallet { // Default the alarm to the end of the world. let timeout = status.submitted + T::UndecidingTimeout::get(); let mut alarm = BlockNumberFor::::max_value(); + // #160560: when enactment scheduling fails, the block to retry it at. + let mut enactment_retry: Option> = None; let branch; match &mut status.deciding { None => { @@ -1228,9 +1333,15 @@ impl, I: 'static> Pallet { let mut queue = TrackQueue::::get(status.track); let maybe_old_pos = queue.iter().position(|(x, _)| *x == index); let new_pos = queue.binary_search_by_key(&ayes, |x| x.1).unwrap_or_else(|x| x); + let mut requeue_evicted: Option = None; branch = if maybe_old_pos.is_none() && new_pos > 0 { - // Just insert. - let _ = queue.force_insert_keep_right(new_pos, (index, ayes)); + // Just insert. #161743: capture any referendum this displaces from the full + // queue so it can be repaired instead of left ghost-queued. + if let Ok(Some((evicted_index, _))) = + queue.force_insert_keep_right(new_pos, (index, ayes)) + { + requeue_evicted = Some(evicted_index); + } ServiceBranch::RequeuedInsertion } else if let Some(old_pos) = maybe_old_pos { // We were in the queue - slide into the correct position. @@ -1252,6 +1363,9 @@ impl, I: 'static> Pallet { ServiceBranch::NotQueued }; TrackQueue::::insert(status.track, queue); + if let Some(evicted_index) = requeue_evicted { + Self::repair_evicted_referendum(evicted_index, now); + } } else { // Are we ready for deciding? branch = if status.decision_deposit.is_some() { @@ -1301,24 +1415,39 @@ impl, I: 'static> Pallet { branch = if is_passing { match deciding.confirming { Some(t) if now >= t => { - // Passed! - Self::ensure_no_alarm(&mut status); - Self::note_one_fewer_deciding(status.track); - let (desired, call) = (status.enactment, status.proposal); - Self::schedule_enactment(index, &track, desired, status.origin, call); - Self::deposit_event(Event::::Confirmed { + // Passed! #160560: only commit the terminal `Approved` record once the + // enactment call is actually scheduled. If the agendas are full, keep + // the referendum confirming and retry on a later alarm instead + // of discarding the call/origin that `Approved` does not retain. + match Self::schedule_enactment( index, - tally: status.tally, - }); - return ( - ReferendumInfo::Approved( - now, - Some(status.submission_deposit), - status.decision_deposit, - ), - true, - ServiceBranch::Approved, - ) + &track, + status.enactment.clone(), + status.origin.clone(), + status.proposal.clone(), + ) { + Ok(()) => { + Self::ensure_no_alarm(&mut status); + Self::note_one_fewer_deciding(status.track); + Self::deposit_event(Event::::Confirmed { + index, + tally: status.tally, + }); + return ( + ReferendumInfo::Approved( + now, + Some(status.submission_deposit), + status.decision_deposit, + ), + true, + ServiceBranch::Approved, + ) + }, + Err(retry_at) => { + enactment_retry = Some(retry_at); + ServiceBranch::ContinueConfirming + }, + } }, Some(_) => ServiceBranch::ContinueConfirming, None => { @@ -1356,15 +1485,37 @@ impl, I: 'static> Pallet { } }; alarm = Self::decision_time(deciding, &status.tally, status.track, &track); + // #160560: a failed enactment scheduling overrides the deciding alarm with a + // (guaranteed future) retry block, so the confirmed referendum is re-serviced and + // retries enactment instead of looping on a past confirm-end block. + if let Some(retry_at) = enactment_retry { + alarm = retry_at; + } }, } - let dirty_alarm = if alarm < BlockNumberFor::::max_value() { - Self::ensure_alarm_at(&mut status, index, alarm) + if alarm < BlockNumberFor::::max_value() { + let dirty_alarm = Self::ensure_alarm_at(&mut status, index, alarm); + // #161395: a deciding referendum left with no scheduler alarm (agendas exhausted) + // would hold its bounded deciding slot with nothing to service it. Release the slot + // so the track keeps progressing and drop the referendum back to a serviceable, + // timeout-armed non-deciding state that can retry or time out. + if status.deciding.is_some() && status.alarm.is_none() { + Self::note_one_fewer_deciding(status.track); + status.deciding = None; + status.in_queue = false; + let _ = Self::ensure_alarm_at( + &mut status, + index, + timeout.max(now.saturating_add(One::one())), + ); + return (ReferendumInfo::Ongoing(status), true, branch) + } + (ReferendumInfo::Ongoing(status), dirty_alarm || dirty, branch) } else { - Self::ensure_no_alarm(&mut status) - }; - (ReferendumInfo::Ongoing(status), dirty_alarm || dirty, branch) + let dirty_alarm = Self::ensure_no_alarm(&mut status); + (ReferendumInfo::Ongoing(status), dirty_alarm || dirty, branch) + } } /// Determine the point at which a referendum will be accepted, move into confirmation with the diff --git a/pallets/referenda/src/tests.rs b/pallets/referenda/src/tests.rs index 94e269fd..c8642a3a 100644 --- a/pallets/referenda/src/tests.rs +++ b/pallets/referenda/src/tests.rs @@ -21,7 +21,10 @@ use super::*; use crate::mock::{RefState::*, *}; use assert_matches::assert_matches; use codec::Decode; -use frame_support::{assert_noop, assert_ok, dispatch::RawOrigin, traits::Contains}; +use frame_support::{ + assert_err, assert_noop, assert_ok, dispatch::RawOrigin, storage::with_storage_layer, + traits::Contains, +}; use pallet_balances::Error as BalancesError; use qp_scheduler::BlockNumberOrTimestamp; use sp_runtime::DispatchError::BadOrigin; @@ -160,17 +163,17 @@ fn full_alarm_block_agenda_retries_on_next_block() { }); } -/// V12 audit #161743: a referendum evicted from a full `TrackQueue` keeps `in_queue = -/// true` in storage while being absent from the queue (ghost-queued). Servicing it must -/// clear the flag and mark the status dirty so it can be re-routed through -/// `ready_for_deciding` or the undeciding timeout instead of stranding it forever. +/// V12 audit #161743/#161418: a referendum evicted from a full `TrackQueue` would keep +/// `in_queue = true` in storage while being absent from the queue (ghost-queued) with no +/// alarm, stranding it forever. The eviction must be repaired *eagerly*: the moment the +/// evicting insertion happens, the displaced referendum's `in_queue` flag is cleared and its +/// undeciding-timeout alarm restored so it stays serviceable without any further nudge. /// -/// Track 0 has `max_deciding = 1` and `MaxQueued = 3`. Once the queue is full, ref4 with -/// more ayes than the queue's tail squeezes in via `force_insert_keep_right`, which evicts -/// the lowest-ayes entry (ref1). Servicing ref1 must then hit the `NotQueued` arm (its -/// ayes sort to position 0) and clear `in_queue`. +/// Track 0 has `max_deciding = 1` and `MaxQueued = 3`. Once the queue is full, ref4 with more +/// ayes than the queue's tail squeezes in via `force_insert_keep_right`, evicting the +/// lowest-ayes entry (ref1). #[test] -fn evicted_from_full_queue_clears_in_queue_on_service() { +fn evicted_from_full_queue_is_repaired_immediately() { ExtBuilder::default().build_and_execute(|| { // Block 1: ref0 occupies the single deciding slot of track 0 from block 5 on. assert_ok!(propose_set_balance(1, 0, 0)); @@ -193,24 +196,15 @@ fn evicted_from_full_queue_clears_in_queue_on_service() { Vec::<_>::from(TrackQueue::::get(0)), vec![(4u32, 2u32), (2u32, 2u32), (3u32, 3u32)] ); - // Pre-fix ghost state: ref1 believes it is queued but is absent from `TrackQueue`. - assert!(Referenda::ensure_ongoing(1).unwrap().in_queue); - // Service ref1: it is not found in the queue and its single aye sorts to position - // 0, so the `NotQueued` arm must clear the flag and store the referendum. - assert_ok!(Referenda::nudge_referendum(RuntimeOrigin::root(), 1)); - assert!(!Referenda::ensure_ongoing(1).unwrap().in_queue); - - // The same service must also restore the undeciding-timeout alarm (submitted at - // block 5 + UndecidingTimeout 20 = block 25): `nudge_referendum` is Root-only on - // this chain, so without an alarm the referendum would stay stranded until a - // second governance intervention. - assert_eq!(Referenda::ensure_ongoing(1).unwrap().alarm.map(|(when, _)| when), Some(25)); - - // When that alarm fires, the referendum (deposit placed, prepare period elapsed) - // re-routes through `ready_for_deciding` or times out. Whatever the exact outcome, - // it must no longer sit stranded with no alarm, no queue entry and no deciding - // status. + // #161418: ref1 was evicted, and repaired eagerly - no ghost state, no missing alarm. + // Its `in_queue` flag is already cleared and its undeciding-timeout alarm (submitted at + // block 5 + UndecidingTimeout 20 = block 25) restored, with no external nudge. + let s1 = Referenda::ensure_ongoing(1).unwrap(); + assert!(!s1.in_queue); + assert_eq!(s1.alarm.map(|(when, _)| when), Some(25)); + + // When that alarm fires, the referendum makes progress instead of stranding. run_to(25); let stranded = matches!( ReferendumInfoFor::::get(1), @@ -283,6 +277,158 @@ fn unschedulable_one_fewer_deciding_releases_deciding_slot_inline() { }); } +/// V12 audit #161327: if the undeciding-timeout alarm cannot be scheduled (its target block and +/// all retry blocks are full), `submit` must fail instead of recording a referendum that is never +/// serviced (no alarm, not queued, not deciding) and whose deposit is locked forever. Being +/// transactional, the `Err` rolls back the reserve and the `ReferendumCount` increment. +#[test] +fn submit_fails_when_timeout_alarm_unschedulable() { + ExtBuilder::default().build_and_execute(|| { + // Undeciding-timeout alarm target once submitted at block 1: 1 + 20 = 21. Fill it and all + // 16 retry blocks so `set_alarm` returns `None`. + let max = <::MaxScheduledPerBlock as frame_support::traits::Get< + u32, + >>::get(); + let filler = RuntimeCall::System(frame_system::Call::remark { remark: vec![] }); + for when in 21..=37 { + for _ in 0..max { + assert_ok!(Scheduler::schedule( + RuntimeOrigin::root(), + when, + 100, + Box::new(filler.clone()), + )); + } + } + assert_err!( + with_storage_layer(|| propose_set_balance(1, 1, 0)), + Error::::AlarmSchedulingFailed + ); + // Nothing recorded, nothing reserved. + assert_eq!(ReferendumCount::::get(), 0); + assert_eq!(Balances::reserved_balance(1), 0); + }); +} + +/// V12 audit #161329: when a full `TrackQueue` rejects a low-ayes referendum (it sorts at index 0 +/// and is not inserted), the referendum must NOT be marked `in_queue`; it must keep its +/// undeciding-timeout alarm so it can time out or retry admission rather than strand with no queue +/// entry and no alarm. +#[test] +fn queue_admission_failure_keeps_timeout_alarm() { + ExtBuilder::default().build_and_execute(|| { + // ref0 holds track 0's single deciding slot from block 5 on. + assert_ok!(propose_set_balance(1, 0, 0)); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(1), 0)); + run_to(5); + assert_eq!(deciding_and_failing_since(0), 5); + + // Fill the queue with [(1,1),(2,2),(3,3)]; ref4 (0 ayes) sorts below the queue minimum, so + // admission into the full queue fails. + for (who, ayes) in [(2u64, 1u32), (3, 2), (4, 3), (5, 0)] { + let index = who as u32 - 1; + assert_ok!(propose_set_balance(who, who, 0)); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(who), index)); + set_tally(index, ayes, 0); + } + run_to(9); + assert_eq!( + Vec::<_>::from(TrackQueue::::get(0)), + vec![(1u32, 1u32), (2u32, 2u32), (3u32, 3u32)] + ); + // ref4 was not admitted: not `in_queue`, but keeps a wake-up (submitted 5 + 20 = 25). + let s4 = Referenda::ensure_ongoing(4).unwrap(); + assert!(!s4.in_queue); + assert_eq!(s4.alarm.map(|(when, _)| when), Some(25)); + }); +} + +/// V12 audit #160560: an approved referendum whose enactment call cannot be scheduled (its +/// preferred block and all retry blocks are full) must NOT be committed as `Approved` (which +/// discards the call/origin). It stays `Ongoing`/confirming and retries the enactment on a later +/// alarm, so the enactment is never lost. +#[test] +fn approved_referendum_retries_enactment_when_agendas_full() { + ExtBuilder::default().build_and_execute(|| { + assert_ok!(Referenda::submit( + RuntimeOrigin::signed(1), + Box::new(RawOrigin::Root.into()), + set_balance_proposal_bounded(1), + DispatchTime::At(10), + )); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(2), 0)); + + // Preferred enactment block once approved at 9: max(10, 9+4) = 13. Fill it and all 16 + // retry blocks so enactment scheduling fully fails at the approval attempt. + let max = <::MaxScheduledPerBlock as frame_support::traits::Get< + u32, + >>::get(); + let filler = RuntimeCall::System(frame_system::Call::remark { remark: vec![] }); + for when in 13..=29 { + for _ in 0..max { + assert_ok!(Scheduler::schedule( + RuntimeOrigin::root(), + when, + 100, + Box::new(filler.clone()), + )); + } + } + + run_to(6); + set_tally(0, 100, 0); + run_to(9); + // Enactment could not be scheduled, so the referendum is NOT approved - it stays ongoing. + assert_matches!(ReferendumInfoFor::::get(0), Some(ReferendumInfo::Ongoing(..))); + assert_eq!(confirming_until(0), 9); + + // Once past the filled agendas the enactment schedules and the referendum is approved. + run_to(30); + assert_eq!(approved_since(0), 30); + // The enactment then executes (earliest 30 + min_enactment 4 = 34). + run_to(34); + assert_eq!(Balances::free_balance(42), 1); + }); +} + +/// V12 audit #161394: cancelling a queued referendum must drop its `TrackQueue` entry so terminal +/// referenda do not occupy the bounded queue's capacity. +#[test] +fn cancel_removes_queued_referendum_from_track_queue() { + ExtBuilder::default().build_and_execute(|| { + // ref0 takes track 0's single deciding slot; ref1 queues behind it. + assert_ok!(propose_set_balance(1, 0, 0)); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(1), 0)); + run_to(3); + assert_ok!(propose_set_balance(2, 2, 0)); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(2), 1)); + run_to(7); + assert!(Referenda::ensure_ongoing(1).unwrap().in_queue); + assert!(Vec::<_>::from(TrackQueue::::get(0)).iter().any(|(i, _)| *i == 1)); + + assert_ok!(Referenda::cancel(RuntimeOrigin::signed(4), 1)); + assert!(!Vec::<_>::from(TrackQueue::::get(0)).iter().any(|(i, _)| *i == 1)); + }); +} + +/// V12 audit #161394: killing a queued referendum must likewise drop its `TrackQueue` entry. +#[test] +fn kill_removes_queued_referendum_from_track_queue() { + ExtBuilder::default().build_and_execute(|| { + assert_ok!(propose_set_balance(1, 0, 0)); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(1), 0)); + run_to(3); + assert_ok!(propose_set_balance(2, 2, 0)); + assert_ok!(Referenda::place_decision_deposit(RuntimeOrigin::signed(2), 1)); + run_to(7); + assert!(Referenda::ensure_ongoing(1).unwrap().in_queue); + assert!(Vec::<_>::from(TrackQueue::::get(0)).iter().any(|(i, _)| *i == 1)); + + assert_ok!(Referenda::kill(RuntimeOrigin::root(), 1)); + assert!(!Vec::<_>::from(TrackQueue::::get(0)).iter().any(|(i, _)| *i == 1)); + }); +} + #[test] fn insta_confirm_then_kill_works() { ExtBuilder::default().build_and_execute(|| { diff --git a/pallets/referenda/src/types.rs b/pallets/referenda/src/types.rs index 9c7fe7e4..14a6b072 100644 --- a/pallets/referenda/src/types.rs +++ b/pallets/referenda/src/types.rs @@ -81,22 +81,23 @@ pub type ReferendumIndex = u32; pub trait InsertSorted { /// Inserts an item into a sorted series. /// - /// Returns `true` if it was inserted, `false` if it would belong beyond the bound of the - /// series. + /// Returns `Ok(Some(evicted))` when inserting into a full series displaced the lowest-keyed + /// element, `Ok(None)` when inserted without displacement, and `Err(item)` when the item would + /// belong beyond the bound of the series and was not inserted. fn insert_sorted_by_key K, K: PartialOrd + Ord>( &mut self, t: T, f: F, - ) -> bool; + ) -> Result, T>; } impl> InsertSorted for BoundedVec { fn insert_sorted_by_key K, K: PartialOrd + Ord>( &mut self, t: T, mut f: F, - ) -> bool { + ) -> Result, T> { let index = self.binary_search_by_key::(&f(&t), f).unwrap_or_else(|x| x); - self.force_insert_keep_right(index, t).is_ok() + self.force_insert_keep_right(index, t) } } @@ -730,28 +731,31 @@ mod tests { #[test] fn insert_sorted_works() { let mut b: BoundedVec> = vec![20, 30, 40].try_into().unwrap(); - assert!(b.insert_sorted_by_key(10, |&x| x)); + // Not full: inserted without displacing anything. + assert_eq!(b.insert_sorted_by_key(10, |&x| x), Ok(None)); assert_eq!(&b[..], &[10, 20, 30, 40][..]); - assert!(b.insert_sorted_by_key(60, |&x| x)); + assert_eq!(b.insert_sorted_by_key(60, |&x| x), Ok(None)); assert_eq!(&b[..], &[10, 20, 30, 40, 60][..]); - assert!(b.insert_sorted_by_key(50, |&x| x)); + assert_eq!(b.insert_sorted_by_key(50, |&x| x), Ok(None)); assert_eq!(&b[..], &[10, 20, 30, 40, 50, 60][..]); - assert!(!b.insert_sorted_by_key(9, |&x| x)); + // Full and item sorts at position 0: not inserted, returned back to the caller. + assert_eq!(b.insert_sorted_by_key(9, |&x| x), Err(9)); assert_eq!(&b[..], &[10, 20, 30, 40, 50, 60][..]); - assert!(b.insert_sorted_by_key(11, |&x| x)); + // Full but item sorts above position 0: inserted, evicting the lowest-keyed element. + assert_eq!(b.insert_sorted_by_key(11, |&x| x), Ok(Some(10))); assert_eq!(&b[..], &[11, 20, 30, 40, 50, 60][..]); - assert!(b.insert_sorted_by_key(21, |&x| x)); + assert_eq!(b.insert_sorted_by_key(21, |&x| x), Ok(Some(11))); assert_eq!(&b[..], &[20, 21, 30, 40, 50, 60][..]); - assert!(b.insert_sorted_by_key(61, |&x| x)); + assert_eq!(b.insert_sorted_by_key(61, |&x| x), Ok(Some(20))); assert_eq!(&b[..], &[21, 30, 40, 50, 60, 61][..]); - assert!(b.insert_sorted_by_key(51, |&x| x)); + assert_eq!(b.insert_sorted_by_key(51, |&x| x), Ok(Some(21))); assert_eq!(&b[..], &[30, 40, 50, 51, 60, 61][..]); } From 5e74e22164fc48f94a30c6e350eb5c5c5ff2125e Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:38:44 +0800 Subject: [PATCH 2/7] V12 round 2: fix ranked-collective zero-electorate tally (181346) Tally::support returns zero (not 100%) when the electorate is empty and clamps ayes to the member count, so a shrunken/emptied collective can no longer push a live referendum to 100% support. Co-Authored-By: Claude Opus 4.8 --- pallets/ranked-collective/src/lib.rs | 9 ++++++++- pallets/ranked-collective/src/tests.rs | 13 +++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/pallets/ranked-collective/src/lib.rs b/pallets/ranked-collective/src/lib.rs index b8172916..cbcd1a51 100644 --- a/pallets/ranked-collective/src/lib.rs +++ b/pallets/ranked-collective/src/lib.rs @@ -129,7 +129,14 @@ impl, I: 'static, M: GetMaxVoters>> self.bare_ayes } fn support(&self, class: ClassOf) -> Perbill { - Perbill::from_rational(self.bare_ayes, M::get_max_voters(class)) + let max_voters = M::get_max_voters(class); + if max_voters == 0 { + // No eligible voters: support is zero, not a degenerate 100%. + Perbill::zero() + } else { + // Clamp so a shrunken electorate cannot exceed 100%. + Perbill::from_rational(self.bare_ayes.min(max_voters), max_voters) + } } fn approval(&self, _: ClassOf) -> Perbill { Perbill::from_rational(self.ayes, 1.max(self.ayes + self.nays)) diff --git a/pallets/ranked-collective/src/tests.rs b/pallets/ranked-collective/src/tests.rs index 3a85928b..244bc990 100644 --- a/pallets/ranked-collective/src/tests.rs +++ b/pallets/ranked-collective/src/tests.rs @@ -610,6 +610,19 @@ fn tally_support_correct() { }); } +#[test] +fn tally_support_zero_when_electorate_empty() { + ExtBuilder::default().build_and_execute(|| { + // No members: max_voters == 0, so support is zero, not a degenerate 100%. + let tally: TallyOf = Tally::from_parts(3, 3, 0); + assert_eq!(tally.support(3), Perbill::zero()); + + // Shrunken electorate: bare_ayes (3) exceeds max_voters (1), clamped to 100%. + assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); + assert_eq!(tally.support(0), Perbill::from_percent(100)); + }); +} + #[test] fn exchange_member_works() { ExtBuilder::default().build_and_execute(|| { From b23089e3bc0cbfdea3b81543a9fa744d3da6b738 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:38:44 +0800 Subject: [PATCH 3/7] V12 round 2: fix scheduler task loss and lookup retry weight (181231, 181227) Terminal Unavailable/PermanentlyOverweight outcomes count as serviced work so a following task is not wrongly deleted; the lookup branch of service_task now charges the unconditional Retries read+write. Co-Authored-By: Claude Opus 4.8 --- pallets/scheduler/src/lib.rs | 12 ++++++-- pallets/scheduler/src/tests.rs | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/pallets/scheduler/src/lib.rs b/pallets/scheduler/src/lib.rs index 0be07d91..f5b73f16 100644 --- a/pallets/scheduler/src/lib.rs +++ b/pallets/scheduler/src/lib.rs @@ -169,7 +169,9 @@ pub(crate) trait MarginalWeightInfo: WeightInfo { let base = Self::service_task_base(); let mut total = match maybe_lookup_len { None => base, - Some(l) => Self::service_task_fetched(l as u32), + // V12 audit #181227: `service_task_fetched` omits the unconditional `Retries` take + // charged in `service_task_base`; compose base so the lookup branch meters it too. + Some(l) => base.saturating_add(Self::service_task_fetched(l as u32)), }; if named { total.saturating_accrue(Self::service_task_named().saturating_sub(base)); @@ -1235,7 +1237,13 @@ impl Pallet { agenda[agenda_index as usize] = match result { // Preimage unavailable or permanently overweight -- task is removed (None). // Not counted as postponed since re-processing this block won't help. - Err((Unavailable, slot)) => slot, + // V12 audit #181231: still counts as serviced work so its charged weight cannot + // cause a later task to be misclassified as permanently overweight against a + // fresh scheduler meter next block. + Err((Unavailable, slot)) => { + *executed += 1; + slot + }, // Too heavy for this block but may fit next block. Err((Overweight, slot)) => { postponed += 1; diff --git a/pallets/scheduler/src/tests.rs b/pallets/scheduler/src/tests.rs index 93f7259a..f1026ca7 100644 --- a/pallets/scheduler/src/tests.rs +++ b/pallets/scheduler/src/tests.rs @@ -1100,6 +1100,59 @@ fn scheduler_removes_retry_config_of_permanently_overweight_call() { }); } +/// V12 audit #181231: a terminal `Unavailable` outcome counts as serviced work, so a +/// following task that only exceeds the *remaining* weight is postponed (kept for a later +/// block) instead of being misclassified as permanently overweight and deleted. +#[test] +fn unavailable_task_does_not_cause_next_task_loss() { + let max_weight: Weight = ::MaximumWeight::get(); + new_test_ext().execute_with(|| { + // Highest-priority task: a lookup whose preimage is never provided, so it hits the + // terminal `Unavailable` path and is removed. + let missing = + RuntimeCall::Logger(LoggerCall::log { i: 1, weight: Weight::from_parts(1000, 0) }); + let hash = ::Hashing::hash_of(&missing); + let len = missing.using_encoded(|x| x.len()) as u32; + assert_ok!(Scheduler::do_schedule( + DispatchTime::At(4), + 0, + root(), + Bounded::Lookup { hash, len }, + )); + + // Lower-priority task: overweight, so it runs after the unavailable one in the same + // service cycle and fails the weight limit. + let heavy = RuntimeCall::Logger(LoggerCall::log { i: 2, weight: max_weight }); + assert_ok!(Scheduler::do_schedule( + DispatchTime::At(4), + 1, + root(), + Preimage::bound(heavy).unwrap(), + )); + + run_to_block(4); + + // The unavailable task is terminally gone. + assert!(System::events().iter().any(|e| matches!( + e.event, + RuntimeEvent::Scheduler(crate::Event::CallUnavailable { .. }) + ))); + // The overweight task must NOT be permanently dropped: no `PermanentlyOverweight` + // event and it is retained in the agenda for a later block. + assert!(!System::events().iter().any(|e| matches!( + e.event, + RuntimeEvent::Scheduler(crate::Event::PermanentlyOverweight { .. }) + ))); + assert_eq!( + Agenda::::get(BlockNumberOrTimestamp::BlockNumber(4)) + .iter() + .filter(|s| s.is_some()) + .count(), + 1, + ); + }); +} + #[test] fn scheduler_respects_priority_ordering() { let max_weight: Weight = ::MaximumWeight::get(); From ccbbc7a86b99aa4e2059df153ee5745fffb433c5 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:38:44 +0800 Subject: [PATCH 4/7] V12 round 2: fix reversible-transfers freeze, tree weights, self-guardian (181299, 181294, 181295, 181386) cancel_transfer makes the scheduler cancel best-effort so a removed task cannot freeze held funds; execute_transfer and per_transfer weights add depth-dependent Poseidon ref-time; genesis rejects a self-guardian high-security row. Co-Authored-By: Claude Opus 4.8 --- pallets/reversible-transfers/src/lib.rs | 28 +++++++--- .../src/tests/test_high_security_account.rs | 15 ++++++ .../src/tests/test_reversible_transfers.rs | 54 ++++++++++++++++++- pallets/reversible-transfers/src/weights.rs | 24 +++++++-- runtime/src/transaction_extensions.rs | 12 +++-- 5 files changed, 119 insertions(+), 14 deletions(-) diff --git a/pallets/reversible-transfers/src/lib.rs b/pallets/reversible-transfers/src/lib.rs index 9007468d..85723ff7 100644 --- a/pallets/reversible-transfers/src/lib.rs +++ b/pallets/reversible-transfers/src/lib.rs @@ -103,7 +103,7 @@ pub mod pallet { dispatch::PostDispatchInfo, traits::{ fungible::MutateHold, schedule::v3::TaskName, tokens::Precision, CallerTrait, - DefensiveResult, StorePreimage, Time, + StorePreimage, Time, }, PalletId, }; @@ -883,12 +883,19 @@ pub mod pallet { list.retain(|id| *id != tx_id); }); - // Cancel scheduler. If the pending transfer exists, the corresponding scheduled task - // should also exist. Failure here indicates an invariant violation between this pallet - // and the scheduler. + // Cancel scheduler best-effort: the funds have already been released above, so a + // failure here must NOT propagate — propagating rolls back the whole transactional + // extrinsic, re-arming the pending transfer and permanently freezing the held funds + // (the scheduler terminally removes a named task on a failed dispatch). Mirrors the + // best-effort cancel in `recover_funds`. let schedule_id = Self::make_schedule_id(&tx_id)?; - T::Scheduler::cancel_named(schedule_id) - .defensive_map_err(|_| Error::::CancellationFailed)?; + if let Err(e) = T::Scheduler::cancel_named(schedule_id) { + log::warn!( + "Failed to cancel scheduled task for tx {:?}: {:?} (funds already released)", + tx_id, + e + ); + } Self::deposit_event(Event::TransactionCancelled { who: who.clone(), tx_id }); Ok(()) @@ -953,6 +960,15 @@ pub mod pallet { impl BuildGenesisConfig for GenesisConfig { fn build(&self) { for (who, guardian, delay) in &self.initial_high_security_accounts { + // A self-guardian silently voids all guardian protection. Enforce the same + // invariant as the signed `set_high_security` path (GuardianCannotBeSelf), + // failing genesis construction rather than admitting an unprotected account. + assert!( + guardian != who, + "Genesis high-security account {:?} cannot be its own guardian", + who + ); + // Basic validation, ensure delay is reasonable if needed let wrapped_delay = BlockNumberOrTimestampOf::::BlockNumber(*delay); diff --git a/pallets/reversible-transfers/src/tests/test_high_security_account.rs b/pallets/reversible-transfers/src/tests/test_high_security_account.rs index da4316ce..d1177316 100644 --- a/pallets/reversible-transfers/src/tests/test_high_security_account.rs +++ b/pallets/reversible-transfers/src/tests/test_high_security_account.rs @@ -381,6 +381,21 @@ fn recover_funds_cancels_across_distinct_agenda_buckets() { }); } +/// Genesis must enforce the same `guardian != who` invariant as the signed +/// `set_high_security` path. A self-guardian row silently voids all guardian +/// protection, so genesis construction must fail rather than admit it. +#[test] +#[should_panic(expected = "cannot be its own guardian")] +fn genesis_rejects_self_guardian() { + use sp_runtime::BuildStorage; + let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); + crate::GenesisConfig:: { + initial_high_security_accounts: vec![(account_id(1), account_id(1), 10)], + } + .assimilate_storage(&mut t) + .unwrap(); +} + #[test] fn recover_funds_weight_charges_agenda_per_pending_transfer() { use crate::weights::WeightInfo; diff --git a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs index 93fef9d0..72457e9d 100644 --- a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs +++ b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs @@ -5,7 +5,7 @@ use frame_support::{ traits::{fungible::InspectHold, Time}, }; use pallet_scheduler::Agenda; -use qp_scheduler::BlockNumberOrTimestamp; +use qp_scheduler::{BlockNumberOrTimestamp, ScheduleNamed}; use sp_core::H256; use sp_runtime::traits::{BadOrigin, BlakeTwo256, Hash}; @@ -641,6 +641,58 @@ fn no_volume_fee_for_regular_reversible_accounts() { }); } +/// A failed scheduled execution makes the scheduler terminally drop the named task while the +/// pending transfer and its hold survive (the failing dispatch is rolled back). Cancelling +/// afterwards must still release the held funds: the best-effort `cancel_named` must not +/// propagate its `NotFound` and roll back the release, which would permanently freeze the funds. +#[test] +fn cancel_releases_funds_when_scheduled_task_already_gone() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + let user = charlie(); // regular one-time-delay account (guardian == user) + let recipient = dave(); + let amount = 10_000u128; + + let initial_user_balance = Balances::free_balance(&user); + let call = transfer_call(recipient.clone(), amount); + let tx_id = calculate_tx_id::(user.clone(), &call); + + assert_ok!(ReversibleTransfers::schedule_transfer_with_delay( + RuntimeOrigin::signed(user.clone()), + recipient.clone(), + amount, + BlockNumberOrTimestamp::BlockNumber(5), + )); + assert_eq!( + Balances::balance_on_hold( + &RuntimeHoldReason::ReversibleTransfers(HoldReason::ScheduledTransfer), + &user + ), + amount + ); + + // Simulate the scheduler terminally removing the named task (as it does on a failed + // dispatch): the pending transfer and hold survive, but `cancel_named` now returns + // NotFound. + let schedule_id = ReversibleTransfers::make_schedule_id(&tx_id).unwrap(); + assert_ok!(>::cancel_named(schedule_id)); + + // Cancelling must still succeed and release the held funds despite the missing task. + assert_ok!(ReversibleTransfers::cancel(RuntimeOrigin::signed(user.clone()), tx_id)); + + assert!(ReversibleTransfers::pending_dispatches(tx_id).is_none()); + assert_eq!( + Balances::balance_on_hold( + &RuntimeHoldReason::ReversibleTransfers(HoldReason::ScheduledTransfer), + &user + ), + 0, + "held funds must be released, not frozen, when the scheduled task is already gone" + ); + assert_eq!(Balances::free_balance(&user), initial_user_balance); + }); +} + /// A one-time schedule freezes *cancel* authority in `pending.guardian` (= sender). /// Later `set_high_security` must not rewrite that: the owner keeps full-refund cancel /// rights, and the new guardian must not be able to cancel/seize via `cancel`. diff --git a/pallets/reversible-transfers/src/weights.rs b/pallets/reversible-transfers/src/weights.rs index 1d6f3cd4..28a6c756 100644 --- a/pallets/reversible-transfers/src/weights.rs +++ b/pallets/reversible-transfers/src/weights.rs @@ -65,11 +65,16 @@ const EXECUTE_TRANSFER_BASE_WRITES: u64 = 5; /// `execute_transfer`'s weight: the benchmarked base (compute + non-tree storage) /// plus the depth-dependent ZK-tree leaf insert performed by the wormhole proof -/// recorder. `insert_leaf` walks the tree leaf-to-root, so DB ops and PoV scale -/// with `tree_ops` via [`pallet_zk_tree::TREE_KEY_POV`]. -fn execute_transfer_weight(db: RuntimeDbWeight, (tree_reads, tree_writes): (u64, u64)) -> Weight { +/// recorder. `insert_leaf` walks the tree leaf-to-root, so hash compute, DB ops, +/// and PoV all scale with the current depth. +fn execute_transfer_weight( + db: RuntimeDbWeight, + (tree_reads, tree_writes): (u64, u64), + tree_hash_ref_time: u64, +) -> Weight { // Minimum execution time: 105_000_000 picoseconds. Weight::from_parts(110_000_000, 8619) + .saturating_add(Weight::from_parts(tree_hash_ref_time, 0)) .saturating_add(Weight::from_parts( 0, tree_reads.saturating_mul(pallet_zk_tree::TREE_KEY_POV), @@ -175,9 +180,11 @@ impl WeightInfo for SubstrateW // Proof Size summary in bytes: // Measured: `639` // Estimated: `8619` + tree + let depth = pallet_zk_tree::Pallet::::depth(); execute_transfer_weight( T::DbWeight::get(), - pallet_zk_tree::Pallet::::insert_leaf_db_ops(), + pallet_zk_tree::insert_leaf_db_ops_at_depth(depth), + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(depth), ) } /// Storage: `ReversibleTransfers::HighSecurityAccounts` (r:1 w:0) @@ -311,6 +318,7 @@ impl WeightInfo for () { execute_transfer_weight( RocksDbWeight::get(), pallet_zk_tree::insert_leaf_db_ops_at_depth(pallet_zk_tree::MAX_TREE_DEPTH), + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(pallet_zk_tree::MAX_TREE_DEPTH), ) } /// Storage: `ReversibleTransfers::HighSecurityAccounts` (r:1 w:0) @@ -375,6 +383,14 @@ mod tests { shallow, deep, ); + // The per-level Poseidon hashing of `insert_leaf` must also be priced, so + // ref-time grows with depth even when `DbWeight` is zero. + assert!( + deep.ref_time() > shallow.ref_time(), + "execute_transfer ref-time must grow with ZK-tree depth (shallow: {:?}, deep: {:?})", + shallow, + deep, + ); // The depth-blind `()` impl prices at `MAX_TREE_DEPTH` and must never // charge less than `SubstrateWeight` at any live depth. assert!( diff --git a/runtime/src/transaction_extensions.rs b/runtime/src/transaction_extensions.rs index 3bdec54a..26f51214 100644 --- a/runtime/src/transaction_extensions.rs +++ b/runtime/src/transaction_extensions.rs @@ -109,12 +109,18 @@ impl WormholeProofRecorderExtension /// /// Per recorded transfer, `record_transfer` touches one `TransferCount` read and one /// write, plus the ZK-tree leaf insert, whose path update walks the tree leaf-to-root - /// and therefore costs reads/writes proportional to the *current* tree depth (read from - /// storage here, so the charge tracks the tree as it deepens over the chain's life). + /// and therefore costs reads/writes and Poseidon hashing proportional to the *current* + /// tree depth (read from storage here, so the charge tracks the tree as it deepens over + /// the chain's life). fn per_transfer_weight() -> Weight { - let (tree_reads, tree_writes) = pallet_zk_tree::Pallet::::insert_leaf_db_ops(); + let depth = pallet_zk_tree::Depth::::get(); + let (tree_reads, tree_writes) = pallet_zk_tree::insert_leaf_db_ops_at_depth(depth); T::DbWeight::get() .reads_writes(1u64.saturating_add(tree_reads), 1u64.saturating_add(tree_writes)) + .saturating_add(Weight::from_parts( + pallet_zk_tree::insert_leaf_hash_ref_time_at_depth(depth), + 0, + )) } fn count_transfers(call: &RuntimeCall) -> u64 { From d61aedba7233aa02b0601ce9adc8b7f96f5400d6 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:39:06 +0800 Subject: [PATCH 5/7] V12 round 2: add multisig v0->v1 proposal migration (181353) A prior commit dropped ProposalData::call_weight and bumped STORAGE_VERSION to 1 without a migration, stranding v0 records. Adds a VersionedMigration that translates stored Proposals to the new layout, registers it in the runtime, and updates the README. Co-Authored-By: Claude Opus 4.8 --- Cargo.lock | 1 + pallets/multisig/Cargo.toml | 9 +++ pallets/multisig/README.md | 9 ++- pallets/multisig/src/lib.rs | 2 + pallets/multisig/src/migrations.rs | 101 +++++++++++++++++++++++++++++ pallets/multisig/src/tests.rs | 43 ++++++++++++ runtime/src/lib.rs | 2 + 7 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 pallets/multisig/src/migrations.rs diff --git a/Cargo.lock b/Cargo.lock index 42370a7f..09f4553a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5978,6 +5978,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "log", "pallet-balances", "pallet-preimage", "pallet-recovery", diff --git a/pallets/multisig/Cargo.toml b/pallets/multisig/Cargo.toml index ec79ad8b..1a5a42a0 100644 --- a/pallets/multisig/Cargo.toml +++ b/pallets/multisig/Cargo.toml @@ -16,6 +16,7 @@ codec = { features = ["derive", "max-encoded-len"], workspace = true } frame-benchmarking = { optional = true, workspace = true } frame-support.workspace = true frame-system.workspace = true +log.workspace = true pallet-balances.workspace = true pallet-reversible-transfers = { path = "../reversible-transfers", default-features = false, optional = true } qp-high-security = { path = "../../primitives/high-security", default-features = false } @@ -59,6 +60,7 @@ std = [ "frame-benchmarking?/std", "frame-support/std", "frame-system/std", + "log/std", "pallet-balances/std", "pallet-reversible-transfers?/std", "pallet-timestamp/std", @@ -70,3 +72,10 @@ std = [ "sp-io/std", "sp-runtime/std", ] +try-runtime = [ + "frame-support/try-runtime", + "frame-system/try-runtime", + "pallet-balances/try-runtime", + "pallet-reversible-transfers?/try-runtime", + "sp-runtime/try-runtime", +] diff --git a/pallets/multisig/README.md b/pallets/multisig/README.md index 563e195a..4840ef78 100644 --- a/pallets/multisig/README.md +++ b/pallets/multisig/README.md @@ -316,7 +316,6 @@ Stores proposal data indexed by (multisig_address, proposal_id): ProposalData { proposer: AccountId, // Who proposed (receives deposit back) call: BoundedVec, // Encoded RuntimeCall to execute - call_weight: Weight, // Declared inner-call weight captured at propose time expiry: BlockNumber, // Deadline for approvals approvals: BoundedVec, // List of signers who approved deposit: Balance, // Reserved deposit (refundable) @@ -486,8 +485,8 @@ This event structure is optimized for indexing by SubSquid and similar indexers: - **No global limits:** Only per-multisig limits (decentralized resistance) ### Call Execution -- Calls are decoded and validated at `propose()` time, then stored as bounded call bytes with the declared `call_weight` -- Calls are decoded again at `execute()` time before dispatch +- Calls are decoded and validated at `propose()` time (including an inner-call weight check against `MaxInnerCallWeight`), then stored as bounded call bytes +- Calls are decoded again at `execute()` time before dispatch, and the inner-call weight is recomputed then (it is not stored) - High-security whitelist enforcement runs at proposal creation for currently high-security multisigs and again at execution time - Allowed calls execute with multisig_address as origin - Standard multisigs can call any pallet (including recursive multisig calls) as long as the call fits size and weight limits @@ -646,9 +645,9 @@ Normal multisigs automatically get refunded for unused high-security overhead. **Weight calculation:** - `propose()` charges upfront for the current worst-case proposal path used by the implementation: `propose_high_security(call.len())`. Actual weight is refunded based on path: `propose(call_size)` for normal multisigs, `propose_high_security(call_size)` for high-security multisigs. No cleanup runs in propose. -- `propose()` rejects calls whose declared `call_weight` exceeds `MaxInnerCallWeight`. +- `propose()` rejects calls whose inner-call weight (from `get_dispatch_info()`) exceeds `MaxInnerCallWeight`. - `execute()` charges upfront for bookkeeping worst-case plus the maximum allowed inner-call weight: `WeightInfo::execute(T::MaxCallSize::get()) + T::MaxInnerCallWeight::get()`. -- `execute()` returns actual weight as bookkeeping for the stored call size plus the inner call's post-dispatch weight, using the stored `call_weight` as fallback when the inner call does not report actual weight. +- `execute()` returns actual weight as bookkeeping for the stored call size plus the inner call's post-dispatch weight, using the inner-call weight recomputed at execute time as fallback when the inner call does not report actual weight. - `claim_deposits()` charges upfront for worst-case iteration and cleanup; actual weight based on proposals iterated and cleaned (dynamic refund). **Security notes:** diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index 2c9edb72..e1406a4e 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -36,6 +36,8 @@ pub use weights::*; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; +pub mod migrations; + #[cfg(test)] mod mock; diff --git a/pallets/multisig/src/migrations.rs b/pallets/multisig/src/migrations.rs new file mode 100644 index 00000000..914ebfd5 --- /dev/null +++ b/pallets/multisig/src/migrations.rs @@ -0,0 +1,101 @@ +//! Storage migrations for `pallet-multisig`. + +extern crate alloc; + +use crate::{ + pallet::{Config, Pallet, Proposals}, + ProposalStatus, +}; +use codec::{Decode, Encode}; +use core::marker::PhantomData; +use frame_support::{ + traits::{Get, UncheckedOnRuntimeUpgrade}, + weights::Weight, +}; + +#[cfg(feature = "try-runtime")] +use alloc::vec::Vec; + +/// v0 -> v1: drop the removed `call_weight` field from every stored proposal. +/// +/// Storage version 0 stored `call_weight: Weight` positionally between `call` and `expiry`. +/// Version 1 recomputes the inner call weight at execute time and no longer stores it. Without +/// this migration a v0 `Proposals` record fails to decode (stranding its deposit and per-signer +/// count), so this one-shot translate rewrites each record into the current layout. +pub mod v1 { + use super::*; + use crate::{ + pallet::{BoundedApprovalsOf, BoundedCallOf, ProposalDataOf}, + BalanceOf, + }; + use frame_system::pallet_prelude::BlockNumberFor; + + /// Storage-version-0 layout of `ProposalData`, kept only to decode pre-upgrade records so the + /// `call_weight` field can be dropped. `Encode` is derived for test fixtures. + #[derive(Encode, Decode)] + pub struct OldProposalData { + pub proposer: AccountId, + pub call: BoundedCall, + pub call_weight: Weight, + pub expiry: BlockNumber, + pub approvals: BoundedApprovals, + pub deposit: Balance, + pub status: ProposalStatus, + } + + pub type OldProposalDataOf = OldProposalData< + ::AccountId, + BalanceOf, + BlockNumberFor, + BoundedCallOf, + BoundedApprovalsOf, + >; + + /// Rewrites every [`Proposals`] entry from the v0 layout to v1 by dropping `call_weight`. + pub struct DropCallWeight(PhantomData); + + impl UncheckedOnRuntimeUpgrade for DropCallWeight { + fn on_runtime_upgrade() -> Weight { + let mut count = 0u64; + Proposals::::translate::, _>(|_addr, _id, old| { + count = count.saturating_add(1); + Some(ProposalDataOf:: { + proposer: old.proposer, + call: old.call, + expiry: old.expiry, + approvals: old.approvals, + deposit: old.deposit, + status: old.status, + }) + }); + log::info!( + target: "runtime::multisig", + "Migrated {count} multisig proposal(s) to v1 (dropped call_weight)", + ); + T::DbWeight::get().reads_writes(count, count) + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + // Every record must decode under the current layout after the translation. + let count = Proposals::::iter().count(); + log::info!(target: "runtime::multisig", "post_upgrade: {count} proposal(s) decode under v1"); + Ok(()) + } + } +} + +/// Versioned v0 -> v1 migration. Runs [`v1::DropCallWeight`] only when the on-chain storage +/// version is 0, then bumps the on-chain storage version to 1. +pub type MigrateV0ToV1 = frame_support::migrations::VersionedMigration< + 0, + 1, + v1::DropCallWeight, + Pallet, + ::DbWeight, +>; diff --git a/pallets/multisig/src/tests.rs b/pallets/multisig/src/tests.rs index de68035d..0f960305 100644 --- a/pallets/multisig/src/tests.rs +++ b/pallets/multisig/src/tests.rs @@ -2636,3 +2636,46 @@ fn execute_rejects_call_when_max_weight_lowered_after_propose() { reset_max_inner_call_weight(); }); } + +#[test] +fn migration_v0_to_v1_drops_call_weight() { + use crate::migrations::{v1::OldProposalData, MigrateV0ToV1}; + use frame_support::traits::{GetStorageVersion, OnRuntimeUpgrade, StorageVersion}; + + new_test_ext().execute_with(|| { + // Pretend the pallet is still at on-chain storage version 0. + StorageVersion::new(0).put::>(); + + let addr = AccountId32::new([9u8; 32]); + let proposer = AccountId32::new([1u8; 32]); + let id = 0u32; + let call: crate::BoundedCallOf = vec![1u8, 2, 3].try_into().unwrap(); + let approvals: crate::BoundedApprovalsOf = vec![proposer.clone()].try_into().unwrap(); + let expiry: u64 = 100; + let deposit: u128 = 1_000; + + // Write a record in the v0 layout (with call_weight) at the real Proposals key. + let old = OldProposalData { + proposer: proposer.clone(), + call: call.clone(), + call_weight: Weight::from_parts(123, 456), + expiry, + approvals: approvals.clone(), + deposit, + status: ProposalStatus::Active, + }; + let key = Proposals::::hashed_key_for(&addr, id); + frame_support::storage::unhashed::put_raw(&key, &old.encode()); + + MigrateV0ToV1::::on_runtime_upgrade(); + + let migrated = Proposals::::get(&addr, id).expect("record migrated to v1 layout"); + assert_eq!(migrated.proposer, proposer); + assert_eq!(migrated.call, call); + assert_eq!(migrated.expiry, expiry); + assert_eq!(migrated.approvals, approvals); + assert_eq!(migrated.deposit, deposit); + assert_eq!(migrated.status, ProposalStatus::Active); + assert_eq!(crate::Pallet::::on_chain_storage_version(), StorageVersion::new(1)); + }); +} diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index b286b31d..6e204b12 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -171,6 +171,8 @@ pub type Migrations = ( pallet_wormhole::migrations::MigrateV1ToV2, // v0 -> v1: set the treasury portion to 50% (50/50 treasury/miner reward split). pallet_treasury::migrations::MigrateV0ToV1, + // v0 -> v1: drop the removed `call_weight` field from stored multisig proposals. + pallet_multisig::migrations::MigrateV0ToV1, ); /// Executive: handles dispatch to the various modules. From e37cd8244573439f23b423f375f51825315a95d8 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:39:06 +0800 Subject: [PATCH 6/7] V12 round 2: harden governance genesis, high-security dispatch, benchmark registry (181308, 181339, 181268, 181332, 181263) Grow the tech collective to the 5-member size the 60/61% curves assume and reject undersized genesis seeds (critical quorum-collapse); re-seed the benchmark dev collective; fund planck treasury signers for multisig operation; enforce the high-security whitelist on Utility::dispatch_as{,_fallible}; register the six active pallets missing from define_benchmarks! (with the utility bench feature). Updates the tech-collective tests to the 5-member invariant. Co-Authored-By: Claude Opus 4.8 --- pallets/utility/src/lib.rs | 23 ++++- pallets/utility/src/tests.rs | 51 ++++++++++ runtime/Cargo.toml | 2 + runtime/src/benchmarks.rs | 6 ++ runtime/src/genesis_config_presets.rs | 107 +++++++++++++++++--- runtime/src/governance/definitions.rs | 7 ++ runtime/tests/governance/tech_collective.rs | 59 ++++++----- 7 files changed, 213 insertions(+), 42 deletions(-) diff --git a/pallets/utility/src/lib.rs b/pallets/utility/src/lib.rs index 6152de57..77444042 100644 --- a/pallets/utility/src/lib.rs +++ b/pallets/utility/src/lib.rs @@ -387,7 +387,17 @@ pub mod pallet { ) -> DispatchResult { ensure_root(origin)?; - let res = call.dispatch_bypass_filter((*as_origin).into()); + // If the effective origin resolves to a signed account, enforce the high-security + // whitelist for it — mirroring `as_derivative` — so Root cannot rewrite the origin to + // dispatch a non-whitelisted call as a high-security account. + let as_origin: T::RuntimeOrigin = (*as_origin).into(); + if let Some(who) = as_origin.as_signer() { + ensure!( + T::HighSecurity::is_call_allowed(who, &call), + Error::::CallNotAllowedForHighSecurity + ); + } + let res = call.dispatch_bypass_filter(as_origin); Self::deposit_event(Event::DispatchedAs { result: res.map(|_| ()).map_err(|e| e.error), @@ -590,7 +600,16 @@ pub mod pallet { ) -> DispatchResult { ensure_root(origin)?; - call.dispatch_bypass_filter((*as_origin).into()).map_err(|e| e.error)?; + // Same high-security guard as `dispatch_as`: a signed effective origin is restricted to + // its whitelisted calls. + let as_origin: T::RuntimeOrigin = (*as_origin).into(); + if let Some(who) = as_origin.as_signer() { + ensure!( + T::HighSecurity::is_call_allowed(who, &call), + Error::::CallNotAllowedForHighSecurity + ); + } + call.dispatch_bypass_filter(as_origin).map_err(|e| e.error)?; Self::deposit_event(Event::DispatchedAs { result: Ok(()) }); diff --git a/pallets/utility/src/tests.rs b/pallets/utility/src/tests.rs index de5b3b08..4d4758f5 100644 --- a/pallets/utility/src/tests.rs +++ b/pallets/utility/src/tests.rs @@ -964,6 +964,57 @@ fn dispatch_as_works() { }) } +#[test] +fn dispatch_as_enforces_high_security_whitelist() { + new_test_ext().execute_with(|| { + qp_high_security::testing::reset(); + qp_high_security::testing::set_high_security(&666u64); + + // A non-whitelisted call dispatched as the high-security account is rejected before + // any dispatch happens. + assert_noop!( + Utility::dispatch_as( + RuntimeOrigin::root(), + Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(666))), + Box::new(call_transfer(777, 1)), + ), + Error::::CallNotAllowedForHighSecurity, + ); + + // A whitelisted call (System::remark) as the same account is allowed. + assert_ok!(Utility::dispatch_as( + RuntimeOrigin::root(), + Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(666))), + Box::new(RuntimeCall::System(SystemCall::remark { remark: Default::default() })), + )); + + // A non-high-security account is unaffected by the guard. + assert_ok!(Utility::dispatch_as( + RuntimeOrigin::root(), + Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(1))), + Box::new(RuntimeCall::System(SystemCall::remark { remark: Default::default() })), + )); + qp_high_security::testing::reset(); + }) +} + +#[test] +fn dispatch_as_fallible_enforces_high_security_whitelist() { + new_test_ext().execute_with(|| { + qp_high_security::testing::reset(); + qp_high_security::testing::set_high_security(&666u64); + assert_noop!( + Utility::dispatch_as_fallible( + RuntimeOrigin::root(), + Box::new(OriginCaller::system(frame_system::RawOrigin::Signed(666))), + Box::new(call_transfer(777, 1)), + ), + Error::::CallNotAllowedForHighSecurity, + ); + qp_high_security::testing::reset(); + }) +} + #[test] fn if_else_with_root_works() { new_test_ext().execute_with(|| { diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index 0ea2b4d3..991bfbca 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -149,6 +149,7 @@ runtime-benchmarks = [ "pallet-timestamp/runtime-benchmarks", "pallet-transaction-payment/runtime-benchmarks", "pallet-treasury/runtime-benchmarks", + "pallet-utility/runtime-benchmarks", "pallet-wormhole/runtime-benchmarks", "sp-runtime/runtime-benchmarks", ] @@ -160,6 +161,7 @@ try-runtime = [ "frame-try-runtime/try-runtime", "pallet-balances/try-runtime", "pallet-mining-rewards/try-runtime", + "pallet-multisig/try-runtime", "pallet-qpow/try-runtime", "pallet-ranked-collective/try-runtime", "pallet-recovery/try-runtime", diff --git a/runtime/src/benchmarks.rs b/runtime/src/benchmarks.rs index d9892d3d..81e67ac5 100644 --- a/runtime/src/benchmarks.rs +++ b/runtime/src/benchmarks.rs @@ -28,11 +28,17 @@ frame_benchmarking::define_benchmarks!( [frame_system, SystemBench::] [pallet_balances, Balances] [pallet_timestamp, Timestamp] + [pallet_transaction_payment, TransactionPayment] [pallet_reversible_transfers, ReversibleTransfers] [pallet_mining_rewards, MiningRewards] + [pallet_preimage, Preimage] [pallet_treasury, TreasuryPallet] [pallet_multisig, Multisig] + [pallet_utility, Utility] [pallet_scheduler, Scheduler] + [pallet_ranked_collective, TechCollective] + [pallet_referenda, TechReferenda] + [pallet_recovery, Recovery] [pallet_qpow, QPoW] [pallet_wormhole, Wormhole] ); diff --git a/runtime/src/genesis_config_presets.rs b/runtime/src/genesis_config_presets.rs index 2bc94376..018af4f3 100644 --- a/runtime/src/genesis_config_presets.rs +++ b/runtime/src/genesis_config_presets.rs @@ -25,12 +25,21 @@ use alloc::{ vec::Vec, }; use pallet_multisig::Pallet as Multisig; -use qp_dilithium_crypto::pair::{crystal_alice, crystal_charlie, dilithium_bob}; +use qp_dilithium_crypto::{ + pair::{crystal_alice, crystal_charlie, dilithium_bob}, + Dilithium87Pair, +}; use serde_json::Value; -use sp_core::crypto::Ss58Codec; +use sp_core::{crypto::Ss58Codec, Pair}; use sp_genesis_builder::{self, PresetId}; use sp_runtime::{traits::IdentifyAccount, Permill}; +/// Minimum tech-collective size the tech-referenda approval/support curves in +/// [`crate::governance::definitions`] are designed for (see the 5-member analysis on +/// `TechCollectiveTracksInfo`). A non-empty genesis seed smaller than this would let a minority +/// authorize Root, so [`seed_tech_collective`] rejects it (fail-early). +pub const MIN_TECH_COLLECTIVE_MEMBERS: usize = 5; + /// Well-known test secret for testing ZK proof spending. /// This is a simple pattern (`[42u8; 32]`) for easy testing. /// Use this secret with `quantus wormhole prove` to spend from the test address. @@ -129,21 +138,46 @@ struct TreasuryGenesis { portion: Permill, } -/// Initial tech collective members for the development preset (configurable independently of -/// treasury). +/// Two extra well-known Dilithium accounts (public seeds `[3u8; 32]` / `[4u8; 32]`) that pad the +/// `dev` and `heisenberg` tech collectives to the [`MIN_TECH_COLLECTIVE_MEMBERS`] size the +/// tech-referenda curves are designed for. These public keys are acceptable only for those +/// non-value-bearing chains — see [`dilithium_default_accounts`]. +fn dilithium_extra_collective_members() -> Vec { + [[3u8; 32], [4u8; 32]] + .into_iter() + .map(|seed| { + Dilithium87Pair::from_seed_slice(&seed) + .expect("static 32-byte seed is valid") + .into_account() + }) + .collect() +} + +/// Initial tech collective members for the development preset. Grown to +/// [`MIN_TECH_COLLECTIVE_MEMBERS`] so the tech-referenda curves behave as designed. fn development_tech_collective_seed() -> Vec { - dilithium_default_accounts() + let mut members = dilithium_default_accounts(); + members.extend(dilithium_extra_collective_members()); + members } -/// Initial tech collective members for Heisenberg (defaults to the same accounts as treasury -/// signers; kept as a separate hook if the two diverge). +/// Initial tech collective members for Heisenberg. Grown to [`MIN_TECH_COLLECTIVE_MEMBERS`] so the +/// tech-referenda curves behave as designed. fn heisenberg_tech_collective_seed() -> Vec { - heisenberg_treasury_signers() + let mut members = heisenberg_treasury_signers(); + members.extend(dilithium_extra_collective_members()); + members } -/// Initial tech collective members for Planck (defaults to the same accounts as treasury signers). +/// Initial tech collective members for Planck: the three treasury signers plus two dedicated +/// members, giving the [`MIN_TECH_COLLECTIVE_MEMBERS`] the tech-referenda curves assume. fn planck_tech_collective_seed() -> Vec { - planck_treasury_signers() + let mut members = planck_treasury_signers(); + members.extend([ + account_from_ss58("qzmTAz3UUw1WGUuVh8nbFmPwcftomduwy6twq6NDR6y9qqtEs"), + account_from_ss58("qzm5QCox8Dp5A3oSXZZYHD8YoYgPz7enykZb6RPUropdCyN5h"), + ]); + members } /// Returns the genesis config populated with given parameters. Treasury is per-profile. @@ -256,15 +290,24 @@ pub fn development_config_genesis() -> Value { TreasuryGenesis { account: treasury_account, portion: Permill::from_percent(50) }; let mut template_value = genesis_template(endowed_accounts, treasury, tech_collective, vec![]); - // `genesis_template` adds a chain-spec-only field; strip before deserializing. - template_value + // `genesis_template` adds a chain-spec-only field that `RuntimeGenesisConfig` cannot + // deserialize; strip it before deserializing, then restore it on the returned JSON so + // `build_state` still seeds the tech collective (otherwise a benchmark dev chain starts + // with an empty collective and nobody can pass RootOrMemberForTechReferendaOrigin). + let tech_collective_members = template_value .as_object_mut() .expect("RuntimeGenesisConfig serializes to a JSON object") .remove(TECH_COLLECTIVE_SEED_MEMBERS_KEY); let mut config: RuntimeGenesisConfig = serde_json::from_value(template_value).expect("genesis_template returns valid config"); config.reversible_transfers = rt_genesis; - return serde_json::to_value(config).expect("Could not build genesis config."); + let mut out = serde_json::to_value(config).expect("Could not build genesis config."); + if let Some(members) = tech_collective_members { + out.as_object_mut() + .expect("RuntimeGenesisConfig serializes to a JSON object") + .insert(TECH_COLLECTIVE_SEED_MEMBERS_KEY.into(), members); + } + return out; } #[cfg(not(feature = "runtime-benchmarks"))] @@ -356,6 +399,13 @@ pub fn seed_tech_collective(members: &[AccountId]) -> Result<(), String> { if members.is_empty() { return Ok(()); } + if members.len() < MIN_TECH_COLLECTIVE_MEMBERS { + return Err(alloc::format!( + "tech collective seed has {} members; the governance curves require at least {}", + members.len(), + MIN_TECH_COLLECTIVE_MEMBERS + )); + } log::info!("🏛️ Seeding tech collective with {} members", members.len()); let ss58 = ss58_version(); for member in members { @@ -383,7 +433,10 @@ pub fn planck_config_genesis() -> Value { let tech_collective = planck_tech_collective_seed(); let treasury_account = planck_treasury_account(); let endowed_accounts = vec![planck_faucet_account()]; - let signer_fee_seed: Vec<_> = treasury_signers.iter().cloned().map(|a| (a, UNIT)).collect(); + // Each signer needs enough to create + propose on the treasury multisig once: MultisigFee + // (0.6) burned + ProposalFee (~1.03) burned + ProposalDeposit (1.0) reserved = ~2.63 UNIT, + // plus transaction fees and the existential deposit. 1 UNIT was insufficient. + let signer_fee_seed: Vec<_> = treasury_signers.iter().cloned().map(|a| (a, 3 * UNIT)).collect(); log_genesis_accounts( "planck", &endowed_accounts, @@ -425,3 +478,29 @@ pub fn preset_names() -> Vec { PresetId::from(PLANCK_RUNTIME_PRESET), ] } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn seed_tech_collective_rejects_undersized_seed() { + let too_few: Vec = (0..(MIN_TECH_COLLECTIVE_MEMBERS as u8 - 1)) + .map(|i| AccountId::new([i; 32])) + .collect(); + assert!(seed_tech_collective(&too_few).is_err()); + // An absent seed (empty) stays valid: it just means the collective is not seeded here. + assert!(seed_tech_collective(&[]).is_ok()); + } + + #[test] + fn all_presets_meet_the_tech_collective_floor() { + for seed in [ + development_tech_collective_seed(), + heisenberg_tech_collective_seed(), + planck_tech_collective_seed(), + ] { + assert!(seed.len() >= MIN_TECH_COLLECTIVE_MEMBERS); + } + } +} diff --git a/runtime/src/governance/definitions.rs b/runtime/src/governance/definitions.rs index 20bc0169..a989f3dc 100644 --- a/runtime/src/governance/definitions.rs +++ b/runtime/src/governance/definitions.rs @@ -99,6 +99,13 @@ impl TechCollectiveTracksInfo { // (3 ayes / 2 nays = 60% approval < 61%). Constant curves: thresholds don't // decay over the decision period. The 24h confirm period guarantees nays can // arrive for a full day before any approval; enactment is delayed another 24h. + // + // These curves assume at least 5 members: every shipped preset seeds >= 5 and + // `genesis_config_presets::seed_tech_collective` rejects a smaller non-empty seed + // (see `MIN_TECH_COLLECTIVE_MEMBERS`). NOTE: `RemoveOrigin` is Root, so a passed + // Root referendum can still shrink the collective below 5; there is no in-pallet + // membership floor on the removal path (that would require a `pallet_ranked_collective` + // change), so removals must preserve this minimum by convention. let info = pallet_referenda::TrackInfo { name: str_array("tech_collective_members"), max_deciding: 1, diff --git a/runtime/tests/governance/tech_collective.rs b/runtime/tests/governance/tech_collective.rs index c64ad071..c87f9621 100644 --- a/runtime/tests/governance/tech_collective.rs +++ b/runtime/tests/governance/tech_collective.rs @@ -1323,14 +1323,14 @@ mod tests { TestCommons::new_fast_governance_test_ext().execute_with(|| { let proposer = TestCommons::account_id(1); let voter = TestCommons::account_id(2); + let extra: Vec<_> = (3..=5u8).map(TestCommons::account_id).collect(); Balances::make_free_balance_be(&proposer, 3000 * UNIT); Balances::make_free_balance_be(&voter, 2000 * UNIT); - assert_ok!(quantus_runtime::genesis_config_presets::seed_tech_collective(&[ - proposer.clone(), - voter.clone(), - ])); + let mut seed = vec![proposer.clone(), voter.clone()]; + seed.extend(extra.iter().cloned()); + assert_ok!(quantus_runtime::genesis_config_presets::seed_tech_collective(&seed)); assert!( pallet_ranked_collective::Members::::contains_key(&proposer), @@ -1366,18 +1366,15 @@ mod tests { referendum_index )); - // Both members must vote aye to clear the 60% support threshold (2/2 = 100%) - assert_ok!(TechCollective::vote( - RuntimeOrigin::signed(proposer.clone()), - referendum_index, - true - )); - - assert_ok!(TechCollective::vote( - RuntimeOrigin::signed(voter.clone()), - referendum_index, - true - )); + // All five seeded members vote aye (5/5 = 100%) to clear the 60% support and + // 61% approval thresholds the curves assume for a five-member collective. + for member in seed.iter() { + assert_ok!(TechCollective::vote( + RuntimeOrigin::signed(member.clone()), + referendum_index, + true + )); + } let track_info = >::Tracks::info( @@ -1414,22 +1411,31 @@ mod tests { fn seed_tech_collective_rejects_duplicates_without_panicking() { TestCommons::new_fast_governance_test_ext().execute_with(|| { let member = TestCommons::account_id(1); + // A valid-size seed (>= MIN_TECH_COLLECTIVE_MEMBERS) that repeats a member, so the + // duplicate — not the size floor — is the reason for rejection. + let dup_seed: Vec<_> = vec![ + member.clone(), + TestCommons::account_id(2), + TestCommons::account_id(3), + TestCommons::account_id(4), + member.clone(), + ]; let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - quantus_runtime::genesis_config_presets::seed_tech_collective(&[ - member.clone(), - member.clone(), - ]) + quantus_runtime::genesis_config_presets::seed_tech_collective(&dup_seed) })); let result = outcome.expect("seeding with a duplicate member must return an error, not panic"); let err = result.expect_err("duplicate member must be rejected"); assert!(err.contains("AlreadyMember"), "error should name the cause: {err}"); - // Overlap with a member already added by other genesis mechanisms must also - // surface as an error. + // Overlap with a member already added above must also surface as an error. + let overlap_seed: Vec<_> = vec![member.clone()] + .into_iter() + .chain((5..=8u8).map(TestCommons::account_id)) + .collect(); assert!( - quantus_runtime::genesis_config_presets::seed_tech_collective(&[member.clone()]) + quantus_runtime::genesis_config_presets::seed_tech_collective(&overlap_seed) .is_err(), "re-seeding an existing member must be rejected" ); @@ -1445,9 +1451,10 @@ mod tests { Balances::make_free_balance_be(&proposer, 3000 * UNIT); Balances::make_free_balance_be(&non_member, 3000 * UNIT); - assert_ok!(quantus_runtime::genesis_config_presets::seed_tech_collective(&[ - proposer.clone() - ])); + let seed: Vec<_> = std::iter::once(proposer.clone()) + .chain((2..=5u8).map(TestCommons::account_id)) + .collect(); + assert_ok!(quantus_runtime::genesis_config_presets::seed_tech_collective(&seed)); assert!( !pallet_ranked_collective::Members::::contains_key(&non_member), From a48bc7ffa6cee936864a920dec91aa6d6cfdbfc8 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Fri, 7 Aug 2026 16:39:06 +0800 Subject: [PATCH 7/7] V12 round 2: fix qpow fork-choice, difficulty, and mining-loop gaps (180135, 181313, 181437, 181438, 181258, 181374, 181403, 180119, 180118, 181245) Serialize the fork-choice decision under a shared import lock; validate genesis difficulty; fail closed on cumulative-work read errors; floor the retarget block time; use the full reorg window; error-log (not swallow) finalization failures; rebuild the mining candidate after a failed import and after major sync; read the build version before snapshotting; gate seal submission on the sync oracle. Co-Authored-By: Claude Opus 4.8 --- client/consensus/qpow/src/chain_management.rs | 7 +-- client/consensus/qpow/src/lib.rs | 48 ++++++++++++------- client/consensus/qpow/src/worker.rs | 47 ++++++++++++++++-- node/src/service.rs | 12 +++-- pallets/qpow/src/lib.rs | 16 +++++++ pallets/qpow/src/tests.rs | 39 +++++++++++++++ 6 files changed, 143 insertions(+), 26 deletions(-) diff --git a/client/consensus/qpow/src/chain_management.rs b/client/consensus/qpow/src/chain_management.rs index 92da188c..09e88251 100644 --- a/client/consensus/qpow/src/chain_management.rs +++ b/client/consensus/qpow/src/chain_management.rs @@ -159,7 +159,8 @@ pub fn is_heavier( (candidate_work == current_work && candidate_number > current_number) } -/// Finalizes blocks that are `max_reorg_depth - 1` blocks behind the current best block. +/// Finalizes blocks that are `max_reorg_depth` blocks behind the current best block, +/// keeping exactly `max_reorg_depth` blocks reorganizable to match the configured window. /// This should be called synchronously after each block import to ensure finalization /// happens before the next block is imported. /// @@ -204,8 +205,8 @@ where ChainManagementError::RuntimeApiError(format!("Failed to get max reorg depth: {:?}", e)) })?; - // Calculate how far back to finalize - let finalize_depth = max_reorg_depth.saturating_sub(1); + // Keep the full maximum reorganization window unfinalized. + let finalize_depth = max_reorg_depth; // Only finalize if we have enough blocks if best_number <= finalize_depth.into() { diff --git a/client/consensus/qpow/src/lib.rs b/client/consensus/qpow/src/lib.rs index ce13bd9e..97756592 100644 --- a/client/consensus/qpow/src/lib.rs +++ b/client/consensus/qpow/src/lib.rs @@ -102,6 +102,9 @@ pub struct PowBlockImport, I, C, CIDP, BE, const LOGGING_ client: Arc, create_inherent_data_providers: Arc, check_inherents_after: <::Header as HeaderT>::Number, + // Serializes the best-work read, fork-choice decision and inner import so + // concurrent imports cannot race on a stale best. Shared across clones. + import_lock: Arc>, _backend: PhantomData, } @@ -120,6 +123,7 @@ impl< client: self.client.clone(), create_inherent_data_providers: self.create_inherent_data_providers.clone(), check_inherents_after: self.check_inherents_after, + import_lock: self.import_lock.clone(), _backend: PhantomData, } } @@ -156,6 +160,7 @@ where client, check_inherents_after, create_inherent_data_providers: Arc::new(create_inherent_data_providers), + import_lock: Arc::new(futures::lock::Mutex::new(())), _backend: PhantomData, } } @@ -290,21 +295,20 @@ where return Err(Error::::InvalidSeal.into()); } - // Get parent's cumulative achieved work from aux storage - let parent_work = get_chain_work::(&*self.client, parent_hash).unwrap_or_else(|e| { - log::warn!(target: LOG_TARGET, "Failed to get parent achieved work for {parent_hash:?}: {e:?}"); - U512::zero() - }); + // Get parent's cumulative achieved work from aux storage. A backend/decode + // failure must fail the import, not silently seed fork choice with zero. + let parent_work = get_chain_work::(&*self.client, parent_hash)?; // Calculate new cumulative achieved work let new_work = parent_work.saturating_add(achieved_difficulty); + // Serialize the best-work read, fork-choice decision and inner import so a + // concurrent import cannot commit a new best between our read and our commit + // and let a weaker block win fork choice. Held until the end of the import. + let _import_guard = self.import_lock.lock().await; + let info = self.client.info(); - let current_best_work = get_chain_work::(&*self.client, info.best_hash) - .unwrap_or_else(|e| { - log::warn!(target: LOG_TARGET, "Failed to get best chain achieved work for {:?}: {e:?}", info.best_hash); - U512::zero() - }); + let current_best_work = get_chain_work::(&*self.client, info.best_hash)?; let is_best = is_heavier( new_work, @@ -374,11 +378,16 @@ where }, }; - // Finalization prunes competing forks that are beyond max_reorg_depth. + // Finalization prunes competing forks that are beyond max_reorg_depth. A + // failure must be surfaced (error log with block context) but must NOT gate + // block import: finalization is retried on every subsequent import, and + // halting on a transient error would harm liveness. if let Err(e) = finalize_canonical_at_depth::(&*self.client) { - log::warn!( + log::error!( target: LOG_TARGET, - "Failed to finalize after block import: {:?}", + "Failed to finalize after importing block #{} ({:?}): {:?} (import not gated; will retry on next import)", + block_number_u64, + block_hash, e ); } @@ -516,15 +525,22 @@ where tx_notifications, MIN_INTERVAL_BETWEEN_TX_REBUILDS, ); - let worker = MiningHandle::new(client.clone(), block_import, justification_sync_link); - let worker_ret = worker.clone(); - // Latest build request - overwrites previous if builder is slow. // Uses a Mutex