diff --git a/Cargo.lock b/Cargo.lock index a6c2bc1fd318..1f171ad756c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14485,6 +14485,7 @@ dependencies = [ "polkadot-parachain-primitives", "polkadot-primitives", "polkadot-primitives-test-helpers", + "rstest", "sp-application-crypto 30.0.0", "sp-core 28.0.0", "sp-keyring", diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs index 942e0c294dd0..1e8212cf6ac2 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -333,4 +333,5 @@ impl pallet_treasury::Config for Runtime { sp_core::ConstU8<1>, ConstU32<1000>, >; + type BlockNumberProvider = crate::System; } diff --git a/polkadot/node/core/candidate-validation/Cargo.toml b/polkadot/node/core/candidate-validation/Cargo.toml index fcacc38cae65..87855dbce415 100644 --- a/polkadot/node/core/candidate-validation/Cargo.toml +++ b/polkadot/node/core/candidate-validation/Cargo.toml @@ -38,3 +38,5 @@ polkadot-node-subsystem-test-helpers = { workspace = true } sp-maybe-compressed-blob = { workspace = true, default-features = true } sp-core = { workspace = true, default-features = true } polkadot-primitives-test-helpers = { workspace = true } +rstest = { workspace = true } +polkadot-primitives = { workspace = true, features = ["test"] } diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index a48669c24825..1e732e2f1f03 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -37,7 +37,7 @@ use polkadot_node_subsystem::{ overseer, FromOrchestra, OverseerSignal, SpawnedSubsystem, SubsystemError, SubsystemResult, SubsystemSender, }; -use polkadot_node_subsystem_util as util; +use polkadot_node_subsystem_util::{self as util, runtime::ClaimQueueSnapshot}; use polkadot_overseer::ActiveLeavesUpdate; use polkadot_parachain_primitives::primitives::ValidationResult as WasmValidationResult; use polkadot_primitives::{ @@ -46,8 +46,9 @@ use polkadot_primitives::{ DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, }, vstaging::{ - CandidateDescriptorV2 as CandidateDescriptor, CandidateEvent, + transpose_claim_queue, CandidateDescriptorV2 as CandidateDescriptor, CandidateEvent, CandidateReceiptV2 as CandidateReceipt, + CommittedCandidateReceiptV2 as CommittedCandidateReceipt, }, AuthorityDiscoveryId, CandidateCommitments, ExecutorParams, Hash, PersistedValidationData, PvfExecKind as RuntimePvfExecKind, PvfPrepKind, SessionIndex, ValidationCode, @@ -148,6 +149,25 @@ impl CandidateValidationSubsystem { } } +// Returns the claim queue at relay parent and logs a warning if it is not available. +async fn claim_queue(relay_parent: Hash, sender: &mut Sender) -> Option +where + Sender: SubsystemSender, +{ + match util::runtime::fetch_claim_queue(sender, relay_parent).await { + Ok(maybe_cq) => maybe_cq, + Err(err) => { + gum::warn!( + target: LOG_TARGET, + ?relay_parent, + ?err, + "Claim queue not available" + ); + None + }, + } +} + fn handle_validation_message( mut sender: S, validation_host: ValidationHost, @@ -167,24 +187,40 @@ where exec_kind, response_sender, .. - } => async move { - let _timer = metrics.time_validate_from_exhaustive(); - let res = validate_candidate_exhaustive( - validation_host, - validation_data, - validation_code, - candidate_receipt, - pov, - executor_params, - exec_kind, - &metrics, - ) - .await; + } => + async move { + let _timer = metrics.time_validate_from_exhaustive(); + let relay_parent = candidate_receipt.descriptor.relay_parent(); + + let maybe_claim_queue = claim_queue(relay_parent, &mut sender).await; + + let maybe_expected_session_index = + match util::request_session_index_for_child(relay_parent, &mut sender) + .await + .await + { + Ok(Ok(expected_session_index)) => Some(expected_session_index), + _ => None, + }; + + let res = validate_candidate_exhaustive( + maybe_expected_session_index, + validation_host, + validation_data, + validation_code, + candidate_receipt, + pov, + executor_params, + exec_kind, + &metrics, + maybe_claim_queue, + ) + .await; - metrics.on_validation_event(&res); - let _ = response_sender.send(res); - } - .boxed(), + metrics.on_validation_event(&res); + let _ = response_sender.send(res); + } + .boxed(), CandidateValidationMessage::PreCheck { relay_parent, validation_code_hash, @@ -637,6 +673,7 @@ where } async fn validate_candidate_exhaustive( + maybe_expected_session_index: Option, mut validation_backend: impl ValidationBackend + Send, persisted_validation_data: PersistedValidationData, validation_code: ValidationCode, @@ -645,11 +682,13 @@ async fn validate_candidate_exhaustive( executor_params: ExecutorParams, exec_kind: PvfExecKind, metrics: &Metrics, + maybe_claim_queue: Option, ) -> Result { let _timer = metrics.time_validate_candidate_exhaustive(); - let validation_code_hash = validation_code.hash(); + let relay_parent = candidate_receipt.descriptor.relay_parent(); let para_id = candidate_receipt.descriptor.para_id(); + gum::debug!( target: LOG_TARGET, ?validation_code_hash, @@ -657,6 +696,27 @@ async fn validate_candidate_exhaustive( "About to validate a candidate.", ); + // We only check the session index for backing. + match (exec_kind, candidate_receipt.descriptor.session_index()) { + (PvfExecKind::Backing | PvfExecKind::BackingSystemParas, Some(session_index)) => { + let Some(expected_session_index) = maybe_expected_session_index else { + let error = "cannot fetch session index from the runtime"; + gum::warn!( + target: LOG_TARGET, + ?relay_parent, + error, + ); + + return Err(ValidationFailed(error.into())) + }; + + if session_index != expected_session_index { + return Ok(ValidationResult::Invalid(InvalidCandidate::InvalidSessionIndex)) + } + }, + (_, _) => {}, + }; + if let Err(e) = perform_basic_checks( &candidate_receipt.descriptor, persisted_validation_data.max_pov_size, @@ -754,15 +814,21 @@ async fn validate_candidate_exhaustive( gum::info!(target: LOG_TARGET, ?para_id, "Invalid candidate (para_head)"); Ok(ValidationResult::Invalid(InvalidCandidate::ParaHeadHashMismatch)) } else { - let outputs = CandidateCommitments { - head_data: res.head_data, - upward_messages: res.upward_messages, - horizontal_messages: res.horizontal_messages, - new_validation_code: res.new_validation_code, - processed_downward_messages: res.processed_downward_messages, - hrmp_watermark: res.hrmp_watermark, + let committed_candidate_receipt = CommittedCandidateReceipt { + descriptor: candidate_receipt.descriptor.clone(), + commitments: CandidateCommitments { + head_data: res.head_data, + upward_messages: res.upward_messages, + horizontal_messages: res.horizontal_messages, + new_validation_code: res.new_validation_code, + processed_downward_messages: res.processed_downward_messages, + hrmp_watermark: res.hrmp_watermark, + }, }; - if candidate_receipt.commitments_hash != outputs.hash() { + + if candidate_receipt.commitments_hash != + committed_candidate_receipt.commitments.hash() + { gum::info!( target: LOG_TARGET, ?para_id, @@ -773,7 +839,48 @@ async fn validate_candidate_exhaustive( // invalid. Ok(ValidationResult::Invalid(InvalidCandidate::CommitmentsHashMismatch)) } else { - Ok(ValidationResult::Valid(outputs, (*persisted_validation_data).clone())) + let core_index = candidate_receipt.descriptor.core_index(); + + match (core_index, exec_kind) { + // Core selectors are optional for V2 descriptors, but we still check the + // descriptor core index. + ( + Some(_core_index), + PvfExecKind::Backing | PvfExecKind::BackingSystemParas, + ) => { + let Some(claim_queue) = maybe_claim_queue else { + let error = "cannot fetch the claim queue from the runtime"; + gum::warn!( + target: LOG_TARGET, + ?relay_parent, + error + ); + + return Err(ValidationFailed(error.into())) + }; + + if let Err(err) = committed_candidate_receipt + .check_core_index(&transpose_claim_queue(claim_queue.0)) + { + gum::warn!( + target: LOG_TARGET, + ?err, + candidate_hash = ?candidate_receipt.hash(), + "Candidate core index is invalid", + ); + return Ok(ValidationResult::Invalid( + InvalidCandidate::InvalidCoreIndex, + )) + } + }, + // No checks for approvals and disputes + (_, _) => {}, + } + + Ok(ValidationResult::Valid( + committed_candidate_receipt.commitments, + (*persisted_validation_data).clone(), + )) } }, } @@ -1003,6 +1110,7 @@ fn perform_basic_checks( return Err(InvalidCandidate::CodeHashMismatch) } + // No-op for `v2` receipts. if let Err(()) = candidate.check_collator_signature() { return Err(InvalidCandidate::BadSignature) } diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs index 997a347631a0..391247858ed6 100644 --- a/polkadot/node/core/candidate-validation/src/tests.rs +++ b/polkadot/node/core/candidate-validation/src/tests.rs @@ -14,7 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::{ + collections::BTreeMap, + sync::atomic::{AtomicUsize, Ordering}, +}; use super::*; use crate::PvfExecKind; @@ -26,12 +29,18 @@ use polkadot_node_subsystem::messages::AllMessages; use polkadot_node_subsystem_util::reexports::SubsystemContext; use polkadot_overseer::ActivatedLeaf; use polkadot_primitives::{ - vstaging::CandidateDescriptorV2, CandidateDescriptor, CoreIndex, GroupIndex, HeadData, - Id as ParaId, OccupiedCoreAssumption, SessionInfo, UpwardMessage, ValidatorId, + vstaging::{ + CandidateDescriptorV2, ClaimQueueOffset, CoreSelector, MutateDescriptorV2, UMPSignal, + UMP_SEPARATOR, + }, + CandidateDescriptor, CoreIndex, GroupIndex, HeadData, Id as ParaId, OccupiedCoreAssumption, + SessionInfo, UpwardMessage, ValidatorId, }; use polkadot_primitives_test_helpers::{ dummy_collator, dummy_collator_signature, dummy_hash, make_valid_candidate_descriptor, + make_valid_candidate_descriptor_v2, }; +use rstest::rstest; use sp_core::{sr25519::Public, testing::TaskExecutor}; use sp_keyring::Sr25519Keyring; use sp_keystore::{testing::MemoryKeystore, Keystore}; @@ -467,25 +476,24 @@ impl ValidationBackend for MockValidateCandidateBackend { } #[test] -fn candidate_validation_ok_is_ok() { +fn session_index_checked_only_in_backing() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; let head_data = HeadData(vec![1, 1, 1]); let validation_code = ValidationCode(vec![2; 16]); - let descriptor = make_valid_candidate_descriptor( + let descriptor = make_valid_candidate_descriptor_v2( ParaId::from(1_u32), dummy_hash(), - validation_data.hash(), + CoreIndex(0), + 100, + dummy_hash(), pov.hash(), validation_code.hash(), head_data.hash(), dummy_hash(), - Sr25519Keyring::Alice, - ) - .into(); - + ); let check = perform_basic_checks( &descriptor, validation_data.max_pov_size, @@ -514,15 +522,59 @@ fn candidate_validation_ok_is_ok() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; + // The session index is invalid + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), + Default::default(), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::InvalidSessionIndex)); + + // Approval doesn't fail since the check is ommited. let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Approval, + &Default::default(), + Default::default(), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, Vec::::new()); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); + + // Approval doesn't fail since the check is ommited. + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data.clone(), validation_code, candidate_receipt, Arc::new(pov), ExecutorParams::default(), - PvfExecKind::Backing, + PvfExecKind::Dispute, &Default::default(), + Default::default(), )) .unwrap(); @@ -536,6 +588,323 @@ fn candidate_validation_ok_is_ok() { }); } +#[rstest] +#[case(true)] +#[case(false)] +fn candidate_validation_ok_is_ok(#[case] v2_descriptor: bool) { + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; + + let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let head_data = HeadData(vec![1, 1, 1]); + let validation_code = ValidationCode(vec![2; 16]); + + let descriptor = if v2_descriptor { + make_valid_candidate_descriptor_v2( + ParaId::from(1_u32), + dummy_hash(), + CoreIndex(1), + 1, + dummy_hash(), + pov.hash(), + validation_code.hash(), + head_data.hash(), + dummy_hash(), + ) + } else { + make_valid_candidate_descriptor( + ParaId::from(1_u32), + dummy_hash(), + validation_data.hash(), + pov.hash(), + validation_code.hash(), + head_data.hash(), + dummy_hash(), + Sr25519Keyring::Alice, + ) + .into() + }; + + let check = perform_basic_checks( + &descriptor, + validation_data.max_pov_size, + &pov, + &validation_code.hash(), + ); + assert!(check.is_ok()); + + let mut validation_result = WasmValidationResult { + head_data, + new_validation_code: Some(vec![2, 2, 2].into()), + upward_messages: Default::default(), + horizontal_messages: Default::default(), + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + + if v2_descriptor { + validation_result.upward_messages.force_push(UMP_SEPARATOR); + validation_result + .upward_messages + .force_push(UMPSignal::SelectCore(CoreSelector(0), ClaimQueueOffset(1)).encode()); + } + + let commitments = CandidateCommitments { + head_data: validation_result.head_data.clone(), + upward_messages: validation_result.upward_messages.clone(), + horizontal_messages: validation_result.horizontal_messages.clone(), + new_validation_code: validation_result.new_validation_code.clone(), + processed_downward_messages: validation_result.processed_downward_messages, + hrmp_watermark: validation_result.hrmp_watermark, + }; + + let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; + let mut cq = BTreeMap::new(); + let _ = cq.insert(CoreIndex(0), vec![1.into(), 2.into()].into()); + let _ = cq.insert(CoreIndex(1), vec![1.into(), 1.into()].into()); + + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), + validation_data.clone(), + validation_code, + candidate_receipt, + Arc::new(pov), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), + Some(ClaimQueueSnapshot(cq)), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, commitments.upward_messages); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); +} + +#[test] +fn invalid_session_or_core_index() { + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; + + let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let head_data = HeadData(vec![1, 1, 1]); + let validation_code = ValidationCode(vec![2; 16]); + + let descriptor = make_valid_candidate_descriptor_v2( + ParaId::from(1_u32), + dummy_hash(), + CoreIndex(1), + 100, + dummy_hash(), + pov.hash(), + validation_code.hash(), + head_data.hash(), + dummy_hash(), + ); + + let check = perform_basic_checks( + &descriptor, + validation_data.max_pov_size, + &pov, + &validation_code.hash(), + ); + assert!(check.is_ok()); + + let mut validation_result = WasmValidationResult { + head_data, + new_validation_code: Some(vec![2, 2, 2].into()), + upward_messages: Default::default(), + horizontal_messages: Default::default(), + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + + validation_result.upward_messages.force_push(UMP_SEPARATOR); + validation_result + .upward_messages + .force_push(UMPSignal::SelectCore(CoreSelector(1), ClaimQueueOffset(0)).encode()); + + let commitments = CandidateCommitments { + head_data: validation_result.head_data.clone(), + upward_messages: validation_result.upward_messages.clone(), + horizontal_messages: validation_result.horizontal_messages.clone(), + new_validation_code: validation_result.new_validation_code.clone(), + processed_downward_messages: validation_result.processed_downward_messages, + hrmp_watermark: validation_result.hrmp_watermark, + }; + + let mut candidate_receipt = + CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; + + let err = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), + Default::default(), + )) + .unwrap(); + + assert_matches!(err, ValidationResult::Invalid(InvalidCandidate::InvalidSessionIndex)); + + let err = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::BackingSystemParas, + &Default::default(), + Default::default(), + )) + .unwrap(); + + assert_matches!(err, ValidationResult::Invalid(InvalidCandidate::InvalidSessionIndex)); + + candidate_receipt.descriptor.set_session_index(1); + + let result = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), + Some(Default::default()), + )) + .unwrap(); + assert_matches!(result, ValidationResult::Invalid(InvalidCandidate::InvalidCoreIndex)); + + let result = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::BackingSystemParas, + &Default::default(), + Some(Default::default()), + )) + .unwrap(); + assert_matches!(result, ValidationResult::Invalid(InvalidCandidate::InvalidCoreIndex)); + + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Approval, + &Default::default(), + Default::default(), + )) + .unwrap(); + + // Validation doesn't fail for approvals, core/session index is not checked. + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, commitments.upward_messages); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); + + // Dispute check passes because we don't check core or session index + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Dispute, + &Default::default(), + Default::default(), + )) + .unwrap(); + + // Validation doesn't fail for approvals, core/session index is not checked. + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, commitments.upward_messages); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); + + // Populate claim queue. + let mut cq = BTreeMap::new(); + let _ = cq.insert(CoreIndex(0), vec![1.into(), 2.into()].into()); + let _ = cq.insert(CoreIndex(1), vec![1.into(), 2.into()].into()); + + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::Backing, + &Default::default(), + Some(ClaimQueueSnapshot(cq.clone())), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, commitments.upward_messages); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); + + let v = executor::block_on(validate_candidate_exhaustive( + Some(1), + MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())), + validation_data.clone(), + validation_code.clone(), + candidate_receipt.clone(), + Arc::new(pov.clone()), + ExecutorParams::default(), + PvfExecKind::BackingSystemParas, + &Default::default(), + Some(ClaimQueueSnapshot(cq)), + )) + .unwrap(); + + assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => { + assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1])); + assert_eq!(outputs.upward_messages, commitments.upward_messages); + assert_eq!(outputs.horizontal_messages, Vec::new()); + assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into())); + assert_eq!(outputs.hrmp_watermark, 0); + assert_eq!(used_validation_data, validation_data); + }); +} + #[test] fn candidate_validation_bad_return_is_invalid() { let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; @@ -566,6 +935,7 @@ fn candidate_validation_bad_return_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result(Err(ValidationError::Invalid( WasmInvalidCandidate::HardTimeout, ))), @@ -576,6 +946,7 @@ fn candidate_validation_bad_return_is_invalid() { ExecutorParams::default(), PvfExecKind::Backing, &Default::default(), + Default::default(), )) .unwrap(); @@ -647,6 +1018,7 @@ fn candidate_validation_one_ambiguous_error_is_valid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)), Ok(validation_result), @@ -658,6 +1030,7 @@ fn candidate_validation_one_ambiguous_error_is_valid() { ExecutorParams::default(), PvfExecKind::Approval, &Default::default(), + Default::default(), )) .unwrap(); @@ -688,6 +1061,7 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result_list(vec![ Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)), Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)), @@ -699,6 +1073,7 @@ fn candidate_validation_multiple_ambiguous_errors_is_invalid() { ExecutorParams::default(), PvfExecKind::Approval, &Default::default(), + Default::default(), )) .unwrap(); @@ -806,6 +1181,7 @@ fn candidate_validation_retry_on_error_helper( let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; return executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result_list(mock_errors), validation_data, validation_code, @@ -814,6 +1190,7 @@ fn candidate_validation_retry_on_error_helper( ExecutorParams::default(), exec_kind, &Default::default(), + Default::default(), )) } @@ -847,6 +1224,7 @@ fn candidate_validation_timeout_is_internal_error() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result(Err(ValidationError::Invalid( WasmInvalidCandidate::HardTimeout, ))), @@ -857,6 +1235,7 @@ fn candidate_validation_timeout_is_internal_error() { ExecutorParams::default(), PvfExecKind::Backing, &Default::default(), + Default::default(), )); assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::Timeout))); @@ -896,6 +1275,7 @@ fn candidate_validation_commitment_hash_mismatch_is_invalid() { }; let result = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data, validation_code, @@ -904,6 +1284,7 @@ fn candidate_validation_commitment_hash_mismatch_is_invalid() { ExecutorParams::default(), PvfExecKind::Backing, &Default::default(), + Default::default(), )) .unwrap(); @@ -947,6 +1328,7 @@ fn candidate_validation_code_mismatch_is_invalid() { >(pool.clone()); let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result(Err(ValidationError::Invalid( WasmInvalidCandidate::HardTimeout, ))), @@ -957,6 +1339,7 @@ fn candidate_validation_code_mismatch_is_invalid() { ExecutorParams::default(), PvfExecKind::Backing, &Default::default(), + Default::default(), )) .unwrap(); @@ -1007,6 +1390,7 @@ fn compressed_code_works() { let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; let v = executor::block_on(validate_candidate_exhaustive( + Some(1), MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data, validation_code, @@ -1015,6 +1399,7 @@ fn compressed_code_works() { ExecutorParams::default(), PvfExecKind::Backing, &Default::default(), + Default::default(), )); assert_matches!(v, Ok(ValidationResult::Valid(_, _))); diff --git a/polkadot/node/primitives/src/lib.rs b/polkadot/node/primitives/src/lib.rs index e2e7aa92b11c..6985e86098b0 100644 --- a/polkadot/node/primitives/src/lib.rs +++ b/polkadot/node/primitives/src/lib.rs @@ -348,6 +348,10 @@ pub enum InvalidCandidate { CodeHashMismatch, /// Validation has generated different candidate commitments. CommitmentsHashMismatch, + /// The candidate receipt contains an invalid session index. + InvalidSessionIndex, + /// The candidate receipt contains an invalid core index. + InvalidCoreIndex, } /// Result of the validation of the candidate. diff --git a/polkadot/primitives/src/vstaging/mod.rs b/polkadot/primitives/src/vstaging/mod.rs index 265fcd899d74..21aab41902be 100644 --- a/polkadot/primitives/src/vstaging/mod.rs +++ b/polkadot/primitives/src/vstaging/mod.rs @@ -208,6 +208,10 @@ pub trait MutateDescriptorV2 { fn set_erasure_root(&mut self, erasure_root: Hash); /// Set the para head of the descriptor. fn set_para_head(&mut self, para_head: Hash); + /// Set the core index of the descriptor. + fn set_core_index(&mut self, core_index: CoreIndex); + /// Set the session index of the descriptor. + fn set_session_index(&mut self, session_index: SessionIndex); } #[cfg(feature = "test")] @@ -228,6 +232,14 @@ impl MutateDescriptorV2 for CandidateDescriptorV2 { self.version = version; } + fn set_core_index(&mut self, core_index: CoreIndex) { + self.core_index = core_index.0 as u16; + } + + fn set_session_index(&mut self, session_index: SessionIndex) { + self.session_index = session_index; + } + fn set_persisted_validation_data_hash(&mut self, persisted_validation_data_hash: Hash) { self.persisted_validation_data_hash = persisted_validation_data_hash; } diff --git a/polkadot/runtime/common/src/impls.rs b/polkadot/runtime/common/src/impls.rs index b6a93cf53685..2f79d223d3c0 100644 --- a/polkadot/runtime/common/src/impls.rs +++ b/polkadot/runtime/common/src/impls.rs @@ -366,6 +366,7 @@ mod tests { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<0>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 5bcde612cf1b..e02a28353d29 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -543,6 +543,7 @@ impl pallet_treasury::Config for Runtime { AssetRate, >; type PayoutPeriod = PayoutSpendPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = polkadot_runtime_common::impls::benchmarks::TreasuryArguments; } diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 970ef5318994..251d92b03bbb 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -823,6 +823,7 @@ impl pallet_treasury::Config for Runtime { AssetRate, >; type PayoutPeriod = PayoutSpendPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = polkadot_runtime_common::impls::benchmarks::TreasuryArguments; } diff --git a/prdoc/pr_3970.prdoc b/prdoc/pr_3970.prdoc new file mode 100644 index 000000000000..5c20e7444782 --- /dev/null +++ b/prdoc/pr_3970.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Update Treasury to Support Block Number Provider + +doc: + - audience: Runtime Dev + description: | + The goal of this PR is to have the treasury pallet work on a parachain which does not produce blocks on a regular schedule, thus can use the relay chain as a block provider. Because blocks are not produced regularly, we cannot make the assumption that block number increases monotonically, and thus have new logic to handle multiple spend periods passing between blocks. To migrate existing treasury implementations, simply add `type BlockNumberProvider = System` to have the same behavior as before. + +crates: +- name: pallet-treasury + bump: major +- name: pallet-bounties + bump: minor +- name: pallet-child-bounties + bump: minor diff --git a/prdoc/pr_5847.prdoc b/prdoc/pr_5847.prdoc new file mode 100644 index 000000000000..fdbf6423da60 --- /dev/null +++ b/prdoc/pr_5847.prdoc @@ -0,0 +1,19 @@ +title: '`candidate-validation`: RFC103 implementation' +doc: +- audience: Node Dev + description: | + Introduces support for new v2 descriptor `core_index` and `session_index` fields. + The subsystem will check the values of the new fields only during backing validations. +crates: +- name: polkadot-node-primitives + bump: major +- name: polkadot-primitives + bump: major +- name: cumulus-relay-chain-inprocess-interface + bump: minor +- name: cumulus-relay-chain-interface + bump: minor +- name: cumulus-client-consensus-aura + bump: minor +- name: polkadot-node-core-candidate-validation + bump: major diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 12e8dc3e5077..7712d8ba9540 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1266,6 +1266,7 @@ impl pallet_treasury::Config for Runtime { type Paymaster = PayAssetFromAccount; type BalanceConverter = AssetRate; type PayoutPeriod = SpendPayoutPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs index de93ba5c4ce7..6fa60e6938b6 100644 --- a/substrate/frame/bounties/src/benchmarking.rs +++ b/substrate/frame/bounties/src/benchmarking.rs @@ -26,13 +26,17 @@ use frame_benchmarking::v1::{ account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError, }; use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin}; -use sp_runtime::traits::Bounded; +use sp_runtime::traits::{BlockNumberProvider, Bounded}; use crate::Pallet as Bounties; use pallet_treasury::Pallet as Treasury; const SEED: u32 = 0; +fn set_block_number, I: 'static>(n: BlockNumberFor) { + >::BlockNumberProvider::set_block_number(n); +} + // Create bounties that are approved for use in `on_initialize`. fn create_approved_bounties, I: 'static>(n: u32) -> Result<(), BenchmarkError> { for i in 0..n { @@ -78,7 +82,8 @@ fn create_bounty, I: 'static>( let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); Bounties::::propose_curator(approve_origin, bounty_id, curator_lookup.clone(), fee)?; Bounties::::accept_curator(RawOrigin::Signed(curator).into(), bounty_id)?; Ok((curator_lookup, bounty_id)) @@ -116,16 +121,17 @@ benchmarks_instance_pallet! { let bounty_id = BountyCount::::get() - 1; let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); }: _(approve_origin, bounty_id, curator_lookup, fee) // Worst case when curator is inactive and any sender unassigns the curator. unassign_curator { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; - frame_system::Pallet::::set_block_number(T::BountyUpdatePeriod::get() + 2u32.into()); + set_block_number::(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 2u32.into()); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), bounty_id) @@ -137,14 +143,15 @@ benchmarks_instance_pallet! { let bounty_id = BountyCount::::get() - 1; let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); Bounties::::propose_curator(approve_origin, bounty_id, curator_lookup, fee)?; }: _(RawOrigin::Signed(curator), bounty_id) award_bounty { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; @@ -155,7 +162,7 @@ benchmarks_instance_pallet! { claim_bounty { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; @@ -164,7 +171,7 @@ benchmarks_instance_pallet! { let beneficiary = T::Lookup::unlookup(beneficiary_account.clone()); Bounties::::award_bounty(RawOrigin::Signed(curator.clone()).into(), bounty_id, beneficiary)?; - frame_system::Pallet::::set_block_number(T::BountyDepositPayoutDelay::get() + 1u32.into()); + set_block_number::(T::SpendPeriod::get() + T::BountyDepositPayoutDelay::get() + 1u32.into()); ensure!(T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary already has balance"); }: _(RawOrigin::Signed(curator), bounty_id) @@ -184,7 +191,7 @@ benchmarks_instance_pallet! { close_bounty_active { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let approve_origin = T::RejectOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; @@ -196,7 +203,7 @@ benchmarks_instance_pallet! { extend_bounty_expiry { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index e30d6fa2d143..9d5931f4a608 100644 --- a/substrate/frame/bounties/src/lib.rs +++ b/substrate/frame/bounties/src/lib.rs @@ -96,7 +96,7 @@ use frame_support::traits::{ }; use sp_runtime::{ - traits::{AccountIdConversion, BadOrigin, Saturating, StaticLookup, Zero}, + traits::{AccountIdConversion, BadOrigin, BlockNumberProvider, Saturating, StaticLookup, Zero}, DispatchResult, Permill, RuntimeDebug, }; @@ -488,7 +488,7 @@ pub mod pallet { // If the sender is not the curator, and the curator is inactive, // slash the curator. if sender != *curator { - let block_number = frame_system::Pallet::::block_number(); + let block_number = Self::treasury_block_number(); if *update_due < block_number { slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... @@ -552,8 +552,8 @@ pub mod pallet { T::Currency::reserve(curator, deposit)?; bounty.curator_deposit = deposit; - let update_due = frame_system::Pallet::::block_number() + - T::BountyUpdatePeriod::get(); + let update_due = + Self::treasury_block_number() + T::BountyUpdatePeriod::get(); bounty.status = BountyStatus::Active { curator: curator.clone(), update_due }; @@ -607,8 +607,7 @@ pub mod pallet { bounty.status = BountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: frame_system::Pallet::::block_number() + - T::BountyDepositPayoutDelay::get(), + unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -639,10 +638,7 @@ pub mod pallet { if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } = bounty.status { - ensure!( - frame_system::Pallet::::block_number() >= unlock_at, - Error::::Premature - ); + ensure!(Self::treasury_block_number() >= unlock_at, Error::::Premature); let bounty_account = Self::bounty_account_id(bounty_id); let balance = T::Currency::free_balance(&bounty_account); let fee = bounty.fee.min(balance); // just to be safe @@ -795,7 +791,7 @@ pub mod pallet { match bounty.status { BountyStatus::Active { ref curator, ref mut update_due } => { ensure!(*curator == signer, Error::::RequireCurator); - *update_due = (frame_system::Pallet::::block_number() + + *update_due = (Self::treasury_block_number() + T::BountyUpdatePeriod::get()) .max(*update_due); }, @@ -860,6 +856,14 @@ impl, I: 'static> Pallet { } impl, I: 'static> Pallet { + /// Get the block number used in the treasury pallet. + /// + /// It may be configured to use the relay chain block number on a parachain. + pub fn treasury_block_number() -> BlockNumberFor { + >::BlockNumberProvider::current_block_number() + } + + /// Calculate the deposit required for a curator. pub fn calculate_curator_deposit(fee: &BalanceOf) -> BalanceOf { let mut deposit = T::CuratorDepositMultiplier::get() * *fee; diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs index c152391d807a..37bcadddae5b 100644 --- a/substrate/frame/bounties/src/tests.rs +++ b/substrate/frame/bounties/src/tests.rs @@ -40,6 +40,12 @@ use super::Event as BountiesEvent; type Block = frame_system::mocking::MockBlock; +// This function directly jumps to a block number, and calls `on_initialize`. +fn go_to_block(n: u64) { + ::BlockNumberProvider::set_block_number(n); + >::on_initialize(n); +} + frame_support::construct_runtime!( pub enum Test { @@ -98,6 +104,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -120,6 +127,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -186,7 +194,9 @@ impl ExtBuilder { .build_storage() .unwrap() .into(); - ext.execute_with(|| System::set_block_number(1)); + ext.execute_with(|| { + ::BlockNumberProvider::set_block_number(1) + }); ext } @@ -232,7 +242,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 100, 3) }); - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -245,7 +255,7 @@ fn unused_pot_should_diminish() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(pallet_balances::TotalIssuance::::get(), init_total_issuance + 100); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 50); assert_eq!(pallet_balances::TotalIssuance::::get(), init_total_issuance + 50); }); @@ -259,7 +269,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 100, 3) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -273,11 +283,11 @@ fn pot_underflow_should_not_diminish() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 150, 3) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!(Balances::deposit_into_existing(&Treasury::account_id(), 100)); - >::on_initialize(4); + go_to_block(4); assert_eq!(Balances::free_balance(3), 150); // Fund has been spent assert_eq!(Treasury::pot(), 25); // Pot has finally changed }); @@ -294,12 +304,12 @@ fn treasury_account_doesnt_get_deleted() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), treasury_balance, 3) }); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), Treasury::pot(), 3) }); - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot is emptied assert_eq!(Balances::free_balance(Treasury::account_id()), 1); // but the account is still there }); @@ -322,7 +332,8 @@ fn inexistent_account_works() { assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 99, 3) }); assert_ok!({ Treasury::spend_local(RuntimeOrigin::root(), 1, 3) }); - >::on_initialize(2); + go_to_block(2); + assert_eq!(Treasury::pot(), 0); // Pot hasn't changed assert_eq!(Balances::free_balance(3), 0); // Balance of `3` hasn't changed @@ -330,7 +341,7 @@ fn inexistent_account_works() { assert_eq!(Treasury::pot(), 99); // Pot now contains funds assert_eq!(Balances::free_balance(Treasury::account_id()), 100); // Account does exist - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot has changed assert_eq!(Balances::free_balance(3), 99); // Balance of `3` has changed @@ -340,8 +351,6 @@ fn inexistent_account_works() { #[test] fn propose_bounty_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -377,8 +386,6 @@ fn propose_bounty_works() { #[test] fn propose_bounty_validation_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -406,7 +413,6 @@ fn propose_bounty_validation_works() { #[test] fn close_bounty_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_noop!(Bounties::close_bounty(RuntimeOrigin::root(), 0), Error::::InvalidIndex); @@ -431,7 +437,6 @@ fn close_bounty_works() { #[test] fn approve_bounty_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_noop!( Bounties::approve_bounty(RuntimeOrigin::root(), 0), @@ -466,7 +471,7 @@ fn approve_bounty_works() { assert_eq!(Balances::reserved_balance(0), deposit); assert_eq!(Balances::free_balance(0), 100 - deposit); - >::on_initialize(2); + go_to_block(2); // return deposit assert_eq!(Balances::reserved_balance(0), 0); @@ -492,7 +497,6 @@ fn approve_bounty_works() { #[test] fn assign_curator_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_noop!( @@ -504,8 +508,7 @@ fn assign_curator_works() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_noop!( Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 50), @@ -562,14 +565,12 @@ fn assign_curator_works() { #[test] fn unassign_curator_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; @@ -615,15 +616,13 @@ fn unassign_curator_works() { #[test] fn award_and_claim_bounty_works() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 10); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee)); @@ -653,8 +652,7 @@ fn award_and_claim_bounty_works() { assert_noop!(Bounties::claim_bounty(RuntimeOrigin::signed(1), 0), Error::::Premature); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); assert_ok!(Balances::transfer_allow_death( RuntimeOrigin::signed(0), @@ -682,23 +680,20 @@ fn award_and_claim_bounty_works() { #[test] fn claim_handles_high_fee() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 30); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 49)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(4), 0, 3)); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); // make fee > balance let res = Balances::slash(&Bounties::bounty_account_id(0), 10); @@ -723,16 +718,13 @@ fn claim_handles_high_fee() { #[test] fn cancel_and_refund() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Balances::transfer_allow_death( RuntimeOrigin::signed(0), @@ -766,14 +758,12 @@ fn cancel_and_refund() { #[test] fn award_and_cancel() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 0, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(0), 0)); @@ -809,14 +799,12 @@ fn award_and_cancel() { #[test] fn expire_and_unassign() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 1, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(1), 0)); @@ -824,16 +812,14 @@ fn expire_and_unassign() { assert_eq!(Balances::free_balance(1), 93); assert_eq!(Balances::reserved_balance(1), 5); - System::set_block_number(22); - >::on_initialize(22); + go_to_block(22); assert_noop!( Bounties::unassign_curator(RuntimeOrigin::signed(0), 0), Error::::Premature ); - System::set_block_number(23); - >::on_initialize(23); + go_to_block(23); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(0), 0)); @@ -857,7 +843,6 @@ fn expire_and_unassign() { #[test] fn extend_expiry() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 10); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); @@ -869,8 +854,7 @@ fn extend_expiry() { Error::::UnexpectedStatus ); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -878,8 +862,7 @@ fn extend_expiry() { assert_eq!(Balances::free_balance(4), 5); assert_eq!(Balances::reserved_balance(4), 5); - System::set_block_number(10); - >::on_initialize(10); + go_to_block(10); assert_noop!( Bounties::extend_bounty_expiry(RuntimeOrigin::signed(0), 0, Vec::new()), @@ -913,8 +896,7 @@ fn extend_expiry() { } ); - System::set_block_number(25); - >::on_initialize(25); + go_to_block(25); assert_noop!( Bounties::unassign_curator(RuntimeOrigin::signed(0), 0), @@ -993,13 +975,11 @@ fn genesis_funding_works() { #[test] fn unassign_curator_self() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 1, 10)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(1), 0)); @@ -1007,8 +987,7 @@ fn unassign_curator_self() { assert_eq!(Balances::free_balance(1), 93); assert_eq!(Balances::reserved_balance(1), 5); - System::set_block_number(8); - >::on_initialize(8); + go_to_block(8); assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(1), 0)); @@ -1040,7 +1019,6 @@ fn accept_curator_handles_different_deposit_calculations() { let value = 88; let fee = 42; - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&user, 100); // Allow for a larger spend limit: @@ -1048,8 +1026,7 @@ fn accept_curator_handles_different_deposit_calculations() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), value, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), bounty_index)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(user), bounty_index)); @@ -1070,8 +1047,7 @@ fn accept_curator_handles_different_deposit_calculations() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), value, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), bounty_index)); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(user), bounty_index)); @@ -1096,8 +1072,7 @@ fn accept_curator_handles_different_deposit_calculations() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), value, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), bounty_index)); - System::set_block_number(6); - >::on_initialize(6); + go_to_block(6); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), bounty_index, user, fee)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(user), bounty_index)); @@ -1114,7 +1089,6 @@ fn approve_bounty_works_second_instance() { // Set burn to 0 to make tracking funds easier. Burn::set(Permill::from_percent(0)); - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&Treasury1::account_id(), 201); assert_eq!(Balances::free_balance(&Treasury::account_id()), 101); @@ -1122,7 +1096,7 @@ fn approve_bounty_works_second_instance() { assert_ok!(Bounties1::propose_bounty(RuntimeOrigin::signed(0), 10, b"12345".to_vec())); assert_ok!(Bounties1::approve_bounty(RuntimeOrigin::root(), 0)); - >::on_initialize(2); + go_to_block(2); >::on_initialize(2); // Bounties 1 is funded... but from where? @@ -1137,8 +1111,6 @@ fn approve_bounty_works_second_instance() { #[test] fn approve_bounty_insufficient_spend_limit_errors() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -1155,8 +1127,6 @@ fn approve_bounty_insufficient_spend_limit_errors() { #[test] fn approve_bounty_instance1_insufficient_spend_limit_errors() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); - Balances::make_free_balance_be(&Treasury1::account_id(), 101); assert_eq!(Treasury1::pot(), 100); @@ -1173,7 +1143,6 @@ fn approve_bounty_instance1_insufficient_spend_limit_errors() { #[test] fn propose_curator_insufficient_spend_limit_errors() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); // Temporarily set a larger spend limit; @@ -1181,8 +1150,7 @@ fn propose_curator_insufficient_spend_limit_errors() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 51, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); SpendLimit::set(50); // 51 will not work since the limit is 50. @@ -1196,7 +1164,6 @@ fn propose_curator_insufficient_spend_limit_errors() { #[test] fn propose_curator_instance1_insufficient_spend_limit_errors() { ExtBuilder::default().build_and_execute(|| { - System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); // Temporarily set a larger spend limit; @@ -1204,7 +1171,6 @@ fn propose_curator_instance1_insufficient_spend_limit_errors() { assert_ok!(Bounties1::propose_bounty(RuntimeOrigin::signed(0), 11, b"12345".to_vec())); assert_ok!(Bounties1::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); >::on_initialize(2); SpendLimit1::set(10); diff --git a/substrate/frame/child-bounties/src/benchmarking.rs b/substrate/frame/child-bounties/src/benchmarking.rs index b1f6370f3340..68e99e21a456 100644 --- a/substrate/frame/child-bounties/src/benchmarking.rs +++ b/substrate/frame/child-bounties/src/benchmarking.rs @@ -25,6 +25,7 @@ use alloc::{vec, vec::Vec}; use frame_benchmarking::v1::{account, benchmarks, whitelisted_caller, BenchmarkError}; use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin}; +use sp_runtime::traits::BlockNumberProvider; use crate::Pallet as ChildBounties; use pallet_bounties::Pallet as Bounties; @@ -56,6 +57,10 @@ struct BenchmarkChildBounty { reason: Vec, } +fn set_block_number(n: BlockNumberFor) { + ::BlockNumberProvider::set_block_number(n); +} + fn setup_bounty( user: u32, description: u32, @@ -116,7 +121,8 @@ fn activate_bounty( let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin, child_bounty_setup.bounty_id)?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + set_block_number::(T::SpendPeriod::get()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); Bounties::::propose_curator( RawOrigin::Root.into(), child_bounty_setup.bounty_id, @@ -231,8 +237,8 @@ benchmarks! { unassign_curator { setup_pot_account::(); let bounty_setup = activate_child_bounty::(0, T::MaximumReasonLength::get())?; - Treasury::::on_initialize(BlockNumberFor::::zero()); - frame_system::Pallet::::set_block_number(T::BountyUpdatePeriod::get() + 1u32.into()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); + set_block_number::(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 1u32.into()); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), bounty_setup.bounty_id, bounty_setup.child_bounty_id) @@ -268,7 +274,7 @@ benchmarks! { let beneficiary_account: T::AccountId = account("beneficiary", 0, SEED); let beneficiary = T::Lookup::unlookup(beneficiary_account.clone()); - frame_system::Pallet::::set_block_number(T::BountyDepositPayoutDelay::get()); + set_block_number::(T::SpendPeriod::get() + T::BountyDepositPayoutDelay::get()); ensure!(T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary already has balance."); @@ -305,7 +311,7 @@ benchmarks! { close_child_bounty_active { setup_pot_account::(); let bounty_setup = activate_child_bounty::(0, T::MaximumReasonLength::get())?; - Treasury::::on_initialize(BlockNumberFor::::zero()); + Treasury::::on_initialize(frame_system::Pallet::::block_number()); }: close_child_bounty(RawOrigin::Root, bounty_setup.bounty_id, bounty_setup.child_bounty_id) verify { assert_last_event::(Event::Canceled { diff --git a/substrate/frame/child-bounties/src/lib.rs b/substrate/frame/child-bounties/src/lib.rs index 660a30ca5d26..1e970b6ae67c 100644 --- a/substrate/frame/child-bounties/src/lib.rs +++ b/substrate/frame/child-bounties/src/lib.rs @@ -67,7 +67,10 @@ use frame_support::traits::{ }; use sp_runtime::{ - traits::{AccountIdConversion, BadOrigin, CheckedSub, Saturating, StaticLookup, Zero}, + traits::{ + AccountIdConversion, BadOrigin, BlockNumberProvider, CheckedSub, Saturating, StaticLookup, + Zero, + }, DispatchResult, RuntimeDebug, }; @@ -523,7 +526,7 @@ pub mod pallet { let (parent_curator, update_due) = Self::ensure_bounty_active(parent_bounty_id)?; if sender == parent_curator || - update_due < frame_system::Pallet::::block_number() + update_due < Self::treasury_block_number() { // Slash the child-bounty curator if // + the call is made by the parent bounty curator. @@ -602,7 +605,7 @@ pub mod pallet { child_bounty.status = ChildBountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: frame_system::Pallet::::block_number() + + unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(), }; Ok(()) @@ -664,7 +667,7 @@ pub mod pallet { // Ensure block number is elapsed for processing the // claim. ensure!( - frame_system::Pallet::::block_number() >= *unlock_at, + Self::treasury_block_number() >= *unlock_at, BountiesError::::Premature, ); @@ -772,6 +775,13 @@ pub mod pallet { } impl Pallet { + /// Get the block number used in the treasury pallet. + /// + /// It may be configured to use the relay chain block number on a parachain. + pub fn treasury_block_number() -> BlockNumberFor { + ::BlockNumberProvider::current_block_number() + } + // This function will calculate the deposit of a curator. fn calculate_curator_deposit( parent_curator: &T::AccountId, diff --git a/substrate/frame/child-bounties/src/tests.rs b/substrate/frame/child-bounties/src/tests.rs index 125844fa70e2..96d01b03560d 100644 --- a/substrate/frame/child-bounties/src/tests.rs +++ b/substrate/frame/child-bounties/src/tests.rs @@ -42,6 +42,12 @@ use super::Event as ChildBountiesEvent; type Block = frame_system::mocking::MockBlock; type BountiesError = pallet_bounties::Error; +// This function directly jumps to a block number, and calls `on_initialize`. +fn go_to_block(n: u64) { + ::BlockNumberProvider::set_block_number(n); + >::on_initialize(n); +} + frame_support::construct_runtime!( pub enum Test { @@ -98,6 +104,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -184,15 +191,14 @@ fn add_child_bounty() { // Curator, child-bounty curator & beneficiary. // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 8; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee)); @@ -278,7 +284,7 @@ fn child_bounty_assign_curator() { // 3, Test for DB state of `ChildBounties`. // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&4, 101); Balances::make_free_balance_be(&8, 101); @@ -287,8 +293,7 @@ fn child_bounty_assign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let fee = 4; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee)); @@ -383,7 +388,7 @@ fn child_bounty_assign_curator() { fn award_claim_child_bounty() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -396,8 +401,7 @@ fn award_claim_child_bounty() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -449,7 +453,7 @@ fn award_claim_child_bounty() { BountiesError::Premature ); - System::set_block_number(9); + go_to_block(9); assert_ok!(ChildBounties::claim_child_bounty(RuntimeOrigin::signed(7), 0, 0)); @@ -474,7 +478,7 @@ fn award_claim_child_bounty() { fn close_child_bounty_added() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -487,8 +491,7 @@ fn close_child_bounty_added() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -504,7 +507,7 @@ fn close_child_bounty_added() { assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(4); + go_to_block(4); // Close child-bounty. // Wrong origin. @@ -531,7 +534,7 @@ fn close_child_bounty_added() { fn close_child_bounty_active() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -544,8 +547,7 @@ fn close_child_bounty_active() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -589,7 +591,7 @@ fn close_child_bounty_active() { fn close_child_bounty_pending() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -602,8 +604,7 @@ fn close_child_bounty_pending() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); let parent_fee = 6; assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, parent_fee)); @@ -650,7 +651,7 @@ fn close_child_bounty_pending() { fn child_bounty_added_unassign_curator() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -663,8 +664,7 @@ fn child_bounty_added_unassign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -692,7 +692,7 @@ fn child_bounty_added_unassign_curator() { fn child_bounty_curator_proposed_unassign_curator() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -705,8 +705,7 @@ fn child_bounty_curator_proposed_unassign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); @@ -767,7 +766,7 @@ fn child_bounty_active_unassign_curator() { // bounty. Unassign from random account. Should slash. new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -782,8 +781,7 @@ fn child_bounty_active_unassign_curator() { assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -797,8 +795,7 @@ fn child_bounty_active_unassign_curator() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(3); - >::on_initialize(3); + go_to_block(3); // Propose and accept curator for child-bounty. let fee = 6; @@ -817,8 +814,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); // Unassign curator - from reject origin. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::root(), 0, 0)); @@ -856,8 +852,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); // Unassign curator again - from parent curator. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(4), 0, 0)); @@ -893,8 +888,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(6); - >::on_initialize(6); + go_to_block(6); // Unassign curator again - from child-bounty curator. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(6), 0, 0)); @@ -932,8 +926,7 @@ fn child_bounty_active_unassign_curator() { } ); - System::set_block_number(7); - >::on_initialize(7); + go_to_block(7); // Unassign curator again - from non curator; non reject origin; some random guy. // Bounty update period is not yet complete. @@ -942,8 +935,7 @@ fn child_bounty_active_unassign_curator() { BountiesError::Premature ); - System::set_block_number(20); - >::on_initialize(20); + go_to_block(20); // Unassign child curator from random account after inactivity. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(3), 0, 0)); @@ -972,7 +964,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { // This can happen when the curator of parent bounty has been unassigned. new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -987,8 +979,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -1002,8 +993,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(3); - >::on_initialize(3); + go_to_block(3); // Propose and accept curator for child-bounty. let fee = 8; @@ -1022,14 +1012,12 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { } ); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); // Unassign parent bounty curator. assert_ok!(Bounties::unassign_curator(RuntimeOrigin::root(), 0)); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); // Try unassign child-bounty curator - from non curator; non reject // origin; some random guy. Bounty update period is not yet complete. @@ -1057,15 +1045,13 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { assert_eq!(Balances::free_balance(8), 101 - expected_child_deposit); assert_eq!(Balances::reserved_balance(8), 0); // slashed - System::set_block_number(6); - >::on_initialize(6); + go_to_block(6); // Propose and accept curator for parent-bounty again. assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 5, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(5), 0)); - System::set_block_number(7); - >::on_initialize(7); + go_to_block(7); // Propose and accept curator for child-bounty again. let fee = 2; @@ -1084,8 +1070,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { } ); - System::set_block_number(8); - >::on_initialize(8); + go_to_block(8); assert_noop!( ChildBounties::unassign_curator(RuntimeOrigin::signed(3), 0, 0), @@ -1095,8 +1080,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { // Unassign parent bounty curator again. assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(5), 0)); - System::set_block_number(9); - >::on_initialize(9); + go_to_block(9); // Unassign curator again - from parent curator. assert_ok!(ChildBounties::unassign_curator(RuntimeOrigin::signed(7), 0, 0)); @@ -1123,7 +1107,7 @@ fn parent_bounty_inactive_unassign_curator_child_bounty() { fn close_parent_with_child_bounty() { new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -1142,8 +1126,7 @@ fn close_parent_with_child_bounty() { Error::::ParentBountyNotActive ); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -1157,8 +1140,7 @@ fn close_parent_with_child_bounty() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); // Try close parent-bounty. // Child bounty active, can't close parent. @@ -1167,8 +1149,6 @@ fn close_parent_with_child_bounty() { BountiesError::HasActiveChildBounty ); - System::set_block_number(2); - // Close child-bounty. assert_ok!(ChildBounties::close_child_bounty(RuntimeOrigin::root(), 0, 0)); @@ -1187,7 +1167,7 @@ fn children_curator_fee_calculation_test() { // from parent bounty fee when claiming bounties. new_test_ext().execute_with(|| { // Make the parent bounty. - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::free_balance(Treasury::account_id()), 101); assert_eq!(Balances::reserved_balance(Treasury::account_id()), 0); @@ -1199,8 +1179,7 @@ fn children_curator_fee_calculation_test() { assert_ok!(Bounties::propose_bounty(RuntimeOrigin::signed(0), 50, b"12345".to_vec())); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), 0)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, 6)); assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(4), 0)); @@ -1214,8 +1193,7 @@ fn children_curator_fee_calculation_test() { )); assert_eq!(last_event(), ChildBountiesEvent::Added { index: 0, child_index: 0 }); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); let fee = 6; @@ -1245,7 +1223,7 @@ fn children_curator_fee_calculation_test() { } ); - System::set_block_number(9); + go_to_block(9); // Claim child-bounty. assert_ok!(ChildBounties::claim_child_bounty(RuntimeOrigin::signed(7), 0, 0)); @@ -1256,7 +1234,7 @@ fn children_curator_fee_calculation_test() { // Award the parent bounty. assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(4), 0, 9)); - System::set_block_number(15); + go_to_block(15); // Claim the parent bounty. assert_ok!(Bounties::claim_bounty(RuntimeOrigin::signed(9), 0)); @@ -1282,7 +1260,7 @@ fn accept_curator_handles_different_deposit_calculations() { let parent_value = 1_000_000; let parent_fee = 10_000; - System::set_block_number(1); + go_to_block(1); Balances::make_free_balance_be(&Treasury::account_id(), parent_value * 3); Balances::make_free_balance_be(&parent_curator, parent_fee * 100); assert_ok!(Bounties::propose_bounty( @@ -1292,8 +1270,7 @@ fn accept_curator_handles_different_deposit_calculations() { )); assert_ok!(Bounties::approve_bounty(RuntimeOrigin::root(), parent_index)); - System::set_block_number(2); - >::on_initialize(2); + go_to_block(2); assert_ok!(Bounties::propose_curator( RuntimeOrigin::root(), @@ -1319,8 +1296,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(3); - >::on_initialize(3); + go_to_block(3); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, @@ -1354,8 +1330,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(4); - >::on_initialize(4); + go_to_block(4); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, @@ -1387,8 +1362,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, @@ -1423,8 +1397,7 @@ fn accept_curator_handles_different_deposit_calculations() { child_value, b"12345-p1".to_vec() )); - System::set_block_number(5); - >::on_initialize(5); + go_to_block(5); assert_ok!(ChildBounties::propose_curator( RuntimeOrigin::signed(parent_curator), parent_index, diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index 7e4a9368ad0c..f6f130b7e261 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -119,6 +119,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -141,6 +142,7 @@ impl pallet_treasury::Config for Test { type Paymaster = PayFromAccount; type BalanceConverter = UnityAssetBalanceConversion; type PayoutPeriod = ConstU64<10>; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index ad74495ce090..b21a36949357 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -89,8 +89,11 @@ use scale_info::TypeInfo; use alloc::{boxed::Box, collections::btree_map::BTreeMap}; use sp_runtime::{ - traits::{AccountIdConversion, CheckedAdd, Saturating, StaticLookup, Zero}, - Permill, RuntimeDebug, + traits::{ + AccountIdConversion, BlockNumberProvider, CheckedAdd, One, Saturating, StaticLookup, + UniqueSaturatedInto, Zero, + }, + PerThing, Permill, RuntimeDebug, }; use frame_support::{ @@ -103,6 +106,7 @@ use frame_support::{ weights::Weight, BoundedVec, PalletId, }; +use frame_system::pallet_prelude::BlockNumberFor; pub use pallet::*; pub use weights::WeightInfo; @@ -275,6 +279,9 @@ pub mod pallet { /// Helper type for benchmarks. #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper: ArgumentsFactory; + + /// Provider for the block number. Normally this is the `frame_system` pallet. + type BlockNumberProvider: BlockNumberProvider>; } /// Number of proposals that have been made. @@ -322,6 +329,10 @@ pub mod pallet { OptionQuery, >; + /// The blocknumber for the last triggered spend period. + #[pallet::storage] + pub(crate) type LastSpendPeriod = StorageValue<_, BlockNumberFor, OptionQuery>; + #[pallet::genesis_config] #[derive(frame_support::DefaultNoBound)] pub struct GenesisConfig, I: 'static = ()> { @@ -414,7 +425,8 @@ pub mod pallet { impl, I: 'static> Hooks> for Pallet { /// ## Complexity /// - `O(A)` where `A` is the number of approvals - fn on_initialize(n: frame_system::pallet_prelude::BlockNumberFor) -> Weight { + fn on_initialize(_do_not_use_local_block_number: BlockNumberFor) -> Weight { + let block_number = T::BlockNumberProvider::current_block_number(); let pot = Self::pot(); let deactivated = Deactivated::::get(); if pot != deactivated { @@ -428,17 +440,29 @@ pub mod pallet { } // Check to see if we should spend some funds! - if (n % T::SpendPeriod::get()).is_zero() { - Self::spend_funds() + let last_spend_period = LastSpendPeriod::::get() + // This unwrap should only occur one time on any blockchain. + // `update_last_spend_period` will populate the `LastSpendPeriod` storage if it is + // empty. + .unwrap_or_else(|| Self::update_last_spend_period()); + let blocks_since_last_spend_period = block_number.saturating_sub(last_spend_period); + let safe_spend_period = T::SpendPeriod::get().max(BlockNumberFor::::one()); + + // Safe because of `max(1)` above. + let (spend_periods_passed, extra_blocks) = ( + blocks_since_last_spend_period / safe_spend_period, + blocks_since_last_spend_period % safe_spend_period, + ); + let new_last_spend_period = block_number.saturating_sub(extra_blocks); + if spend_periods_passed > BlockNumberFor::::zero() { + Self::spend_funds(spend_periods_passed, new_last_spend_period) } else { Weight::zero() } } #[cfg(feature = "try-runtime")] - fn try_state( - _: frame_system::pallet_prelude::BlockNumberFor, - ) -> Result<(), sp_runtime::TryRuntimeError> { + fn try_state(_: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { Self::do_try_state()?; Ok(()) } @@ -594,7 +618,7 @@ pub mod pallet { let max_amount = T::SpendOrigin::ensure_origin(origin)?; let beneficiary = T::BeneficiaryLookup::lookup(*beneficiary)?; - let now = frame_system::Pallet::::block_number(); + let now = T::BlockNumberProvider::current_block_number(); let valid_from = valid_from.unwrap_or(now); let expire_at = valid_from.saturating_add(T::PayoutPeriod::get()); ensure!(expire_at > now, Error::::SpendExpired); @@ -672,7 +696,7 @@ pub mod pallet { pub fn payout(origin: OriginFor, index: SpendIndex) -> DispatchResult { ensure_signed(origin)?; let mut spend = Spends::::get(index).ok_or(Error::::InvalidIndex)?; - let now = frame_system::Pallet::::block_number(); + let now = T::BlockNumberProvider::current_block_number(); ensure!(now >= spend.valid_from, Error::::EarlyPayout); ensure!(spend.expire_at > now, Error::::SpendExpired); ensure!( @@ -718,7 +742,7 @@ pub mod pallet { ensure_signed(origin)?; let mut spend = Spends::::get(index).ok_or(Error::::InvalidIndex)?; - let now = frame_system::Pallet::::block_number(); + let now = T::BlockNumberProvider::current_block_number(); if now > spend.expire_at && !matches!(spend.status, State::Attempted { .. }) { // spend has expired and no further status update is expected. @@ -792,6 +816,25 @@ impl, I: 'static> Pallet { T::PalletId::get().into_account_truncating() } + // Backfill the `LastSpendPeriod` storage, assuming that no configuration has changed + // since introducing this code. Used specifically for a migration-less switch to populate + // `LastSpendPeriod`. + fn update_last_spend_period() -> BlockNumberFor { + let block_number = T::BlockNumberProvider::current_block_number(); + let spend_period = T::SpendPeriod::get().max(BlockNumberFor::::one()); + let time_since_last_spend = block_number % spend_period; + // If it happens that this logic runs directly on a spend period block, we need to backdate + // to the last spend period so a spend still occurs this block. + let last_spend_period = if time_since_last_spend.is_zero() { + block_number.saturating_sub(spend_period) + } else { + // Otherwise, this is the last time we had a spend period. + block_number.saturating_sub(time_since_last_spend) + }; + LastSpendPeriod::::put(last_spend_period); + last_spend_period + } + /// Public function to proposal_count storage. pub fn proposal_count() -> ProposalIndex { ProposalCount::::get() @@ -808,7 +851,11 @@ impl, I: 'static> Pallet { } /// Spend some money! returns number of approvals before spend. - pub fn spend_funds() -> Weight { + pub fn spend_funds( + spend_periods_passed: BlockNumberFor, + new_last_spend_period: BlockNumberFor, + ) -> Weight { + LastSpendPeriod::::put(new_last_spend_period); let mut total_weight = Weight::zero(); let mut budget_remaining = Self::pot(); @@ -860,10 +907,15 @@ impl, I: 'static> Pallet { &mut missed_any, ); - if !missed_any { - // burn some proportion of the remaining budget if we run a surplus. - let burn = (T::Burn::get() * budget_remaining).min(budget_remaining); - budget_remaining -= burn; + if !missed_any && !T::Burn::get().is_zero() { + // Get the amount of treasury that should be left after potentially multiple spend + // periods have passed. + let one_minus_burn = T::Burn::get().left_from_one(); + let percent_left = + one_minus_burn.saturating_pow(spend_periods_passed.unique_saturated_into()); + let new_budget_remaining = percent_left * budget_remaining; + let burn = budget_remaining.saturating_sub(new_budget_remaining); + budget_remaining = new_budget_remaining; let (debit, credit) = T::Currency::pair(burn); imbalance.subsume(debit); diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 106bfb530a88..a99dd0dd4449 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -97,6 +97,12 @@ fn set_status(id: u64, s: PaymentStatus) { STATUS.with(|m| m.borrow_mut().insert(id, s)); } +// This function directly jumps to a block number, and calls `on_initialize`. +fn go_to_block(n: u64) { + ::BlockNumberProvider::set_block_number(n); + >::on_initialize(n); +} + pub struct TestPay; impl Pay for TestPay { type Beneficiary = u128; @@ -187,6 +193,7 @@ impl Config for Test { type Paymaster = TestPay; type BalanceConverter = MulBy>; type PayoutPeriod = SpendPayoutPeriod; + type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] type BenchmarkHelper = (); } @@ -268,7 +275,7 @@ fn spend_local_origin_permissioning_works() { fn spend_local_origin_works() { ExtBuilder::default().build().execute_with(|| { // Check that accumulate works when we have Some value in Dummy already. - Balances::make_free_balance_be(&Treasury::account_id(), 101); + Balances::make_free_balance_be(&Treasury::account_id(), 102); // approve spend of some amount to beneficiary `6`. assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); @@ -278,12 +285,12 @@ fn spend_local_origin_works() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(12), 20, 6)); assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(13), 50, 6)); // free balance of `6` is zero, spend period has not passed. - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(6), 0); // free balance of `6` is `100`, spend period has passed. - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(6), 100); - // `100` spent, `1` burned. + // `100` spent, `1` burned, `1` in ED. assert_eq!(Treasury::pot(), 0); }); } @@ -304,7 +311,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); - >::on_initialize(1); + go_to_block(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -317,7 +324,7 @@ fn unused_pot_should_diminish() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(pallet_balances::TotalIssuance::::get(), init_total_issuance + 100); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 50); assert_eq!(pallet_balances::TotalIssuance::::get(), init_total_issuance + 50); }); @@ -331,7 +338,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); - >::on_initialize(2); + go_to_block(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -345,11 +352,11 @@ fn pot_underflow_should_not_diminish() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 150, 3)); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed let _ = Balances::deposit_into_existing(&Treasury::account_id(), 100).unwrap(); - >::on_initialize(4); + go_to_block(4); assert_eq!(Balances::free_balance(3), 150); // Fund has been spent assert_eq!(Treasury::pot(), 25); // Pot has finally changed }); @@ -366,12 +373,12 @@ fn treasury_account_doesnt_get_deleted() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), treasury_balance, 3)); - >::on_initialize(2); + go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), Treasury::pot(), 3)); - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot is emptied assert_eq!(Balances::free_balance(Treasury::account_id()), 1); // but the account is still there }); @@ -395,7 +402,8 @@ fn inexistent_account_works() { assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 99, 3)); assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); - >::on_initialize(2); + go_to_block(2); + assert_eq!(Treasury::pot(), 0); // Pot hasn't changed assert_eq!(Balances::free_balance(3), 0); // Balance of `3` hasn't changed @@ -403,7 +411,7 @@ fn inexistent_account_works() { assert_eq!(Treasury::pot(), 99); // Pot now contains funds assert_eq!(Balances::free_balance(Treasury::account_id()), 100); // Account does exist - >::on_initialize(4); + go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot has changed assert_eq!(Balances::free_balance(3), 99); // Balance of `3` has changed @@ -936,3 +944,35 @@ fn try_state_spends_invariant_3_works() { ); }); } + +#[test] +fn multiple_spend_periods_work() { + ExtBuilder::default().build().execute_with(|| { + // Check that accumulate works when we have Some value in Dummy already. + // 100 will be spent, 1024 will be the burn amount, 1 for ED + Balances::make_free_balance_be(&Treasury::account_id(), 100 + 1024 + 1); + // approve spend of total amount 100 to beneficiary `6`. + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(10), 5, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(11), 10, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(12), 20, 6)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(13), 50, 6)); + // free balance of `6` is zero, spend period has not passed. + go_to_block(1); + assert_eq!(Balances::free_balance(6), 0); + // free balance of `6` is `100`, spend period has passed. + go_to_block(2); + assert_eq!(Balances::free_balance(6), 100); + // `100` spent, 50% burned + assert_eq!(Treasury::pot(), 512); + + // 3 more spends periods pass at once, and an extra block. + go_to_block(2 + (3 * 2) + 1); + // Pot should be reduced by 50% 3 times, so 1/8th the amount. + assert_eq!(Treasury::pot(), 64); + // Even though we are on block 9, the last spend period was block 8. + assert_eq!(LastSpendPeriod::::get(), Some(8)); + }); +}