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

Fix: Local runtime upgrade scripts #1308

Merged
merged 10 commits into from
May 25, 2023
Merged

Fix: Local runtime upgrade scripts #1308

merged 10 commits into from
May 25, 2023

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Apr 6, 2023

Description

How to test

cd scripts/js/upgrade
yarn execute $path_to_wasm "test-release-v0.10.25-latest" "centrifuge-local"

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@wischli wischli added D0-ready Pull request can be merged without special precaution and notification. dependencies Pull requests that update a dependency file. labels Apr 6, 2023
@wischli wischli requested a review from branan April 6, 2023 16:20
@wischli wischli self-assigned this Apr 18, 2023
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

question 👀

@@ -4,16 +4,24 @@ const fs = require('fs')
const util = require('util');
const exec = util.promisify(require('child_process').exec);

const PREIMAGE_LENGTH_BOUND = 34;
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these values obtained or updatable based on what in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

I.e, are these values always the same for any pre-image proposing authorising a runtime upgrade? Or do we need to keep changing them? If so, what's the best way to do that? I am 90% we don't, but just want to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will definitely improve the name to something like AUTHORIZE_UPGRADE_PREIMAGE_LENGTH_BOUND. Regarding the value itself: The WASM code of authorize_upgrade(code) is hashed with H256 which encoded to 32 bytes. I remember to have tried with 32 as bound as well and failed, so I suppose the other two are some wrapper bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope 71da5cd brings sufficient clarity

NunoAlexandre
NunoAlexandre previously approved these changes May 24, 2023
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

My question is not a blocker, if you have tested these changes and the code looks fine so it's good to go for me.

// 39 from edemocracy.xternalProposeMajority(Lookup(H256, 34)))
// 42 from democracy.fastTrack(H256, ...)
// 1 from utility.batchAll
// 2 extra
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the "2 extra" bytes is the first for the pallet index and the second for the extrinsic index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Needs to be >= 34
// 32 bytes from the encoding of the H256 hashed WASM blob
// 2 for extra stuff
const AUTHORIZE_UPGRADE_PREIMAGE_LENGTH_BOUND = 34;
Copy link
Contributor

Choose a reason for hiding this comment

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

Leaving this one here and I feel this is really something missing in the polkadot dev ecosystem: https://ruudvanasseldonk.com/2022/03/20/please-put-units-in-names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree and applied in 860e281

NunoAlexandre
NunoAlexandre previously approved these changes May 25, 2023
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Left a nugget but again not a blocker for me. Thanks for fixing this and taking the suggestions in 🏄‍♂️

Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

❤️

@wischli wischli enabled auto-merge (squash) May 25, 2023 16:25
@wischli wischli merged commit a5ab082 into main May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification. dependencies Pull requests that update a dependency file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants