Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions pallets/drand/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,26 @@ pub mod pallet {
.map_err(|_| Error::<T>::PulseVerificationError)?;

if is_verified {
ensure!(
pulse.round > last_stored_round,
Error::<T>::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::<T>::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::<T>::InvalidRoundNumber
);
}

// Store the pulse
Pulses::<T>::insert(pulse.round, pulse.clone());
Expand Down Expand Up @@ -674,6 +690,15 @@ impl<T: Config> Pallet<T> {
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));
Expand Down
128 changes: 128 additions & 0 deletions pallets/drand/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Test>::put(998);
OldestStoredRound::<Test>::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::<Test>::InvalidRoundNumber,
);

// State is unchanged: no leap, nothing stored.
assert_eq!(LastStoredRound::<Test>::get(), 998);
assert!(Pulses::<Test>::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::<Test>::put(999);
OldestStoredRound::<Test>::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::<Test>::get(), ROUND_NUMBER);
assert!(Pulses::<Test>::get(ROUND_NUMBER).is_some());
});
}

#[test]
fn it_blocks_non_root_from_submit_beacon_info() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -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::<Test>::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(|| {
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down