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 to polkadot v0.9.39 #4705

Merged
merged 93 commits into from
Jul 31, 2023
Merged

Update to polkadot v0.9.39 #4705

merged 93 commits into from
Jul 31, 2023

Conversation

mnaamani
Copy link
Member

@mnaamani mnaamani commented Mar 21, 2023

Implements #4704

Changes:

  • Update substrate fork polkadot-v0.9.39 + staking patch (BondingRestriction), vesting pallet changes already included in upstream substrate. Staking pallet: add bonding restriction substrate#11
  • Use new commit 50cf239147a6f569e563bcadec6c7a1c5ad5c67e
  • Update wasmtime version to 6.0.1 with latest security fix
  • bump runtime spec version to 3000 and joystream-node version
  • Use new compiler version - need rustc v1.66.0+ some pallets do not build with older versions.
  • Fix all broken rust code (pallets,runtime,node,chainspec builder)
  • Fix deprecated rust types/imports due to substrate codebase refactoring
  • Fix deprecated double_map::remove_prefix(), should use clear_prefix() instead - Supressed warning
    • used in proposals_engine pallet by reactivate_pending_constitutionality_proposals() and remove_proposal_data()
  • Fix all clippy (linter) errors.
  • Update chain-metadata
  • Fix weight generation code and scripts, and generate placeholder weights (not done on ref hardware)
  • Regenerate weights on ref hardware
  • Identify new polkadot-js api version to be used.. existing one seems to work but complains of lack of decoration on some rpc methods..
  • Drop patched ss58-registry to include joystream address prefix. Our changes are included and published in crates.io
  • Update runtime changelog
  • Add pallet staking runtime api to node rpc interface
  • Add all required substrate pallet migrations to runtime Executive
    • Enumerate all migrations expected between v0.9.24-1 to v0.9.39 (https://github.com/paritytech/polkadot/releases)
      • v0.9.27: Staking - pallet_staking::migrations::v10::MigrateToV10<Runtime> (from V7_0_0)
      • v0.9.30: Staking: pallet_staking::migrations::v11::MigrateToV11<Runtime,VoterList,StakingMigrationV11OldPallet> and pallet_staking::migrations::v12::MigrateToV12<Runtime>
      • v0.9.31: Multisig: pallet_multisig::migrations::v1::MigrateToV1<Runtime>
      • v0.9.34: pallet_election_provider_multi_phase::migrations::v1::MigrateToV1<Runtime> and pallet_balances::migration::MigrateToTrackInactive<Runtime, xcm_config::CheckAccount>
      • v0.9.37 - pallet_balances::migration::ResetInactive<Runtime>
        "We need to apply this migration again, because ResetInactive resets the state again." pallet_balances::migration::MigrateToTrackInactive<Runtime, xcm_config::CheckAccount> pallet_staking::migrations::v13::MigrateToV13<Runtime>
      • v0.9.38 - "Remove stale entries in the set id -> session index storage map (after this release they will be properly pruned after the bonding duration has elapsed)"
        pallet_grandpa::migrations::CleanupSetIdSessionMap<Runtime>
    • How to test properly -> add the try-runtime command feature to the node, and build with try-runtime feature flag.

Testing:

  • A new genesis config for Multiplier appears in tx_payment pallet, if not set during upgrade what value will it have and is it problematic. -> no the pallet has NextFeeMultiplierOnEmpty() -> 1 so it is not a problem and no migration is required.
  • Ensure integration tests work and pass
  • Ensure runtime upgrade integration tests pass - on runtime upgrade processor seem to stop processing, indexer appears to not correctly handle the block with upgrade. Currently working around it. Hydra Indexer unstable during runtime upgrade #4741 - on more recent run everything seems to have worked correctly https://github.com/Joystream/joystream/actions/runs/4872691408/jobs/8692880377 in this setup we kept the old node running for the upgrade, then started up new node after upgrade..
  • Test running new node against existing chain
  • Test running old node and upgrade to new runtime (covered by integration test)
    • any database migrations run - no
  • Test all expected substrate pallet runtime migrations run successfully.

Package Versioning:

  • Bump package versions

Misc Fixes:

TBD:

┆Issue is synchronized with this Asana task by Unito

@vercel
Copy link

vercel bot commented Mar 21, 2023

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

1 Ignored Deployment
Name Status Preview Updated (UTC)
pioneer-testnet ⬜️ Ignored (Inspect) May 23, 2023 6:23am

@mnaamani
Copy link
Member Author

mnaamani commented Apr 11, 2023

Integration test failing:

2023-04-06T11:50:00.385Z integration-tests:job:direct channel payment by members flow 0 failed:
Error: Could not find matching query-node event (expected 2966:3)!
    at DirectChannelPaymentsHappyCaseFixture.findMatchingQueryNodeEvent (/home/runner/work/joystream/joystream/tests/network-tests/src/Fixture.ts:100:13)
    at /home/runner/work/joystream/joystream/tests/network-tests/src/Fixture.ts:121:27
    at Array.forEach (<anonymous>)
    at DirectChannelPaymentsHappyCaseFixture.assertQueryNodeEventsAreValid (/home/runner/work/joystream/joystream/tests/network-tests/src/Fixture.ts:119:17)
    at /home/runner/work/joystream/joystream/tests/network-tests/src/fixtures/content/channelPayouts/DirectChannelPaymentHappyCaseFixture.ts:75:25
    at QueryNodeApi.tryQueryWithTimeout (/home/runner/work/joystream/joystream/tests/network-tests/src/QueryNodeApi.ts:521:9)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async DirectChannelPaymentsHappyCaseFixture.runQueryNodeChecks (/home/runner/work/joystream/joystream/tests/network-tests/src/fixtures/content/channelPayouts/DirectChannelPaymentHappyCaseFixture.ts:73:5)
    at async FixtureRunner.runQueryNodeChecks (/home/runner/work/joystream/joystream/tests/network-tests/src/Fixture.ts:190:5)
    at async FixtureRunner.runWithQueryNodeChecks (/home/runner/work/joystream/joystream/tests/network-tests/src/Fixture.ts:195:5)
2023-04-06T11:50:00.412Z integration-tests:job:direct channel payment by members [Failed]

edit:
Fixed in 5156465

@mnaamani mnaamani marked this pull request as ready for review May 4, 2023 04:36
@ignazio-bovo
Copy link
Contributor

I should mention that the new version should be 2002 and not 3000

Copy link
Contributor

@ignazio-bovo ignazio-bovo left a comment

Choose a reason for hiding this comment

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

I left a few questions, but overall seems very good

with:
context: .
file: joystream-node.Dockerfile
# arm64 cross compiling takes longer than 6h, so we only build for amd64
# platforms: linux/amd64,linux/arm64
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
# platforms: linux/amd64,linux/arm64
# platforms: linux/amd64

Since you said "only amd64" above.

Copy link
Member Author

Choose a reason for hiding this comment

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

The line is already commented out, but left it for reference if we want to add it back.
It might be better to drop this whole workflow and improve joystream-node-docker.yml which deploys two worker machines on AWS (amd64 and arm64) to build natively for each platform arch have the ansible playbook build for all the runtime profiles.

@@ -36,16 +36,15 @@ jobs:

- name: Build
env:
WASM_BUILD_TOOLCHAIN: nightly-2022-05-11
WASM_BUILD_TOOLCHAIN: nightly-2022-11-15
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this toolchain ver. recommended by substrate?

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 the nightly version at time the version was released
https://github.com/paritytech/polkadot/releases/tag/v0.9.39-rc6

@@ -78,6 +77,19 @@ jobs:
yarn cargo-checks && yarn cargo-build
./target/release/call-sizes
if: env.GIT_DIFF
- name: OnRuntimeUpgrade (try-runtime)
Copy link
Contributor

Choose a reason for hiding this comment

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

This make the runtime-upgrade in the network tests obsolete, if I correctly understood the try runtime feature.
Should we drop the checks using the fork-off and move them to the pre/post hooks in the OnRuntimeUpgrade impl.?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point.
There might still be some use for the runtime upgrade integration tests to see how it affects the query-node, storage and distributor nodes during an upgrade?

# https://doc.rust-lang.org/rustc/linker-plugin-lto.html
lto = "fat"
# https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units
codegen-units = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, where did you get the values from?

Copy link
Member Author

Choose a reason for hiding this comment

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

let len_fee =
<Runtime as pallet_transaction_payment::Config>::LengthToFee::weight_to_fee(&length);
let len_fee = <Runtime as pallet_transaction_payment::Config>::LengthToFee::weight_to_fee(
&Weight::from_ref_time(length),
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
&Weight::from_ref_time(length),
&Weight::from_parts(length, 0u64),

I did some research and this function will be deprecated: https://paritytech.github.io/substrate/master/sp_weights/struct.Weight.html#method.from_ref_time
The suggestion is what they recommend

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 51fb763

@mnaamani
Copy link
Member Author

I updated the PR from latest master branch.
I also updated the nara branch to match master.

@mnaamani
Copy link
Member Author

I should mention that the new version should be 2002 and not 3000

yes it has already been modified to 2002.

@mnaamani mnaamani requested review from ignazio-bovo and removed request for Lezek123 July 31, 2023 07:02
@mnaamani mnaamani merged commit 19de3f9 into nara Jul 31, 2023
52 checks passed
@mnaamani mnaamani deleted the update-to-polkadot-v0.9.39 branch September 23, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants