This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
paras: add governance control dispatchables #4575
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
3f99599
paras: add governance control dispatchables
pepyakin 13f49c5
Merge branch 'master' of https://github.com/paritytech/polkadot into …
9a634bc
cargo run --quiet --release --features=runtime-benchmarks -- benchmar…
a40ad6a
cargo run --quiet --release --features=runtime-benchmarks -- benchmar…
0399890
Merge branch 'master' of https://github.com/paritytech/polkadot into …
d36360b
cargo run --quiet --release --features=runtime-benchmarks -- benchmar…
570c4af
cargo run --quiet --release --features runtime-benchmarks -- benchmar…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -392,6 +392,8 @@ pub trait WeightInfo { | |
fn force_schedule_code_upgrade(c: u32) -> Weight; | ||
fn force_note_new_head(s: u32) -> Weight; | ||
fn force_queue_action() -> Weight; | ||
fn add_trusted_validation_code(c: u32) -> Weight; | ||
fn poke_unused_validation_code() -> Weight; | ||
} | ||
|
||
pub struct TestWeightInfo; | ||
|
@@ -411,6 +413,12 @@ impl WeightInfo for TestWeightInfo { | |
fn force_queue_action() -> Weight { | ||
Weight::MAX | ||
} | ||
fn add_trusted_validation_code(_c: u32) -> Weight { | ||
Weight::MAX | ||
} | ||
fn poke_unused_validation_code() -> Weight { | ||
Weight::MAX | ||
} | ||
} | ||
|
||
#[frame_support::pallet] | ||
|
@@ -763,6 +771,79 @@ pub mod pallet { | |
Ok(()) | ||
} | ||
|
||
/// Adds the validation code to the storage. | ||
/// | ||
/// The code will not be added if it is already present. Additionally, if PVF pre-checking | ||
/// is running for that code, it will be instantly accepted. | ||
/// | ||
/// Otherwise, the code will be added into the storage. Note that the code will be added | ||
/// into storage with reference count 0. This is to account the fact that there are no users | ||
/// for this code yet. The caller will have to make sure that this code eventually gets | ||
/// used by some parachain or removed from the storage to avoid storage leaks. For the latter | ||
/// prefer to use the `poke_unused_validation_code` dispatchable to raw storage manipulation. | ||
/// | ||
/// This function is mainly meant to be used for upgrading parachains that do not follow | ||
/// the go-ahead signal while the PVF pre-checking feature is enabled. | ||
#[pallet::weight(<T as Config>::WeightInfo::add_trusted_validation_code(validation_code.0.len() as u32))] | ||
pub fn add_trusted_validation_code( | ||
origin: OriginFor<T>, | ||
validation_code: ValidationCode, | ||
) -> DispatchResult { | ||
ensure_root(origin)?; | ||
let code_hash = validation_code.hash(); | ||
|
||
if let Some(vote) = <Self as Store>::PvfActiveVoteMap::get(&code_hash) { | ||
// Remove the existing vote. | ||
PvfActiveVoteMap::<T>::remove(&code_hash); | ||
PvfActiveVoteList::<T>::mutate(|l| { | ||
if let Ok(i) = l.binary_search(&code_hash) { | ||
l.remove(i); | ||
} | ||
}); | ||
|
||
let cfg = configuration::Pallet::<T>::config(); | ||
Self::enact_pvf_accepted( | ||
<frame_system::Pallet<T>>::block_number(), | ||
&code_hash, | ||
&vote.causes, | ||
vote.age, | ||
&cfg, | ||
); | ||
return Ok(()) | ||
} | ||
|
||
if <Self as Store>::CodeByHash::contains_key(&code_hash) { | ||
// There is no vote, but the code exists. Nothing to do here. | ||
return Ok(()) | ||
} | ||
|
||
// At this point the code is unknown and there is no PVF pre-checking vote for it, so we | ||
// can just add the code into the storage. | ||
// | ||
// NOTE That we do not use `increase_code_ref` here, because the code is not yet used | ||
// by any parachain. | ||
<Self as Store>::CodeByHash::insert(code_hash, &validation_code); | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Remove the validation code from the storage iff the reference count is 0. | ||
/// | ||
/// This is better than removing the storage directly, because it will not remove the code | ||
/// that was suddenly got used by some parachain while this dispatchable was pending | ||
/// dispatching. | ||
#[pallet::weight(<T as Config>::WeightInfo::poke_unused_validation_code())] | ||
pub fn poke_unused_validation_code( | ||
origin: OriginFor<T>, | ||
validation_code_hash: ValidationCodeHash, | ||
) -> DispatchResult { | ||
ensure_root(origin)?; | ||
if <Self as Store>::CodeByHashRefs::get(&validation_code_hash) == 0 { | ||
<Self as Store>::CodeByHash::remove(&validation_code_hash); | ||
} | ||
Ok(()) | ||
} | ||
|
||
/// Includes a statement for a PVF pre-checking vote. Potentially, finalizes the vote and | ||
/// enacts the results if that was the last vote before achieving the supermajority. | ||
#[pallet::weight(Weight::MAX)] | ||
|
@@ -1609,6 +1690,8 @@ impl<T: Config> Pallet<T> { | |
weight += T::DbWeight::get().reads(1); | ||
match PvfActiveVoteMap::<T>::get(&code_hash) { | ||
None => { | ||
// We deliberately are using `CodeByHash` here instead of the `CodeByHashRefs`. This | ||
// is because the code may have been added by `add_trusted_validation_code`. | ||
let known_code = CodeByHash::<T>::contains_key(&code_hash); | ||
weight += T::DbWeight::get().reads(1); | ||
|
||
|
@@ -1812,7 +1895,10 @@ impl<T: Config> Pallet<T> { | |
fn decrease_code_ref(code_hash: &ValidationCodeHash) -> Weight { | ||
let mut weight = T::DbWeight::get().reads(1); | ||
let refs = <Self as Store>::CodeByHashRefs::get(code_hash); | ||
debug_assert!(refs != 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is removed just to pass an extra paranoid test: |
||
if refs == 0 { | ||
log::error!(target: LOG_TARGET, "Code refs is already zero for {:?}", code_hash); | ||
return weight | ||
} | ||
if refs <= 1 { | ||
weight += T::DbWeight::get().writes(2); | ||
<Self as Store>::CodeByHash::remove(code_hash); | ||
|
@@ -1842,7 +1928,7 @@ impl<T: Config> Pallet<T> { | |
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use frame_support::{assert_err, assert_ok}; | ||
use frame_support::{assert_err, assert_ok, assert_storage_noop}; | ||
use keyring::Sr25519Keyring; | ||
use primitives::{ | ||
v0::PARACHAIN_KEY_TYPE_ID, | ||
|
@@ -1855,7 +1941,10 @@ mod tests { | |
|
||
use crate::{ | ||
configuration::HostConfiguration, | ||
mock::{new_test_ext, Configuration, MockGenesisConfig, Paras, ParasShared, System, Test}, | ||
mock::{ | ||
new_test_ext, Configuration, MockGenesisConfig, Origin, Paras, ParasShared, System, | ||
Test, | ||
}, | ||
}; | ||
|
||
static VALIDATORS: &[Sr25519Keyring] = &[ | ||
|
@@ -3066,6 +3155,186 @@ mod tests { | |
}); | ||
} | ||
|
||
#[test] | ||
fn add_trusted_validation_code_inserts_with_no_users() { | ||
// This test is to ensure that trusted validation code is inserted into the storage | ||
// with the reference count equal to 0. | ||
let validation_code = ValidationCode(vec![1, 2, 3]); | ||
new_test_ext(Default::default()).execute_with(|| { | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), validation_code.clone())); | ||
assert_eq!(<Paras as Store>::CodeByHashRefs::get(&validation_code.hash()), 0,); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn add_trusted_validation_code_idempotent() { | ||
// This test makes sure that calling add_trusted_validation_code twice with the same | ||
// parameters is a no-op. | ||
let validation_code = ValidationCode(vec![1, 2, 3]); | ||
new_test_ext(Default::default()).execute_with(|| { | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), validation_code.clone())); | ||
assert_storage_noop!({ | ||
assert_ok!(Paras::add_trusted_validation_code( | ||
Origin::root(), | ||
validation_code.clone() | ||
)); | ||
}); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn poke_unused_validation_code_removes_code_cleanly() { | ||
// This test makes sure that calling poke_unused_validation_code with a code that is currently | ||
// in the storage but has no users will remove it cleanly from the storage. | ||
let validation_code = ValidationCode(vec![1, 2, 3]); | ||
new_test_ext(Default::default()).execute_with(|| { | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), validation_code.clone())); | ||
assert_ok!(Paras::poke_unused_validation_code(Origin::root(), validation_code.hash())); | ||
|
||
assert_eq!(<Paras as Store>::CodeByHashRefs::get(&validation_code.hash()), 0); | ||
assert!(!<Paras as Store>::CodeByHash::contains_key(&validation_code.hash())); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn poke_unused_validation_code_doesnt_remove_code_with_users() { | ||
let para_id = 100.into(); | ||
let validation_code = ValidationCode(vec![1, 2, 3]); | ||
new_test_ext(Default::default()).execute_with(|| { | ||
// First we add the code to the storage. | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), validation_code.clone())); | ||
|
||
// Then we add a user to the code, say by upgrading. | ||
run_to_block(2, None); | ||
Paras::schedule_code_upgrade( | ||
para_id, | ||
validation_code.clone(), | ||
1, | ||
&Configuration::config(), | ||
); | ||
Paras::note_new_head(para_id, HeadData::default(), 1); | ||
|
||
// Finally we poke the code, which should not remove it from the storage. | ||
assert_storage_noop!({ | ||
assert_ok!(Paras::poke_unused_validation_code( | ||
Origin::root(), | ||
validation_code.hash() | ||
)); | ||
}); | ||
check_code_is_stored(&validation_code); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn increase_code_ref_doesnt_have_allergy_on_add_trusted_validation_code() { | ||
// Verify that accidential calling of increase_code_ref or decrease_code_ref does not lead | ||
// to a disaster. | ||
// NOTE that this test is extra paranoid, as it is not really possible to hit | ||
// `decrease_code_ref` without calling `increase_code_ref` first. | ||
let code = ValidationCode(vec![1, 2, 3]); | ||
|
||
new_test_ext(Default::default()).execute_with(|| { | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), code.clone())); | ||
Paras::increase_code_ref(&code.hash(), &code); | ||
Paras::increase_code_ref(&code.hash(), &code); | ||
assert!(<Paras as Store>::CodeByHash::contains_key(code.hash())); | ||
assert_eq!(<Paras as Store>::CodeByHashRefs::get(code.hash()), 2); | ||
}); | ||
|
||
new_test_ext(Default::default()).execute_with(|| { | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), code.clone())); | ||
Paras::decrease_code_ref(&code.hash()); | ||
assert!(<Paras as Store>::CodeByHash::contains_key(code.hash())); | ||
assert_eq!(<Paras as Store>::CodeByHashRefs::get(code.hash()), 0); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn add_trusted_validation_code_insta_approval() { | ||
// In particular, this tests that `kick_off_pvf_check` reacts to the `add_trusted_validation_code` | ||
// and uses the `CodeByHash::contains_key` which is what `add_trusted_validation_code` uses. | ||
let para_id = 100.into(); | ||
let validation_code = ValidationCode(vec![1, 2, 3]); | ||
let validation_upgrade_delay = 25; | ||
let minimum_validation_upgrade_delay = 2; | ||
let genesis_config = MockGenesisConfig { | ||
configuration: crate::configuration::GenesisConfig { | ||
config: HostConfiguration { | ||
pvf_checking_enabled: true, | ||
validation_upgrade_delay, | ||
minimum_validation_upgrade_delay, | ||
..Default::default() | ||
}, | ||
..Default::default() | ||
}, | ||
..Default::default() | ||
}; | ||
new_test_ext(genesis_config).execute_with(|| { | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), validation_code.clone())); | ||
|
||
// Then some parachain upgrades it's code with the relay-parent 1. | ||
run_to_block(2, None); | ||
Paras::schedule_code_upgrade( | ||
para_id, | ||
validation_code.clone(), | ||
1, | ||
&Configuration::config(), | ||
); | ||
Paras::note_new_head(para_id, HeadData::default(), 1); | ||
|
||
// Verify that the code upgrade has `expected_at` set to `26`. This is the behavior | ||
// equal to that of `pvf_checking_enabled: false`. | ||
assert_eq!( | ||
<Paras as Store>::FutureCodeUpgrades::get(¶_id), | ||
Some(1 + validation_upgrade_delay) | ||
); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn add_trusted_validation_code_enacts_existing_pvf_vote() { | ||
// This test makes sure that calling `add_trusted_validation_code` with a code that is | ||
// already going through PVF pre-checking voting will conclude the voting and enact the | ||
// code upgrade. | ||
let para_id = 100.into(); | ||
let validation_code = ValidationCode(vec![1, 2, 3]); | ||
let validation_upgrade_delay = 25; | ||
let minimum_validation_upgrade_delay = 2; | ||
let genesis_config = MockGenesisConfig { | ||
configuration: crate::configuration::GenesisConfig { | ||
config: HostConfiguration { | ||
pvf_checking_enabled: true, | ||
validation_upgrade_delay, | ||
minimum_validation_upgrade_delay, | ||
..Default::default() | ||
}, | ||
..Default::default() | ||
}, | ||
..Default::default() | ||
}; | ||
new_test_ext(genesis_config).execute_with(|| { | ||
// First, some parachain upgrades it's code with the relay-parent 1. | ||
run_to_block(2, None); | ||
Paras::schedule_code_upgrade( | ||
para_id, | ||
validation_code.clone(), | ||
1, | ||
&Configuration::config(), | ||
); | ||
Paras::note_new_head(para_id, HeadData::default(), 1); | ||
|
||
// No upgrade should be scheduled at this point. PVF pre-checking vote should run for | ||
// that PVF. | ||
assert!(<Paras as Store>::FutureCodeUpgrades::get(¶_id).is_none()); | ||
assert!(<Paras as Store>::PvfActiveVoteMap::contains_key(&validation_code.hash())); | ||
|
||
// Then we add a trusted validation code. That should conclude the vote. | ||
assert_ok!(Paras::add_trusted_validation_code(Origin::root(), validation_code.clone())); | ||
assert!(<Paras as Store>::FutureCodeUpgrades::get(¶_id).is_some()); | ||
assert!(!<Paras as Store>::PvfActiveVoteMap::contains_key(&validation_code.hash())); | ||
}); | ||
} | ||
|
||
#[test] | ||
fn verify_upgrade_go_ahead_signal_is_externally_accessible() { | ||
use primitives::v1::well_known_keys; | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in this case, we completely ignore the onboarding/upgrade machinery? I guess that's fine/intended, but I would like to understand it better: Like how does the actual upgrade work then for example? Code is already on the relay chain, so what steps must a parachain do now to upgrade in this state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A parachain would attempt an upgrade. When that candidate is enacted, the inclusion module would call
schedule_code_upgrade
and then control will fall intokick_off_pvf_check
and this branch will be taken.From the parachain's standpoint, it will look like instant PVF approval or equally as good old pre-go-ahead signal upgrade.
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep it does - thanks! So a parachain would still need to get it's full runtime upgrade through the backing phase, although the code is already on chain. We just bypass the voting, because we trust governance to have the code checked by some other means. 💡