From 104163e4a8647f4e88485d680f2d985ac88f952a Mon Sep 17 00:00:00 2001 From: Tomasz Polaczyk Date: Wed, 18 Dec 2024 11:18:04 +0100 Subject: [PATCH] Refactor on_container_author_noted hook to take a Vec --- pallets/author-noting/src/lib.rs | 55 ++++++++--------- pallets/inflation-rewards/src/lib.rs | 83 ++++++++++++++------------ pallets/inflation-rewards/src/tests.rs | 48 ++++++++------- pallets/services-payment/src/lib.rs | 44 +++++++------- pallets/services-payment/src/tests.rs | 25 ++++++-- pallets/xcm-core-buyer/src/lib.rs | 21 +++---- pallets/xcm-core-buyer/src/tests.rs | 12 +++- primitives/traits/src/lib.rs | 17 +++--- 8 files changed, 172 insertions(+), 133 deletions(-) diff --git a/pallets/author-noting/src/lib.rs b/pallets/author-noting/src/lib.rs index 866dedf06..b1304dedb 100644 --- a/pallets/author-noting/src/lib.rs +++ b/pallets/author-noting/src/lib.rs @@ -44,11 +44,12 @@ use { sp_consensus_aura::{inherents::InherentType, Slot, AURA_ENGINE_ID}, sp_inherents::{InherentIdentifier, IsFatalError}, sp_runtime::{traits::Header, DigestItem, DispatchResult, RuntimeString}, + sp_std::vec::Vec, tp_author_noting_inherent::INHERENT_IDENTIFIER, tp_traits::{ - AuthorNotingHook, ContainerChainBlockInfo, GenericStateProof, GenericStorageReader, - GetContainerChainAuthor, GetCurrentContainerChains, LatestAuthorInfoFetcher, - NativeStorageReader, ReadEntryErr, + AuthorNotingHook, AuthorNotingInfo, ContainerChainBlockInfo, GenericStateProof, + GenericStorageReader, GetContainerChainAuthor, GetCurrentContainerChains, + LatestAuthorInfoFetcher, NativeStorageReader, ReadEntryErr, }, }; @@ -158,9 +159,8 @@ pub mod pallet { let storage_reader = T::RelayOrPara::create_storage_reader(data); let parent_tanssi_slot = u64::from(T::SlotBeacon::slot()).into(); + let mut infos = Vec::with_capacity(registered_para_ids.len()); - // TODO: we should probably fetch all authors-containers first - // then pass the vector to the hook, this would allow for a better estimation for para_id in registered_para_ids { match Self::fetch_block_info_from_proof( &storage_reader, @@ -173,31 +173,23 @@ pub mod pallet { |maybe_old_block_info: &mut Option< ContainerChainBlockInfo, >| { - if let Some(ref mut old_block_info) = maybe_old_block_info { - if block_info.block_number > old_block_info.block_number { - // We only reward author if the block increases - total_weight = total_weight.saturating_add( - T::AuthorNotingHook::on_container_author_noted( - &block_info.author, - block_info.block_number, - para_id, - ), - ); - let _ = core::mem::replace(old_block_info, block_info); - } - } else { - // If there is no previous block, we should reward the author of the first block - total_weight = total_weight.saturating_add( - T::AuthorNotingHook::on_container_author_noted( - &block_info.author, - block_info.block_number, - para_id, - ), - ); - let _ = core::mem::replace( - maybe_old_block_info, - Some(block_info), - ); + // No block number is the same as the last block number being 0: + // the first block created by collators is block number 1. + let old_block_number = maybe_old_block_info + .as_ref() + .map(|old_block_info| old_block_info.block_number) + .unwrap_or(0); + // We only reward author if the block increases + // If there is no previous block, we should reward the author of the first block + if block_info.block_number > old_block_number { + let bi = block_info.clone(); + let info = AuthorNotingInfo { + author: block_info.author, + block_number: block_info.block_number, + para_id, + }; + infos.push(info); + *maybe_old_block_info = Some(bi); } }, ); @@ -209,6 +201,9 @@ pub mod pallet { ), } } + + total_weight = total_weight + .saturating_add(T::AuthorNotingHook::on_container_authors_noted(&infos)); } // We correctly set the data diff --git a/pallets/inflation-rewards/src/lib.rs b/pallets/inflation-rewards/src/lib.rs index ca90e7a97..fee68b2a1 100644 --- a/pallets/inflation-rewards/src/lib.rs +++ b/pallets/inflation-rewards/src/lib.rs @@ -28,8 +28,9 @@ mod mock; #[cfg(test)] mod tests; +use tp_traits::AuthorNotingInfo; use { - dp_core::{BlockNumber, ParaId}, + dp_core::ParaId, frame_support::{ pallet_prelude::*, traits::{ @@ -249,53 +250,57 @@ pub mod pallet { // There will be no additional check other than checking if we have already // rewarded this author for **in this tanssi block** // Any additional check should be done in the calling function -// TODO: consider passing a vector here impl AuthorNotingHook for Pallet { - fn on_container_author_noted( - author: &T::AccountId, - _block_number: BlockNumber, - para_id: ParaId, - ) -> Weight { + fn on_container_authors_noted(info: &[AuthorNotingInfo]) -> Weight { let mut total_weight = T::DbWeight::get().reads_writes(1, 0); // We take chains to reward, to see what containers are left to reward if let Some(mut container_chains_to_reward) = ChainsToReward::::get() { - // If we find the index is because we still have not rewarded it - if let Ok(index) = container_chains_to_reward.para_ids.binary_search(¶_id) { - // we distribute rewards to the author - match T::StakingRewardsDistributor::distribute_rewards( - author.clone(), - T::Currency::withdraw( - &T::PendingRewardsAccount::get(), - container_chains_to_reward.rewards_per_chain, - Precision::BestEffort, - Preservation::Expendable, - Fortitude::Force, - ) - .unwrap_or(CreditOf::::zero()), - ) { - Ok(frame_support::dispatch::PostDispatchInfo { actual_weight, .. }) => { - Self::deposit_event(Event::RewardedContainer { - account_id: author.clone(), - balance: container_chains_to_reward.rewards_per_chain, - para_id, - }); - if let Some(weight) = actual_weight { - total_weight += weight + for info in info { + let author = &info.author; + let para_id = info.para_id; + + // If we find the index is because we still have not rewarded it + if let Ok(index) = container_chains_to_reward.para_ids.binary_search(¶_id) { + // we distribute rewards to the author + match T::StakingRewardsDistributor::distribute_rewards( + author.clone(), + T::Currency::withdraw( + &T::PendingRewardsAccount::get(), + container_chains_to_reward.rewards_per_chain, + Precision::BestEffort, + Preservation::Expendable, + Fortitude::Force, + ) + .unwrap_or(CreditOf::::zero()), + ) { + Ok(frame_support::dispatch::PostDispatchInfo { actual_weight, .. }) => { + Self::deposit_event(Event::RewardedContainer { + account_id: author.clone(), + balance: container_chains_to_reward.rewards_per_chain, + para_id, + }); + if let Some(weight) = actual_weight { + total_weight += weight + } + } + Err(e) => { + log::warn!("Fail to distribute rewards: {:?}", e) } } - Err(e) => { - log::debug!("Fail to distribute rewards: {:?}", e) - } + // we remove the para id from container-chains to reward + // this makes sure we dont reward it twice in the same block + container_chains_to_reward.para_ids.remove(index); } - // we remove the para id from container-chains to reward - // this makes sure we dont reward it twice in the same block - container_chains_to_reward.para_ids.remove(index); - - total_weight += T::DbWeight::get().writes(1); - // Keep track of chains to reward - ChainsToReward::::put(container_chains_to_reward); } + + total_weight += T::DbWeight::get().writes(1); + // Keep track of chains to reward + ChainsToReward::::put(container_chains_to_reward); + } else { + // TODO: why would ChainsToReward ever be None? + log::warn!("ChainsToReward is None"); } + total_weight } diff --git a/pallets/inflation-rewards/src/tests.rs b/pallets/inflation-rewards/src/tests.rs index c24446ecd..ae97c2876 100644 --- a/pallets/inflation-rewards/src/tests.rs +++ b/pallets/inflation-rewards/src/tests.rs @@ -130,11 +130,13 @@ fn test_reward_container_chain_author() { // Note container author let registered_para_ids = ::ContainerChains::current_container_chains(); - as AuthorNotingHook>::on_container_author_noted( - &container_author, - 1, - registered_para_ids[0], - ); + as AuthorNotingHook>::on_container_authors_noted(&[ + AuthorNotingInfo { + author: container_author.clone(), + block_number: 1, + para_id: registered_para_ids[0], + }, + ]); // Author should be rewarded immediately assert_eq!( @@ -148,11 +150,13 @@ fn test_reward_container_chain_author() { let new_supply_2 = total_supply_2 - total_supply_1; // Note next container author - as AuthorNotingHook>::on_container_author_noted( - &container_author_2, - 2, - registered_para_ids[0], - ); + as AuthorNotingHook>::on_container_authors_noted(&[ + AuthorNotingInfo { + author: container_author_2.clone(), + block_number: 2, + para_id: registered_para_ids[0], + }, + ]); // Author should be rewarded immediately assert_eq!( @@ -177,18 +181,22 @@ fn test_cannot_reward_twice_in_same_tanssi_block() { // Note container author let registered_para_ids = ::ContainerChains::current_container_chains(); - as AuthorNotingHook>::on_container_author_noted( - &container_author, - 1, - registered_para_ids[0], - ); + as AuthorNotingHook>::on_container_authors_noted(&[ + AuthorNotingInfo { + author: container_author.clone(), + block_number: 1, + para_id: registered_para_ids[0], + }, + ]); // Regardless if we inject a new block, we cannot reward twice the same paraId - as AuthorNotingHook>::on_container_author_noted( - &container_author, - 2, - registered_para_ids[0], - ); + as AuthorNotingHook>::on_container_authors_noted(&[ + AuthorNotingInfo { + author: container_author.clone(), + block_number: 2, + para_id: registered_para_ids[0], + }, + ]); // Author should be rewarded only once assert_eq!( diff --git a/pallets/services-payment/src/lib.rs b/pallets/services-payment/src/lib.rs index 315f81cd6..d5df5d35e 100644 --- a/pallets/services-payment/src/lib.rs +++ b/pallets/services-payment/src/lib.rs @@ -36,7 +36,7 @@ use { serde::{Deserialize, Serialize}, sp_io::hashing::blake2_256, sp_runtime::{traits::TrailingZeroInput, DispatchError}, - tp_traits::{AuthorNotingHook, BlockNumber, CollatorAssignmentHook, CollatorAssignmentTip}, + tp_traits::{AuthorNotingHook, CollatorAssignmentHook, CollatorAssignmentTip}, }; #[cfg(any(test, feature = "runtime-benchmarks"))] @@ -50,6 +50,7 @@ pub mod weights; pub use weights::WeightInfo; pub use pallet::*; +use tp_traits::AuthorNotingInfo; #[frame_support::pallet] pub mod pallet { @@ -540,31 +541,32 @@ impl AuthorNotingHook for Pallet { // This hook is called when pallet_author_noting sees that the block number of a container chain has increased. // Currently we always charge 1 credit, even if a container chain produced more that 1 block in between tanssi // blocks. - fn on_container_author_noted( - _author: &T::AccountId, - _block_number: BlockNumber, - para_id: ParaId, - ) -> Weight { - if Pallet::::burn_block_production_free_credit_for_para(¶_id).is_err() { - let (amount_to_charge, _weight) = T::ProvideBlockProductionCost::block_cost(¶_id); + fn on_container_authors_noted(info: &[AuthorNotingInfo]) -> Weight { + for info in info { + let para_id = info.para_id; + if Pallet::::burn_block_production_free_credit_for_para(¶_id).is_err() { + let (amount_to_charge, _weight) = + T::ProvideBlockProductionCost::block_cost(¶_id); - match T::Currency::withdraw( - &Self::parachain_tank(para_id), - amount_to_charge, - WithdrawReasons::FEE, - ExistenceRequirement::KeepAlive, - ) { - Err(e) => log::warn!( - "Failed to withdraw block production payment for container chain {}: {:?}", - u32::from(para_id), - e - ), - Ok(imbalance) => { - T::OnChargeForBlock::on_unbalanced(imbalance); + match T::Currency::withdraw( + &Self::parachain_tank(para_id), + amount_to_charge, + WithdrawReasons::FEE, + ExistenceRequirement::KeepAlive, + ) { + Err(e) => log::warn!( + "Failed to withdraw block production payment for container chain {}: {:?}", + u32::from(para_id), + e + ), + Ok(imbalance) => { + T::OnChargeForBlock::on_unbalanced(imbalance); + } } } } + // TODO: weight should depend on info.len() T::WeightInfo::on_container_author_noted() } diff --git a/pallets/services-payment/src/tests.rs b/pallets/services-payment/src/tests.rs index 7f6f2eea2..beca21060 100644 --- a/pallets/services-payment/src/tests.rs +++ b/pallets/services-payment/src/tests.rs @@ -14,6 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Tanssi. If not, see +use tp_traits::AuthorNotingInfo; use { crate::{ mock::*, pallet as pallet_services_payment, BlockProductionCredits, @@ -239,7 +240,11 @@ fn credits_should_be_substracted_from_tank_if_no_free_credits() { 1000u128 ); - PaymentServices::on_container_author_noted(&1, 1, 1.into()); + PaymentServices::on_container_authors_noted(&[AuthorNotingInfo { + author: 1, + block_number: 1, + para_id: 1.into(), + }]); assert_eq!( Balances::balance(&crate::Pallet::::parachain_tank(1.into())), @@ -266,7 +271,11 @@ fn credits_should_not_be_substracted_from_tank_if_it_involves_death() { 100u128 ); - PaymentServices::on_container_author_noted(&1, 1, 1.into()); + PaymentServices::on_container_authors_noted(&[AuthorNotingInfo { + author: 1, + block_number: 1, + para_id: 1.into(), + }]); assert_eq!( Balances::balance(&crate::Pallet::::parachain_tank(1.into())), @@ -303,7 +312,11 @@ fn not_having_enough_tokens_in_tank_should_not_error() { 1u128 ); - PaymentServices::on_container_author_noted(&1, 1, 1.into()); + PaymentServices::on_container_authors_noted(&[AuthorNotingInfo { + author: 1, + block_number: 1, + para_id: 1.into(), + }]); assert_eq!( Balances::balance(&crate::Pallet::::parachain_tank(1.into())), @@ -456,7 +469,11 @@ fn tip_should_be_charged_on_collators_assignment() { false )); - PaymentServices::on_container_author_noted(&1, 1, para_id.into()); + PaymentServices::on_container_authors_noted(&[AuthorNotingInfo { + author: 1, + block_number: 1, + para_id: para_id.into(), + }]); let (assignment_cost, _weight) = ::ProvideCollatorAssignmentCost::collator_assignment_cost( diff --git a/pallets/xcm-core-buyer/src/lib.rs b/pallets/xcm-core-buyer/src/lib.rs index 221a29b8c..3dc7f64ec 100644 --- a/pallets/xcm-core-buyer/src/lib.rs +++ b/pallets/xcm-core-buyer/src/lib.rs @@ -34,6 +34,7 @@ mod benchmarks; pub mod weights; pub use weights::WeightInfo; +use tp_traits::AuthorNotingInfo; use { dp_core::ParaId, frame_support::{ @@ -50,9 +51,7 @@ use { latest::{Asset, Assets, InteriorLocation, Response, Xcm}, prelude::*, }, - tp_traits::{ - AuthorNotingHook, BlockNumber, LatestAuthorInfoFetcher, ParathreadParams, SlotFrequency, - }, + tp_traits::{AuthorNotingHook, LatestAuthorInfoFetcher, ParathreadParams, SlotFrequency}, tp_xcm_core_buyer::BuyCoreCollatorProof, }; @@ -116,14 +115,16 @@ pub enum BuyingError { } impl AuthorNotingHook for Pallet { - fn on_container_author_noted( - _author: &T::AccountId, - _block_number: BlockNumber, - para_id: ParaId, - ) -> Weight { - PendingBlocks::::remove(para_id); + fn on_container_authors_noted(info: &[AuthorNotingInfo]) -> Weight { + let mut writes = 0; + + for info in info { + let para_id = info.para_id; + PendingBlocks::::remove(para_id); + writes += 1; + } - T::DbWeight::get().writes(1) + T::DbWeight::get().writes(writes) } #[cfg(feature = "runtime-benchmarks")] diff --git a/pallets/xcm-core-buyer/src/tests.rs b/pallets/xcm-core-buyer/src/tests.rs index e88910bb0..60143d545 100644 --- a/pallets/xcm-core-buyer/src/tests.rs +++ b/pallets/xcm-core-buyer/src/tests.rs @@ -359,7 +359,11 @@ fn slot_frequency_is_taken_into_account() { _ => panic!("We checked for the event variant above; qed") }; assert_ok!(XcmCoreBuyer::query_response(RuntimeOrigin::root(), query_id, Response::DispatchResult(MaybeErrorCode::Success))); - Pallet::::on_container_author_noted(&1u64, 5, another_para_id); + Pallet::::on_container_authors_noted(&[AuthorNotingInfo { + author: 1u64, + block_number: 5, + para_id: another_para_id, + }]); // Add latest author info entry to indicate block was produced MockData::mutate(|stored_mock_data| { @@ -576,7 +580,11 @@ fn force_buy_two_messages_fails_after_receiving_order_success_response() { assert_noop!(XcmCoreBuyer::force_buy_core(RuntimeOrigin::root(), para_id,), Error::::BlockProductionPending); // Now if the pallet gets notification that pending block for that para id is incremented it is possible to buy again. - Pallet::::on_container_author_noted(&1u64, 5, para_id); + Pallet::::on_container_authors_noted(&[AuthorNotingInfo { + author: 1u64, + block_number: 5, + para_id, + }]); assert_ok!(XcmCoreBuyer::force_buy_core(RuntimeOrigin::root(), para_id,)); assert_ok!(XcmCoreBuyer::query_response(RuntimeOrigin::root(), query_id, Response::DispatchResult(MaybeErrorCode::Success))); diff --git a/primitives/traits/src/lib.rs b/primitives/traits/src/lib.rs index 3be563c6d..816ab62bb 100644 --- a/primitives/traits/src/lib.rs +++ b/primitives/traits/src/lib.rs @@ -89,16 +89,19 @@ impl CollatorAssignmentTip for () { None } } + +pub struct AuthorNotingInfo { + pub author: AccountId, + pub block_number: BlockNumber, + pub para_id: ParaId, +} + /// The author-noting hook to react to container chains authoring. pub trait AuthorNotingHook { /// This hook is called partway through the `set_latest_author_data` inherent in author-noting. /// /// The hook should never panic and is required to return the weight consumed. - fn on_container_author_noted( - author: &AccountId, - block_number: BlockNumber, - para_id: ParaId, - ) -> Weight; + fn on_container_authors_noted(info: &[AuthorNotingInfo]) -> Weight; #[cfg(feature = "runtime-benchmarks")] fn prepare_worst_case_for_bench(author: &AccountId, block_number: BlockNumber, para_id: ParaId); @@ -106,9 +109,9 @@ pub trait AuthorNotingHook { #[impl_trait_for_tuples::impl_for_tuples(5)] impl AuthorNotingHook for Tuple { - fn on_container_author_noted(a: &AccountId, b: BlockNumber, p: ParaId) -> Weight { + fn on_container_authors_noted(info: &[AuthorNotingInfo]) -> Weight { let mut weight: Weight = Default::default(); - for_tuples!( #( weight.saturating_accrue(Tuple::on_container_author_noted(a, b, p)); )* ); + for_tuples!( #( weight.saturating_accrue(Tuple::on_container_authors_noted(info)); )* ); weight }