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

Update Substrate to 1.8.0 #2667

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Update Substrate to 1.8.0 #2667

merged 3 commits into from
Apr 9, 2024

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Apr 5, 2024

This PR updates Substrate to polkadot-sdk version 1.8.0. 1.9.0 will be updated separately due to more breaking changes there.

There were minor breaking changes that resulted in subspace-v4 branch in polkadot-sdk fork and subspace-v6 in frontier fork, but nothing major. Going forward to decrease maintenance burden around frontier I plan to only upgrade to polkadot-sdk releases when frontier is update to them so I don't have to fix compatibility myself that they will not merge upstream afterwards (frontier is always lagging behind).

I had to break enable_non_root_calls (and probably remove eventually) due to Sudo::key() being removed and other relevant information to identify if call is done by root or not seemingly not present in SignedExtension API.

The biggest changes this time came to RPC layer, but were mostly mechanical. TransactionExtension is something Gav authored that will likely cause more work upgrading in the future, but doesn't impact us as much for now.

Sync changes were not too breaking for our fork, but I like the direction they are moving, @shamil-gadelshin please take a look there.

As to migrations and hooks, they recommend to reduce on_initialize logic to the minimum, I don't think it is a hard requirement for now, but something to keep in mind going forward.

And I removed all versions from Substrate dependencies, they changed too many of them and it was painful and a bit pointless to update everywhere.

Interesting PRs on Substrate side:

The only potentially interesting PRs on frontier side are these:

P.S. Running this version locally already, seems to work so far.

Code contributor checklist:

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 5, 2024

Windows failed in CI because our Windows runners do not support symlinks right now, trying in my fork now to make sure things do actually pass once that is fixed

Copy link
Member

@shamil-gadelshin shamil-gadelshin left a comment

Choose a reason for hiding this comment

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

Sync changes were not too breaking for our fork, but I like the direction they are moving

I understood the changes from sync-related PR as a "preparation of common steps for different sync strategies that can be shared". If I understood it correctly then the new strategy can be composed out of existing steps and it can be convenient.

NingLin-P
NingLin-P previously approved these changes Apr 8, 2024
vedhavyas
vedhavyas previously approved these changes Apr 8, 2024
@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 8, 2024

I could still use some help with the failing test here

@NingLin-P
Copy link
Member

The upgrade breaks many domain staking tests due to this PR paritytech/polkadot-sdk#2657, which replaces the MaxHolds with VariantCountOf<T::RuntimeHoldReason>, the value is 1 in the test and 2 in production so an account can at most create 1 or 2 hold:
https://github.com/subspace/subspace/blob/287305a92b72b919b80537f4445a4c4de20895a3/crates/pallet-domains/src/tests.rs#L135
https://github.com/subspace/subspace/blob/287305a92b72b919b80537f4445a4c4de20895a3/crates/subspace-runtime/src/lib.rs#L411

The value is insufficient in both test and production, because an account can nominate multiple operators and for each nomination 2 holds can be created (one for the staking fund and another for withdrawal of the storage fund), besides, opening XDM channel also creates a hold.

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 8, 2024

Hm... so does this mean we need to have extra tracking for those holds now and combine them together? That seems like a very annoying breaking change upstream.

@NingLin-P
Copy link
Member

NingLin-P commented Apr 8, 2024

so does this mean we need to have extra tracking for those holds now and combine them together?

This is a static limit used as the upper bound of BoundedVec, we don't need to track the hold but instead need to count how many holds an account can create and set the limit properly, which also means limiting how many operators an account can nominate.

We can use the previous MaxHolds value of 100 for this PR and discuss this value in a separate issue since it may not be easy to decide immediately.

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 8, 2024

Feel free to push tweaks to this PR directly

@NingLin-P NingLin-P dismissed stale reviews from vedhavyas, shamil-gadelshin, and themself via 5c08787 April 8, 2024 16:14
@@ -133,11 +132,10 @@ impl pallet_domains::HoldIdentifier<Test> for HoldIdentifier {
}

impl VariantCount for HoldIdentifier {
const VARIANT_COUNT: u32 = mem::variant_count::<Self>() as u32;
const VARIANT_COUNT: u32 = 10;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is wrong though, check the description of the trait. It is supposed to return the same number as number of enum variants. Substrate can't use mem::variant_count because they rely on stable compiler, but we can because we use nightly.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this violates the usage of the trait, but the max number of hold limits is hardcoded to HoldIdentifier::VARIANT_COUNT in pallet-balances, which doesn't fit into our usage (even in test) so we need this hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a hack on top of the hack. If it works by accident right now, I'm fine having it temporarily with a TODO explaining why we are hardcoding it to the wrong number. And we should get rid of it before it bites us again.

Copy link
Member

Choose a reason for hiding this comment

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

For the reviewer, see more discussion at #2674. PR is forced push to add TODO comment.

The value is used as the max number of hold an account can create, it is set to 100
for now to workaround some issues with this limit, should revist the value later.

Signed-off-by: linning <[email protected]>
@nazar-pc nazar-pc added this pull request to the merge queue Apr 9, 2024
Merged via the queue into main with commit a6b576d Apr 9, 2024
9 checks passed
@nazar-pc nazar-pc deleted the update-substrate branch April 9, 2024 09:54
@nazar-pc nazar-pc mentioned this pull request Apr 9, 2024
1 task
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.

4 participants