-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Nara] Restric curators deletion powers #4874
[Nara] Restric curators deletion powers #4874
Conversation
…ystem prune to resolve
Cleaner solution, and requires on runtime upgrade migration for the content pallet curator permissions
…rator moderation actions
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.
I have no major remarks.
I am just wandering: shouldn't we replace the runtime migration tests in the CI with the try_runtime
ones?
runtime-modules/content/src/lib.rs
Outdated
|
||
// syntactic sugar for logging. | ||
#[macro_export] | ||
macro_rules! log { |
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.
maybe I would put this in the migration.rs
file where it is used
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.
I will try, although I think the macro export statement might only work in the crate root.
Although we don't use the log statement anywhere else, it could be useful if we want to add some logs in future?
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.
I have no major remarks.
I am just wandering: shouldn't we replace the runtime migration tests in the CI with thetry_runtime
ones?
.permissions_by_level | ||
.iter() | ||
.for_each(|(level, permissions)| { | ||
let mut permissions_v1 = BTreeSet::new(); |
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.
Also maybe add a log!
statement here
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.
Done in e759b32
Yes we do here: |
…' into nara-restric-curators-del-powers
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.
LGTM
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 this looks like excellent work, but I have to admit that getting back to this really showed me how stale my Substrate skills are, and with limited time, my due dilligence is of limited value unfortunately. I'm ok with merging this, but also asked for a superficial change and a question to make sure I'm understanding properly.
I'm glad there is a somewhat standardised approach to migrations in Substrate now, it looks like a nice approach which can work for complex changes.
|
||
current.put::<Pallet<T>>(); | ||
|
||
<T as frame_system::Config>::BlockWeights::get().max_block |
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.
Why is this set to max? and is this the right notion of max? is the idea here to exclude all other processing in this block, so as to avoid having to come up with an accurate weight function to begin with?
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.
I was going through a lot of the migrations which we are including the nara update and saw more than one returning this max block weight. And since one of the migrations was doing it, it become almost irrelevant what was returned here.
As far as I know the amount of block space allocated for on_runtime_upgrade is not constrained, but accurate weight computation would be important to know if we were planning a migration of a large map for example. This way we could plan ahead and decide if the total weight was larger than what validators could produce in required time.
Found this relevant discussion paritytech/substrate#10064
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.
But to answer your question more clearly, yes by returning max_block in this (or any other migration) will ensure that the block being executed (the first block with the new runtime active) will prevent any extrinsics from being included. So only migrations, on_initialize and on_finalize logic will execute.
The value is constructed in
Line 186 in 14b0fa3
pub RuntimeBlockWeights: BlockWeights = BlockWeights::builder() |
log!(info, "level: {:?}, permissions: {:?}", level, permissions); | ||
let mut permissions_v1 = BTreeSet::new(); | ||
permissions.iter().for_each(|action| match action { | ||
ContentModerationActionV0::HideVideo => { |
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.
I think it would have been better to separate out conversion from writing/translation, but don't think its worth fiddling with now.
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.
You are right, I didn't invest too much time on code style here.
// Nara release. enum variants removed: | ||
// - ContentModerationAction::DeleteVideo | ||
// - ContentModerationAction::DeleteChannel | ||
const CURRENT_STORAGE_VERSION: StorageVersion = StorageVersion::new(1); |
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.
Rather than using these opaque version identifiers, what if we used some more explicit representation which would be easier to self-document, like they seemingly do in Substrate frame pallets:
https://paritytech.github.io/polkadot-sdk/master/src/pallet_staking/migrations.rs.html#36
I guess we could could use release names? like nara, etc.?
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.
If you look carefully at the enum ObsoleteReleases
you will note they are actually phasing out that pattern for the more common convention I saw used in multiple other pallets. eg. https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/assets/src/migration.rs
Although I agree a u32
it is not very self documenting which is why I used the nara
name for the module.
group_count | ||
); | ||
let pre_upgrade_group_count = usize::from_be_bytes(state.try_into().unwrap()); | ||
ensure!( |
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.
I'm not sure I understand the consequences of this sort of check at runtime, is that really appropriate? I see the case for asserting this in debug mode or something, but if there is some sort of bug, is there any added profit from failing this hook? what happens then?
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.
When we build the production runtime we will not build it with the try-runtime
feature, so the pre_/post_ methods will not exist and will not execute.
The feature is meant specifically for testing a new runtime upgrade during development against some existing chain state.
|
||
use iterable_enums::ContentModerationActionV0; | ||
|
||
<CuratorGroupById<T>>::translate_values( |
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.
Are we not concerned about this, for some reason being gigantic by some weird accident or attack, and then trying to iterate it all in one block? Seems like this iteration must be either unrolled across blocks if it is truly to be dynamic, or it has to have a hardcap which we know allows compute to finish in time.
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.
That is a great point, I guess it also goes back to the discussion here #4874 (comment)
I believe there is already a limit on the number of curators per group but not on number of groups.
So yes it could be a problem if the content lead decides to create a very large number of groups before the runtime upgrade.
Should we tackle this possibility ?
Will address your questions bedeho, but will merge for Ignazio's sake. |
Implements #4589
delete_video_as_moderator
delete_channel_as_moderator
v3.0.0
(convention to bump major version on each new runtime version)ContentModerationAction::DeleteVideo
,ContentModerationAction::DeleteChannel
variants, and add migration to update permissions by level of each curator group incontent::CuratorGroupById
storage map.Includes work from #4875
The runtime migration can be tested against the live network by running:
RUN_MIGRATION_TESTS=true RUST_LOG=info cargo +nightly-2022-11-15 \ test remote_tests::run_migrations --release --features try-runtime
The runtime upgrade can also be tested against any other chain with:
WS=ws://127.0.0.1:9944/ RUN_MIGRATION_TESTS=true RUST_LOG=info cargo +nightly-2022-11-15 \ test remote_tests::run_migrations --release --features try-runtime
or
We have a problem with the query-node. We have already solved how to allow processor to handle different version of an event type after runtime upgrade, however it cannot handle an event being removed entierly from a runtime.
So for now the
ChannelDeletedByModerator
andVideoDeletedByModerator
events are still part of the runtime despite no longer being emitted, only to be able to handle old runtime events. - I guess that is one more reason to move to subsquid.. (not like we needed any more reasons) - created an issue #4877 -> resolved in #4875 and Joystream/hydra#531With respect to mappings related to curator permissions there are none specific to
ContentModerationAction
. Query node is only keeping track of the "channel agent" permissions. So no changes to mappings is required.┆Issue is synchronized with this Asana task by Unito