[Nara] Restric curators deletion powers by mnaamani · Pull Request #4874 · Joystream/joystream

mnaamani · 2023-09-15T19:03:11Z

Implements #4589

Dropped delete_video_as_moderator
Dropped delete_channel_as_moderator
Regenerated chain metadata and types
Bumped types version to v3.0.0 (convention to bump major version on each new runtime version)
Removed use of extrinsics in cli,integration tests
Update benchmarks and weights (remove unused weight functions related to dropped extrinsics
Remove ContentModerationAction::DeleteVideo, ContentModerationAction::DeleteChannel variants, and add migration to update permissions by level of each curator group in content::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

cargo +nightly-2022-11-15 build --release --features try-runtime
RUST_LOG=info,runtime=debug ./target/release/joystream-node try-runtime \
   --runtime ./target/release/wbuild/joystream-node-runtime/joystream_node_runtime.compact.compressed.wasm \
   on-runtime-upgrade live --uri ws://127.0.0.1:9944/

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 and VideoDeletedByModerator 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#531

With 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

…ario

…ystem prune to resolve

Cleaner solution, and requires on runtime upgrade migration for the content pallet curator permissions

…rator moderation actions

dobertRowneySr

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?

dobertRowneySr · 2023-09-25T15:21:04Z

runtime-modules/content/src/lib.rs

+
+// syntactic sugar for logging.
+#[macro_export]
+macro_rules! log {


maybe I would put this in the migration.rs file where it is used

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?

dobertRowneySr

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?

dobertRowneySr · 2023-09-26T06:59:12Z

runtime-modules/content/src/migrations.rs

+                        .permissions_by_level
+                        .iter()
+                        .for_each(|(level, permissions)| {
+                            let mut permissions_v1 = BTreeSet::new();


Also maybe add a log! statement here

Done in e759b32

mnaamani · 2023-09-26T18:15:36Z

I am just wandering: shouldn't we replace the runtime migration tests in the CI with the try_runtime ones?

Yes we do here:
https://github.com/Joystream/joystream/actions/runs/6245191588/job/16953377405?pr=4874#step:7:2
and
https://github.com/Joystream/joystream/actions/runs/6245191588/job/16953377405?pr=4874#step:8:2

…' into nara-restric-curators-del-powers

dobertRowneySr

LGTM

…node

bedeho

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.

bedeho · 2023-10-03T17:05:30Z

runtime-modules/content/src/migrations.rs

+
+            current.put::<Pallet<T>>();
+
+            <T as frame_system::Config>::BlockWeights::get().max_block


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?

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

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

joystream/runtime/src/lib.rs

Line 186 in 14b0fa3

pub RuntimeBlockWeights: BlockWeights = BlockWeights::builder()

bedeho · 2023-10-03T17:06:23Z

runtime-modules/content/src/migrations.rs

+                            log!(info, "level: {:?}, permissions: {:?}", level, permissions);
+                            let mut permissions_v1 = BTreeSet::new();
+                            permissions.iter().for_each(|action| match action {
+                                ContentModerationActionV0::HideVideo => {


I think it would have been better to separate out conversion from writing/translation, but don't think its worth fiddling with now.

You are right, I didn't invest too much time on code style here.

bedeho · 2023-10-03T17:38:05Z

runtime-modules/content/src/lib.rs

+// Nara release. enum variants removed:
+// - ContentModerationAction::DeleteVideo
+// - ContentModerationAction::DeleteChannel
+const CURRENT_STORAGE_VERSION: StorageVersion = StorageVersion::new(1);


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.?

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.

bedeho · 2023-10-03T17:46:34Z

runtime-modules/content/src/migrations.rs

+                group_count
+            );
+            let pre_upgrade_group_count = usize::from_be_bytes(state.try_into().unwrap());
+            ensure!(


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?

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.

bedeho · 2023-10-03T17:49:16Z

runtime-modules/content/src/migrations.rs

+
+            use iterable_enums::ContentModerationActionV0;
+
+            <CuratorGroupById<T>>::translate_values(


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.

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 ?

mnaamani · 2023-10-04T07:40:27Z

Will address your questions bedeho, but will merge for Ignazio's sake.

mnaamani added 4 commits September 15, 2023 22:40

fix(Joystream#4589) restric content curation deletion powers

b4efb82

fix(content benchmarks) fix weights for content pallet

615713d

chore(integration tests) include missing content flows from full scen…

6c944c4

…ario

correct permission to delete video assets

8802a1e

mnaamani mentioned this pull request Sep 18, 2023

[Nara] Remove channel and video deletion by moderators mappings and CLI commands #4875

Merged

mnaamani added 6 commits September 18, 2023 15:14

fix(github workflow) github runner running out of space, run docker s…

27ebbd4

…ystem prune to resolve

drop ContentModerationAction DeleteChannel, DeleteVideo enum variants

3c91e99

Cleaner solution, and requires on runtime upgrade migration for the content pallet curator permissions

Merge branch 'nara' into nara-restric-curators-del-powers

985fad1

add missing import for StorageVersion trait

7ad10a7

content pallet: drop enum variants DeleteVideo, DeleteChannel from cu…

7c2ebba

…rator moderation actions

content-pallet: clippy fixes

a60933f

mnaamani added the nara-network label Sep 20, 2023

mnaamani marked this pull request as ready for review September 20, 2023 09:43

bedeho requested review from bedeho and dobertRowneySr September 20, 2023 10:08

mnaamani changed the title ~~Restric curators deletion powers~~ [Nara] Restric curators deletion powers Sep 25, 2023

sync-by-unito bot assigned dobertRowneySr Sep 25, 2023

dobertRowneySr mentioned this pull request Sep 25, 2023

[Nara] remove mappings for 'VideoDeletedByModerator' & 'ChannelDeletedByModerator' events Joystream/orion#193

Merged

dobertRowneySr reviewed Sep 26, 2023

View reviewed changes

dobertRowneySr suggested changes Sep 26, 2023

View reviewed changes

dobertRowneySr reviewed Sep 26, 2023

View reviewed changes

mnaamani added 8 commits September 26, 2023 22:53

content pallet migrations: log level/permission migrations

e759b32

Merge branch 'nara' into nara-restric-curators-del-powers

a2c62eb

Merge branch 'nara_remove_channel_and_video_deletion_mappings_and_cli…

4c3ebc7

…' into nara-restric-curators-del-powers

runtime upgrade test pre-post tests only

20bbf7f

Merge branch 'nara' into nara-restric-curators-del-powers

79e9d70

update chain metadata

60364d1

runtime upgrade test pre-upgrade create curator groups

f59e6d3

hire content lead before pre-upgrade runtime tests

8170034

use tx instead of call to construct extrinsic

8bff348

mnaamani requested a review from dobertRowneySr September 27, 2023 20:34

mnaamani added 3 commits September 28, 2023 10:03

Merge branch 'nara' into nara-restric-curators-del-powers

89e4320

integration tests additional checks on curator permissions migrations

9b70ead

content migrations - try-runtime log curator groups after migration

fa21827

dobertRowneySr approved these changes Sep 29, 2023

View reviewed changes

make some diskspace for github runner for docker builds of joystream/…

65cd38c

…node

bedeho approved these changes Oct 3, 2023

View reviewed changes

mnaamani merged commit 620fcbf into Joystream:nara Oct 4, 2023


		current.put::<Pallet<T>>();

		<T as frame_system::Config>::BlockWeights::get().max_block


		use iterable_enums::ContentModerationActionV0;

		<CuratorGroupById<T>>::translate_values(

Conversation

mnaamani commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dobertRowneySr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobertRowneySr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani commented Sep 26, 2023

Uh oh!

dobertRowneySr left a comment

Choose a reason for hiding this comment

Uh oh!

bedeho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaamani commented Oct 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mnaamani commented Sep 15, 2023 •

edited

Loading

bedeho left a comment •

edited

Loading