From f1843140bcaacd09253f0fb87f7d7c89d9b069e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 29 Dec 2020 14:51:25 +0100 Subject: [PATCH 01/12] Add proper `commit_all` to `TestExternalities` This pr adds a propoer `commit_all` function to `TestExternalities` to commit all changes from the overlay to the internal backend. Besides that it fixes some bugs with handling empty dbs when calculating a delta storage root. It also changes the way data is added to the in memory backend. --- .../state-machine/src/in_memory_backend.rs | 88 ++++++------------- .../src/overlayed_changes/mod.rs | 6 ++ primitives/state-machine/src/testing.rs | 35 +++++++- primitives/trie/src/lib.rs | 17 +++- 4 files changed, 81 insertions(+), 65 deletions(-) diff --git a/primitives/state-machine/src/in_memory_backend.rs b/primitives/state-machine/src/in_memory_backend.rs index f211f60202730..e5e90f3930e01 100644 --- a/primitives/state-machine/src/in_memory_backend.rs +++ b/primitives/state-machine/src/in_memory_backend.rs @@ -17,47 +17,14 @@ //! State machine in memory backend. use crate::{ - StorageKey, StorageValue, StorageCollection, - trie_backend::TrieBackend, + StorageKey, StorageValue, StorageCollection, trie_backend::TrieBackend, backend::Backend, }; -use std::{collections::{BTreeMap, HashMap}}; +use std::collections::{BTreeMap, HashMap}; use hash_db::Hasher; -use sp_trie::{ - MemoryDB, TrieMut, - trie_types::TrieDBMut, -}; +use sp_trie::MemoryDB; use codec::Codec; use sp_core::storage::{ChildInfo, Storage}; -/// Insert input pairs into memory db. -fn insert_into_memory_db(mut root: H::Out, mdb: &mut MemoryDB, input: I) -> H::Out -where - H: Hasher, - I: IntoIterator)>, -{ - { - let mut trie = if root == Default::default() { - TrieDBMut::::new(mdb, &mut root) - } else { - TrieDBMut::::from_existing(mdb, &mut root).unwrap() - }; - for (key, value) in input { - if let Err(e) = match value { - Some(value) => { - trie.insert(&key, &value) - }, - None => { - trie.remove(&key) - }, - } { - panic!("Failed to write to trie: {}", e); - } - } - trie.commit(); - } - root -} - /// Create a new empty instance of in-memory backend. pub fn new_in_mem() -> TrieBackend, H> where @@ -92,32 +59,17 @@ where &mut self, changes: T, ) { - let mut new_child_roots = Vec::new(); - let mut root_map = None; - let root = self.root().clone(); - for (child_info, map) in changes { - if let Some(child_info) = child_info.as_ref() { - let prefix_storage_key = child_info.prefixed_storage_key(); - let ch = insert_into_memory_db::(root, self.backend_storage_mut(), map.clone().into_iter()); - new_child_roots.push((prefix_storage_key.into_inner(), Some(ch.as_ref().into()))); - } else { - root_map = Some(map); - } - } + let (top, child) = changes.into_iter().partition::, _>(|v| v.0.is_none()); + let (root, transaction) = self.full_storage_root( + top.iter().map(|(_, v)| v).flatten().map(|(k, v)| (&k[..], v.as_deref())), + child.iter() + .filter_map(|v| + v.0.as_ref().map(|c| (c, v.1.iter().map(|(k, v)| (&k[..], v.as_deref())))) + ), + ); - let root = match root_map { - Some(map) => insert_into_memory_db::( - root, - self.backend_storage_mut(), - map.into_iter().chain(new_child_roots.into_iter()), - ), - None => insert_into_memory_db::( - root, - self.backend_storage_mut(), - new_child_roots.into_iter(), - ), - }; self.essence.set_root(root); + self.backend_storage_mut().consolidate(transaction); } /// Merge trie nodes into this backend. @@ -158,7 +110,9 @@ where { fn from(inner: HashMap, BTreeMap>) -> Self { let mut backend = new_in_mem(); - backend.insert(inner.into_iter().map(|(k, m)| (k, m.into_iter().map(|(k, v)| (k, Some(v))).collect()))); + backend.insert( + inner.into_iter().map(|(k, m)| (k, m.into_iter().map(|(k, v)| (k, Some(v))).collect())), + ); backend } } @@ -232,4 +186,16 @@ mod tests { let storage_key = child_info.prefixed_storage_key(); assert!(trie_backend.storage(storage_key.as_slice()).unwrap().is_some()); } + + #[test] + fn insert_multiple_times_child_data_works() { + let mut storage = new_in_mem::(); + let child_info = ChildInfo::new_default(b"1"); + + storage.insert(vec![(Some(child_info.clone()), vec![(b"2".to_vec(), Some(b"3".to_vec()))])]); + storage.insert(vec![(Some(child_info.clone()), vec![(b"1".to_vec(), Some(b"3".to_vec()))])]); + + assert_eq!(storage.child_storage(&child_info, &b"2"[..]), Ok(Some(b"3".to_vec()))); + assert_eq!(storage.child_storage(&child_info, &b"1"[..]), Ok(Some(b"3".to_vec()))); + } } diff --git a/primitives/state-machine/src/overlayed_changes/mod.rs b/primitives/state-machine/src/overlayed_changes/mod.rs index 6ef09fc81505d..f35db1f3c523b 100644 --- a/primitives/state-machine/src/overlayed_changes/mod.rs +++ b/primitives/state-machine/src/overlayed_changes/mod.rs @@ -215,6 +215,12 @@ impl Default for StorageChanges } impl OverlayedChanges { + /// Clear all changes. + pub fn clear(&mut self) { + self.top = Default::default(); + self.children.clear(); + } + /// Whether no changes are contained in the top nor in any of the child changes. pub fn is_empty(&self) -> bool { self.top.is_empty() && self.children.is_empty() diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index 4dcd308285625..18d98eb9b383a 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -154,7 +154,7 @@ impl TestExternalities } /// Return a new backend with all pending value. - pub fn commit_all(&self) -> InMemoryBackend { + pub fn as_backend(&self) -> InMemoryBackend { let top: Vec<_> = self.overlay.changes() .map(|(k, v)| (k.clone(), v.value().cloned())) .collect(); @@ -172,6 +172,12 @@ impl TestExternalities self.backend.update(transaction) } + /// Commit all pending changes to the underlying backend. + pub fn commit_all(&mut self) { + self.backend = self.as_backend(); + self.overlay.clear(); + } + /// Execute the given closure while `self` is set as externalities. /// /// Returns the result of the given closure. @@ -209,7 +215,7 @@ impl PartialEq for TestExternalities /// This doesn't test if they are in the same state, only if they contains the /// same data at this state fn eq(&self, other: &TestExternalities) -> bool { - self.commit_all().eq(&other.commit_all()) + self.as_backend().eq(&other.as_backend()) } } @@ -258,7 +264,7 @@ impl sp_externalities::ExtensionStore for TestExternalities where #[cfg(test)] mod tests { use super::*; - use sp_core::{H256, traits::Externalities}; + use sp_core::{H256, traits::Externalities, storage::ChildInfo}; use sp_runtime::traits::BlakeTwo256; use hex_literal::hex; @@ -289,4 +295,27 @@ mod tests { fn assert_send() {} assert_send::>(); } + + #[test] + fn commit_all_and_kill_child_storage() { + let mut ext = TestExternalities::::default(); + let child_info = ChildInfo::new_default(&b"test_child"[..]); + + { + let mut ext = ext.ext(); + ext.place_child_storage(&child_info, b"doe".to_vec(), Some(b"reindeer".to_vec())); + ext.place_child_storage(&child_info, b"dog".to_vec(), Some(b"puppy".to_vec())); + ext.place_child_storage(&child_info, b"dog2".to_vec(), Some(b"puppy2".to_vec())); + } + + ext.commit_all(); + + { + let mut ext = ext.ext(); + + assert!(!ext.kill_child_storage(&child_info, Some(2)), "Should not delete all keys"); + + assert!(ext.child_storage(&child_info, &b"dog2"[..]).is_some()); + } + } } diff --git a/primitives/trie/src/lib.rs b/primitives/trie/src/lib.rs index 2687d8e422796..252ca8ee6ae2f 100644 --- a/primitives/trie/src/lib.rs +++ b/primitives/trie/src/lib.rs @@ -184,7 +184,11 @@ pub fn delta_trie_root( DB: hash_db::HashDB, { { - let mut trie = TrieDBMut::::from_existing(&mut *db, &mut root)?; + let mut trie = if root == Default::default() { + TrieDBMut::::new(db, &mut root) + } else { + TrieDBMut::::from_existing(db, &mut root)? + }; let mut delta = delta.into_iter().collect::>(); delta.sort_by(|l, r| l.0.borrow().cmp(r.0.borrow())); @@ -532,6 +536,17 @@ mod tests { assert_eq!(root1, root2); } + + #[test] + fn delta_trie_root_works_with_empty_db() { + let mut db = MemoryDB::default(); + delta_trie_root::( + &mut db, + Default::default(), + vec![(&b"test"[..], Some(&b"value"[..]))], + ).unwrap(); + } + #[test] fn empty_is_equivalent() { let input: Vec<(&[u8], &[u8])> = vec![]; From d3a6ec4bdc3abcc6de6886041e4bfd948eeb0a61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 29 Dec 2020 20:38:32 +0100 Subject: [PATCH 02/12] Update primitives/state-machine/src/testing.rs Co-authored-by: cheme --- primitives/state-machine/src/testing.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index 18d98eb9b383a..742264eb59b09 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -315,6 +315,8 @@ mod tests { assert!(!ext.kill_child_storage(&child_info, Some(2)), "Should not delete all keys"); + assert!(ext.child_storage(&child_info, &b"doe"[..]).is_none()); + assert!(ext.child_storage(&child_info, &b"dog"[..]).is_none()); assert!(ext.child_storage(&child_info, &b"dog2"[..]).is_some()); } } From 65ec63b6a1925ab2c4d75406e1b01f741c31b75c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 29 Dec 2020 13:31:44 -0400 Subject: [PATCH 03/12] Don't allow self proxies (#7803) --- frame/proxy/src/lib.rs | 3 +++ frame/proxy/src/tests.rs | 1 + 2 files changed, 4 insertions(+) diff --git a/frame/proxy/src/lib.rs b/frame/proxy/src/lib.rs index 6342f0c052b82..93f1d8e80d5c6 100644 --- a/frame/proxy/src/lib.rs +++ b/frame/proxy/src/lib.rs @@ -168,6 +168,8 @@ decl_error! { NoPermission, /// Announcement, if made at all, was made too recently. Unannounced, + /// Cannot add self as proxy. + NoSelfProxy, } } @@ -567,6 +569,7 @@ impl Module { proxy_type: T::ProxyType, delay: T::BlockNumber, ) -> DispatchResult { + ensure!(delegator != &delegatee, Error::::NoSelfProxy); Proxies::::try_mutate(delegator, |(ref mut proxies, ref mut deposit)| { ensure!(proxies.len() < T::MaxProxies::get() as usize, Error::::TooMany); let proxy_def = ProxyDefinition { delegate: delegatee, proxy_type, delay }; diff --git a/frame/proxy/src/tests.rs b/frame/proxy/src/tests.rs index 0821105235623..b1dca43b6a707 100644 --- a/frame/proxy/src/tests.rs +++ b/frame/proxy/src/tests.rs @@ -396,6 +396,7 @@ fn add_remove_proxies_works() { assert_eq!(Balances::reserved_balance(1), 2); assert_ok!(Proxy::remove_proxy(Origin::signed(1), 2, ProxyType::JustTransfer, 0)); assert_eq!(Balances::reserved_balance(1), 0); + assert_noop!(Proxy::add_proxy(Origin::signed(1), 1, ProxyType::Any, 0), Error::::NoSelfProxy); }); } From ded9b081b1ffbb95ae97757dda56c1893a2669a7 Mon Sep 17 00:00:00 2001 From: RK Date: Wed, 30 Dec 2020 00:07:19 +0530 Subject: [PATCH 04/12] Allow council to slash treasury tip (#7753) * wk2051 | D4 |Allow council to slash treasury tip | p1 * Update frame/tips/src/lib.rs Co-authored-by: Xiliang Chen * wk2051 | D5 |Allow council to slash treasury tip | p2 * wk2051 | D5 |Allow council to slash treasury tip | p3 * wk2051 | D5 |Allow council to slash treasury tip | p4 * wk2051 | D5 |Allow council to slash treasury tip | p5 * random change * cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_tips --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/tips/src/weights.rs --template=./.maintain/frame-weight-template.hbs * fix typo * Update frame/tips/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/tips/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/tips/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/tips/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/tips/src/tests.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * wk2052 | D1 | Allow council to slash treasury tip | p6 Co-authored-by: Xiliang Chen Co-authored-by: Shawn Tabrizi Co-authored-by: Parity Benchmarking Bot Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/bounties/src/benchmarking.rs | 20 +++++------ frame/tips/README.md | 1 + frame/tips/src/benchmarking.rs | 26 ++++++++++++-- frame/tips/src/lib.rs | 56 ++++++++++++++++-------------- frame/tips/src/tests.rs | 38 +++++++++++++++++--- frame/tips/src/weights.rs | 51 +++++++++++++++++---------- frame/treasury/src/benchmarking.rs | 4 +-- 7 files changed, 133 insertions(+), 63 deletions(-) diff --git a/frame/bounties/src/benchmarking.rs b/frame/bounties/src/benchmarking.rs index 5a323ff0aafcc..21f68b0781919 100644 --- a/frame/bounties/src/benchmarking.rs +++ b/frame/bounties/src/benchmarking.rs @@ -77,7 +77,7 @@ fn create_bounty() -> Result<( Ok((curator_lookup, bounty_id)) } -fn setup_pod_account() { +fn setup_pot_account() { let pot_account = Bounties::::account_id(); let value = T::Currency::minimum_balance().saturating_mul(1_000_000_000u32.into()); let _ = T::Currency::make_free_balance_be(&pot_account, value); @@ -109,7 +109,7 @@ benchmarks! { }: _(RawOrigin::Root, bounty_id) propose_curator { - setup_pod_account::(); + setup_pot_account::(); let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; @@ -120,7 +120,7 @@ benchmarks! { // Worst case when curator is inactive and any sender unassigns the curator. unassign_curator { - setup_pod_account::(); + setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); let bounty_id = BountyCount::get() - 1; @@ -129,7 +129,7 @@ benchmarks! { }: _(RawOrigin::Signed(caller), bounty_id) accept_curator { - setup_pod_account::(); + setup_pot_account::(); let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; @@ -140,7 +140,7 @@ benchmarks! { }: _(RawOrigin::Signed(curator), bounty_id) award_bounty { - setup_pod_account::(); + setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); @@ -150,7 +150,7 @@ benchmarks! { }: _(RawOrigin::Signed(curator), bounty_id, beneficiary) claim_bounty { - setup_pod_account::(); + setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); @@ -170,14 +170,14 @@ benchmarks! { } close_bounty_proposed { - setup_pod_account::(); + setup_pot_account::(); let (caller, curator, fee, value, reason) = setup_bounty::(0, 0); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::get() - 1; }: close_bounty(RawOrigin::Root, bounty_id) close_bounty_active { - setup_pod_account::(); + setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); let bounty_id = BountyCount::get() - 1; @@ -187,7 +187,7 @@ benchmarks! { } extend_bounty_expiry { - setup_pod_account::(); + setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); @@ -200,7 +200,7 @@ benchmarks! { spend_funds { let b in 1 .. 100; - setup_pod_account::(); + setup_pot_account::(); create_approved_bounties::(b)?; let mut budget_remaining = BalanceOf::::max_value(); diff --git a/frame/tips/README.md b/frame/tips/README.md index 457e5b3bd0e79..36148e276edc2 100644 --- a/frame/tips/README.md +++ b/frame/tips/README.md @@ -30,3 +30,4 @@ any finders fee, in case of a public (and bonded) original report. - `tip_new` - Report an item worthy of a tip and declare a specific amount to tip. - `tip` - Declare or redeclare an amount to tip for a particular reason. - `close_tip` - Close and pay out a tip. +- `slash_tip` - Remove and slash an already-open tip. \ No newline at end of file diff --git a/frame/tips/src/benchmarking.rs b/frame/tips/src/benchmarking.rs index 71f9002b9bf11..4f0338b9c5dbb 100644 --- a/frame/tips/src/benchmarking.rs +++ b/frame/tips/src/benchmarking.rs @@ -79,7 +79,7 @@ fn create_tips(t: u32, hash: T::Hash, value: BalanceOf) -> Ok(()) } -fn setup_pod_account() { +fn setup_pot_account() { let pot_account = TipsMod::::account_id(); let value = T::Currency::minimum_balance().saturating_mul(1_000_000_000u32.into()); let _ = T::Currency::make_free_balance_be(&pot_account, value); @@ -148,7 +148,7 @@ benchmarks! { let t in 1 .. MAX_TIPPERS; // Make sure pot is funded - setup_pod_account::(); + setup_pot_account::(); // Set up a new tip proposal let (member, reason, beneficiary, value) = setup_tip::(0, t)?; @@ -164,6 +164,7 @@ benchmarks! { let reason_hash = T::Hashing::hash(&reason[..]); let hash = T::Hashing::hash_of(&(&reason_hash, &beneficiary)); ensure!(Tips::::contains_key(hash), "tip does not exist"); + create_tips::(t, hash.clone(), value)?; let caller = account("caller", t, SEED); @@ -172,6 +173,26 @@ benchmarks! { frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), hash) + slash_tip { + let t in 1 .. MAX_TIPPERS; + + // Make sure pot is funded + setup_pot_account::(); + + // Set up a new tip proposal + let (member, reason, beneficiary, value) = setup_tip::(0, t)?; + let value = T::Currency::minimum_balance().saturating_mul(100u32.into()); + TipsMod::::tip_new( + RawOrigin::Signed(member).into(), + reason.clone(), + beneficiary.clone(), + value + )?; + + let reason_hash = T::Hashing::hash(&reason[..]); + let hash = T::Hashing::hash_of(&(&reason_hash, &beneficiary)); + ensure!(Tips::::contains_key(hash), "tip does not exist"); + }: _(RawOrigin::Root, hash) } #[cfg(test)] @@ -188,6 +209,7 @@ mod tests { assert_ok!(test_benchmark_tip_new::()); assert_ok!(test_benchmark_tip::()); assert_ok!(test_benchmark_close_tip::()); + assert_ok!(test_benchmark_slash_tip::()); }); } } diff --git a/frame/tips/src/lib.rs b/frame/tips/src/lib.rs index 3507b220d5dbf..eaa785a5638ec 100644 --- a/frame/tips/src/lib.rs +++ b/frame/tips/src/lib.rs @@ -58,8 +58,6 @@ mod tests; mod benchmarking; pub mod weights; -use sp_std::if_std; - use sp_std::prelude::*; use frame_support::{decl_module, decl_storage, decl_event, ensure, decl_error, Parameter}; use frame_support::traits::{ @@ -70,8 +68,7 @@ use frame_support::traits::{ use sp_runtime::{ Percent, RuntimeDebug, traits::{ Zero, AccountIdConversion, Hash, BadOrigin }}; - -use frame_support::traits::{Contains, ContainsLengthBound}; +use frame_support::traits::{Contains, ContainsLengthBound, OnUnbalanced, EnsureOrigin}; use codec::{Encode, Decode}; use frame_system::{self as system, ensure_signed}; pub use weights::WeightInfo; @@ -170,6 +167,8 @@ decl_event!( TipClosed(Hash, AccountId, Balance), /// A tip suggestion has been retracted. \[tip_hash\] TipRetracted(Hash), + /// A tip suggestion has been slashed. \[tip_hash, finder, deposit\] + TipSlashed(Hash, AccountId, Balance), } ); @@ -408,6 +407,32 @@ decl_module! { Tips::::remove(hash); Self::payout_tip(hash, tip); } + + /// Remove and slash an already-open tip. + /// + /// May only be called from `T::RejectOrigin`. + /// + /// As a result, the finder is slashed and the deposits are lost. + /// + /// Emits `TipSlashed` if successful. + /// + /// # + /// `T` is charged as upper bound given by `ContainsLengthBound`. + /// The actual cost depends on the implementation of `T::Tippers`. + /// # + #[weight = ::WeightInfo::slash_tip(T::Tippers::max_len() as u32)] + fn slash_tip(origin, hash: T::Hash) { + T::RejectOrigin::ensure_origin(origin)?; + + let tip = Tips::::take(hash).ok_or(Error::::UnknownTip)?; + + if !tip.deposit.is_zero() { + let imbalance = T::Currency::slash_reserved(&tip.finder, tip.deposit).0; + T::OnSlash::on_unbalanced(imbalance); + } + Reasons::::remove(&tip.reason); + Self::deposit_event(RawEvent::TipSlashed(hash, tip.finder, tip.deposit)); + } } } @@ -523,10 +548,6 @@ impl Module { use frame_support::{Twox64Concat, migration::StorageKeyIterator}; - if_std! { - println!("Inside migrate_retract_tip_for_tip_new()!"); - } - for (hash, old_tip) in StorageKeyIterator::< T::Hash, OldOpenTip, T::BlockNumber, T::Hash>, @@ -534,25 +555,11 @@ impl Module { >::new(b"Treasury", b"Tips").drain() { - if_std! { - println!("Inside loop migrate_retract_tip_for_tip_new()!"); - } - let (finder, deposit, finders_fee) = match old_tip.finder { Some((finder, deposit)) => { - if_std! { - // This code is only being compiled and executed when the `std` feature is enabled. - println!("OK case!"); - println!("value is: {:#?},{:#?}", finder, deposit); - } (finder, deposit, true) }, None => { - if_std! { - // This code is only being compiled and executed when the `std` feature is enabled. - println!("None case!"); - // println!("value is: {:#?},{:#?}", T::AccountId::default(), Zero::zero()); - } (T::AccountId::default(), Zero::zero(), false) }, }; @@ -567,10 +574,5 @@ impl Module { }; Tips::::insert(hash, new_tip) } - - if_std! { - println!("Exit migrate_retract_tip_for_tip_new()!"); - } - } } diff --git a/frame/tips/src/tests.rs b/frame/tips/src/tests.rs index e6f9cd4e66b77..3bfecafeaf970 100644 --- a/frame/tips/src/tests.rs +++ b/frame/tips/src/tests.rs @@ -286,6 +286,40 @@ fn close_tip_works() { }); } +#[test] +fn slash_tip_works() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + Balances::make_free_balance_be(&Treasury::account_id(), 101); + assert_eq!(Treasury::pot(), 100); + + assert_eq!(Balances::reserved_balance(0), 0); + assert_eq!(Balances::free_balance(0), 100); + + assert_ok!(TipsModTestInst::report_awesome(Origin::signed(0), b"awesome.dot".to_vec(), 3)); + + assert_eq!(Balances::reserved_balance(0), 12); + assert_eq!(Balances::free_balance(0), 88); + + let h = tip_hash(); + assert_eq!(last_event(), RawEvent::NewTip(h)); + + // can't remove from any origin + assert_noop!( + TipsModTestInst::slash_tip(Origin::signed(0), h.clone()), + BadOrigin, + ); + + // can remove from root. + assert_ok!(TipsModTestInst::slash_tip(Origin::root(), h.clone())); + assert_eq!(last_event(), RawEvent::TipSlashed(h, 0, 12)); + + // tipper slashed + assert_eq!(Balances::reserved_balance(0), 0); + assert_eq!(Balances::free_balance(0), 88); + }); +} + #[test] fn retract_tip_works() { new_test_ext().execute_with(|| { @@ -409,12 +443,8 @@ fn test_last_reward_migration() { s.top = data.into_iter().collect(); - println!("Executing the test!"); - sp_io::TestExternalities::new(s).execute_with(|| { - println!("Calling migrate_retract_tip_for_tip_new()!"); - TipsModTestInst::migrate_retract_tip_for_tip_new(); // Test w/ finder diff --git a/frame/tips/src/weights.rs b/frame/tips/src/weights.rs index ad2d3104cafe5..c1d9982910013 100644 --- a/frame/tips/src/weights.rs +++ b/frame/tips/src/weights.rs @@ -18,11 +18,11 @@ //! Autogenerated weights for pallet_tips //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0 -//! DATE: 2020-12-16, STEPS: [50, ], REPEAT: 20, LOW RANGE: [], HIGH RANGE: [] +//! DATE: 2020-12-20, STEPS: [50, ], REPEAT: 20, LOW RANGE: [], HIGH RANGE: [] //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128 // Executed Command: -// ./target/release/substrate +// target/release/substrate // benchmark // --chain=dev // --steps=50 @@ -49,83 +49,98 @@ pub trait WeightInfo { fn tip_new(r: u32, t: u32, ) -> Weight; fn tip(t: u32, ) -> Weight; fn close_tip(t: u32, ) -> Weight; + fn slash_tip(t: u32, ) -> Weight; } /// Weights for pallet_tips using the Substrate node and recommended hardware. pub struct SubstrateWeight(PhantomData); impl WeightInfo for SubstrateWeight { fn report_awesome(r: u32, ) -> Weight { - (74_814_000 as Weight) + (73_795_000 as Weight) // Standard Error: 0 .saturating_add((2_000 as Weight).saturating_mul(r as Weight)) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(2 as Weight)) } fn retract_tip() -> Weight { - (62_962_000 as Weight) + (61_753_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(2 as Weight)) } fn tip_new(r: u32, t: u32, ) -> Weight { - (48_132_000 as Weight) + (47_731_000 as Weight) // Standard Error: 0 .saturating_add((2_000 as Weight).saturating_mul(r as Weight)) // Standard Error: 0 - .saturating_add((155_000 as Weight).saturating_mul(t as Weight)) + .saturating_add((154_000 as Weight).saturating_mul(t as Weight)) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(2 as Weight)) } fn tip(t: u32, ) -> Weight { - (36_168_000 as Weight) + (35_215_000 as Weight) // Standard Error: 1_000 - .saturating_add((695_000 as Weight).saturating_mul(t as Weight)) + .saturating_add((712_000 as Weight).saturating_mul(t as Weight)) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } fn close_tip(t: u32, ) -> Weight { - (119_313_000 as Weight) + (117_027_000 as Weight) // Standard Error: 1_000 - .saturating_add((372_000 as Weight).saturating_mul(t as Weight)) + .saturating_add((375_000 as Weight).saturating_mul(t as Weight)) .saturating_add(T::DbWeight::get().reads(3 as Weight)) .saturating_add(T::DbWeight::get().writes(3 as Weight)) } + fn slash_tip(t: u32, ) -> Weight { + (37_184_000 as Weight) + // Standard Error: 0 + .saturating_add((11_000 as Weight).saturating_mul(t as Weight)) + .saturating_add(T::DbWeight::get().reads(1 as Weight)) + .saturating_add(T::DbWeight::get().writes(2 as Weight)) + } } // For backwards compatibility and tests impl WeightInfo for () { fn report_awesome(r: u32, ) -> Weight { - (74_814_000 as Weight) + (73_795_000 as Weight) // Standard Error: 0 .saturating_add((2_000 as Weight).saturating_mul(r as Weight)) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } fn retract_tip() -> Weight { - (62_962_000 as Weight) + (61_753_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } fn tip_new(r: u32, t: u32, ) -> Weight { - (48_132_000 as Weight) + (47_731_000 as Weight) // Standard Error: 0 .saturating_add((2_000 as Weight).saturating_mul(r as Weight)) // Standard Error: 0 - .saturating_add((155_000 as Weight).saturating_mul(t as Weight)) + .saturating_add((154_000 as Weight).saturating_mul(t as Weight)) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } fn tip(t: u32, ) -> Weight { - (36_168_000 as Weight) + (35_215_000 as Weight) // Standard Error: 1_000 - .saturating_add((695_000 as Weight).saturating_mul(t as Weight)) + .saturating_add((712_000 as Weight).saturating_mul(t as Weight)) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } fn close_tip(t: u32, ) -> Weight { - (119_313_000 as Weight) + (117_027_000 as Weight) // Standard Error: 1_000 - .saturating_add((372_000 as Weight).saturating_mul(t as Weight)) + .saturating_add((375_000 as Weight).saturating_mul(t as Weight)) .saturating_add(RocksDbWeight::get().reads(3 as Weight)) .saturating_add(RocksDbWeight::get().writes(3 as Weight)) } + fn slash_tip(t: u32, ) -> Weight { + (37_184_000 as Weight) + // Standard Error: 0 + .saturating_add((11_000 as Weight).saturating_mul(t as Weight)) + .saturating_add(RocksDbWeight::get().reads(1 as Weight)) + .saturating_add(RocksDbWeight::get().writes(2 as Weight)) + } } diff --git a/frame/treasury/src/benchmarking.rs b/frame/treasury/src/benchmarking.rs index 16ed1b01ae0d0..39b398ab8916f 100644 --- a/frame/treasury/src/benchmarking.rs +++ b/frame/treasury/src/benchmarking.rs @@ -59,7 +59,7 @@ fn create_approved_proposals, I: Instance>(n: u32) -> Result<(), &' Ok(()) } -fn setup_pod_account, I: Instance>() { +fn setup_pot_account, I: Instance>() { let pot_account = Treasury::::account_id(); let value = T::Currency::minimum_balance().saturating_mul(1_000_000_000u32.into()); let _ = T::Currency::make_free_balance_be(&pot_account, value); @@ -97,7 +97,7 @@ benchmarks_instance! { on_initialize_proposals { let p in 0 .. 100; - setup_pod_account::(); + setup_pot_account::(); create_approved_proposals::(p)?; }: { Treasury::::on_initialize(T::BlockNumber::zero()); From 8478fade06b6836ae93cfaab46ae2bfe5cbfbd7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 29 Dec 2020 20:53:03 +0100 Subject: [PATCH 05/12] Review feedback --- .../state-machine/src/in_memory_backend.rs | 6 ++--- primitives/trie/src/lib.rs | 25 ++++++------------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/primitives/state-machine/src/in_memory_backend.rs b/primitives/state-machine/src/in_memory_backend.rs index e5e90f3930e01..7d5c50d090a50 100644 --- a/primitives/state-machine/src/in_memory_backend.rs +++ b/primitives/state-machine/src/in_memory_backend.rs @@ -21,7 +21,7 @@ use crate::{ }; use std::collections::{BTreeMap, HashMap}; use hash_db::Hasher; -use sp_trie::MemoryDB; +use sp_trie::{MemoryDB, empty_trie_root, Layout}; use codec::Codec; use sp_core::storage::{ChildInfo, Storage}; @@ -31,9 +31,7 @@ where H::Out: Codec + Ord, { let db = MemoryDB::default(); - let mut backend = TrieBackend::new(db, Default::default()); - backend.insert(std::iter::empty()); - backend + TrieBackend::new(db, empty_trie_root::>()) } impl TrieBackend, H> diff --git a/primitives/trie/src/lib.rs b/primitives/trie/src/lib.rs index 252ca8ee6ae2f..4914d85f5811f 100644 --- a/primitives/trie/src/lib.rs +++ b/primitives/trie/src/lib.rs @@ -184,11 +184,7 @@ pub fn delta_trie_root( DB: hash_db::HashDB, { { - let mut trie = if root == Default::default() { - TrieDBMut::::new(db, &mut root) - } else { - TrieDBMut::::from_existing(db, &mut root)? - }; + let mut trie = TrieDBMut::::from_existing(db, &mut root)?; let mut delta = delta.into_iter().collect::>(); delta.sort_by(|l, r| l.0.borrow().cmp(r.0.borrow())); @@ -227,9 +223,13 @@ pub fn read_trie_value_with< Ok(TrieDB::::new(&*db, root)?.get_with(key, query).map(|x| x.map(|val| val.to_vec()))?) } +/// Determine the empty trie root. +pub fn empty_trie_root() -> ::Out { + L::trie_root::<_, Vec, Vec>(core::iter::empty()) +} + /// Determine the empty child trie root. -pub fn empty_child_trie_root( -) -> ::Out { +pub fn empty_child_trie_root() -> ::Out { L::trie_root::<_, Vec, Vec>(core::iter::empty()) } @@ -536,17 +536,6 @@ mod tests { assert_eq!(root1, root2); } - - #[test] - fn delta_trie_root_works_with_empty_db() { - let mut db = MemoryDB::default(); - delta_trie_root::( - &mut db, - Default::default(), - vec![(&b"test"[..], Some(&b"value"[..]))], - ).unwrap(); - } - #[test] fn empty_is_equivalent() { let input: Vec<(&[u8], &[u8])> = vec![]; From f45e0a5a1a94c13921c8fa15c4f735bb3ea256ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Dec 2020 09:33:20 +0100 Subject: [PATCH 06/12] Review feedback --- .../state-machine/src/in_memory_backend.rs | 6 +++ .../src/overlayed_changes/mod.rs | 6 --- primitives/state-machine/src/testing.rs | 43 +++++++++++++++++-- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/primitives/state-machine/src/in_memory_backend.rs b/primitives/state-machine/src/in_memory_backend.rs index 7d5c50d090a50..c0f8054a6f01f 100644 --- a/primitives/state-machine/src/in_memory_backend.rs +++ b/primitives/state-machine/src/in_memory_backend.rs @@ -77,6 +77,12 @@ where Self::new(clone, root) } + /// Apply the given transaction to this backend and set the root to the given value. + pub fn apply_transaction(&mut self, root: H::Out, transaction: MemoryDB) { + self.backend_storage_mut().consolidate(transaction); + self.essence.set_root(root); + } + /// Compare with another in-memory backend. pub fn eq(&self, other: &Self) -> bool { self.root() == other.root() diff --git a/primitives/state-machine/src/overlayed_changes/mod.rs b/primitives/state-machine/src/overlayed_changes/mod.rs index f35db1f3c523b..6ef09fc81505d 100644 --- a/primitives/state-machine/src/overlayed_changes/mod.rs +++ b/primitives/state-machine/src/overlayed_changes/mod.rs @@ -215,12 +215,6 @@ impl Default for StorageChanges } impl OverlayedChanges { - /// Clear all changes. - pub fn clear(&mut self) { - self.top = Default::default(); - self.children.clear(); - } - /// Whether no changes are contained in the top nor in any of the child changes. pub fn is_empty(&self) -> bool { self.top.is_empty() && self.children.is_empty() diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index 742264eb59b09..fa8d7da73a47f 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -173,9 +173,28 @@ impl TestExternalities } /// Commit all pending changes to the underlying backend. - pub fn commit_all(&mut self) { - self.backend = self.as_backend(); - self.overlay.clear(); + /// + /// # Panic + /// + /// This will panic if there are still open transactions. + pub fn commit_all(&mut self) -> Result<(), String> { + let changes_trie_state = match self.changes_trie_config.clone() { + Some(config) => Some(ChangesTrieState { + config, + zero: 0.into(), + storage: &self.changes_trie_storage, + }), + None => None, + }; + let changes = self.overlay.drain_storage_changes( + &self.backend, + changes_trie_state.as_ref(), + Default::default(), + &mut Default::default(), + )?; + + self.backend.apply_transaction(changes.transaction_storage_root, changes.transaction); + Ok(()) } /// Execute the given closure while `self` is set as externalities. @@ -308,7 +327,7 @@ mod tests { ext.place_child_storage(&child_info, b"dog2".to_vec(), Some(b"puppy2".to_vec())); } - ext.commit_all(); + ext.commit_all().unwrap(); { let mut ext = ext.ext(); @@ -320,4 +339,20 @@ mod tests { assert!(ext.child_storage(&child_info, &b"dog2"[..]).is_some()); } } + + #[test] + fn as_backend_generates_same_backend_as_commit_all() { + let mut ext = TestExternalities::::default(); + { + let mut ext = ext.ext(); + ext.set_storage(b"doe".to_vec(), b"reindeer".to_vec()); + ext.set_storage(b"dog".to_vec(), b"puppy".to_vec()); + ext.set_storage(b"dogglesworth".to_vec(), b"cat".to_vec()); + } + + let backend = ext.as_backend(); + + ext.commit_all().unwrap(); + assert!(ext.backend.eq(&backend), "Both backend should be equal."); + } } From 117f2f305e0bd6c731b5e21f7c2caac43fd063ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Dec 2020 09:44:14 +0100 Subject: [PATCH 07/12] Update docs --- primitives/state-machine/src/testing.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index fa8d7da73a47f..af674b8fa14a7 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -153,7 +153,10 @@ impl TestExternalities &mut self.changes_trie_storage } - /// Return a new backend with all pending value. + /// Return a new backend with all pending values. + /// + /// In contrast to [`commit_all`](Self::commit_all) this will not panic if there are open + /// transactions. pub fn as_backend(&self) -> InMemoryBackend { let top: Vec<_> = self.overlay.changes() .map(|(k, v)| (k.clone(), v.value().cloned())) From 4db5ec5c0fcffa2880e248abb87086a14c8b17b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Dec 2020 09:44:25 +0100 Subject: [PATCH 08/12] More docs --- primitives/state-machine/src/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index af674b8fa14a7..e9a7af535222d 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -153,7 +153,7 @@ impl TestExternalities &mut self.changes_trie_storage } - /// Return a new backend with all pending values. + /// Return a new backend with all pending changes. /// /// In contrast to [`commit_all`](Self::commit_all) this will not panic if there are open /// transactions. From e2a186230ef7baa10cae45c2492414bd28d67ed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Dec 2020 10:38:52 +0100 Subject: [PATCH 09/12] Make it private --- primitives/state-machine/src/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index e9a7af535222d..8a5aa0086d402 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -157,7 +157,7 @@ impl TestExternalities /// /// In contrast to [`commit_all`](Self::commit_all) this will not panic if there are open /// transactions. - pub fn as_backend(&self) -> InMemoryBackend { + fn as_backend(&self) -> InMemoryBackend { let top: Vec<_> = self.overlay.changes() .map(|(k, v)| (k.clone(), v.value().cloned())) .collect(); From ac77bb24045b1b319826451c6970c8d153caadc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Dec 2020 12:16:34 +0100 Subject: [PATCH 10/12] Use `None` --- primitives/state-machine/src/testing.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index 8a5aa0086d402..ba6050da19fbc 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -181,17 +181,9 @@ impl TestExternalities /// /// This will panic if there are still open transactions. pub fn commit_all(&mut self) -> Result<(), String> { - let changes_trie_state = match self.changes_trie_config.clone() { - Some(config) => Some(ChangesTrieState { - config, - zero: 0.into(), - storage: &self.changes_trie_storage, - }), - None => None, - }; let changes = self.overlay.drain_storage_changes( &self.backend, - changes_trie_state.as_ref(), + None, Default::default(), &mut Default::default(), )?; From 283afa49a0d333fde52603bdfef7981564d67fa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Dec 2020 12:53:47 +0100 Subject: [PATCH 11/12] Use apply transaction --- primitives/state-machine/src/in_memory_backend.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/primitives/state-machine/src/in_memory_backend.rs b/primitives/state-machine/src/in_memory_backend.rs index c0f8054a6f01f..ca300aec919c4 100644 --- a/primitives/state-machine/src/in_memory_backend.rs +++ b/primitives/state-machine/src/in_memory_backend.rs @@ -66,8 +66,7 @@ where ), ); - self.essence.set_root(root); - self.backend_storage_mut().consolidate(transaction); + self.apply_transaction(root, transaction); } /// Merge trie nodes into this backend. From 72664afaca7ba96fd8b0d252daaa8347fe66438e Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Wed, 30 Dec 2020 14:02:02 +0100 Subject: [PATCH 12/12] Update primitives/state-machine/src/testing.rs --- primitives/state-machine/src/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/state-machine/src/testing.rs b/primitives/state-machine/src/testing.rs index ba6050da19fbc..23c3abe4910c5 100644 --- a/primitives/state-machine/src/testing.rs +++ b/primitives/state-machine/src/testing.rs @@ -181,7 +181,7 @@ impl TestExternalities /// /// This will panic if there are still open transactions. pub fn commit_all(&mut self) -> Result<(), String> { - let changes = self.overlay.drain_storage_changes( + let changes = self.overlay.drain_storage_changes::<_, _, N>( &self.backend, None, Default::default(),