From 2ae79be8e028a995b850621ee55f46c041eceefe Mon Sep 17 00:00:00 2001 From: davidk-pt Date: Tue, 5 Nov 2024 09:31:48 +0200 Subject: [PATCH 01/11] Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet (#5961) Resolves issue https://github.com/paritytech/polkadot-sdk/issues/5928 Adds `approve_bounty_with_curator` call to the `bounties` pallet to combine functions of `approve_bounty` and `propose_curator` into one call. Also adds a new status `ApprovedWithCurator` required to distinguish if bounty was approved with curator when skipping through `Funded` status and moving to `CuratorProposed` status. If `unassign_curator` is called after `approve_bounty_with_curator` the process will fall back to the old flow of calling `propose_curator` separately. --------- Co-authored-by: DavidK Co-authored-by: command-bot <> Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com> --- .../rococo/src/weights/pallet_bounties.rs | 82 +++--- prdoc/pr_5961.prdoc | 15 ++ substrate/frame/bounties/src/benchmarking.rs | 15 ++ substrate/frame/bounties/src/lib.rs | 72 +++++- substrate/frame/bounties/src/tests.rs | 233 +++++++++++++++++- substrate/frame/bounties/src/weights.rs | 191 +++++++------- 6 files changed, 482 insertions(+), 126 deletions(-) create mode 100644 prdoc/pr_5961.prdoc diff --git a/polkadot/runtime/rococo/src/weights/pallet_bounties.rs b/polkadot/runtime/rococo/src/weights/pallet_bounties.rs index 8f8be5f2386f7..e1f630ec4ce7b 100644 --- a/polkadot/runtime/rococo/src/weights/pallet_bounties.rs +++ b/polkadot/runtime/rococo/src/weights/pallet_bounties.rs @@ -17,25 +17,23 @@ //! Autogenerated weights for `pallet_bounties` //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0 -//! DATE: 2024-02-29, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2024-10-22, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `runner-bn-ce5rx-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz` +//! HOSTNAME: `runner-augrssgt-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz` //! WASM-EXECUTION: `Compiled`, CHAIN: `Some("rococo-dev")`, DB CACHE: 1024 // Executed Command: -// ./target/production/polkadot +// target/production/polkadot // benchmark // pallet -// --chain=rococo-dev // --steps=50 // --repeat=20 -// --no-storage-info -// --no-median-slopes -// --no-min-squares -// --pallet=pallet_bounties // --extrinsic=* -// --execution=wasm // --wasm-execution=compiled +// --heap-pages=4096 +// --json-file=/builds/parity/mirrors/polkadot-sdk/.git/.artifacts/bench.json +// --pallet=pallet_bounties +// --chain=rococo-dev // --header=./polkadot/file_header.txt // --output=./polkadot/runtime/rococo/src/weights/ @@ -63,11 +61,11 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `210` // Estimated: `3593` - // Minimum execution time: 21_772_000 picoseconds. - Weight::from_parts(22_861_341, 0) + // Minimum execution time: 26_614_000 picoseconds. + Weight::from_parts(28_274_660, 0) .saturating_add(Weight::from_parts(0, 3593)) - // Standard Error: 3 - .saturating_add(Weight::from_parts(721, 0).saturating_mul(d.into())) + // Standard Error: 4 + .saturating_add(Weight::from_parts(779, 0).saturating_mul(d.into())) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(4)) } @@ -79,8 +77,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `302` // Estimated: `3642` - // Minimum execution time: 11_218_000 picoseconds. - Weight::from_parts(11_796_000, 0) + // Minimum execution time: 14_692_000 picoseconds. + Weight::from_parts(15_070_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) @@ -91,22 +89,36 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `322` // Estimated: `3642` - // Minimum execution time: 10_959_000 picoseconds. - Weight::from_parts(11_658_000, 0) + // Minimum execution time: 13_695_000 picoseconds. + Weight::from_parts(14_220_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(1)) .saturating_add(T::DbWeight::get().writes(1)) } /// Storage: `Bounties::Bounties` (r:1 w:1) /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) + /// Storage: `Bounties::BountyApprovals` (r:1 w:1) + /// Proof: `Bounties::BountyApprovals` (`max_values`: Some(1), `max_size`: Some(402), added: 897, mode: `MaxEncodedLen`) + fn approve_bounty_with_curator() -> Weight { + // Proof Size summary in bytes: + // Measured: `322` + // Estimated: `3642` + // Minimum execution time: 18_428_000 picoseconds. + Weight::from_parts(19_145_000, 0) + .saturating_add(Weight::from_parts(0, 3642)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + /// Storage: `Bounties::Bounties` (r:1 w:1) + /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) /// Storage: `System::Account` (r:1 w:1) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) fn unassign_curator() -> Weight { // Proof Size summary in bytes: // Measured: `498` // Estimated: `3642` - // Minimum execution time: 37_419_000 picoseconds. - Weight::from_parts(38_362_000, 0) + // Minimum execution time: 44_648_000 picoseconds. + Weight::from_parts(45_860_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) @@ -119,8 +131,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `494` // Estimated: `3642` - // Minimum execution time: 27_328_000 picoseconds. - Weight::from_parts(27_661_000, 0) + // Minimum execution time: 33_973_000 picoseconds. + Weight::from_parts(34_979_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) @@ -133,8 +145,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `400` // Estimated: `3642` - // Minimum execution time: 16_067_000 picoseconds. - Weight::from_parts(16_865_000, 0) + // Minimum execution time: 20_932_000 picoseconds. + Weight::from_parts(21_963_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(1)) @@ -151,8 +163,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `764` // Estimated: `8799` - // Minimum execution time: 101_153_000 picoseconds. - Weight::from_parts(102_480_000, 0) + // Minimum execution time: 114_942_000 picoseconds. + Weight::from_parts(117_653_000, 0) .saturating_add(Weight::from_parts(0, 8799)) .saturating_add(T::DbWeight::get().reads(5)) .saturating_add(T::DbWeight::get().writes(6)) @@ -169,8 +181,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `444` // Estimated: `3642` - // Minimum execution time: 38_838_000 picoseconds. - Weight::from_parts(39_549_000, 0) + // Minimum execution time: 47_649_000 picoseconds. + Weight::from_parts(49_016_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(3)) .saturating_add(T::DbWeight::get().writes(3)) @@ -187,8 +199,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `680` // Estimated: `6196` - // Minimum execution time: 68_592_000 picoseconds. - Weight::from_parts(70_727_000, 0) + // Minimum execution time: 80_298_000 picoseconds. + Weight::from_parts(82_306_000, 0) .saturating_add(Weight::from_parts(0, 6196)) .saturating_add(T::DbWeight::get().reads(4)) .saturating_add(T::DbWeight::get().writes(4)) @@ -199,8 +211,8 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `358` // Estimated: `3642` - // Minimum execution time: 11_272_000 picoseconds. - Weight::from_parts(11_592_000, 0) + // Minimum execution time: 14_237_000 picoseconds. + Weight::from_parts(14_969_000, 0) .saturating_add(Weight::from_parts(0, 3642)) .saturating_add(T::DbWeight::get().reads(1)) .saturating_add(T::DbWeight::get().writes(1)) @@ -216,11 +228,11 @@ impl pallet_bounties::WeightInfo for WeightInfo { // Proof Size summary in bytes: // Measured: `0 + b * (297 ±0)` // Estimated: `1887 + b * (5206 ±0)` - // Minimum execution time: 2_844_000 picoseconds. - Weight::from_parts(2_900_000, 0) + // Minimum execution time: 3_174_000 picoseconds. + Weight::from_parts(3_336_000, 0) .saturating_add(Weight::from_parts(0, 1887)) - // Standard Error: 9_467 - .saturating_add(Weight::from_parts(32_326_595, 0).saturating_mul(b.into())) + // Standard Error: 10_408 + .saturating_add(Weight::from_parts(37_811_366, 0).saturating_mul(b.into())) .saturating_add(T::DbWeight::get().reads(1)) .saturating_add(T::DbWeight::get().reads((3_u64).saturating_mul(b.into()))) .saturating_add(T::DbWeight::get().writes(1)) diff --git a/prdoc/pr_5961.prdoc b/prdoc/pr_5961.prdoc new file mode 100644 index 0000000000000..46a5be8e49d51 --- /dev/null +++ b/prdoc/pr_5961.prdoc @@ -0,0 +1,15 @@ +# 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: "Bounties Pallet: add `approve_bounty_with_curator` call" + +doc: + - audience: [Runtime Dev, Runtime User] + description: | + Adds `approve_bounty_with_curator` call to the bounties pallet to combine `approve_bounty` and `propose_curator` into one call. If `unassign_curator` is called after `approve_bounty_with_curator` the process falls back to the previous flow of calling `propose_curator` separately. Introduces a new `ApprovedWithCurator` bounty status when bounty is approved with curator. + +crates: + - name: pallet-bounties + bump: major + - name: rococo-runtime + bump: minor diff --git a/substrate/frame/bounties/src/benchmarking.rs b/substrate/frame/bounties/src/benchmarking.rs index 6fa60e6938b6a..8ad85d5420edd 100644 --- a/substrate/frame/bounties/src/benchmarking.rs +++ b/substrate/frame/bounties/src/benchmarking.rs @@ -125,6 +125,21 @@ benchmarks_instance_pallet! { Treasury::::on_initialize(frame_system::Pallet::::block_number()); }: _(approve_origin, bounty_id, curator_lookup, fee) + approve_bounty_with_curator { + setup_pot_account::(); + let (caller, curator, fee, value, reason) = setup_bounty::(0, T::MaximumReasonLength::get()); + let curator_lookup = T::Lookup::unlookup(curator.clone()); + Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; + let bounty_id = BountyCount::::get() - 1; + let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + Treasury::::on_initialize(BlockNumberFor::::zero()); + }: _(approve_origin, bounty_id, curator_lookup, fee) + verify { + assert_last_event::( + Event::CuratorProposed { bounty_id, curator }.into() + ); + } + // Worst case when curator is inactive and any sender unassigns the curator. unassign_curator { setup_pot_account::(); diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index 9d5931f4a608e..6e0bfa6084e1a 100644 --- a/substrate/frame/bounties/src/lib.rs +++ b/substrate/frame/bounties/src/lib.rs @@ -73,6 +73,8 @@ //! - `approve_bounty` - Accept a specific treasury amount to be earmarked for a predefined body of //! work. //! - `propose_curator` - Assign an account to a bounty as candidate curator. +//! - `approve_bounty_with_curator` - Accept a specific treasury amount for a predefined body of +//! work with assigned candidate curator account. //! - `accept_curator` - Accept a bounty assignment from the Council, setting a curator deposit. //! - `extend_bounty_expiry` - Extend the expiry block number of the bounty and stay active. //! - `award_bounty` - Close and pay out the specified amount for the completed work. @@ -174,6 +176,11 @@ pub enum BountyStatus { /// When the bounty can be claimed. unlock_at: BlockNumber, }, + /// The bounty is approved with curator assigned. + ApprovedWithCurator { + /// The assigned curator of this bounty. + curator: AccountId, + }, } /// The child bounty manager. @@ -471,6 +478,15 @@ pub mod pallet { // No curator to unassign at this point. return Err(Error::::UnexpectedStatus.into()) }, + BountyStatus::ApprovedWithCurator { ref curator } => { + // Bounty not yet funded, but bounty was approved with curator. + // `RejectOrigin` or curator himself can unassign from this bounty. + ensure!(maybe_sender.map_or(true, |sender| sender == *curator), BadOrigin); + // This state can only be while the bounty is not yet funded so we return + // bounty to the `Approved` state without curator + bounty.status = BountyStatus::Approved; + return Ok(()); + }, BountyStatus::CuratorProposed { ref curator } => { // A curator has been proposed, but not accepted yet. // Either `RejectOrigin` or the proposed curator can unassign the curator. @@ -723,7 +739,7 @@ pub mod pallet { Some(>::WeightInfo::close_bounty_proposed()).into() ) }, - BountyStatus::Approved => { + BountyStatus::Approved | BountyStatus::ApprovedWithCurator { .. } => { // For weight reasons, we don't allow a council to cancel in this phase. // We ask for them to wait until it is funded before they can cancel. return Err(Error::::UnexpectedStatus.into()) @@ -804,6 +820,52 @@ pub mod pallet { Self::deposit_event(Event::::BountyExtended { index: bounty_id }); Ok(()) } + + /// Approve bountry and propose a curator simultaneously. + /// This call is a shortcut to calling `approve_bounty` and `propose_curator` separately. + /// + /// May only be called from `T::SpendOrigin`. + /// + /// - `bounty_id`: Bounty ID to approve. + /// - `curator`: The curator account whom will manage this bounty. + /// - `fee`: The curator fee. + /// + /// ## Complexity + /// - O(1). + #[pallet::call_index(9)] + #[pallet::weight(>::WeightInfo::approve_bounty_with_curator())] + pub fn approve_bounty_with_curator( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + curator: AccountIdLookupOf, + #[pallet::compact] fee: BalanceOf, + ) -> DispatchResult { + let max_amount = T::SpendOrigin::ensure_origin(origin)?; + let curator = T::Lookup::lookup(curator)?; + Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { + // approve bounty + let bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; + ensure!( + bounty.value <= max_amount, + pallet_treasury::Error::::InsufficientPermission + ); + ensure!(bounty.status == BountyStatus::Proposed, Error::::UnexpectedStatus); + ensure!(fee < bounty.value, Error::::InvalidFee); + + BountyApprovals::::try_append(bounty_id) + .map_err(|()| Error::::TooManyQueued)?; + + bounty.status = BountyStatus::ApprovedWithCurator { curator: curator.clone() }; + bounty.fee = fee; + + Ok(()) + })?; + + Self::deposit_event(Event::::BountyApproved { index: bounty_id }); + Self::deposit_event(Event::::CuratorProposed { bounty_id, curator }); + + Ok(()) + } } #[pallet::hooks] @@ -946,7 +1008,13 @@ impl, I: 'static> pallet_treasury::SpendFunds for Pallet BountiesEvent { - System::events() +fn last_events(n: usize) -> Vec> { + let mut res = System::events() .into_iter() - .map(|r| r.event) - .filter_map(|e| if let RuntimeEvent::Bounties(inner) = e { Some(inner) } else { None }) - .last() - .unwrap() + .rev() + .filter_map( + |e| if let RuntimeEvent::Bounties(inner) = e.event { Some(inner) } else { None }, + ) + .take(n) + .collect::>(); + res.reverse(); + res +} + +fn last_event() -> BountiesEvent { + last_events(1).into_iter().next().unwrap() +} + +fn expect_events(e: Vec>) { + assert_eq!(last_events(e.len()), e); } #[test] @@ -1181,3 +1193,212 @@ fn propose_curator_instance1_insufficient_spend_limit_errors() { ); }); } + +#[test] +fn approve_bounty_with_curator_works() { + ExtBuilder::default().build_and_execute(|| { + let fee = 10; + let curator = 4; + 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_noop!( + Bounties::approve_bounty_with_curator(RuntimeOrigin::signed(1), 0, curator, 10), + BadOrigin + ); + + SpendLimit::set(1); + assert_noop!( + Bounties::approve_bounty_with_curator(RuntimeOrigin::root(), 0, curator, 10), + TreasuryError::InsufficientPermission + ); + SpendLimit::set(u64::MAX); + + assert_noop!( + Bounties::approve_bounty_with_curator(RuntimeOrigin::root(), 0, curator, 51), + Error::::InvalidFee + ); + + assert_eq!(pallet_bounties::BountyApprovals::::get().len(), 0); + assert_ok!(Bounties::approve_bounty_with_curator(RuntimeOrigin::root(), 0, curator, 10)); + assert_eq!(pallet_bounties::BountyApprovals::::get().len(), 1); + + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::ApprovedWithCurator { curator }, + } + ); + + expect_events(vec![ + BountiesEvent::BountyApproved { index: 0 }, + BountiesEvent::CuratorProposed { bounty_id: 0, curator }, + ]); + + assert_noop!( + Bounties::approve_bounty_with_curator(RuntimeOrigin::root(), 0, curator, 10), + Error::::UnexpectedStatus + ); + + System::set_block_number(2); + >::on_initialize(2); + assert_eq!(pallet_bounties::BountyApprovals::::get().len(), 0); + + expect_events(vec![BountiesEvent::BountyBecameActive { index: 0 }]); + + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::CuratorProposed { curator }, + } + ); + + assert_noop!( + Bounties::accept_curator(RuntimeOrigin::signed(curator), 0), + pallet_balances::Error::::InsufficientBalance + ); + Balances::make_free_balance_be(&curator, 6); + assert_ok!(Bounties::accept_curator(RuntimeOrigin::signed(curator), 0)); + + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 5, + value: 50, + bond: 85, + status: BountyStatus::Active { curator, update_due: 22 }, + } + ); + + assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(curator), 0, 5)); + System::set_block_number(5); + >::on_initialize(5); + assert_ok!(Bounties::claim_bounty(RuntimeOrigin::signed(curator), 0)); + assert_eq!( + last_event(), + BountiesEvent::BountyClaimed { index: 0, payout: 40, beneficiary: 5 } + ); + assert_eq!(Balances::free_balance(5), 40); // 50 - 10 + }); +} + +#[test] +fn approve_bounty_with_curator_early_unassign_works() { + ExtBuilder::default().build_and_execute(|| { + let fee = 10; + let curator = 4; + 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_with_curator(RuntimeOrigin::root(), 0, curator, 10)); + + // unassign curator while bounty is not yet funded + assert_ok!(Bounties::unassign_curator(RuntimeOrigin::root(), 0)); + + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::Approved, + } + ); + + assert_eq!(last_event(), BountiesEvent::CuratorUnassigned { bounty_id: 0 }); + + System::set_block_number(2); + >::on_initialize(2); + assert_eq!(last_event(), BountiesEvent::BountyBecameActive { index: 0 }); + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::Funded, + } + ); + + // assign curator again through separate process + let new_fee = 15; + let new_curator = 5; + assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, new_curator, new_fee)); + + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee: new_fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::CuratorProposed { curator: new_curator }, + } + ); + assert_eq!( + last_event(), + BountiesEvent::CuratorProposed { bounty_id: 0, curator: new_curator } + ); + }); +} + +#[test] +fn approve_bounty_with_curator_proposed_unassign_works() { + ExtBuilder::default().build_and_execute(|| { + let fee = 10; + let curator = 4; + 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_with_curator(RuntimeOrigin::root(), 0, curator, 10)); + + System::set_block_number(2); + >::on_initialize(2); + + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::CuratorProposed { curator }, + } + ); + + assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(curator), 0)); + assert_eq!( + pallet_bounties::Bounties::::get(0).unwrap(), + Bounty { + proposer: 0, + fee, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::Funded, + } + ); + assert_eq!(last_event(), BountiesEvent::CuratorUnassigned { bounty_id: 0 }); + }); +} diff --git a/substrate/frame/bounties/src/weights.rs b/substrate/frame/bounties/src/weights.rs index c9f551ec9bb24..7230fa4a6a77f 100644 --- a/substrate/frame/bounties/src/weights.rs +++ b/substrate/frame/bounties/src/weights.rs @@ -18,27 +18,25 @@ //! Autogenerated weights for `pallet_bounties` //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0 -//! DATE: 2024-04-08, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2024-10-22, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `runner-anb7yjbi-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz` +//! HOSTNAME: `runner-augrssgt-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz` //! WASM-EXECUTION: `Compiled`, CHAIN: `Some("dev")`, DB CACHE: `1024` // Executed Command: -// ./target/production/substrate-node +// target/production/substrate-node // benchmark // pallet -// --chain=dev // --steps=50 // --repeat=20 -// --pallet=pallet_bounties -// --no-storage-info -// --no-median-slopes -// --no-min-squares // --extrinsic=* // --wasm-execution=compiled // --heap-pages=4096 -// --output=./substrate/frame/bounties/src/weights.rs +// --json-file=/builds/parity/mirrors/polkadot-sdk/.git/.artifacts/bench.json +// --pallet=pallet_bounties +// --chain=dev // --header=./substrate/HEADER-APACHE2 +// --output=./substrate/frame/bounties/src/weights.rs // --template=./substrate/.maintain/frame-weight-template.hbs #![cfg_attr(rustfmt, rustfmt_skip)] @@ -54,6 +52,7 @@ pub trait WeightInfo { fn propose_bounty(d: u32, ) -> Weight; fn approve_bounty() -> Weight; fn propose_curator() -> Weight; + fn approve_bounty_with_curator() -> Weight; fn unassign_curator() -> Weight; fn accept_curator() -> Weight; fn award_bounty() -> Weight; @@ -78,12 +77,12 @@ impl WeightInfo for SubstrateWeight { /// The range of component `d` is `[0, 300]`. fn propose_bounty(d: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `309` + // Measured: `343` // Estimated: `3593` - // Minimum execution time: 25_206_000 picoseconds. - Weight::from_parts(26_925_800, 3593) - // Standard Error: 239 - .saturating_add(Weight::from_parts(501, 0).saturating_mul(d.into())) + // Minimum execution time: 31_284_000 picoseconds. + Weight::from_parts(33_484_932, 3593) + // Standard Error: 299 + .saturating_add(Weight::from_parts(1_444, 0).saturating_mul(d.into())) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(4_u64)) } @@ -93,10 +92,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `Bounties::BountyApprovals` (`max_values`: Some(1), `max_size`: Some(402), added: 897, mode: `MaxEncodedLen`) fn approve_bounty() -> Weight { // Proof Size summary in bytes: - // Measured: `401` + // Measured: `434` // Estimated: `3642` - // Minimum execution time: 13_150_000 picoseconds. - Weight::from_parts(13_708_000, 3642) + // Minimum execution time: 17_656_000 picoseconds. + Weight::from_parts(18_501_000, 3642) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } @@ -104,23 +103,36 @@ impl WeightInfo for SubstrateWeight { /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) fn propose_curator() -> Weight { // Proof Size summary in bytes: - // Measured: `421` + // Measured: `454` // Estimated: `3642` - // Minimum execution time: 12_277_000 picoseconds. - Weight::from_parts(12_769_000, 3642) + // Minimum execution time: 15_416_000 picoseconds. + Weight::from_parts(16_463_000, 3642) .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) } /// Storage: `Bounties::Bounties` (r:1 w:1) /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) + /// Storage: `Bounties::BountyApprovals` (r:1 w:1) + /// Proof: `Bounties::BountyApprovals` (`max_values`: Some(1), `max_size`: Some(402), added: 897, mode: `MaxEncodedLen`) + fn approve_bounty_with_curator() -> Weight { + // Proof Size summary in bytes: + // Measured: `454` + // Estimated: `3642` + // Minimum execution time: 21_802_000 picoseconds. + Weight::from_parts(22_884_000, 3642) + .saturating_add(T::DbWeight::get().reads(2_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } + /// Storage: `Bounties::Bounties` (r:1 w:1) + /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) /// Storage: `System::Account` (r:1 w:1) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) fn unassign_curator() -> Weight { // Proof Size summary in bytes: - // Measured: `597` + // Measured: `630` // Estimated: `3642` - // Minimum execution time: 29_041_000 picoseconds. - Weight::from_parts(29_979_000, 3642) + // Minimum execution time: 45_843_000 picoseconds. + Weight::from_parts(47_558_000, 3642) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } @@ -130,10 +142,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) fn accept_curator() -> Weight { // Proof Size summary in bytes: - // Measured: `593` + // Measured: `626` // Estimated: `3642` - // Minimum execution time: 27_936_000 picoseconds. - Weight::from_parts(28_925_000, 3642) + // Minimum execution time: 35_720_000 picoseconds. + Weight::from_parts(37_034_000, 3642) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } @@ -143,10 +155,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `ChildBounties::ParentChildBounties` (`max_values`: None, `max_size`: Some(16), added: 2491, mode: `MaxEncodedLen`) fn award_bounty() -> Weight { // Proof Size summary in bytes: - // Measured: `605` + // Measured: `638` // Estimated: `3642` - // Minimum execution time: 16_759_000 picoseconds. - Weight::from_parts(17_699_000, 3642) + // Minimum execution time: 23_318_000 picoseconds. + Weight::from_parts(24_491_000, 3642) .saturating_add(T::DbWeight::get().reads(2_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) } @@ -160,10 +172,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `Bounties::BountyDescriptions` (`max_values`: None, `max_size`: Some(314), added: 2789, mode: `MaxEncodedLen`) fn claim_bounty() -> Weight { // Proof Size summary in bytes: - // Measured: `969` + // Measured: `1069` // Estimated: `8799` - // Minimum execution time: 112_056_000 picoseconds. - Weight::from_parts(114_275_000, 8799) + // Minimum execution time: 127_643_000 picoseconds. + Weight::from_parts(130_844_000, 8799) .saturating_add(T::DbWeight::get().reads(5_u64)) .saturating_add(T::DbWeight::get().writes(6_u64)) } @@ -177,10 +189,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `Bounties::BountyDescriptions` (`max_values`: None, `max_size`: Some(314), added: 2789, mode: `MaxEncodedLen`) fn close_bounty_proposed() -> Weight { // Proof Size summary in bytes: - // Measured: `649` + // Measured: `683` // Estimated: `3642` - // Minimum execution time: 32_625_000 picoseconds. - Weight::from_parts(33_719_000, 3642) + // Minimum execution time: 49_963_000 picoseconds. + Weight::from_parts(51_484_000, 3642) .saturating_add(T::DbWeight::get().reads(3_u64)) .saturating_add(T::DbWeight::get().writes(3_u64)) } @@ -194,10 +206,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `Bounties::BountyDescriptions` (`max_values`: None, `max_size`: Some(314), added: 2789, mode: `MaxEncodedLen`) fn close_bounty_active() -> Weight { // Proof Size summary in bytes: - // Measured: `885` + // Measured: `985` // Estimated: `6196` - // Minimum execution time: 76_895_000 picoseconds. - Weight::from_parts(79_161_000, 6196) + // Minimum execution time: 89_310_000 picoseconds. + Weight::from_parts(92_223_000, 6196) .saturating_add(T::DbWeight::get().reads(4_u64)) .saturating_add(T::DbWeight::get().writes(4_u64)) } @@ -205,10 +217,10 @@ impl WeightInfo for SubstrateWeight { /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) fn extend_bounty_expiry() -> Weight { // Proof Size summary in bytes: - // Measured: `457` + // Measured: `490` // Estimated: `3642` - // Minimum execution time: 12_635_000 picoseconds. - Weight::from_parts(13_423_000, 3642) + // Minimum execution time: 16_630_000 picoseconds. + Weight::from_parts(17_171_000, 3642) .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(1_u64)) } @@ -221,12 +233,12 @@ impl WeightInfo for SubstrateWeight { /// The range of component `b` is `[0, 100]`. fn spend_funds(b: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `37 + b * (297 ±0)` + // Measured: `205 + b * (297 ±0)` // Estimated: `1887 + b * (5206 ±0)` - // Minimum execution time: 2_840_000 picoseconds. - Weight::from_parts(6_076_743, 1887) - // Standard Error: 18_569 - .saturating_add(Weight::from_parts(34_771_846, 0).saturating_mul(b.into())) + // Minimum execution time: 4_334_000 picoseconds. + Weight::from_parts(1_256_424, 1887) + // Standard Error: 42_406 + .saturating_add(Weight::from_parts(36_979_844, 0).saturating_mul(b.into())) .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().reads((3_u64).saturating_mul(b.into()))) .saturating_add(T::DbWeight::get().writes(1_u64)) @@ -248,12 +260,12 @@ impl WeightInfo for () { /// The range of component `d` is `[0, 300]`. fn propose_bounty(d: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `309` + // Measured: `343` // Estimated: `3593` - // Minimum execution time: 25_206_000 picoseconds. - Weight::from_parts(26_925_800, 3593) - // Standard Error: 239 - .saturating_add(Weight::from_parts(501, 0).saturating_mul(d.into())) + // Minimum execution time: 31_284_000 picoseconds. + Weight::from_parts(33_484_932, 3593) + // Standard Error: 299 + .saturating_add(Weight::from_parts(1_444, 0).saturating_mul(d.into())) .saturating_add(RocksDbWeight::get().reads(2_u64)) .saturating_add(RocksDbWeight::get().writes(4_u64)) } @@ -263,10 +275,10 @@ impl WeightInfo for () { /// Proof: `Bounties::BountyApprovals` (`max_values`: Some(1), `max_size`: Some(402), added: 897, mode: `MaxEncodedLen`) fn approve_bounty() -> Weight { // Proof Size summary in bytes: - // Measured: `401` + // Measured: `434` // Estimated: `3642` - // Minimum execution time: 13_150_000 picoseconds. - Weight::from_parts(13_708_000, 3642) + // Minimum execution time: 17_656_000 picoseconds. + Weight::from_parts(18_501_000, 3642) .saturating_add(RocksDbWeight::get().reads(2_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } @@ -274,23 +286,36 @@ impl WeightInfo for () { /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) fn propose_curator() -> Weight { // Proof Size summary in bytes: - // Measured: `421` + // Measured: `454` // Estimated: `3642` - // Minimum execution time: 12_277_000 picoseconds. - Weight::from_parts(12_769_000, 3642) + // Minimum execution time: 15_416_000 picoseconds. + Weight::from_parts(16_463_000, 3642) .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().writes(1_u64)) } /// Storage: `Bounties::Bounties` (r:1 w:1) /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) + /// Storage: `Bounties::BountyApprovals` (r:1 w:1) + /// Proof: `Bounties::BountyApprovals` (`max_values`: Some(1), `max_size`: Some(402), added: 897, mode: `MaxEncodedLen`) + fn approve_bounty_with_curator() -> Weight { + // Proof Size summary in bytes: + // Measured: `454` + // Estimated: `3642` + // Minimum execution time: 21_802_000 picoseconds. + Weight::from_parts(22_884_000, 3642) + .saturating_add(RocksDbWeight::get().reads(2_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) + } + /// Storage: `Bounties::Bounties` (r:1 w:1) + /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) /// Storage: `System::Account` (r:1 w:1) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) fn unassign_curator() -> Weight { // Proof Size summary in bytes: - // Measured: `597` + // Measured: `630` // Estimated: `3642` - // Minimum execution time: 29_041_000 picoseconds. - Weight::from_parts(29_979_000, 3642) + // Minimum execution time: 45_843_000 picoseconds. + Weight::from_parts(47_558_000, 3642) .saturating_add(RocksDbWeight::get().reads(2_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } @@ -300,10 +325,10 @@ impl WeightInfo for () { /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) fn accept_curator() -> Weight { // Proof Size summary in bytes: - // Measured: `593` + // Measured: `626` // Estimated: `3642` - // Minimum execution time: 27_936_000 picoseconds. - Weight::from_parts(28_925_000, 3642) + // Minimum execution time: 35_720_000 picoseconds. + Weight::from_parts(37_034_000, 3642) .saturating_add(RocksDbWeight::get().reads(2_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } @@ -313,10 +338,10 @@ impl WeightInfo for () { /// Proof: `ChildBounties::ParentChildBounties` (`max_values`: None, `max_size`: Some(16), added: 2491, mode: `MaxEncodedLen`) fn award_bounty() -> Weight { // Proof Size summary in bytes: - // Measured: `605` + // Measured: `638` // Estimated: `3642` - // Minimum execution time: 16_759_000 picoseconds. - Weight::from_parts(17_699_000, 3642) + // Minimum execution time: 23_318_000 picoseconds. + Weight::from_parts(24_491_000, 3642) .saturating_add(RocksDbWeight::get().reads(2_u64)) .saturating_add(RocksDbWeight::get().writes(1_u64)) } @@ -330,10 +355,10 @@ impl WeightInfo for () { /// Proof: `Bounties::BountyDescriptions` (`max_values`: None, `max_size`: Some(314), added: 2789, mode: `MaxEncodedLen`) fn claim_bounty() -> Weight { // Proof Size summary in bytes: - // Measured: `969` + // Measured: `1069` // Estimated: `8799` - // Minimum execution time: 112_056_000 picoseconds. - Weight::from_parts(114_275_000, 8799) + // Minimum execution time: 127_643_000 picoseconds. + Weight::from_parts(130_844_000, 8799) .saturating_add(RocksDbWeight::get().reads(5_u64)) .saturating_add(RocksDbWeight::get().writes(6_u64)) } @@ -347,10 +372,10 @@ impl WeightInfo for () { /// Proof: `Bounties::BountyDescriptions` (`max_values`: None, `max_size`: Some(314), added: 2789, mode: `MaxEncodedLen`) fn close_bounty_proposed() -> Weight { // Proof Size summary in bytes: - // Measured: `649` + // Measured: `683` // Estimated: `3642` - // Minimum execution time: 32_625_000 picoseconds. - Weight::from_parts(33_719_000, 3642) + // Minimum execution time: 49_963_000 picoseconds. + Weight::from_parts(51_484_000, 3642) .saturating_add(RocksDbWeight::get().reads(3_u64)) .saturating_add(RocksDbWeight::get().writes(3_u64)) } @@ -364,10 +389,10 @@ impl WeightInfo for () { /// Proof: `Bounties::BountyDescriptions` (`max_values`: None, `max_size`: Some(314), added: 2789, mode: `MaxEncodedLen`) fn close_bounty_active() -> Weight { // Proof Size summary in bytes: - // Measured: `885` + // Measured: `985` // Estimated: `6196` - // Minimum execution time: 76_895_000 picoseconds. - Weight::from_parts(79_161_000, 6196) + // Minimum execution time: 89_310_000 picoseconds. + Weight::from_parts(92_223_000, 6196) .saturating_add(RocksDbWeight::get().reads(4_u64)) .saturating_add(RocksDbWeight::get().writes(4_u64)) } @@ -375,10 +400,10 @@ impl WeightInfo for () { /// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`) fn extend_bounty_expiry() -> Weight { // Proof Size summary in bytes: - // Measured: `457` + // Measured: `490` // Estimated: `3642` - // Minimum execution time: 12_635_000 picoseconds. - Weight::from_parts(13_423_000, 3642) + // Minimum execution time: 16_630_000 picoseconds. + Weight::from_parts(17_171_000, 3642) .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().writes(1_u64)) } @@ -391,12 +416,12 @@ impl WeightInfo for () { /// The range of component `b` is `[0, 100]`. fn spend_funds(b: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `37 + b * (297 ±0)` + // Measured: `205 + b * (297 ±0)` // Estimated: `1887 + b * (5206 ±0)` - // Minimum execution time: 2_840_000 picoseconds. - Weight::from_parts(6_076_743, 1887) - // Standard Error: 18_569 - .saturating_add(Weight::from_parts(34_771_846, 0).saturating_mul(b.into())) + // Minimum execution time: 4_334_000 picoseconds. + Weight::from_parts(1_256_424, 1887) + // Standard Error: 42_406 + .saturating_add(Weight::from_parts(36_979_844, 0).saturating_mul(b.into())) .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().reads((3_u64).saturating_mul(b.into()))) .saturating_add(RocksDbWeight::get().writes(1_u64)) From 762f824cb3adf23c72a6645f575057bfb543850b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 5 Nov 2024 11:22:28 +0200 Subject: [PATCH 02/11] authority-discovery: Populate DHT records with public listen addresses (#6298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR's main goal is to add public listen addresses to the DHT authorities records. This change improves the discoverability of validators that did not provide the `--public-addresses` flag. This PR populates the authority DHT records with public listen addresses if any. The change effectively ensures that addresses are added to the DHT record in following order: 1. Public addresses provided by CLI `--public-addresses` 2. Maximum of 4 public (global) listen addresses (if any) 3. Any external addresses discovered from the network (ie from `/identify` protocol) While at it, this PR adds the following constraints on the number of addresses: - Total number of addresses cached is bounded at 16 (increased from 10). - A maximum number of 32 addresses are published to DHT records (previously unbounded). - A maximum of 4 global listen addresses are utilized. This PR also removes the following warning: `WARNING: No public address specified, validator node may not be reachable.` ### Next Steps - [ ] deploy and monitor in versi network Closes: https://github.com/paritytech/polkadot-sdk/issues/6280 Part of: https://github.com/paritytech/polkadot-sdk/issues/5266 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile Co-authored-by: Dmitry Markin Co-authored-by: Bastian Köcher --- prdoc/pr_6298.prdoc | 25 +++ .../client/authority-discovery/src/worker.rs | 176 +++++++++++++----- .../authority-discovery/src/worker/tests.rs | 4 +- substrate/client/cli/src/commands/run_cmd.rs | 12 +- 4 files changed, 153 insertions(+), 64 deletions(-) create mode 100644 prdoc/pr_6298.prdoc diff --git a/prdoc/pr_6298.prdoc b/prdoc/pr_6298.prdoc new file mode 100644 index 0000000000000..fa8d73b119432 --- /dev/null +++ b/prdoc/pr_6298.prdoc @@ -0,0 +1,25 @@ +title: Populate authority DHT records with public listen addresses + +doc: + - audience: [ Node Dev, Node Operator ] + description: | + This PR populates the authority DHT records with public listen addresses if any. + The change effectively ensures that addresses are added to the DHT record in the + following order: + 1. Public addresses provided by CLI `--public-addresses` + 2. Maximum of 4 public (global) listen addresses (if any) + 3. Any external addresses discovered from the network (ie from `/identify` protocol) + + While at it, this PR adds the following constraints on the number of addresses: + - Total number of addresses cached is bounded at 16 (increased from 10). + - A maximum number of 32 addresses are published to DHT records (previously unbounded). + - A maximum of 4 global listen addresses are utilized. + + This PR replaces the following warning: + `WARNING: No public address specified, validator node may not be reachable.` + with a more descriptive one originated from the authority-discovery + mechanism itself: `No public addresses configured and no global listen addresses found`. + +crates: + - name: sc-authority-discovery + bump: patch diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 6f4fbac77e059..9319fbe6321e7 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -71,7 +71,13 @@ pub mod tests; const LOG_TARGET: &str = "sub-authority-discovery"; /// Maximum number of addresses cached per authority. Additional addresses are discarded. -const MAX_ADDRESSES_PER_AUTHORITY: usize = 10; +const MAX_ADDRESSES_PER_AUTHORITY: usize = 16; + +/// Maximum number of global listen addresses published by the node. +const MAX_GLOBAL_LISTEN_ADDRESSES: usize = 4; + +/// Maximum number of addresses to publish in a single record. +const MAX_ADDRESSES_TO_PUBLISH: usize = 32; /// Maximum number of in-flight DHT lookups at any given point in time. const MAX_IN_FLIGHT_LOOKUPS: usize = 8; @@ -174,6 +180,9 @@ pub struct Worker { metrics: Option, + /// Flag to ensure the warning about missing public addresses is only printed once. + warn_public_addresses: bool, + role: Role, phantom: PhantomData, @@ -271,20 +280,7 @@ where config .public_addresses .into_iter() - .map(|mut address| { - if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { - if peer_id != *local_peer_id.as_ref() { - error!( - target: LOG_TARGET, - "Discarding invalid local peer ID in public address {address}.", - ); - } - // Always discard `/p2p/...` protocol for proper address comparison (local - // peer id will be added before publishing). - address.pop(); - } - address - }) + .map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id)) .collect() }; @@ -309,6 +305,7 @@ where addr_cache, role, metrics, + warn_public_addresses: false, phantom: PhantomData, last_known_records: HashMap::new(), } @@ -373,47 +370,70 @@ where } } - fn addresses_to_publish(&self) -> impl Iterator { + fn addresses_to_publish(&mut self) -> impl Iterator { let local_peer_id = self.network.local_peer_id(); let publish_non_global_ips = self.publish_non_global_ips; + + // Checks that the address is global. + let address_is_global = |address: &Multiaddr| { + address.iter().all(|protocol| match protocol { + // The `ip_network` library is used because its `is_global()` method is stable, + // while `is_global()` in the standard library currently isn't. + multiaddr::Protocol::Ip4(ip) => IpNetwork::from(ip).is_global(), + multiaddr::Protocol::Ip6(ip) => IpNetwork::from(ip).is_global(), + _ => true, + }) + }; + + // These are the addresses the node is listening for incoming connections, + // as reported by installed protocols (tcp / websocket etc). + // + // We double check the address is global. In other words, we double check the node + // is not running behind a NAT. + // Note: we do this regardless of the `publish_non_global_ips` setting, since the + // node discovers many external addresses via the identify protocol. + let mut global_listen_addresses = self + .network + .listen_addresses() + .into_iter() + .filter_map(|address| { + address_is_global(&address) + .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) + }) + .take(MAX_GLOBAL_LISTEN_ADDRESSES) + .peekable(); + + // Similar to listen addresses that takes into consideration `publish_non_global_ips`. + let mut external_addresses = self + .network + .external_addresses() + .into_iter() + .filter_map(|address| { + (publish_non_global_ips || address_is_global(&address)) + .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) + }) + .peekable(); + + let has_global_listen_addresses = global_listen_addresses.peek().is_some(); + trace!( + target: LOG_TARGET, + "Node has public addresses: {}, global listen addresses: {}, external addresses: {}", + !self.public_addresses.is_empty(), + has_global_listen_addresses, + external_addresses.peek().is_some(), + ); + + let mut seen_addresses = HashSet::new(); + let addresses = self .public_addresses .clone() .into_iter() - .chain(self.network.external_addresses().into_iter().filter_map(|mut address| { - // Make sure the reported external address does not contain `/p2p/...` protocol. - if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { - if peer_id != *local_peer_id.as_ref() { - error!( - target: LOG_TARGET, - "Network returned external address '{address}' with peer id \ - not matching the local peer id '{local_peer_id}'.", - ); - debug_assert!(false); - } - address.pop(); - } - - if self.public_addresses.contains(&address) { - // Already added above. - None - } else { - Some(address) - } - })) - .filter(move |address| { - if publish_non_global_ips { - return true - } - - address.iter().all(|protocol| match protocol { - // The `ip_network` library is used because its `is_global()` method is stable, - // while `is_global()` in the standard library currently isn't. - multiaddr::Protocol::Ip4(ip) if !IpNetwork::from(ip).is_global() => false, - multiaddr::Protocol::Ip6(ip) if !IpNetwork::from(ip).is_global() => false, - _ => true, - }) - }) + .chain(global_listen_addresses) + .chain(external_addresses) + // Deduplicate addresses. + .filter(|address| seen_addresses.insert(address.clone())) + .take(MAX_ADDRESSES_TO_PUBLISH) .collect::>(); if !addresses.is_empty() { @@ -421,6 +441,21 @@ where target: LOG_TARGET, "Publishing authority DHT record peer_id='{local_peer_id}' with addresses='{addresses:?}'", ); + + if !self.warn_public_addresses && + self.public_addresses.is_empty() && + !has_global_listen_addresses + { + self.warn_public_addresses = true; + + error!( + target: LOG_TARGET, + "No public addresses configured and no global listen addresses found. \ + Authority DHT record may contain unreachable addresses. \ + Consider setting `--public-addr` to the public IP address of this node. \ + This will become a hard requirement in future versions for authorities." + ); + } } // The address must include the local peer id. @@ -437,7 +472,8 @@ where let key_store = match &self.role { Role::PublishAndDiscover(key_store) => key_store, Role::Discover => return Ok(()), - }; + } + .clone(); let addresses = serialize_addresses(self.addresses_to_publish()); if addresses.is_empty() { @@ -946,6 +982,44 @@ where } } +/// Removes the `/p2p/..` from the address if it is present. +#[derive(Debug, Clone, PartialEq, Eq)] +enum AddressType { + /// The address is specified as a public address via the CLI. + PublicAddress(Multiaddr), + /// The address is a global listen address. + GlobalListenAddress(Multiaddr), + /// The address is discovered via the network (ie /identify protocol). + ExternalAddress(Multiaddr), +} + +impl AddressType { + /// Removes the `/p2p/..` from the address if it is present. + /// + /// In case the peer id in the address does not match the local peer id, an error is logged for + /// `ExternalAddress` and `GlobalListenAddress`. + fn without_p2p(self, local_peer_id: PeerId) -> Multiaddr { + // Get the address and the source str for logging. + let (mut address, source) = match self { + AddressType::PublicAddress(address) => (address, "public address"), + AddressType::GlobalListenAddress(address) => (address, "global listen address"), + AddressType::ExternalAddress(address) => (address, "external address"), + }; + + if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { + if peer_id != *local_peer_id.as_ref() { + error!( + target: LOG_TARGET, + "Network returned '{source}' '{address}' with peer id \ + not matching the local peer id '{local_peer_id}'.", + ); + } + address.pop(); + } + address + } +} + /// NetworkProvider provides [`Worker`] with all necessary hooks into the /// underlying Substrate networking. Using this trait abstraction instead of /// `sc_network::NetworkService` directly is necessary to unit test [`Worker`]. diff --git a/substrate/client/authority-discovery/src/worker/tests.rs b/substrate/client/authority-discovery/src/worker/tests.rs index d71a85db8b813..8018b5ea492dc 100644 --- a/substrate/client/authority-discovery/src/worker/tests.rs +++ b/substrate/client/authority-discovery/src/worker/tests.rs @@ -1018,7 +1018,7 @@ fn addresses_to_publish_adds_p2p() { )); let (_to_worker, from_service) = mpsc::channel(0); - let worker = Worker::new( + let mut worker = Worker::new( from_service, Arc::new(TestApi { authorities: vec![] }), network.clone(), @@ -1056,7 +1056,7 @@ fn addresses_to_publish_respects_existing_p2p_protocol() { }); let (_to_worker, from_service) = mpsc::channel(0); - let worker = Worker::new( + let mut worker = Worker::new( from_service, Arc::new(TestApi { authorities: vec![] }), network.clone(), diff --git a/substrate/client/cli/src/commands/run_cmd.rs b/substrate/client/cli/src/commands/run_cmd.rs index f91d18aca7497..f79e5b558e37e 100644 --- a/substrate/client/cli/src/commands/run_cmd.rs +++ b/substrate/client/cli/src/commands/run_cmd.rs @@ -201,17 +201,7 @@ impl CliConfiguration for RunCmd { } fn network_params(&self) -> Option<&NetworkParams> { - let network_params = &self.network_params; - let is_authority = self.role(self.is_dev().ok()?).ok()?.is_authority(); - if is_authority && network_params.public_addr.is_empty() { - eprintln!( - "WARNING: No public address specified, validator node may not be reachable. - Consider setting `--public-addr` to the public IP address of this node. - This will become a hard requirement in future versions." - ); - } - - Some(network_params) + Some(&self.network_params) } fn keystore_params(&self) -> Option<&KeystoreParams> { From 6969be36057efd54d2f11e239bc88ff4b9a65418 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 5 Nov 2024 11:47:22 +0200 Subject: [PATCH 03/11] snowbridge: allow account conversion for Ethereum accounts (#6221) Replace `GlobalConsensusEthereumConvertsFor` with `EthereumLocationsConverterFor` that allows `Location` to `AccountId` conversion for the Ethereum network root as before, but also for Ethereum contracts and accounts. The new converter only matches explicit `parents: 2` Ethereum locations, meaning it should be used only on/by parachains. --- .../primitives/router/src/inbound/mod.rs | 15 ++++++++---- .../primitives/router/src/inbound/tests.rs | 24 +++++++++++++++---- .../bridge-hub-rococo/src/tests/snowbridge.rs | 5 ++-- .../src/tests/snowbridge.rs | 6 ++--- .../assets/asset-hub-rococo/src/xcm_config.rs | 4 ++-- .../asset-hub-westend/src/xcm_config.rs | 4 ++-- prdoc/pr_6221.prdoc | 10 ++++++++ 7 files changed, 48 insertions(+), 20 deletions(-) create mode 100644 prdoc/pr_6221.prdoc diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index a9324ac42470b..357f77f831cc5 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -253,7 +253,7 @@ where let bridge_location = Location::new(2, GlobalConsensus(network)); - let owner = GlobalConsensusEthereumConvertsFor::<[u8; 32]>::from_chain_id(&chain_id); + let owner = EthereumLocationsConverterFor::<[u8; 32]>::from_chain_id(&chain_id); let asset_id = Self::convert_token_address(network, token); let create_call_index: [u8; 2] = CreateAssetCall::get(); let inbound_queue_pallet_index = InboundQueuePalletInstance::get(); @@ -454,22 +454,27 @@ where } } -pub struct GlobalConsensusEthereumConvertsFor(PhantomData); -impl ConvertLocation for GlobalConsensusEthereumConvertsFor +pub struct EthereumLocationsConverterFor(PhantomData); +impl ConvertLocation for EthereumLocationsConverterFor where AccountId: From<[u8; 32]> + Clone, { fn convert_location(location: &Location) -> Option { match location.unpack() { - (_, [GlobalConsensus(Ethereum { chain_id })]) => + (2, [GlobalConsensus(Ethereum { chain_id })]) => Some(Self::from_chain_id(chain_id).into()), + (2, [GlobalConsensus(Ethereum { chain_id }), AccountKey20 { network: _, key }]) => + Some(Self::from_chain_id_with_key(chain_id, *key).into()), _ => None, } } } -impl GlobalConsensusEthereumConvertsFor { +impl EthereumLocationsConverterFor { pub fn from_chain_id(chain_id: &u64) -> [u8; 32] { (b"ethereum-chain", chain_id).using_encoded(blake2_256) } + pub fn from_chain_id_with_key(chain_id: &u64, key: [u8; 20]) -> [u8; 32] { + (b"ethereum-chain", chain_id, key).using_encoded(blake2_256) + } } diff --git a/bridges/snowbridge/primitives/router/src/inbound/tests.rs b/bridges/snowbridge/primitives/router/src/inbound/tests.rs index e0e90e516be16..786aa594f653e 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/tests.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/tests.rs @@ -1,4 +1,4 @@ -use super::GlobalConsensusEthereumConvertsFor; +use super::EthereumLocationsConverterFor; use crate::inbound::CallIndex; use frame_support::{assert_ok, parameter_types}; use hex_literal::hex; @@ -17,14 +17,28 @@ parameter_types! { } #[test] -fn test_contract_location_with_network_converts_successfully() { +fn test_ethereum_network_converts_successfully() { let expected_account: [u8; 32] = hex!("ce796ae65569a670d0c1cc1ac12515a3ce21b5fbf729d63d7b289baad070139d"); let contract_location = Location::new(2, [GlobalConsensus(NETWORK)]); let account = - GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&contract_location) - .unwrap(); + EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&contract_location).unwrap(); + + assert_eq!(account, expected_account); +} + +#[test] +fn test_contract_location_with_network_converts_successfully() { + let expected_account: [u8; 32] = + hex!("9038d35aba0e78e072d29b2d65be9df5bb4d7d94b4609c9cf98ea8e66e544052"); + let contract_location = Location::new( + 2, + [GlobalConsensus(NETWORK), AccountKey20 { network: None, key: [123u8; 20] }], + ); + + let account = + EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&contract_location).unwrap(); assert_eq!(account, expected_account); } @@ -34,7 +48,7 @@ fn test_contract_location_with_incorrect_location_fails_convert() { let contract_location = Location::new(2, [GlobalConsensus(Polkadot), Parachain(1000)]); assert_eq!( - GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&contract_location), + EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&contract_location), None, ); } diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs index d91a0c6895f98..912e74af69810 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs @@ -25,7 +25,7 @@ use snowbridge_pallet_inbound_queue_fixtures::{ }; use snowbridge_pallet_system; use snowbridge_router_primitives::inbound::{ - Command, Destination, GlobalConsensusEthereumConvertsFor, MessageV1, VersionedMessage, + Command, Destination, EthereumLocationsConverterFor, MessageV1, VersionedMessage, }; use sp_core::H256; use sp_runtime::{DispatchError::Token, TokenError::FundsUnavailable}; @@ -318,8 +318,7 @@ fn send_token_from_ethereum_to_penpal() { // Fund ethereum sovereign on AssetHub let ethereum_sovereign: AccountId = - GlobalConsensusEthereumConvertsFor::::convert_location(&origin_location) - .unwrap(); + EthereumLocationsConverterFor::::convert_location(&origin_location).unwrap(); AssetHubRococo::fund_accounts(vec![(ethereum_sovereign.clone(), INITIAL_FUND)]); // Create asset on the Penpal parachain. diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge.rs index 4e9dd5a77dd7b..f1c27aedf1640 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge.rs @@ -22,7 +22,7 @@ use hex_literal::hex; use rococo_westend_system_emulated_network::asset_hub_westend_emulated_chain::genesis::AssetHubWestendAssetOwner; use snowbridge_core::{outbound::OperatingMode, AssetMetadata, TokenIdOf}; use snowbridge_router_primitives::inbound::{ - Command, Destination, GlobalConsensusEthereumConvertsFor, MessageV1, VersionedMessage, + Command, Destination, EthereumLocationsConverterFor, MessageV1, VersionedMessage, }; use sp_core::H256; use testnet_parachains_constants::westend::snowbridge::EthereumNetwork; @@ -297,7 +297,7 @@ fn transfer_relay_token() { let expected_token_id = TokenIdOf::convert_location(&expected_asset_id).unwrap(); let ethereum_sovereign: AccountId = - GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(&Location::new( + EthereumLocationsConverterFor::<[u8; 32]>::convert_location(&Location::new( 2, [GlobalConsensus(EthereumNetwork::get())], )) @@ -445,7 +445,7 @@ fn transfer_ah_token() { let ethereum_destination = Location::new(2, [GlobalConsensus(Ethereum { chain_id: CHAIN_ID })]); let ethereum_sovereign: AccountId = - GlobalConsensusEthereumConvertsFor::<[u8; 32]>::convert_location(ðereum_destination) + EthereumLocationsConverterFor::<[u8; 32]>::convert_location(ðereum_destination) .unwrap() .into(); AssetHubWestend::fund_accounts(vec![(ethereum_sovereign.clone(), INITIAL_FUND)]); diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs index 637c5900f7da4..56310959aa80b 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs @@ -42,7 +42,7 @@ use parachains_common::{ }; use polkadot_parachain_primitives::primitives::Sibling; use polkadot_runtime_common::xcm_sender::ExponentialPrice; -use snowbridge_router_primitives::inbound::GlobalConsensusEthereumConvertsFor; +use snowbridge_router_primitives::inbound::EthereumLocationsConverterFor; use sp_runtime::traits::{AccountIdConversion, ConvertInto, TryConvertInto}; use testnet_parachains_constants::rococo::snowbridge::{ EthereumNetwork, INBOUND_QUEUE_PALLET_INDEX, @@ -104,7 +104,7 @@ pub type LocationToAccountId = ( GlobalConsensusParachainConvertsFor, // Ethereum contract sovereign account. // (Used to get convert ethereum contract locations to sovereign account) - GlobalConsensusEthereumConvertsFor, + EthereumLocationsConverterFor, ); /// Means for transacting the native currency on this chain. diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs index b554d00508be1..f16dbcc025191 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs @@ -42,7 +42,7 @@ use parachains_common::{ }; use polkadot_parachain_primitives::primitives::Sibling; use polkadot_runtime_common::xcm_sender::ExponentialPrice; -use snowbridge_router_primitives::inbound::GlobalConsensusEthereumConvertsFor; +use snowbridge_router_primitives::inbound::EthereumLocationsConverterFor; use sp_runtime::traits::{AccountIdConversion, ConvertInto, TryConvertInto}; use xcm::latest::prelude::*; use xcm_builder::{ @@ -100,7 +100,7 @@ pub type LocationToAccountId = ( GlobalConsensusParachainConvertsFor, // Ethereum contract sovereign account. // (Used to get convert ethereum contract locations to sovereign account) - GlobalConsensusEthereumConvertsFor, + EthereumLocationsConverterFor, ); /// Means for transacting the native currency on this chain. diff --git a/prdoc/pr_6221.prdoc b/prdoc/pr_6221.prdoc new file mode 100644 index 0000000000000..57c81b322f929 --- /dev/null +++ b/prdoc/pr_6221.prdoc @@ -0,0 +1,10 @@ +title: "snowbridge: allow account conversion for Ethereum accounts" + +doc: + - audience: Runtime Dev + description: | + Replaced `GlobalConsensusEthereumConvertsFor` with `EthereumLocationsConverterFor` that allows `Location` + to `AccountId` conversion for the Ethereum network root as before, but also for Ethereum contracts and accounts. +crates: + - name: snowbridge-router-primitives + bump: major From ec61396ec8315989f6df33253ccff5445e99f848 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 5 Nov 2024 12:25:51 +0200 Subject: [PATCH 04/11] Remove leftover references of Wococo (#6361) Remove references of now defunct Wococo network. The XCM `NetworkId::Wococo` will also be removed with [XCMv5 PR](https://github.com/paritytech/polkadot-sdk/pull/4826) --- bridges/primitives/messages/src/lane.rs | 4 ++-- .../parachains/runtimes/assets/common/src/matching.rs | 10 +++++----- polkadot/cli/src/cli.rs | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/bridges/primitives/messages/src/lane.rs b/bridges/primitives/messages/src/lane.rs index 0f14ce93e1142..75237a44d5385 100644 --- a/bridges/primitives/messages/src/lane.rs +++ b/bridges/primitives/messages/src/lane.rs @@ -108,8 +108,8 @@ impl TypeId for LegacyLaneId { /// concatenation (separated by some binary data). I.e.: /// /// ```nocompile -/// let endpoint1 = X2(GlobalConsensus(NetworkId::Rococo), Parachain(42)); -/// let endpoint2 = X2(GlobalConsensus(NetworkId::Wococo), Parachain(777)); +/// let endpoint1 = X2(GlobalConsensus(NetworkId::Polkadot), Parachain(42)); +/// let endpoint2 = X2(GlobalConsensus(NetworkId::Kusama), Parachain(777)); /// /// let final_lane_key = if endpoint1 < endpoint2 { /// (endpoint1, VALUES_SEPARATOR, endpoint2) diff --git a/cumulus/parachains/runtimes/assets/common/src/matching.rs b/cumulus/parachains/runtimes/assets/common/src/matching.rs index 9ac2056a67f43..5a452f6eed1b0 100644 --- a/cumulus/parachains/runtimes/assets/common/src/matching.rs +++ b/cumulus/parachains/runtimes/assets/common/src/matching.rs @@ -149,7 +149,7 @@ mod tests { parameter_types! { pub UniversalLocation: InteriorLocation = [GlobalConsensus(Rococo), Parachain(1000)].into(); - pub ExpectedNetworkId: NetworkId = Wococo; + pub ExpectedNetworkId: NetworkId = Westend; } #[test] @@ -158,13 +158,13 @@ mod tests { let asset: Location = ( Parent, Parent, - GlobalConsensus(Wococo), + GlobalConsensus(Westend), Parachain(1000), PalletInstance(1), GeneralIndex(1), ) .into(); - let origin: Location = (Parent, Parent, GlobalConsensus(Wococo), Parachain(1000)).into(); + let origin: Location = (Parent, Parent, GlobalConsensus(Westend), Parachain(1000)).into(); assert!(FromNetwork::::contains(&asset, &origin)); // asset and origin from local consensus fails @@ -195,14 +195,14 @@ mod tests { GeneralIndex(1), ) .into(); - let origin: Location = (Parent, Parent, GlobalConsensus(Wococo), Parachain(1000)).into(); + let origin: Location = (Parent, Parent, GlobalConsensus(Westend), Parachain(1000)).into(); assert!(!FromNetwork::::contains(&asset, &origin)); // origin from different consensus fails let asset: Location = ( Parent, Parent, - GlobalConsensus(Wococo), + GlobalConsensus(Westend), Parachain(1000), PalletInstance(1), GeneralIndex(1), diff --git a/polkadot/cli/src/cli.rs b/polkadot/cli/src/cli.rs index eb67a39563421..777bb9c606712 100644 --- a/polkadot/cli/src/cli.rs +++ b/polkadot/cli/src/cli.rs @@ -79,7 +79,7 @@ pub struct RunCmd { /// Disable the BEEFY gadget. /// - /// Currently enabled by default on 'Rococo', 'Wococo' and 'Versi'. + /// Currently enabled by default. #[arg(long)] pub no_beefy: bool, From 3c6ea86a2645b0d944101806e9cb1c1aeb4eb643 Mon Sep 17 00:00:00 2001 From: clangenb <37865735+clangenb@users.noreply.github.com> Date: Tue, 5 Nov 2024 12:08:37 +0100 Subject: [PATCH 05/11] migrate pallet-remarks to v2 bench syntax (#6291) Part of: * #6202 --------- Co-authored-by: Oliver Tale-Yazdi Co-authored-by: GitHub Action Co-authored-by: Giuseppe Re --- prdoc/pr_6291.prdoc | 9 ++++++++ substrate/frame/remark/src/benchmarking.rs | 25 ++++++++++++++++------ 2 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 prdoc/pr_6291.prdoc diff --git a/prdoc/pr_6291.prdoc b/prdoc/pr_6291.prdoc new file mode 100644 index 0000000000000..73053c9d47bd0 --- /dev/null +++ b/prdoc/pr_6291.prdoc @@ -0,0 +1,9 @@ +title: migrate pallet-remarks to v2 bench syntax +doc: +- audience: Runtime Dev + description: |- + Part of: + * #6202 +crates: +- name: pallet-remark + bump: patch diff --git a/substrate/frame/remark/src/benchmarking.rs b/substrate/frame/remark/src/benchmarking.rs index 15b72b4748dd4..41d49c3b930bb 100644 --- a/substrate/frame/remark/src/benchmarking.rs +++ b/substrate/frame/remark/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; use alloc::vec; -use frame_benchmarking::v1::{benchmarks, whitelisted_caller}; +use frame_benchmarking::v2::*; use frame_system::{EventRecord, Pallet as System, RawOrigin}; #[cfg(test)] @@ -34,13 +34,24 @@ fn assert_last_event(generic_event: ::RuntimeEvent) { assert_eq!(event, &system_event); } -benchmarks! { - store { - let l in 1 .. 1024*1024; +#[benchmarks] +mod benchmarks { + use super::*; + + #[benchmark] + fn store(l: Linear<1, { 1024 * 1024 }>) { let caller: T::AccountId = whitelisted_caller(); - }: _(RawOrigin::Signed(caller.clone()), vec![0u8; l as usize]) - verify { - assert_last_event::(Event::Stored { sender: caller, content_hash: sp_io::hashing::blake2_256(&vec![0u8; l as usize]).into() }.into()); + + #[extrinsic_call] + _(RawOrigin::Signed(caller.clone()), vec![0u8; l as usize]); + + assert_last_event::( + Event::Stored { + sender: caller, + content_hash: sp_io::hashing::blake2_256(&vec![0u8; l as usize]).into(), + } + .into(), + ); } impl_benchmark_test_suite!(Remark, crate::mock::new_test_ext(), crate::mock::Test); From be26d6288351f57f3b94e3fb7ee24c875fca1e32 Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Tue, 5 Nov 2024 12:20:09 +0100 Subject: [PATCH 06/11] [pallet-staking] Additional check for virtual stakers (#5985) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/paritytech/polkadot-sdk/issues/5791. This is not strictly necessary but serves as a defensive check. The staking pallet exposes [apis](https://paritytech.github.io/polkadot-sdk/master/sp_staking/trait.StakingUnchecked.html#tymethod.virtual_bond) that other runtime pallets (pallet-delegated-staking) can use to create virtual stakers. However, there’s no way for pallet-staking to ensure that the staker is truly keyless. If the caller (this is a trusted caller so this would only happen due to a bug) registers an account with a private key as a virtual_staker, these accounts could later interact directly with pallet-staking dispatchables (such as [bond_extra](https://paritytech.github.io/polkadot-sdk/master/pallet_staking/dispatchables/fn.bond_extra.html)) and bypass any locking mechanism. The check above ensures this scenario can never occur by performing an integrity check. --------- Co-authored-by: Bastian Köcher Co-authored-by: command-bot <> --- substrate/frame/staking/src/pallet/impls.rs | 10 +++++++++- substrate/primitives/panic-handler/src/lib.rs | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 6c4fe8140e8ef..6499037411401 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1881,8 +1881,12 @@ impl StakingInterface for Pallet { } /// Whether `who` is a virtual staker whose funds are managed by another pallet. + /// + /// There is an assumption that, this account is keyless and managed by another pallet in the + /// runtime. Hence, it can never sign its own transactions. fn is_virtual_staker(who: &T::AccountId) -> bool { - VirtualStakers::::contains_key(who) + frame_system::Pallet::::account_nonce(who).is_zero() && + VirtualStakers::::contains_key(who) } fn slash_reward_fraction() -> Perbill { @@ -2103,6 +2107,10 @@ impl Pallet { Ledger::::get(stash.clone()).unwrap().stash == stash, "ledger corrupted for virtual staker" ); + ensure!( + frame_system::Pallet::::account_nonce(&stash).is_zero(), + "virtual stakers are keyless and should not have any nonce" + ); let reward_destination = >::get(stash.clone()).unwrap(); if let RewardDestination::Account(payee) = reward_destination { ensure!( diff --git a/substrate/primitives/panic-handler/src/lib.rs b/substrate/primitives/panic-handler/src/lib.rs index 81ccaaee828e4..c4a7eb8dc67c7 100644 --- a/substrate/primitives/panic-handler/src/lib.rs +++ b/substrate/primitives/panic-handler/src/lib.rs @@ -30,7 +30,7 @@ use std::{ cell::Cell, io::{self, Write}, marker::PhantomData, - panic::{self, PanicHookInfo}, + panic::{self, PanicInfo}, sync::LazyLock, thread, }; @@ -149,7 +149,7 @@ fn strip_control_codes(input: &str) -> std::borrow::Cow { } /// Function being called when a panic happens. -fn panic_hook(info: &PanicHookInfo, report_url: &str, version: &str) { +fn panic_hook(info: &PanicInfo, report_url: &str, version: &str) { let location = info.location(); let file = location.as_ref().map(|l| l.file()).unwrap_or(""); let line = location.as_ref().map(|l| l.line()).unwrap_or(0); From 94389a939c222a866affa0bb9b66541e36614810 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Tue, 5 Nov 2024 13:56:41 +0200 Subject: [PATCH 07/11] litep2p: Update litep2p to v0.8.0 (#6353) This PR updates litep2p to the latest release. - `KademliaEvent::PutRecordSucess` is renamed to fix word typo - `KademliaEvent::GetProvidersSuccess` and `KademliaEvent::IncomingProvider` are needed for bootnodes on DHT work and will be utilized later ### Added - kad: Providers part 8: unit, e2e, and `libp2p` conformance tests ([#258](https://github.com/paritytech/litep2p/pull/258)) - kad: Providers part 7: better types and public API, public addresses & known providers ([#246](https://github.com/paritytech/litep2p/pull/246)) - kad: Providers part 6: stop providing ([#245](https://github.com/paritytech/litep2p/pull/245)) - kad: Providers part 5: `GET_PROVIDERS` query ([#236](https://github.com/paritytech/litep2p/pull/236)) - kad: Providers part 4: refresh local providers ([#235](https://github.com/paritytech/litep2p/pull/235)) - kad: Providers part 3: publish provider records (start providing) ([#234](https://github.com/paritytech/litep2p/pull/234)) ### Changed - transport_service: Improve connection stability by downgrading connections on substream inactivity ([#260](https://github.com/paritytech/litep2p/pull/260)) - transport: Abort canceled dial attempts for TCP, WebSocket and Quic ([#255](https://github.com/paritytech/litep2p/pull/255)) - kad/executor: Add timeout for writting frames ([#277](https://github.com/paritytech/litep2p/pull/277)) - kad: Avoid cloning the `KademliaMessage` and use reference for `RoutingTable::closest` ([#233](https://github.com/paritytech/litep2p/pull/233)) - peer_state: Robust state machine transitions ([#251](https://github.com/paritytech/litep2p/pull/251)) - address_store: Improve address tracking and add eviction algorithm ([#250](https://github.com/paritytech/litep2p/pull/250)) - kad: Remove unused serde cfg ([#262](https://github.com/paritytech/litep2p/pull/262)) - req-resp: Refactor to move functionality to dedicated methods ([#244](https://github.com/paritytech/litep2p/pull/244)) - transport_service: Improve logs and move code from tokio::select macro ([#254](https://github.com/paritytech/litep2p/pull/254)) ### Fixed - tcp/websocket/quic: Fix cancel memory leak ([#272](https://github.com/paritytech/litep2p/pull/272)) - transport: Fix pending dials memory leak ([#271](https://github.com/paritytech/litep2p/pull/271)) - ping: Fix memory leak of unremoved `pending_opens` ([#274](https://github.com/paritytech/litep2p/pull/274)) - identify: Fix memory leak of unused `pending_opens` ([#273](https://github.com/paritytech/litep2p/pull/273)) - kad: Fix not retrieving local records ([#221](https://github.com/paritytech/litep2p/pull/221)) See release changelog for more details: https://github.com/paritytech/litep2p/releases/tag/v0.8.0 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile Co-authored-by: Dmitry Markin --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- prdoc/pr_6353.prdoc | 10 ++++++++++ substrate/client/network/src/litep2p/discovery.rs | 5 ++++- 4 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 prdoc/pr_6353.prdoc diff --git a/Cargo.lock b/Cargo.lock index 59b6d92bde5d5..a870c0bd9d4c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9400,9 +9400,9 @@ dependencies = [ [[package]] name = "litep2p" -version = "0.7.0" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d4ab2528b02b6dbbc3e6ec4b55ccde885647c622a315b7da45081ed2dfe4b813" +checksum = "7286b1971f85d1d60be40ef49e81c1f3b5a0d8b83cfa02ab53591cdacae22901" dependencies = [ "async-trait", "bs58", diff --git a/Cargo.toml b/Cargo.toml index f3042a8a3bdfa..5f4d78ef32134 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -848,7 +848,7 @@ linked-hash-map = { version = "0.5.4" } linked_hash_set = { version = "0.1.4" } linregress = { version = "0.5.1" } lite-json = { version = "0.2.0", default-features = false } -litep2p = { version = "0.7.0", features = ["websocket"] } +litep2p = { version = "0.8.0", features = ["websocket"] } log = { version = "0.4.22", default-features = false } macro_magic = { version = "0.5.1" } maplit = { version = "1.0.2" } diff --git a/prdoc/pr_6353.prdoc b/prdoc/pr_6353.prdoc new file mode 100644 index 0000000000000..8a5a152628a0b --- /dev/null +++ b/prdoc/pr_6353.prdoc @@ -0,0 +1,10 @@ +title: Update litep2p network backend to version 0.8.0 + +doc: + - audience: [ Node Dev, Node Operator ] + description: | + Release 0.8.0 of litep2p includes several improvements and memory leak fixes enhancing the stability and performance of the litep2p network backend. + +crates: + - name: sc-network + bump: patch diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 13cf8a4c6ee0c..7b2e713dffd23 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -553,7 +553,7 @@ impl Stream for Discovery { return Poll::Ready(Some(DiscoveryEvent::GetRecordSuccess { query_id, records })); }, - Poll::Ready(Some(KademliaEvent::PutRecordSucess { query_id, key: _ })) => + Poll::Ready(Some(KademliaEvent::PutRecordSuccess { query_id, key: _ })) => return Poll::Ready(Some(DiscoveryEvent::PutRecordSuccess { query_id })), Poll::Ready(Some(KademliaEvent::QueryFailed { query_id })) => { match this.find_node_query_id == Some(query_id) { @@ -576,6 +576,9 @@ impl Stream for Discovery { return Poll::Ready(Some(DiscoveryEvent::IncomingRecord { record })) }, + // Content provider events are ignored for now. + Poll::Ready(Some(KademliaEvent::GetProvidersSuccess { .. })) | + Poll::Ready(Some(KademliaEvent::IncomingProvider { .. })) => {}, } match Pin::new(&mut this.identify_event_stream).poll_next(cx) { From 74ec1ee226ace087748f38dfeffc869cd5534ac8 Mon Sep 17 00:00:00 2001 From: Alin Dima Date: Tue, 5 Nov 2024 14:05:31 +0200 Subject: [PATCH 08/11] refactor and harden check_core_index (#6217) Resolves https://github.com/paritytech/polkadot-sdk/issues/6179 --- .../consensus/aura/src/collators/lookahead.rs | 8 +- .../slot_based/block_builder_task.rs | 6 +- cumulus/pallets/parachain-system/src/lib.rs | 4 +- cumulus/primitives/core/src/lib.rs | 4 - .../node/collation-generation/src/error.rs | 4 +- polkadot/node/collation-generation/src/lib.rs | 2 +- .../src/inclusion_emulator/mod.rs | 15 +- polkadot/primitives/src/vstaging/mod.rs | 206 ++++++++++-------- .../runtime/parachains/src/inclusion/mod.rs | 38 +--- .../parachains/src/paras_inherent/tests.rs | 7 +- prdoc/pr_6217.prdoc | 24 ++ 11 files changed, 162 insertions(+), 156 deletions(-) create mode 100644 prdoc/pr_6217.prdoc diff --git a/cumulus/client/consensus/aura/src/collators/lookahead.rs b/cumulus/client/consensus/aura/src/collators/lookahead.rs index 8ac43fbd116e5..2dbcf5eb58e96 100644 --- a/cumulus/client/consensus/aura/src/collators/lookahead.rs +++ b/cumulus/client/consensus/aura/src/collators/lookahead.rs @@ -36,17 +36,15 @@ use cumulus_client_collator::service::ServiceInterface as CollatorServiceInterfa use cumulus_client_consensus_common::{self as consensus_common, ParachainBlockImportMarker}; use cumulus_client_consensus_proposer::ProposerInterface; use cumulus_primitives_aura::AuraUnincludedSegmentApi; -use cumulus_primitives_core::{ - ClaimQueueOffset, CollectCollationInfo, PersistedValidationData, DEFAULT_CLAIM_QUEUE_OFFSET, -}; +use cumulus_primitives_core::{ClaimQueueOffset, CollectCollationInfo, PersistedValidationData}; use cumulus_relay_chain_interface::RelayChainInterface; use polkadot_node_primitives::{PoV, SubmitCollationParams}; use polkadot_node_subsystem::messages::CollationGenerationMessage; use polkadot_overseer::Handle as OverseerHandle; use polkadot_primitives::{ - BlockNumber as RBlockNumber, CollatorPair, Hash as RHash, HeadData, Id as ParaId, - OccupiedCoreAssumption, + vstaging::DEFAULT_CLAIM_QUEUE_OFFSET, BlockNumber as RBlockNumber, CollatorPair, Hash as RHash, + HeadData, Id as ParaId, OccupiedCoreAssumption, }; use futures::prelude::*; diff --git a/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs b/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs index e75b52aeebd34..4251512307046 100644 --- a/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs +++ b/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs @@ -20,13 +20,11 @@ use cumulus_client_collator::service::ServiceInterface as CollatorServiceInterfa use cumulus_client_consensus_common::{self as consensus_common, ParachainBlockImportMarker}; use cumulus_client_consensus_proposer::ProposerInterface; use cumulus_primitives_aura::AuraUnincludedSegmentApi; -use cumulus_primitives_core::{ - GetCoreSelectorApi, PersistedValidationData, DEFAULT_CLAIM_QUEUE_OFFSET, -}; +use cumulus_primitives_core::{GetCoreSelectorApi, PersistedValidationData}; use cumulus_relay_chain_interface::RelayChainInterface; use polkadot_primitives::{ - vstaging::{ClaimQueueOffset, CoreSelector}, + vstaging::{ClaimQueueOffset, CoreSelector, DEFAULT_CLAIM_QUEUE_OFFSET}, BlockId, CoreIndex, Hash as RelayHash, Header as RelayHeader, Id as ParaId, OccupiedCoreAssumption, }; diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 98989a852b8dc..39fc8321a072e 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -35,12 +35,12 @@ use core::{cmp, marker::PhantomData}; use cumulus_primitives_core::{ relay_chain::{ self, - vstaging::{ClaimQueueOffset, CoreSelector}, + vstaging::{ClaimQueueOffset, CoreSelector, DEFAULT_CLAIM_QUEUE_OFFSET}, }, AbridgedHostConfiguration, ChannelInfo, ChannelStatus, CollationInfo, GetChannelInfo, InboundDownwardMessage, InboundHrmpMessage, ListChannelInfos, MessageSendError, OutboundHrmpMessage, ParaId, PersistedValidationData, UpwardMessage, UpwardMessageSender, - XcmpMessageHandler, XcmpMessageSource, DEFAULT_CLAIM_QUEUE_OFFSET, + XcmpMessageHandler, XcmpMessageSource, }; use cumulus_primitives_parachain_inherent::{MessageQueueChain, ParachainInherentData}; use frame_support::{ diff --git a/cumulus/primitives/core/src/lib.rs b/cumulus/primitives/core/src/lib.rs index dfb574ef33018..f88e663db19ee 100644 --- a/cumulus/primitives/core/src/lib.rs +++ b/cumulus/primitives/core/src/lib.rs @@ -333,10 +333,6 @@ pub mod rpsr_digest { } } -/// The default claim queue offset to be used if it's not configured/accessible in the parachain -/// runtime -pub const DEFAULT_CLAIM_QUEUE_OFFSET: u8 = 0; - /// Information about a collation. /// /// This was used in version 1 of the [`CollectCollationInfo`] runtime api. diff --git a/polkadot/node/collation-generation/src/error.rs b/polkadot/node/collation-generation/src/error.rs index 68902f58579af..2599026080df7 100644 --- a/polkadot/node/collation-generation/src/error.rs +++ b/polkadot/node/collation-generation/src/error.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use polkadot_primitives::vstaging::CandidateReceiptError; +use polkadot_primitives::vstaging::CommittedCandidateReceiptError; use thiserror::Error; #[derive(Debug, Error)] @@ -34,7 +34,7 @@ pub enum Error { #[error("Collation submitted before initialization")] SubmittedBeforeInit, #[error("V2 core index check failed: {0}")] - CandidateReceiptCheck(CandidateReceiptError), + CandidateReceiptCheck(CommittedCandidateReceiptError), #[error("PoV size {0} exceeded maximum size of {1}")] POVSizeExceeded(usize, usize), } diff --git a/polkadot/node/collation-generation/src/lib.rs b/polkadot/node/collation-generation/src/lib.rs index 9e975acf10b85..b371017a8289a 100644 --- a/polkadot/node/collation-generation/src/lib.rs +++ b/polkadot/node/collation-generation/src/lib.rs @@ -554,7 +554,7 @@ async fn construct_and_distribute_receipt( ccr.to_plain() } else { - if commitments.selected_core().is_some() { + if commitments.core_selector().map_err(Error::CandidateReceiptCheck)?.is_some() { gum::warn!( target: LOG_TARGET, ?pov_hash, diff --git a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs index 20ca62d41f5b9..48d3f27b1fa6d 100644 --- a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs +++ b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs @@ -82,9 +82,9 @@ /// in practice at most once every few weeks. use polkadot_node_subsystem::messages::HypotheticalCandidate; use polkadot_primitives::{ - async_backing::Constraints as PrimitiveConstraints, BlockNumber, CandidateCommitments, - CandidateHash, Hash, HeadData, Id as ParaId, PersistedValidationData, UpgradeRestriction, - ValidationCodeHash, + async_backing::Constraints as PrimitiveConstraints, vstaging::skip_ump_signals, BlockNumber, + CandidateCommitments, CandidateHash, Hash, HeadData, Id as ParaId, PersistedValidationData, + UpgradeRestriction, ValidationCodeHash, }; use std::{collections::HashMap, sync::Arc}; @@ -601,13 +601,8 @@ impl Fragment { persisted_validation_data: &PersistedValidationData, ) -> Result { // Filter UMP signals and the separator. - let upward_messages = if let Some(separator_index) = - commitments.upward_messages.iter().position(|message| message.is_empty()) - { - commitments.upward_messages.split_at(separator_index).0 - } else { - &commitments.upward_messages - }; + let upward_messages = + skip_ump_signals(commitments.upward_messages.iter()).collect::>(); let ump_messages_sent = upward_messages.len(); let ump_bytes_sent = upward_messages.iter().map(|msg| msg.len()).sum(); diff --git a/polkadot/primitives/src/vstaging/mod.rs b/polkadot/primitives/src/vstaging/mod.rs index ca9c3e1bebad0..271f78efe0901 100644 --- a/polkadot/primitives/src/vstaging/mod.rs +++ b/polkadot/primitives/src/vstaging/mod.rs @@ -39,6 +39,10 @@ use sp_staking::SessionIndex; /// Async backing primitives pub mod async_backing; +/// The default claim queue offset to be used if it's not configured/accessible in the parachain +/// runtime +pub const DEFAULT_CLAIM_QUEUE_OFFSET: u8 = 0; + /// A type representing the version of the candidate descriptor and internal version number. #[derive(PartialEq, Eq, Encode, Decode, Clone, TypeInfo, RuntimeDebug, Copy)] #[cfg_attr(feature = "std", derive(Hash))] @@ -430,49 +434,45 @@ pub enum UMPSignal { /// Separator between `XCM` and `UMPSignal`. pub const UMP_SEPARATOR: Vec = vec![]; -impl CandidateCommitments { - /// Returns the core selector and claim queue offset the candidate has committed to, if any. - pub fn selected_core(&self) -> Option<(CoreSelector, ClaimQueueOffset)> { - // We need at least 2 messages for the separator and core selector - if self.upward_messages.len() < 2 { - return None - } - - let separator_pos = - self.upward_messages.iter().rposition(|message| message == &UMP_SEPARATOR)?; - - // Use first commitment - let message = self.upward_messages.get(separator_pos + 1)?; +/// Utility function for skipping the ump signals. +pub fn skip_ump_signals<'a>( + upward_messages: impl Iterator>, +) -> impl Iterator> { + upward_messages.take_while(|message| *message != &UMP_SEPARATOR) +} - match UMPSignal::decode(&mut message.as_slice()).ok()? { - UMPSignal::SelectCore(core_selector, cq_offset) => Some((core_selector, cq_offset)), - } - } +impl CandidateCommitments { + /// Returns the core selector and claim queue offset determined by `UMPSignal::SelectCore` + /// commitment, if present. + pub fn core_selector( + &self, + ) -> Result, CommittedCandidateReceiptError> { + let mut signals_iter = + self.upward_messages.iter().skip_while(|message| *message != &UMP_SEPARATOR); + + if signals_iter.next().is_some() { + let Some(core_selector_message) = signals_iter.next() else { return Ok(None) }; + // We should have exactly one signal beyond the separator + if signals_iter.next().is_some() { + return Err(CommittedCandidateReceiptError::TooManyUMPSignals) + } - /// Returns the core index determined by `UMPSignal::SelectCore` commitment - /// and `assigned_cores`. - /// - /// Returns `None` if there is no `UMPSignal::SelectCore` commitment or - /// assigned cores is empty. - /// - /// `assigned_cores` must be a sorted vec of all core indices assigned to a parachain. - pub fn committed_core_index(&self, assigned_cores: &[&CoreIndex]) -> Option { - if assigned_cores.is_empty() { - return None + match UMPSignal::decode(&mut core_selector_message.as_slice()) + .map_err(|_| CommittedCandidateReceiptError::UmpSignalDecode)? + { + UMPSignal::SelectCore(core_index_selector, cq_offset) => + Ok(Some((core_index_selector, cq_offset))), + } + } else { + Ok(None) } - - self.selected_core().and_then(|(core_selector, _cq_offset)| { - let core_index = - **assigned_cores.get(core_selector.0 as usize % assigned_cores.len())?; - Some(core_index) - }) } } -/// CandidateReceipt construction errors. +/// CommittedCandidateReceiptError construction errors. #[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, RuntimeDebug)] #[cfg_attr(feature = "std", derive(thiserror::Error))] -pub enum CandidateReceiptError { +pub enum CommittedCandidateReceiptError { /// The specified core index is invalid. #[cfg_attr(feature = "std", error("The specified core index is invalid"))] InvalidCoreIndex, @@ -485,6 +485,9 @@ pub enum CandidateReceiptError { /// The core selector or claim queue offset is invalid. #[cfg_attr(feature = "std", error("The core selector or claim queue offset is invalid"))] InvalidSelectedCore, + #[cfg_attr(feature = "std", error("Could not decode UMP signal"))] + /// Could not decode UMP signal. + UmpSignalDecode, /// The parachain is not assigned to any core at specified claim queue offset. #[cfg_attr( feature = "std", @@ -498,6 +501,10 @@ pub enum CandidateReceiptError { /// Unknown version. #[cfg_attr(feature = "std", error("Unknown internal version"))] UnknownVersion(InternalVersion), + /// The allowed number of `UMPSignal` messages in the queue was exceeded. + /// Currenly only one such message is allowed. + #[cfg_attr(feature = "std", error("Too many UMP signals"))] + TooManyUMPSignals, } macro_rules! impl_getter { @@ -590,57 +597,63 @@ impl CandidateDescriptorV2 { impl CommittedCandidateReceiptV2 { /// Checks if descriptor core index is equal to the committed core index. - /// Input `cores_per_para` is a claim queue snapshot stored as a mapping - /// between `ParaId` and the cores assigned per depth. + /// Input `cores_per_para` is a claim queue snapshot at the candidate's relay parent, stored as + /// a mapping between `ParaId` and the cores assigned per depth. pub fn check_core_index( &self, cores_per_para: &TransposedClaimQueue, - ) -> Result<(), CandidateReceiptError> { + ) -> Result<(), CommittedCandidateReceiptError> { match self.descriptor.version() { // Don't check v1 descriptors. CandidateDescriptorVersion::V1 => return Ok(()), CandidateDescriptorVersion::V2 => {}, CandidateDescriptorVersion::Unknown => - return Err(CandidateReceiptError::UnknownVersion(self.descriptor.version)), + return Err(CommittedCandidateReceiptError::UnknownVersion(self.descriptor.version)), } - if cores_per_para.is_empty() { - return Err(CandidateReceiptError::NoAssignment) - } - - let (offset, core_selected) = - if let Some((_core_selector, cq_offset)) = self.commitments.selected_core() { - (cq_offset.0, true) - } else { - // If no core has been selected then we use offset 0 (top of claim queue) - (0, false) - }; + let (maybe_core_index_selector, cq_offset) = self.commitments.core_selector()?.map_or_else( + || (None, ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET)), + |(sel, off)| (Some(sel), off), + ); - // The cores assigned to the parachain at above computed offset. let assigned_cores = cores_per_para .get(&self.descriptor.para_id()) - .ok_or(CandidateReceiptError::NoAssignment)? - .get(&offset) - .ok_or(CandidateReceiptError::NoAssignment)? - .into_iter() - .collect::>(); - - let core_index = if core_selected { - self.commitments - .committed_core_index(assigned_cores.as_slice()) - .ok_or(CandidateReceiptError::NoAssignment)? - } else { - // `SelectCore` commitment is mandatory for elastic scaling parachains. - if assigned_cores.len() > 1 { - return Err(CandidateReceiptError::NoCoreSelected) - } + .ok_or(CommittedCandidateReceiptError::NoAssignment)? + .get(&cq_offset.0) + .ok_or(CommittedCandidateReceiptError::NoAssignment)?; - **assigned_cores.get(0).ok_or(CandidateReceiptError::NoAssignment)? - }; + if assigned_cores.is_empty() { + return Err(CommittedCandidateReceiptError::NoAssignment) + } let descriptor_core_index = CoreIndex(self.descriptor.core_index as u32); + + let core_index_selector = if let Some(core_index_selector) = maybe_core_index_selector { + // We have a committed core selector, we can use it. + core_index_selector + } else if assigned_cores.len() > 1 { + // We got more than one assigned core and no core selector. Special care is needed. + if !assigned_cores.contains(&descriptor_core_index) { + // core index in the descriptor is not assigned to the para. Error. + return Err(CommittedCandidateReceiptError::InvalidCoreIndex) + } else { + // the descriptor core index is indeed assigned to the para. This is the most we can + // check for now + return Ok(()) + } + } else { + // No core selector but there's only one assigned core, use it. + CoreSelector(0) + }; + + let core_index = assigned_cores + .iter() + .nth(core_index_selector.0 as usize % assigned_cores.len()) + .ok_or(CommittedCandidateReceiptError::InvalidSelectedCore) + .copied()?; + if core_index != descriptor_core_index { - return Err(CandidateReceiptError::CoreIndexMismatch) + return Err(CommittedCandidateReceiptError::CoreIndexMismatch) } Ok(()) @@ -1037,7 +1050,7 @@ mod tests { assert_eq!(new_ccr.descriptor.version(), CandidateDescriptorVersion::Unknown); assert_eq!( new_ccr.check_core_index(&BTreeMap::new()), - Err(CandidateReceiptError::UnknownVersion(InternalVersion(100))) + Err(CommittedCandidateReceiptError::UnknownVersion(InternalVersion(100))) ) } @@ -1075,7 +1088,6 @@ mod tests { new_ccr.descriptor.core_index = 0; new_ccr.descriptor.para_id = ParaId::new(1000); - new_ccr.commitments.upward_messages.force_push(UMP_SEPARATOR); new_ccr.commitments.upward_messages.force_push(UMP_SEPARATOR); let mut cq = BTreeMap::new(); @@ -1089,7 +1101,14 @@ mod tests { new_ccr.commitments.upward_messages.force_push(vec![0, 13, 200].encode()); // No `SelectCore` can be decoded. - assert_eq!(new_ccr.commitments.selected_core(), None); + assert_eq!( + new_ccr.commitments.core_selector(), + Err(CommittedCandidateReceiptError::UmpSignalDecode) + ); + + // Has two cores assigned but no core commitment. Will pass the check if the descriptor core + // index is indeed assigned to the para. + new_ccr.commitments.upward_messages.clear(); let mut cq = BTreeMap::new(); cq.insert( @@ -1100,28 +1119,46 @@ mod tests { CoreIndex(100), vec![new_ccr.descriptor.para_id(), new_ccr.descriptor.para_id()].into(), ); + assert_eq!(new_ccr.check_core_index(&transpose_claim_queue(cq.clone())), Ok(())); + new_ccr.descriptor.set_core_index(CoreIndex(1)); assert_eq!( new_ccr.check_core_index(&transpose_claim_queue(cq.clone())), - Err(CandidateReceiptError::NoCoreSelected) + Err(CommittedCandidateReceiptError::InvalidCoreIndex) ); + new_ccr.descriptor.set_core_index(CoreIndex(0)); new_ccr.commitments.upward_messages.clear(); new_ccr.commitments.upward_messages.force_push(UMP_SEPARATOR); - new_ccr .commitments .upward_messages .force_push(UMPSignal::SelectCore(CoreSelector(0), ClaimQueueOffset(1)).encode()); - // Duplicate + // No assignments. + assert_eq!( + new_ccr.check_core_index(&transpose_claim_queue(Default::default())), + Err(CommittedCandidateReceiptError::NoAssignment) + ); + + // Mismatch between descriptor index and commitment. + new_ccr.descriptor.set_core_index(CoreIndex(1)); + assert_eq!( + new_ccr.check_core_index(&transpose_claim_queue(cq.clone())), + Err(CommittedCandidateReceiptError::CoreIndexMismatch) + ); + new_ccr.descriptor.set_core_index(CoreIndex(0)); + + // Too many UMP signals. new_ccr .commitments .upward_messages .force_push(UMPSignal::SelectCore(CoreSelector(1), ClaimQueueOffset(1)).encode()); - // Duplicate doesn't override first signal. - assert_eq!(new_ccr.check_core_index(&transpose_claim_queue(cq)), Ok(())); + assert_eq!( + new_ccr.check_core_index(&transpose_claim_queue(cq)), + Err(CommittedCandidateReceiptError::TooManyUMPSignals) + ); } #[test] @@ -1191,7 +1228,7 @@ mod tests { Decode::decode(&mut encoded_ccr.as_slice()).unwrap(); assert_eq!(v1_ccr.descriptor.version(), CandidateDescriptorVersion::V1); - assert!(v1_ccr.commitments.selected_core().is_some()); + assert!(v1_ccr.commitments.core_selector().unwrap().is_some()); let mut cq = BTreeMap::new(); cq.insert(CoreIndex(0), vec![v1_ccr.descriptor.para_id()].into()); @@ -1199,11 +1236,6 @@ mod tests { assert!(v1_ccr.check_core_index(&transpose_claim_queue(cq)).is_ok()); - assert_eq!( - v1_ccr.commitments.committed_core_index(&vec![&CoreIndex(10), &CoreIndex(5)]), - Some(CoreIndex(5)), - ); - assert_eq!(v1_ccr.descriptor.core_index(), None); } @@ -1228,11 +1260,9 @@ mod tests { cq.insert(CoreIndex(0), vec![new_ccr.descriptor.para_id()].into()); cq.insert(CoreIndex(1), vec![new_ccr.descriptor.para_id()].into()); - // Should fail because 2 cores are assigned, - assert_eq!( - new_ccr.check_core_index(&transpose_claim_queue(cq)), - Err(CandidateReceiptError::NoCoreSelected) - ); + // Passes even if 2 cores are assigned, because elastic scaling MVP could still inject the + // core index in the `BackedCandidate`. + assert_eq!(new_ccr.check_core_index(&transpose_claim_queue(cq)), Ok(())); // Adding collator signature should make it decode as v1. old_ccr.descriptor.signature = dummy_collator_signature(); diff --git a/polkadot/runtime/parachains/src/inclusion/mod.rs b/polkadot/runtime/parachains/src/inclusion/mod.rs index ea3a5d3cdda9f..8ad9711a0f388 100644 --- a/polkadot/runtime/parachains/src/inclusion/mod.rs +++ b/polkadot/runtime/parachains/src/inclusion/mod.rs @@ -46,7 +46,7 @@ use pallet_message_queue::OnQueueChanged; use polkadot_primitives::{ effective_minimum_backing_votes, supermajority_threshold, vstaging::{ - BackedCandidate, CandidateDescriptorV2 as CandidateDescriptor, + skip_ump_signals, BackedCandidate, CandidateDescriptorV2 as CandidateDescriptor, CandidateReceiptV2 as CandidateReceipt, CommittedCandidateReceiptV2 as CommittedCandidateReceipt, }, @@ -412,11 +412,6 @@ pub(crate) enum UmpAcceptanceCheckErr { TotalSizeExceeded { total_size: u64, limit: u64 }, /// A para-chain cannot send UMP messages while it is offboarding. IsOffboarding, - /// The allowed number of `UMPSignal` messages in the queue was exceeded. - /// Currenly only one such message is allowed. - TooManyUMPSignals { count: u32 }, - /// The UMP queue contains an invalid `UMPSignal` - NoUmpSignal, } impl fmt::Debug for UmpAcceptanceCheckErr { @@ -445,12 +440,6 @@ impl fmt::Debug for UmpAcceptanceCheckErr { UmpAcceptanceCheckErr::IsOffboarding => { write!(fmt, "upward message rejected because the para is off-boarding") }, - UmpAcceptanceCheckErr::TooManyUMPSignals { count } => { - write!(fmt, "the ump queue has too many `UMPSignal` messages ({} > 1 )", count) - }, - UmpAcceptanceCheckErr::NoUmpSignal => { - write!(fmt, "Required UMP signal not found") - }, } } } @@ -925,25 +914,7 @@ impl Pallet { upward_messages: &[UpwardMessage], ) -> Result<(), UmpAcceptanceCheckErr> { // Filter any pending UMP signals and the separator. - let upward_messages = if let Some(separator_index) = - upward_messages.iter().position(|message| message.is_empty()) - { - let (upward_messages, ump_signals) = upward_messages.split_at(separator_index); - - if ump_signals.len() > 2 { - return Err(UmpAcceptanceCheckErr::TooManyUMPSignals { - count: ump_signals.len() as u32, - }) - } - - if ump_signals.len() == 1 { - return Err(UmpAcceptanceCheckErr::NoUmpSignal) - } - - upward_messages - } else { - upward_messages - }; + let upward_messages = skip_ump_signals(upward_messages.iter()).collect::>(); // Cannot send UMP messages while off-boarding. if paras::Pallet::::is_offboarding(para) { @@ -997,10 +968,7 @@ impl Pallet { /// to deal with the messages as given. Messages that are too long will be ignored since such /// candidates should have already been rejected in [`Self::check_upward_messages`]. pub(crate) fn receive_upward_messages(para: ParaId, upward_messages: &[Vec]) { - let bounded = upward_messages - .iter() - // Stop once we hit the `UMPSignal` separator. - .take_while(|message| !message.is_empty()) + let bounded = skip_ump_signals(upward_messages.iter()) .filter_map(|d| { BoundedSlice::try_from(&d[..]) .inspect_err(|_| { diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index eef26b83368f5..146be0ee0aadc 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1864,11 +1864,8 @@ mod enter { v2_descriptor: true, candidate_modifier: Some(|mut candidate: CommittedCandidateReceiptV2| { if candidate.descriptor.para_id() == 1.into() { - // Drop the core selector to make it invalid - candidate - .commitments - .upward_messages - .truncate(candidate.commitments.upward_messages.len() - 1); + // Make the core selector invalid + candidate.commitments.upward_messages[1].truncate(0); } candidate }), diff --git a/prdoc/pr_6217.prdoc b/prdoc/pr_6217.prdoc new file mode 100644 index 0000000000000..2fa800b58d2cf --- /dev/null +++ b/prdoc/pr_6217.prdoc @@ -0,0 +1,24 @@ +title: 'Unify and harden UMP signal checks in check_core_index' +doc: +- audience: [Runtime Dev, Node Dev] + description: | + Refactors and hardens the core index checks on the candidate commitments. + Also adds a utility for skipping the ump signals + +crates: +- name: cumulus-client-consensus-aura + bump: patch +- name: cumulus-pallet-parachain-system + bump: patch +- name: cumulus-primitives-core + bump: major +- name: polkadot-node-collation-generation + bump: major +- name: polkadot-node-core-candidate-validation + bump: patch +- name: polkadot-node-subsystem-util + bump: patch +- name: polkadot-primitives + bump: major +- name: polkadot-runtime-parachains + bump: patch From 7725890d114ea420e2d946b4d9e0a823d20cf0d8 Mon Sep 17 00:00:00 2001 From: davidk-pt Date: Tue, 5 Nov 2024 14:32:19 +0200 Subject: [PATCH 09/11] [Deprecation] deprecate treasury `spend_local` call and related items (#6169) Resolves https://github.com/paritytech/polkadot-sdk/issues/5930 `spend_local` from `treasury` pallet and associated types are deprecated. `spend_local` was being used before with native currency in the treasury. This PR provides a documentation on how to migrate to the `spend` call instead. ### Migration #### For users who were using only `spend_local` before To replace `spend_local` functionality configure `Paymaster` pallet configuration to be `PayFromAccount` and configure `AssetKind` to be `()` and use `spend` call instead. This way `spend` call will function as deprecated `spend_local`. Example: ``` impl pallet_treasury::Config for Runtime { .. type AssetKind = (); type Paymaster = PayFromAccount; // convert balance 1:1 ratio with native currency type BalanceConverter = UnityAssetBalanceConversion; .. } ``` #### For users who were already using `spend` with all other assets, except the native asset Use `NativeOrWithId` type for `AssetKind` and have a `UnionOf` for native and non-native assets, then use that with `PayAssetFromAccount`. Example from `kitchensink-runtime`: ``` // Union of native currency and assets pub type NativeAndAssets = UnionOf, AccountId>; impl pallet_treasury::Config for Runtime { .. type AssetKind = NativeOrWithId; type Paymaster = PayAssetFromAccount; type BalanceConverter = AssetRate; .. } // AssetRate pallet configuration impl pallet_asset_rate::Config for Runtime { .. type Currency = Balances; type AssetKind = NativeOrWithId; .. } ``` --------- Co-authored-by: DavidK Co-authored-by: Muharem --- prdoc/pr_6169.prdoc | 63 +++++++++++++++++ substrate/bin/node/runtime/src/lib.rs | 47 +++++++++++-- substrate/frame/bounties/src/lib.rs | 1 + substrate/frame/bounties/src/tests.rs | 7 ++ substrate/frame/child-bounties/src/tests.rs | 1 + substrate/frame/tips/src/tests.rs | 1 + substrate/frame/treasury/src/benchmarking.rs | 6 +- substrate/frame/treasury/src/lib.rs | 38 ++++++++++ substrate/frame/treasury/src/migration.rs | 2 + substrate/frame/treasury/src/tests.rs | 74 ++++++++++++++------ 10 files changed, 213 insertions(+), 27 deletions(-) create mode 100644 prdoc/pr_6169.prdoc diff --git a/prdoc/pr_6169.prdoc b/prdoc/pr_6169.prdoc new file mode 100644 index 0000000000000..0416fe008051f --- /dev/null +++ b/prdoc/pr_6169.prdoc @@ -0,0 +1,63 @@ +title: "[Deprecation] deprecate treasury `spend_local` call and related items" + +doc: + - audience: Runtime Dev + description: | + Deprecates `spend_local` from the treasury pallet and items associated with it. + + ### Migration + + #### For users who were using only `spend_local` before + + To replace `spend_local` functionality configure `Paymaster` pallet configuration to be `PayFromAccount` and configure `AssetKind` to be `()` and use `spend` call instead. + This way `spend` call will function as deprecated `spend_local`. + + Example: + ``` + impl pallet_treasury::Config for Runtime { + .. + type AssetKind = (); + type Paymaster = PayFromAccount; + // convert balance 1:1 ratio with native currency + type BalanceConverter = UnityAssetBalanceConversion; + .. + } + ``` + + #### For users who were already using `spend` with all other assets, except the native asset + + Use `NativeOrWithId` type for `AssetKind` and have a `UnionOf` for native and non-native assets, then use that with `PayAssetFromAccount`. + + Example from `kitchensink-runtime`: + ``` + // Union of native currency and assets + pub type NativeAndAssets = + UnionOf, AccountId>; + + impl pallet_treasury::Config for Runtime { + .. + type AssetKind = NativeOrWithId; + type Paymaster = PayAssetFromAccount; + type BalanceConverter = AssetRate; + .. + } + + // AssetRate pallet configuration + impl pallet_asset_rate::Config for Runtime { + .. + type Currency = Balances; + type AssetKind = NativeOrWithId; + .. + } + ``` + + +crates: +- name: pallet-treasury + bump: patch +- name: pallet-bounties + bump: patch +- name: pallet-child-bounties + bump: patch +- name: pallet-tips + bump: patch diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 8c2992bdb696a..76b09c127c350 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -24,6 +24,13 @@ extern crate alloc; +#[cfg(feature = "runtime-benchmarks")] +use pallet_asset_rate::AssetKindFactory; +#[cfg(feature = "runtime-benchmarks")] +use pallet_treasury::ArgumentsFactory; +#[cfg(feature = "runtime-benchmarks")] +use polkadot_sdk::sp_core::crypto::FromEntropy; + use polkadot_sdk::*; use alloc::{vec, vec::Vec}; @@ -267,6 +274,36 @@ impl Contains> for TxPauseWhitelistedCalls { } } +#[cfg(feature = "runtime-benchmarks")] +pub struct AssetRateArguments; +#[cfg(feature = "runtime-benchmarks")] +impl AssetKindFactory> for AssetRateArguments { + fn create_asset_kind(seed: u32) -> NativeOrWithId { + if seed % 2 > 0 { + NativeOrWithId::Native + } else { + NativeOrWithId::WithId(seed / 2) + } + } +} + +#[cfg(feature = "runtime-benchmarks")] +pub struct PalletTreasuryArguments; +#[cfg(feature = "runtime-benchmarks")] +impl ArgumentsFactory, AccountId> for PalletTreasuryArguments { + fn create_asset_kind(seed: u32) -> NativeOrWithId { + if seed % 2 > 0 { + NativeOrWithId::Native + } else { + NativeOrWithId::WithId(seed / 2) + } + } + + fn create_beneficiary(seed: [u8; 32]) -> AccountId { + AccountId::from_entropy(&mut seed.as_slice()).unwrap() + } +} + impl pallet_tx_pause::Config for Runtime { type RuntimeEvent = RuntimeEvent; type RuntimeCall = RuntimeCall; @@ -1260,15 +1297,15 @@ impl pallet_treasury::Config for Runtime { type WeightInfo = pallet_treasury::weights::SubstrateWeight; type MaxApprovals = MaxApprovals; type SpendOrigin = EnsureWithSuccess, AccountId, MaxBalance>; - type AssetKind = u32; + type AssetKind = NativeOrWithId; type Beneficiary = AccountId; type BeneficiaryLookup = Indices; - type Paymaster = PayAssetFromAccount; + type Paymaster = PayAssetFromAccount; type BalanceConverter = AssetRate; type PayoutPeriod = SpendPayoutPeriod; type BlockNumberProvider = System; #[cfg(feature = "runtime-benchmarks")] - type BenchmarkHelper = (); + type BenchmarkHelper = PalletTreasuryArguments; } impl pallet_asset_rate::Config for Runtime { @@ -1276,11 +1313,11 @@ impl pallet_asset_rate::Config for Runtime { type RemoveOrigin = EnsureRoot; type UpdateOrigin = EnsureRoot; type Currency = Balances; - type AssetKind = u32; + type AssetKind = NativeOrWithId; type RuntimeEvent = RuntimeEvent; type WeightInfo = pallet_asset_rate::weights::SubstrateWeight; #[cfg(feature = "runtime-benchmarks")] - type BenchmarkHelper = (); + type BenchmarkHelper = AssetRateArguments; } parameter_types! { diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs index 6e0bfa6084e1a..06b0e76cfc7e9 100644 --- a/substrate/frame/bounties/src/lib.rs +++ b/substrate/frame/bounties/src/lib.rs @@ -333,6 +333,7 @@ pub mod pallet { /// Bounty indices that have been approved but not yet funded. #[pallet::storage] + #[allow(deprecated)] pub type BountyApprovals, I: 'static = ()> = StorageValue<_, BoundedVec, ValueQuery>; diff --git a/substrate/frame/bounties/src/tests.rs b/substrate/frame/bounties/src/tests.rs index fcc854e83be0f..447d0edb4122d 100644 --- a/substrate/frame/bounties/src/tests.rs +++ b/substrate/frame/bounties/src/tests.rs @@ -231,6 +231,7 @@ fn expect_events(e: Vec>) { } #[test] +#[allow(deprecated)] fn genesis_config_works() { ExtBuilder::default().build_and_execute(|| { assert_eq!(Treasury::pot(), 0); @@ -248,6 +249,7 @@ fn minting_works() { } #[test] +#[allow(deprecated)] fn accepted_spend_proposal_ignored_outside_spend_period() { ExtBuilder::default().build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); @@ -274,6 +276,7 @@ fn unused_pot_should_diminish() { } #[test] +#[allow(deprecated)] fn accepted_spend_proposal_enacted_on_spend_period() { ExtBuilder::default().build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); @@ -288,6 +291,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { } #[test] +#[allow(deprecated)] fn pot_underflow_should_not_diminish() { ExtBuilder::default().build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); @@ -308,6 +312,7 @@ fn pot_underflow_should_not_diminish() { // Treasury account doesn't get deleted if amount approved to spend is all its free balance. // i.e. pot should not include existential deposit needed for account survival. #[test] +#[allow(deprecated)] fn treasury_account_doesnt_get_deleted() { ExtBuilder::default().build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); @@ -330,6 +335,7 @@ fn treasury_account_doesnt_get_deleted() { // In case treasury account is not existing then it works fine. // This is useful for chain that will just update runtime. #[test] +#[allow(deprecated)] fn inexistent_account_works() { let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); pallet_balances::GenesisConfig:: { balances: vec![(0, 100), (1, 99), (2, 1)] } @@ -423,6 +429,7 @@ fn propose_bounty_validation_works() { } #[test] +#[allow(deprecated)] fn close_bounty_works() { ExtBuilder::default().build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); diff --git a/substrate/frame/child-bounties/src/tests.rs b/substrate/frame/child-bounties/src/tests.rs index 96d01b03560d0..0e58f7c917800 100644 --- a/substrate/frame/child-bounties/src/tests.rs +++ b/substrate/frame/child-bounties/src/tests.rs @@ -161,6 +161,7 @@ fn last_event() -> ChildBountiesEvent { } #[test] +#[allow(deprecated)] fn genesis_config_works() { new_test_ext().execute_with(|| { assert_eq!(Treasury::pot(), 0); diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index f6f130b7e261a..530efb708e414 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -209,6 +209,7 @@ fn last_event() -> TipEvent { } #[test] +#[allow(deprecated)] fn genesis_config_works() { build_and_execute(|| { assert_eq!(Treasury::pot(), 0); diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index 650e5376fa4b2..a03ee149db9b1 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -78,6 +78,7 @@ fn create_approved_proposals, I: 'static>(n: u32) -> Result<(), &'s for i in 0..n { let (_, value, lookup) = setup_proposal::(i); + #[allow(deprecated)] if let Ok(origin) = &spender { Treasury::::spend_local(origin.clone(), value, lookup)?; } @@ -136,6 +137,7 @@ mod benchmarks { let (spend_exists, proposal_id) = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { let (_, value, beneficiary_lookup) = setup_proposal::(SEED); + #[allow(deprecated)] Treasury::::spend_local(origin, value, beneficiary_lookup)?; let proposal_id = ProposalCount::::get() - 1; @@ -149,8 +151,8 @@ mod benchmarks { #[block] { - let res = - Treasury::::remove_approval(reject_origin as T::RuntimeOrigin, proposal_id); + #[allow(deprecated)] + let res = Treasury::::remove_approval(reject_origin as T::RuntimeOrigin, proposal_id); if spend_exists { assert_ok!(res); diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index b21a36949357b..faacda1c07832 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -240,6 +240,9 @@ pub mod pallet { /// Runtime hooks to external pallet using treasury to compute spend funds. type SpendFunds: SpendFunds; + /// DEPRECATED: associated with `spend_local` call and will be removed in May 2025. + /// Refer to for migration to `spend`. + /// /// The maximum number of approvals that can wait in the spending queue. /// /// NOTE: This parameter is also used within the Bounties Pallet extension if enabled. @@ -284,10 +287,16 @@ pub mod pallet { type BlockNumberProvider: BlockNumberProvider>; } + /// DEPRECATED: associated with `spend_local` call and will be removed in May 2025. + /// Refer to for migration to `spend`. + /// /// Number of proposals that have been made. #[pallet::storage] pub type ProposalCount = StorageValue<_, ProposalIndex, ValueQuery>; + /// DEPRECATED: associated with `spend_local` call and will be removed in May 2025. + /// Refer to for migration to `spend`. + /// /// Proposals that have been made. #[pallet::storage] pub type Proposals, I: 'static = ()> = StorageMap< @@ -303,6 +312,9 @@ pub mod pallet { pub type Deactivated, I: 'static = ()> = StorageValue<_, BalanceOf, ValueQuery>; + /// DEPRECATED: associated with `spend_local` call and will be removed in May 2025. + /// Refer to for migration to `spend`. + /// /// Proposal indices that have been approved but not yet awarded. #[pallet::storage] pub type Approvals, I: 'static = ()> = @@ -494,6 +506,10 @@ pub mod pallet { /// Emits [`Event::SpendApproved`] if successful. #[pallet::call_index(3)] #[pallet::weight(T::WeightInfo::spend_local())] + #[deprecated( + note = "The `spend_local` call will be removed by May 2025. Migrate to the new flow and use the `spend` call." + )] + #[allow(deprecated)] pub fn spend_local( origin: OriginFor, #[pallet::compact] amount: BalanceOf, @@ -523,7 +539,9 @@ pub mod pallet { .unwrap_or(Ok(()))?; let beneficiary = T::Lookup::lookup(beneficiary)?; + #[allow(deprecated)] let proposal_index = ProposalCount::::get(); + #[allow(deprecated)] Approvals::::try_append(proposal_index) .map_err(|_| Error::::TooManyApprovals)?; let proposal = Proposal { @@ -532,7 +550,9 @@ pub mod pallet { beneficiary: beneficiary.clone(), bond: Default::default(), }; + #[allow(deprecated)] Proposals::::insert(proposal_index, proposal); + #[allow(deprecated)] ProposalCount::::put(proposal_index + 1); Self::deposit_event(Event::SpendApproved { proposal_index, amount, beneficiary }); @@ -562,12 +582,17 @@ pub mod pallet { /// in the first place. #[pallet::call_index(4)] #[pallet::weight((T::WeightInfo::remove_approval(), DispatchClass::Operational))] + #[deprecated( + note = "The `remove_approval` call will be removed by May 2025. It associated with the deprecated `spend_local` call." + )] + #[allow(deprecated)] pub fn remove_approval( origin: OriginFor, #[pallet::compact] proposal_id: ProposalIndex, ) -> DispatchResult { T::RejectOrigin::ensure_origin(origin)?; + #[allow(deprecated)] Approvals::::try_mutate(|v| -> DispatchResult { if let Some(index) = v.iter().position(|x| x == &proposal_id) { v.remove(index); @@ -836,16 +861,28 @@ impl, I: 'static> Pallet { } /// Public function to proposal_count storage. + #[deprecated( + note = "This function will be removed by May 2025. Configure pallet to use PayFromAccount for Paymaster type instead" + )] pub fn proposal_count() -> ProposalIndex { + #[allow(deprecated)] ProposalCount::::get() } /// Public function to proposals storage. + #[deprecated( + note = "This function will be removed by May 2025. Configure pallet to use PayFromAccount for Paymaster type instead" + )] pub fn proposals(index: ProposalIndex) -> Option>> { + #[allow(deprecated)] Proposals::::get(index) } /// Public function to approvals storage. + #[deprecated( + note = "This function will be removed by May 2025. Configure pallet to use PayFromAccount for Paymaster type instead" + )] + #[allow(deprecated)] pub fn approvals() -> BoundedVec { Approvals::::get() } @@ -864,6 +901,7 @@ impl, I: 'static> Pallet { let mut missed_any = false; let mut imbalance = PositiveImbalanceOf::::zero(); + #[allow(deprecated)] let proposals_len = Approvals::::mutate(|v| { let proposals_approvals_len = v.len() as u32; v.retain(|&index| { diff --git a/substrate/frame/treasury/src/migration.rs b/substrate/frame/treasury/src/migration.rs index c0de4ce431088..7c8c587f16641 100644 --- a/substrate/frame/treasury/src/migration.rs +++ b/substrate/frame/treasury/src/migration.rs @@ -43,11 +43,13 @@ pub mod cleanup_proposals { { fn on_runtime_upgrade() -> frame_support::weights::Weight { let mut approval_index = BTreeSet::new(); + #[allow(deprecated)] for approval in Approvals::::get().iter() { approval_index.insert(*approval); } let mut proposals_processed = 0; + #[allow(deprecated)] for (proposal_index, p) in Proposals::::iter() { if !approval_index.contains(&proposal_index) { let err_amount = T::Currency::unreserve(&p.proposer, p.bond); diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index a99dd0dd4449f..e9efb7c0956f1 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -249,6 +249,7 @@ fn genesis_config_works() { #[test] fn spend_local_origin_permissioning_works() { + #[allow(deprecated)] ExtBuilder::default().build().execute_with(|| { assert_noop!(Treasury::spend_local(RuntimeOrigin::signed(1), 1, 1), BadOrigin); assert_noop!( @@ -273,6 +274,7 @@ fn spend_local_origin_permissioning_works() { #[docify::export] #[test] fn spend_local_origin_works() { + #[allow(deprecated)] 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(), 102); @@ -309,7 +311,10 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); + } go_to_block(1); assert_eq!(Balances::free_balance(3), 0); @@ -336,7 +341,10 @@ fn accepted_spend_proposal_enacted_on_spend_period() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 100, 3)); + } go_to_block(2); assert_eq!(Balances::free_balance(3), 100); @@ -350,7 +358,10 @@ fn pot_underflow_should_not_diminish() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 150, 3)); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 150, 3)); + } go_to_block(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed @@ -370,13 +381,19 @@ fn treasury_account_doesnt_get_deleted() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); let treasury_balance = Balances::free_balance(&Treasury::account_id()); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), treasury_balance, 3)); + >::on_initialize(2); + assert_eq!(Treasury::pot(), 100); // Pot hasn't changed - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), treasury_balance, 3)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), treasury_balance, 3)); - go_to_block(2); - assert_eq!(Treasury::pot(), 100); // Pot hasn't changed + go_to_block(2); + assert_eq!(Treasury::pot(), 100); // Pot hasn't changed - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), Treasury::pot(), 3)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), Treasury::pot(), 3)); + } go_to_block(4); assert_eq!(Treasury::pot(), 0); // Pot is emptied @@ -399,8 +416,11 @@ fn inexistent_account_works() { assert_eq!(Balances::free_balance(Treasury::account_id()), 0); // Account does not exist assert_eq!(Treasury::pot(), 0); // Pot is empty - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 99, 3)); - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 99, 3)); + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); + } go_to_block(2); @@ -439,6 +459,7 @@ fn genesis_funding_works() { #[test] fn max_approvals_limited() { + #[allow(deprecated)] ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), u64::MAX); Balances::make_free_balance_be(&0, u64::MAX); @@ -457,6 +478,7 @@ fn max_approvals_limited() { #[test] fn remove_already_removed_approval_fails() { + #[allow(deprecated)] ExtBuilder::default().build().execute_with(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); @@ -796,7 +818,10 @@ fn try_state_proposals_invariant_1_works() { ExtBuilder::default().build().execute_with(|| { use frame_support::pallet_prelude::DispatchError::Other; // Add a proposal and approve using `spend_local` - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); + } assert_eq!(Proposals::::iter().count(), 1); assert_eq!(ProposalCount::::get(), 1); @@ -816,8 +841,11 @@ fn try_state_proposals_invariant_1_works() { fn try_state_proposals_invariant_2_works() { ExtBuilder::default().build().execute_with(|| { use frame_support::pallet_prelude::DispatchError::Other; - // Add a proposal and approve using `spend_local` - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); + #[allow(deprecated)] + { + // Add a proposal and approve using `spend_local` + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 1, 3)); + } assert_eq!(Proposals::::iter().count(), 1); assert_eq!(Approvals::::get().len(), 1); @@ -846,7 +874,10 @@ fn try_state_proposals_invariant_3_works() { ExtBuilder::default().build().execute_with(|| { use frame_support::pallet_prelude::DispatchError::Other; // Add a proposal and approve using `spend_local` - assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 10, 3)); + #[allow(deprecated)] + { + assert_ok!(Treasury::spend_local(RuntimeOrigin::signed(14), 10, 3)); + } assert_eq!(Proposals::::iter().count(), 1); assert_eq!(Approvals::::get().len(), 1); @@ -952,13 +983,16 @@ fn multiple_spend_periods_work() { // 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)); + #[allow(deprecated)] + { + 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); From 16e877be9afef58e3376d46ea96067aecd5b0dad Mon Sep 17 00:00:00 2001 From: Alexander Samusev <41779041+alvicsam@users.noreply.github.com> Date: Tue, 5 Nov 2024 13:54:29 +0100 Subject: [PATCH 10/11] Run check semver in MQ (#6287) For marking `Check SemVer` required --- .github/workflows/check-semver.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/check-semver.yml b/.github/workflows/check-semver.yml index 65f3339b7ac71..78602410cdf65 100644 --- a/.github/workflows/check-semver.yml +++ b/.github/workflows/check-semver.yml @@ -4,6 +4,7 @@ on: pull_request: types: [opened, synchronize, reopened, ready_for_review] workflow_dispatch: + merge_group: concurrency: group: check-semver-${{ github.event.pull_request.number || github.ref }} From abb8142b2cec0538cbe50481b142f27a6b6b67a8 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Tue, 5 Nov 2024 11:33:38 -0300 Subject: [PATCH 11/11] fix: remove nonexistent imports --- .../parachains/runtimes/testing/penpal/src/xcm_config.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs b/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs index 58d41c7b5deee..8009d11b80675 100644 --- a/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs @@ -44,7 +44,6 @@ use pallet_xcm::XcmPassthrough; use parachains_common::{xcm_config::AssetFeeAsExistentialDepositMultiplier, TREASURY_PALLET_ID}; use polkadot_parachain_primitives::primitives::Sibling; use polkadot_runtime_common::{impls::ToAuthor, xcm_sender::ExponentialPrice}; -use snowbridge_router_primitives::inbound::GlobalConsensusEthereumConvertsFor; use sp_runtime::traits::{AccountIdConversion, ConvertInto, Identity, TryConvertInto}; use xcm::latest::{prelude::*, WESTEND_GENESIS_HASH}; use xcm_builder::{ @@ -94,12 +93,6 @@ pub type LocationToAccountId = ( AccountId32Aliases, // Foreign locations alias into accounts according to a hash of their standard description. HashedDescription>, - // Different global consensus parachain sovereign account. - // (Used for over-bridge transfers and reserve processing) - GlobalConsensusParachainConvertsFor, - // Ethereum contract sovereign account. - // (Used to get convert ethereum contract locations to sovereign account) - GlobalConsensusEthereumConvertsFor, ); /// Means for transacting assets on this chain.