From c36ca229ac39fe23ac29ed9a8896e7fd35022374 Mon Sep 17 00:00:00 2001 From: Loom Agent Date: Sat, 11 Jul 2026 20:59:02 +0000 Subject: [PATCH] fix(drand): reject round skips to prevent LastStoredRound wedge `write_pulse` is an unsigned, feeless extrinsic whose only round gate was `pulse.round > last_stored_round`, with no upper bound. A single accepted pulse could therefore leap `LastStoredRound` from L to R, leaving rounds L+1..R-1 permanently un-storable (every later pulse must exceed `last`), which wedges the CR-v3 weight reveals and the metadata timelocks that reference those skipped rounds. Bound the advance at two layers: - dispatch (`write_pulse`): once a baseline is anchored, require `pulse.round == last_stored_round + 1`. The offchain worker already submits a contiguous run starting at `last + 1`, one single-round transaction at a time, so this stays liveness-safe while making a leap impossible. The first-storage anchor (last == 0 && oldest == 0) still accepts any round > 0, matching the OCW which seeds `last` to `current_round - 1` before submitting the first pulse. - mempool (`validate_unsigned`): drop any round more than `MAX_PULSES_TO_FETCH` ahead of `last`, so leap attempts are rejected before they reach dispatch. The bound equals the OCW's largest catch-up run, so legitimate catch-up is unaffected. spec_version bumped to 426 for the dispatchable behavior change. Refs RaoFoundation/subtensor#2794 --- pallets/drand/src/lib.rs | 33 ++++++++-- pallets/drand/src/tests.rs | 128 +++++++++++++++++++++++++++++++++++++ runtime/src/lib.rs | 2 +- 3 files changed, 158 insertions(+), 5 deletions(-) diff --git a/pallets/drand/src/lib.rs b/pallets/drand/src/lib.rs index f9a281038a..26f66ac31a 100644 --- a/pallets/drand/src/lib.rs +++ b/pallets/drand/src/lib.rs @@ -356,10 +356,26 @@ pub mod pallet { .map_err(|_| Error::::PulseVerificationError)?; if is_verified { - ensure!( - pulse.round > last_stored_round, - Error::::InvalidRoundNumber - ); + if is_first_storage { + // No pulse has ever been stored, so this pulse anchors + // LastStoredRound/OldestStoredRound. Accept any round > 0, + // matching the offchain worker, which seeds LastStoredRound + // to `current_round - 1` before submitting the first pulse. + ensure!( + pulse.round > last_stored_round, + Error::::InvalidRoundNumber + ); + } else { + // Once anchored, a pulse must advance by exactly one round. + // A larger jump would leap `LastStoredRound` past the skipped + // rounds, which could then never be stored (every later pulse + // must be `last + 1`), permanently wedging the reveals and + // metadata timelocks that reference them. See #2794. + ensure!( + pulse.round == last_stored_round.saturating_add(1), + Error::::InvalidRoundNumber + ); + } // Store the pulse Pulses::::insert(pulse.round, pulse.clone()); @@ -674,6 +690,15 @@ impl Pallet { return InvalidTransaction::Stale.into(); } + // Reject rounds that would advance LastStoredRound too far in a single + // step. A leap past unfillable rounds wedges the reveals and timelocks + // that reference them (#2794). `MAX_PULSES_TO_FETCH` is the most rounds + // the offchain worker ever submits in one catch-up run, so anything + // further ahead is not a legitimate pulse and is dropped before dispatch. + if r > last.saturating_add(MAX_PULSES_TO_FETCH) { + return InvalidTransaction::Stale.into(); + } + // Priority favors lower rounds first. let priority = T::UnsignedPriority::get().saturating_add(u64::MAX.saturating_sub(r)); diff --git a/pallets/drand/src/tests.rs b/pallets/drand/src/tests.rs index e07fbb08ca..9f79ba1b6a 100644 --- a/pallets/drand/src/tests.rs +++ b/pallets/drand/src/tests.rs @@ -190,6 +190,97 @@ fn it_rejects_pulses_with_non_incremental_round_numbers() { }); } +#[test] +fn write_pulse_rejects_round_skip() { + // A single pulse must not be allowed to leap LastStoredRound past the rounds + // in between, or those rounds can never be stored and any reveal/timelock that + // references them is wedged (#2794). Here round 1000 is a valid (BLS-verified) + // pulse, but LastStoredRound is seeded to 998 so round 1000 is a skip of two. + new_test_ext().execute_with(|| { + let block_number = 100_000_000; + let alice = sp_keyring::Sr25519Keyring::Alice; + System::set_block_number(block_number); + + let info: BeaconInfoResponse = serde_json::from_str(DRAND_INFO_RESPONSE).unwrap(); + let config_payload = BeaconConfigurationPayload { + block_number, + config: info.clone().try_into_beacon_config().unwrap(), + public: alice.public(), + }; + let signature = None; + assert_ok!(Drand::set_beacon_config( + RuntimeOrigin::root(), + config_payload, + signature + )); + + // Seed an existing baseline so this is not the anchor (first) storage. + LastStoredRound::::put(998); + OldestStoredRound::::put(998); + + let u_p: DrandResponseBody = serde_json::from_str(DRAND_PULSE).unwrap(); + let p: Pulse = u_p.try_into_pulse().unwrap(); + let pulses_payload = PulsesPayload { + pulses: vec![p.clone()], + block_number, + public: alice.public(), + }; + + // Round 1000 is NOT last(998) + 1, so it must be rejected. + assert_noop!( + Drand::write_pulse(RuntimeOrigin::none(), pulses_payload, signature), + Error::::InvalidRoundNumber, + ); + + // State is unchanged: no leap, nothing stored. + assert_eq!(LastStoredRound::::get(), 998); + assert!(Pulses::::get(ROUND_NUMBER).is_none()); + }); +} + +#[test] +fn write_pulse_accepts_consecutive_round() { + // The strict-advance rule must still accept the legitimate next round. + // Round 1000 == last(999) + 1, so it is stored. + new_test_ext().execute_with(|| { + let block_number = 100_000_000; + let alice = sp_keyring::Sr25519Keyring::Alice; + System::set_block_number(block_number); + + let info: BeaconInfoResponse = serde_json::from_str(DRAND_INFO_RESPONSE).unwrap(); + let config_payload = BeaconConfigurationPayload { + block_number, + config: info.clone().try_into_beacon_config().unwrap(), + public: alice.public(), + }; + let signature = None; + assert_ok!(Drand::set_beacon_config( + RuntimeOrigin::root(), + config_payload, + signature + )); + + LastStoredRound::::put(999); + OldestStoredRound::::put(999); + + let u_p: DrandResponseBody = serde_json::from_str(DRAND_PULSE).unwrap(); + let p: Pulse = u_p.try_into_pulse().unwrap(); + let pulses_payload = PulsesPayload { + pulses: vec![p.clone()], + block_number, + public: alice.public(), + }; + + assert_ok!(Drand::write_pulse( + RuntimeOrigin::none(), + pulses_payload, + signature + )); + assert_eq!(LastStoredRound::::get(), ROUND_NUMBER); + assert!(Pulses::::get(ROUND_NUMBER).is_some()); + }); +} + #[test] fn it_blocks_non_root_from_submit_beacon_info() { new_test_ext().execute_with(|| { @@ -301,6 +392,43 @@ fn test_validate_unsigned_write_pulse() { }); } +#[test] +fn validate_unsigned_rejects_round_too_far_ahead() { + // A round that would leap LastStoredRound by more than the offchain worker ever + // submits in one run is not a legitimate catch-up pulse. Drop it at the mempool + // before it can reach dispatch (#2794). + new_test_ext().execute_with(|| { + let block_number = 100_000_000; + let alice = sp_keyring::Sr25519Keyring::Alice; + System::set_block_number(block_number); + + LastStoredRound::::put(100); + + // 151 == last(100) + MAX_PULSES_TO_FETCH(50) + 1, i.e. one beyond the cap. + let pulse = Pulse { + round: 100 + crate::MAX_PULSES_TO_FETCH + 1, + randomness: frame_support::BoundedVec::truncate_from(vec![0u8; 32]), + signature: frame_support::BoundedVec::truncate_from(vec![1u8; 96]), + }; + let pulses_payload = PulsesPayload { + block_number, + pulses: vec![pulse], + public: alice.public(), + }; + let signature = alice.sign(&pulses_payload.encode()); + + let call = Call::write_pulse { + pulses_payload: pulses_payload.clone(), + signature: Some(signature), + }; + + let source = TransactionSource::External; + let validity = Drand::validate_unsigned(source, &call); + + assert_noop!(validity, InvalidTransaction::Stale); + }); +} + #[test] fn test_not_validate_unsigned_write_pulse_with_bad_proof() { new_test_ext().execute_with(|| { diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 5f10e03469..7ced04588e 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -234,7 +234,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // `spec_version`, and `authoring_version` are the same between Wasm and native. // This value is set to 100 to notify Polkadot-JS App (https://polkadot.js.org/apps) to use // the compatible custom types. - spec_version: 425, + spec_version: 426, impl_version: 1, apis: RUNTIME_API_VERSIONS, transaction_version: 1,