Skip to content
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

Mainnet: liberated runtime (drop sudo pallet) #4478

Conversation

mnaamani
Copy link
Member

@mnaamani mnaamani commented Dec 1, 2022

Dropping sudo pallet and updated chain metadata and types.
This of course breaks a few checks, any code that depends on making sudo calls.

@vercel
Copy link

vercel bot commented Dec 1, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
pioneer-testnet ⬜️ Ignored (Inspect) Dec 6, 2022 at 5:38AM (UTC)

Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Should we also remove pallet-sudo from the runtime dependencies?
  2. I think we can also remove sudo_upload_data_objects from the storage pallet.
  3. Argus and Colossus have dev commands that rely on sudo, so the builds are now failing. We need to remember to remove them before the release. Some devops scripts and api-scripts will also need an update.

@mnaamani
Copy link
Member Author

mnaamani commented Dec 5, 2022

  1. Should we also remove pallet-sudo from the runtime dependencies?

    1. I think we can also remove sudo_upload_data_objects from the storage pallet.

    2. Argus and Colossus have dev commands that rely on sudo, so the builds are now failing. We need to remember to remove them before the release. Some devops scripts and api-scripts will also need an update.

  1. Makes sense
  2. Done
  3. Fix builds of colossus and argus. Are you suggesting to also remove the dev commands from mainnet branch ?

Only made a few minor changes to some devops and github workflows, but left api-scripts and migration-scripts intact to determine what to do later.

Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional notes:

  • There's still a mention of sudo in devops/aws/README.md

Besides those minor issues everything looks good.
I ran a following tests to confirm storage/distribution nodes from mainnet branch remain compatible with the no-sudo runtime:

  1. Checked-out mainnet branch
  2. yarn build:packages
  3. RUNTIME_PROFILE=TESTING yarn start
  4. In a separate workspace I checked out this PR's branch and ran: RUNTIME_PROFILE=TESTING ./scripts/cargo-build.sh
  5. Executed a runtime upgrade (via sudo) to this PR's runtime using https://polkadot.js.org/apps
  6. Commented out all tests except channelsAndVideosCliJob, initDistributionBucket and initStorageBucket from tests/integration-tests/scenarios/full.ts
  7. Copied tests/network-tests/output.json file from the workspace where I had mainnet branch checked-out to the workspace where I had this PR checked-out
  8. Executed REUSE_KEYS=true ./tests/network-tests/run-test-scenario.sh full on this PR's branch
  9. Tests executed successfully, see output below:
$ node -r ts-node/register --unhandled-rejections=strict src/scenarios/full.ts
  integration-tests:api-factory Connecting to chain, attempt 1.. +0ms
Runtime Version: 12.1002.0
  integration-tests:scenario Full +0ms
  integration-tests:job:manage channels and videos through CLI Running +0ms
  integration-tests:flow:cliChannelsAndVideos Started +0ms
  integration-tests:flow:cliChannelsAndVideos Creating a channel... +23s
  integration-tests:query-node-api:query getChannelById({"id":"1"}) +0ms
  integration-tests:query-node-api:try:toString()) Unexpected query result (expected false to equal true) +0ms
  integration-tests:query-node-api:try:toString()) Retrying query in 9000ms... +0ms
  integration-tests:query-node-api:query getChannelById({"id":"1"}) +9s
  integration-tests:flow:cliChannelsAndVideos Updating channel... +29s
  integration-tests:query-node-api:query getChannelById({"id":"1"}) +8s
  integration-tests:query-node-api:try:toString()) Unexpected query result (expected 'Test channel' to equal 'Test channel [UPDATED!]') +0ms
  integration-tests:query-node-api:try:toString()) Retrying query in 9000ms... +0ms
  integration-tests:query-node-api:query getChannelById({"id":"1"}) +9s
  integration-tests:flow:cliChannelsAndVideos Creating a video... +17s
  integration-tests:query-node-api:query getVideoById({"videoId":"1"}) +7s
  integration-tests:query-node-api:try:toString()) Unexpected query result (Video not found) +0ms
  integration-tests:query-node-api:try:toString()) Retrying query in 9000ms... +0ms
  integration-tests:query-node-api:query getVideoById({"videoId":"1"}) +9s
  integration-tests:flow:cliChannelsAndVideos Updating video... +17s
  integration-tests:query-node-api:query getVideoById({"videoId":"1"}) +8s
  integration-tests:query-node-api:try:toString()) Unexpected query result (expected 'Test video' to equal 'Test video [UPDATED!]') +0ms
  integration-tests:query-node-api:try:toString()) Retrying query in 9000ms... +0ms
  integration-tests:query-node-api:query getVideoById({"videoId":"1"}) +9s
  integration-tests:flow:cliChannelsAndVideos Done +17s
  integration-tests:job:manage channels and videos through CLI [Succeeded] +2m
  integration-tests:job:init storage and distribution buckets via CLI Running +0ms
  integration-tests:flow:initDistributionBucketViaCLI Started +0ms
  integration-tests:flow:initStorageBucketViaCLI Started +0ms
  integration-tests:flow:initStorageBucketViaCLI Done +26s
  integration-tests:flow:initDistributionBucketViaCLI Done +42s
  integration-tests:job:init storage and distribution buckets via CLI [Succeeded] +42s
Job Results:
manage channels and videos through CLI: Succeeded
init storage and distribution buckets via CLI: Succeeded
Writing generated account to output.json
Done in 156.25s.
Done in 156.46s.

real	2m36,571s
user	2m55,918s
sys	0m6,312s

Comment on lines 3052 to 3054
/// Create a dynamic bag. Development mode.
/// <weight>
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed there's a leftover comment

Suggested change
/// Create a dynamic bag. Development mode.
/// <weight>
///

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funny this comment is wrong for the function :) will fix.

@@ -74,6 +74,9 @@ scenario('Full', async ({ job, env }) => {
proposalsDiscussion,
]).requires(councilFailuresJob)

return
// Below flows depend on sudo which has been disabled. So skipping for now.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like:

  • creatingMembers,
  • creatingFoundingMembers,
  • invitingMebers and
  • proposals

flow also depends on sudo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I noticed it failed in CI checks. I think will leave the effort to fix integrations properly in a future PR as it will need quite a bit of refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also make sense to remove the createMembership dispatch from membership pallet, since they require root origin and I doubt we will add a proposal type that create a membership.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

@@ -48,7 +48,6 @@
environment:
JOYSTREAM_NODE_WS: 'ws://{{ inventory_hostname }}:9944/'
# TREASURY_ACCOUNT_URI: '{% if endowed_key is defined %}{{ endowed_key }}{% else %}{{ sudo_key }}{% endif %}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# TREASURY_ACCOUNT_URI: '{% if endowed_key is defined %}{{ endowed_key }}{% else %}{{ sudo_key }}{% endif %}'
# TREASURY_ACCOUNT_URI: '{{ endowed_key }}'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will drop all env variables here expect the ws endpoint.
The other variables where there because previously instead of starting only query-node the playbook also attempted to start a faucet, and initialized it with the faucet setup scenario from integration tests.

@@ -214,13 +198,10 @@ export async function sendAndFollowNamedTx<T>(
api: ApiPromise,
account: KeyringPair,
tx: SubmittableExtrinsic<'promise'>,
sudoCall = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also be removed from the parameters list in the comment above (@param sudoCall)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

@mnaamani
Copy link
Member Author

mnaamani commented Dec 6, 2022

Thank you for verifying the behavior of the apps after runtime upgrade.

Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'll leave it up to you whether to also fix #4478 (comment) and remove the createMembership dispatch as part of this PR

@mnaamani mnaamani merged commit 58d2775 into Joystream:mainnet-liberated Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants