Update to Polkadot v1.1.0 #536

gianfra-t · 2024-06-04T13:18:36Z

Issue #534.

Changes in Pallets

Removal of Header and BlockNumber in frame_system.
Replacement of type Index with Nonce.
Change the macro use for defining the Default trait of the GenesisConfig struct with a manual definition, to fix an error on the macro construct_runtime when using the previous #[derive(default)] macro.
Default trait has to be defined for all GenesisConfig structs, not just when std feature is enabled.
construct_runtime can be used without defining the components of the module. This is less error prone since hard to debug errors may appear when missing a component. (If we are okay with this change, I can later modify the rest).
Replace the use of the to-be-deprecated GenesisBuild.
New field in pallet_aura config. AllowMultipleBlocksPerSlot should be false if used in normal scenario (not augmented with a custom pallet).
New field in pallet_grandpa MaxNominators. The value is set at 1000, previously this was set by default as 200. This value is used for weight calculations.

Node changes

Changes made using as base the substrate-node-template from the developer hub, at a commit using version 1.0.0.

Most relevant change is how the off-chain worker is spawned. Here is the change for the template between the relevant version.

Other code changes

Modification of parameters of SubxtClientConfig .

rpc_max_response_size is defined as the default value, in megabytes.
rpc_max_request_size is deprecated and has no effect.

Subxt update

subxt had to be updated from 0.25.0 to 0.28.0 due to a conflict with schnorkell package. This is the lowest version that solves that issue. Nevertheless, this new version introduced some breaking changes that have to be handled:

flag substrate-compat has to be used now for many of the functions and exports regarding substrate compatibility.
Pallet and error information of ModuleError are now accessed differently, which impacts this match.
Introduction of dynamic types.. This mostly affects the way in which we replace types define on the metadata with type defined in the code. Types that do not implement EncodeAsType and DecodeAsType now should be wrapped in a Static struct, which is the case with CurrencyId and all the corresponding uses of it on the code.
To avoid wrapping as static the type AccountId, we use the AccountId type from subxt.
The last point introduces the need to transform from Keyring to subxt's AccountId, which is done now as in the package tests. Here is an example on our code.
Since the implementation of AccountId from subxt does not have some of the traits required, we need to transform to the sp version of it before using that functionality, a good example is here where we use need sp_runtime version for hashing.

…pe to avoid Static wrapper

gianfra-t · 2024-06-06T20:33:12Z

@pendulum-chain/devs keep in mind that for some reason the tests fail on the CI environment but locally they seem to be okay, so that needs to be solved first. If anyone has any idea about that Timeout please let me know!

…/runs/10079792308/job/27899770357?pr=536#step:14:24

* add extra cargo.toml specially for CI testing.

ebma

We need to change the version from 1.0.9 back to 1.0.10 again. Also, I think at least one merge commit has to be faulty. At least I found a lot of changes that look like regressions and should be reverted.

Why exactly do we need the new all-features feature?

.github/workflows/ci-main.yml

ebma · 2024-07-26T14:58:56Z

.cargo/config.toml

@b-yap we just recently added this file so I think we want to keep it, right?

ebma · 2024-07-26T14:59:51Z

clients/README.md

-
-> **Note**: [Rustflags are required](https://github.com/tokio-rs/console?tab=readme-ov-file#instrumenting-your-program) to make `tokio-console` work.
-It has been defined directly in [.cargo/config.toml](../.cargo/config.toml). 
-
+The vault is `tokio-console` ready, with the feature **_`allow-debugger`_**. _Remember to [set the rustflags](https://github.com/tokio-rs/console?tab=readme-ov-file#instrumenting-your-program)!_
 ```
-cargo run --bin vault --features allow-debugger
+RUSTFLAGS="--cfg tokio_unstable" cargo run --bin vault --features allow-debugger


This also looks like a regression to me.

ebma · 2024-07-26T15:00:10Z

clients/runner/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "runner"
-version = "1.0.10"
+version = "1.0.9"


We shouldn't turn back the version.

Sure! Must be as you say a faulty merge commit to main, because it is up-to-date now.

ebma · 2024-07-26T15:23:09Z

clients/runtime/src/lib.rs

@@ -111,13 +139,12 @@ pub struct WrapperKeepOpaque<T> {
 pub struct SpacewalkRuntime;

 impl Config for SpacewalkRuntime {
- type Index = Index;
- type BlockNumber = BlockNumber;
+ type AssetId = ();


What is the AssetId used for? Is it okay to leave it empty?

It is a new field from the subxt Config trait, but as the comment of this type says, I believe it is not relevant for our standalone testchain.

ebma · 2024-07-26T15:31:09Z

clients/vault/src/system.rs

- async fn try_shutdown_agent(&mut self) {
- let opt_agent = self.agent.clone();
- self.agent = None;
-
- if let Some(arc_agent) = opt_agent {
- tracing::info!("try_shutdown_agent(): shutting down agent");
- arc_agent.shutdown().await;
- } else {
- tracing::debug!("try_shutdown_agent(): no agent found");
- }
- }


ebma · 2024-07-26T15:32:49Z

clients/wallet/src/mock.rs

- _public_key(dest_secret)
-}
-
-pub fn default_source() -> PublicKey {
- let source_secret = get_source_secret_key_from_env(IS_PUBLIC_NETWORK);
- _public_key(source_secret)
-}
-
-fn _public_key(secret: String) -> PublicKey {
- let dest_secret_key = SecretKey::from_encoding(secret).expect("should work");


Do we not need a default source anymore?

ebma · 2024-07-26T15:33:17Z

clients/wallet/src/operations.rs

+ const DEFAULT_SOURCE_PUBLIC_KEY: &str =
+ "GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN";
+ const DEFAULT_DEST_PUBLIC_KEY: &str =
+ "GBWYDOKJT5BEYJCCBJRHL54AMLRWDALR2NLEY7CKMPSOUKWVJTCFQFYI";
+
 fn default_testing_stellar_pubkeys() -> (PublicKey, PublicKey) {
- (default_source(), default_destination())
+ (
+ public_key_from_encoding(DEFAULT_SOURCE_PUBLIC_KEY),
+ public_key_from_encoding(DEFAULT_DEST_PUBLIC_KEY),
+ )


Also regression?

ebma · 2024-07-26T15:34:16Z

pallets/issue/src/benchmarking.rs

@@ -28,17 +29,12 @@ fn deposit_tokens<T: crate::Config>(
 account_id: &T::AccountId,
 amount: BalanceOf<T>,
 ) {
- assert_ok!(<orml_currencies::Pallet<T>>::deposit(currency_id, account_id, amount));
+ assert_ok!(<orml_tokens::Pallet<T>>::deposit(currency_id, account_id, amount));


Regression. Using orml_currencies here is important.

ebma · 2024-07-26T15:52:07Z

testchain/node/src/service.rs

@@ -24,6 +25,10 @@ use spacewalk_runtime_testnet::RuntimeApi as TestnetRuntimeApi;
 // Native executor instance.
 pub struct TestnetExecutor;

+/// The minimum period of blocks on which justifications will be
+/// imported and generated.
+const GRANDPA_JUSTIFICATION_PERIOD: u32 = 512;


Have we tried fiddling with this parameter? I found a test in the polkadot-sdk where it's defined to just 32. I honestly don't really understand the implications of this parameter, just something that came to my mind.

You mean for the test issue? No, this is the standard value used in one of the templates of the polkadot-sdk. Yet, for our instant seal this shouldn't matter since we are not using the consensus at all.

gianfra-t · 2024-07-29T09:16:44Z

EDIT: please ignore this comment as we will no longer need this feature, we can use it as in this commit.

@ebma the all-features is used for filtering out the application of the metadata types multiple times when all the features are enabled. Apparently this was not a problem with the previous versions of subxt and it probably took the last one, but after the upgrade we got a collision since it tried to use the types from all the chains on the substitution.

Since when running the test like we generally do with the --all-features command it will as well enable this one. The name does not necessarily needs to match and could be anything, it will just be enabled due to the command as well.

…time macro

…ruct_runtime macro" This reverts commit 33a6067.

…in construct_runtime macro"" This reverts commit 17f8100.

…acro

This reverts commit 3345c90.

gianfra-t · 2024-07-30T15:03:14Z

@pendulum-chain/devs I wanted to comment on the test issue we were facing a while ago, and summarize how we are dealing with it at the moment.

As a summary of the attempts and experiments using extra logs in this PR we identify that the issue happens when sealing a block on linux operating systems (especially in low spec CI runners), the creation of a new block and the application of the inherents consumes all the time specified by the whole process of applying extrinsics, therefore ending up hitting the deadline condition before applying a single extrinsic. Please refer to the following example CI run, logs were set before and after each of these operations. We can see that we spend already 4 seconds on each by the timestamps.

Therefore, in order to workaround this issue, we modify this constant to a larger number (in this case, 60 seconds) such that the extrinsics have enough time to be applied. We can then use this forked polkadot-sdk repo only on the CI runs, by just renaming the Cargo.toml that contains the patch during the run.

ebma

The last CI job failed due to a shaky test so let's assume that it works generally. Thanks for all your efforts with this issue @gianfra-t. Let's merge this now.

gianfra-t added 8 commits May 31, 2024 17:02

update dependencies, fixing compilation errors (WIP)

792522b

update pallets to 1.1.0

d68c728

fix subxt feature flags

2e01bf8

fix client configuration new fields

691c276

fix clippy issues and missing std flag

ce07e26

update fixes and missing configs for runtimes

aa3b152

fixes to node configuration (wip)

43b8e8e

node client fixes

64f1e2e

gianfra-t linked an issue Jun 4, 2024 that may be closed by this pull request

Update to Polkadot version 1.1.0 #534

Closed

gianfra-t added 10 commits June 5, 2024 09:52

bump subxt to minimum viable version

757fc15

use static wrapper for account_id and currency_id metadata replacement

791bcec

use account_id struct from subxt implementing Encode and Decode as ty…

3c789cc

…pe to avoid Static wrapper

fix account type conversion using new subxt type

5a08ccf

missing flag, cleanup unused types

0b51df6

temprary remove conditional metadata

fcd9d62

use foucoco metadata only

fb69df3

warnings and type conversion

2d3bb68

re-implement hashable trait

3f2869f

replace account_id on tests

d2662c6

gianfra-t force-pushed the polkadot-v1.1.0 branch from d2d452a to d2662c6 Compare June 6, 2024 15:05

gianfra-t added 3 commits June 6, 2024 14:35

use subxt version 0.29 across crates

4c93ad2

modification of service config values

d0aab2b

testing increase in timeout for tests

42922b6

gianfra-t marked this pull request as ready for review June 6, 2024 20:30

gianfra-t requested a review from a team June 6, 2024 20:31

gianfra-t force-pushed the polkadot-v1.1.0 branch 4 times, most recently from 8f6e331 to db6e39d Compare June 7, 2024 16:49

b-yap and others added 6 commits July 24, 2024 23:42

ignore failing subxt tests and print out proof error

8268af8

update cargo lock https://github.com/pendulum-chain/spacewalk/actions…

b43a1eb

…/runs/10079792308/job/27899770357?pr=536#step:14:24

fix cargo.lock; update only dia-oracle

ac9eb47

update network

714cf5d

add back iowa

238b1db

[DO NOT MERGE] Test issue on Linux due to update. (#540)

a2f1957

* add extra cargo.toml specially for CI testing.

ebma reviewed Jul 26, 2024

View reviewed changes

gianfra-t added 5 commits July 28, 2024 11:12

stellar relay config sdftest2 ip change

7489452

fixing some regressions from incomplete merge

62ddca5

fixing more merge issues

ae87c82

missing changes from #538

d5ffec6

fix AccountId PrettyPrint impl

a518fb4

gianfra-t added 13 commits July 29, 2024 06:53

fix compare spec version name

02621db

fix benchmark changes merge issue

ba8724d

remove when possible explicit declaration of pallets in construct_run…

33a6067

…time macro

cargo fmt

2ca77fc

remove all-features feature

15ef005

testing inherent modification

3345c90

Revert "remove when possible explicit declaration of pallets in const…

17f8100

…ruct_runtime macro" This reverts commit 33a6067.

Revert "Revert "remove when possible explicit declaration of pallets …

7422812

…in construct_runtime macro"" This reverts commit 17f8100.

regenerate metadata after removal of explicit pallet delcaration in m…

5f1058f

…acro

Revert "testing inherent modification"

daa838c

This reverts commit 3345c90.

use pendulum's polkadot-sdk fork with modified constant

512a09b

modification of stellar sdftest3 ip

f46a753

modify also vault resources sdftest3 ip

0026854

ebma approved these changes Jul 30, 2024

View reviewed changes

gianfra-t merged commit d74009b into main Jul 31, 2024
1 of 2 checks passed

gianfra-t deleted the polkadot-v1.1.0 branch July 31, 2024 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Polkadot v1.1.0 #536

Update to Polkadot v1.1.0 #536

gianfra-t commented Jun 4, 2024 •

edited

Loading

gianfra-t commented Jun 6, 2024

ebma left a comment

ebma Jul 26, 2024

ebma Jul 26, 2024

ebma Jul 26, 2024

gianfra-t Jul 29, 2024

ebma Jul 26, 2024

gianfra-t Jul 29, 2024

ebma Jul 26, 2024

ebma Jul 26, 2024

ebma Jul 26, 2024

ebma Jul 26, 2024

ebma Jul 26, 2024

gianfra-t Jul 29, 2024

gianfra-t commented Jul 29, 2024 •

edited

Loading

gianfra-t commented Jul 30, 2024

ebma left a comment

Update to Polkadot v1.1.0 #536

Update to Polkadot v1.1.0 #536

Conversation

gianfra-t commented Jun 4, 2024 • edited Loading

Changes in Pallets

Node changes

Other code changes

Subxt update

gianfra-t commented Jun 6, 2024

ebma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianfra-t commented Jul 29, 2024 • edited Loading

gianfra-t commented Jul 30, 2024

ebma left a comment

Choose a reason for hiding this comment

gianfra-t commented Jun 4, 2024 •

edited

Loading

gianfra-t commented Jul 29, 2024 •

edited

Loading