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

Bump to polkadot-v1.3.0 #598

Merged
merged 69 commits into from
Jan 23, 2024
Merged

Bump to polkadot-v1.3.0 #598

merged 69 commits into from
Jan 23, 2024

Conversation

HCastano
Copy link
Collaborator

@HCastano HCastano commented Jan 19, 2024

This PR updates our version of Substrate from polkadot-v1.0.0 to polkadot-v1.3.0.
This represents several months worth of changes, so there is a lot going on here.

Some notable changes include:

When a breaking change was encountered I did my best to find the corresponding PR and
note it down in the commit message fixing the break.

For runtime config values, I tended to take the values from the most recent production
Polkadot runtime (v1.1.0). These are for the most part sane values to start with, and
we can tune them later if we need.

For reviewers: the place I would focus on is the runtime/lib.rs file. It's important to
at least sanity check any new configuraton values and make sure we're not introducing a
bug from cargo-culting stuff in.

If that looks fine, then the next thing would be the service.rs file, and finally a
quick look through the pallets.

Some helpful links:

Starts introducing some of the changes brought in by
paritytech/polkadot-sdk#1484.
This includes the deprecation of `Balances::transfer` from
paritytech/polkadot-sdk#1226.
Haven't fixed the Rust code yet though
Values were mostly taken from the Polkadot runtime config
Copy link

vercel bot commented Jan 19, 2024

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

Name Status Preview Comments Updated (UTC)
entropy-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2024 6:27pm

@HCastano HCastano marked this pull request as ready for review January 22, 2024 22:05
Copy link
Contributor

@ameba23 ameba23 left a comment

Choose a reason for hiding this comment

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

Great work. This looks like no easy task, and the Cargo.tomls are now looking much neater.

When you say there is still stuff to do relating to there no longer being separate controller accounts - what is that, and is that something also needed before a release?

crates/shared/Cargo.toml Show resolved Hide resolved
node/cli/src/service.rs Show resolved Hide resolved
@@ -38,28 +41,34 @@ fn pause_transaction_work() {
TransactionPause::pause_transaction(
RuntimeOrigin::signed(5),
b"Balances".to_vec(),
b"transfer".to_vec()
b"transfer_allow_death".to_vec()
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds scary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha yeah. What this dispatchable does is that it allows you to transfer an amount of tokens out of your account which brings your account balance under the existential deposit. Once this happens the account will be reaped from the chain.

The alternative to this is transfer_keep_alive in case you don't want to accidentally reap an account.

// One cent: $10,000 / MB
pub const PreimageByteDeposit: Balance = CENTS;
pub const PreimageBaseDeposit: Balance = deposit(2, 64);
pub const PreimageByteDeposit: Balance = deposit(0, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do these values come from?

Copy link
Collaborator Author

@HCastano HCastano Jan 23, 2024

Choose a reason for hiding this comment

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

These were taken from the production Polkadot runtime, whereas the values from before were from the Substrate node template (although probably at a later point in time). As far as the actual values here, not sure about why 64 bytes was used as the base deposit.

One thing I noticed here actually is that our deposit implementation is also still following that of the Substrate node template, and it's orders of magnitude lower than the Polkadot and Kusama implementation.

We may want to re-consider this before launch. cc @JesseAbram

Copy link
Member

@JesseAbram JesseAbram left a comment

Choose a reason for hiding this comment

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

this looked brutal I do find it weird that tests are passing but metadata did not need to be updated but I guess no interfaces were changed

@@ -1366,7 +1456,6 @@ construct_runtime!(
AuthorityDiscovery: pallet_authority_discovery =34,
Offences: pallet_offences = 35,
Historical: pallet_session_historical = 36,
RandomnessCollectiveFlip: pallet_insecure_randomness_collective_flip = 37,
Copy link
Collaborator Author

@HCastano HCastano Jan 23, 2024

Choose a reason for hiding this comment

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

this looked brutal I do find it weird that tests are passing but metadata did not need to be updated but I guess no interfaces were changed

@JesseAbram good observation. As far as our pallets are concerned, no interfaces were changed, only imports and changes to internal calls.

For the runtime though we do have changes, for example I removed this pallet. I guess the reason why all that tests pass is because we never made use of this anywhere, or any of the other pallets with API changes.

I've included updated metadata in this PR.

@HCastano
Copy link
Collaborator Author

HCastano commented Jan 23, 2024

When you say there is still stuff to do relating to there no longer being separate controller accounts - what is that, and is that something also needed before a release?

A controller account was used as a way to interact on behalf of a stash account. However, with the improved support for proxy accounts recently there's an argument to be made that people should just use a staking specific proxy instead of a controller account. From what I can tell there's been a lot of discussion here starting from early last year - so I gotta catch up on what's actually happened before doing anything.

We don't need to do this before this release since they're only soft deprecated right now. We'll really have to get on it once they're actually removed from the codebase.

@HCastano HCastano merged commit 3511676 into master Jan 23, 2024
10 checks passed
@HCastano HCastano deleted the hc/bump-to-polkadot-v1.3.0 branch January 23, 2024 18:34
@ameba23
Copy link
Contributor

ameba23 commented Jan 23, 2024

Congrats on getting this over the line @HCastano

@HCastano HCastano added the Bump `spec_version` A change which affects the current runtime behaviour label Jan 23, 2024
@ameba23 ameba23 mentioned this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bump `spec_version` A change which affects the current runtime behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants