Move all tests to use u32 as BlockNumber

[gupnik]

Is there an existing issue?

• [X] I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• [X] This is not a support question.

Description of bug

As we expand the usage of derive-impl being tracked in https://github.com/paritytech/polkadot-sdk/issues/171, it seems that we would need to maintain two default configs to support both u32 and u64 BlockNumber types.

Since there's no apparent reason to use different types for tests, it might be better to move them to use u32.

Steps to reproduce

No response

[philoniare]

@gupnik I'd like to work on this one, could you please assign it to me?

[ggwpez]

@gupnik looking at this issue again: Why u32 and not u64?
It looks like our TestDefaultConfig uses u64 for BlockHashCount.

[kianenigma]

@gupnik looking at this issue again: Why u32 and not u64? It looks like our TestDefaultConfig uses u64 for BlockHashCount.

I don't think there is any reasonable advantage to either. I suggest @philoniare to do whichever produces the smaller diff.

[philoniare]

@gupnik looking at this issue again: Why u32 and not u64? It looks like our TestDefaultConfig uses u64 for BlockHashCount.

I don't think there is any reasonable advantage to either. I suggest @philoniare to do whichever produces the smaller diff.

Got it, will do. Appreciate the guidance 🙏

[gupnik]

@gupnik looking at this issue again: Why u32 and not u64? It looks like our TestDefaultConfig uses u64 for BlockHashCount.

@ggwpez I think the discussion at that time was to make it use the same type as the relay, but yes fine to use either.

[philoniare]

Ended up choosing the u64 as it had smaller diff

[xlc]

can we make it u32 😮‍💨 all relay and system parachains are using u32 and there is zero reason to use u64 let's correct the mistake, not make it even worse just because it had smaller diff

[gupnik]

can we make it u32 😮‍💨 all relay and system parachains are using u32 and there is zero reason to use u64 let's correct the mistake, not make it even worse just because it had smaller diff

Yeah, I initially thought the same but does it really matter? The framework accepts any type with certain constraints and it should be acceptable to add any type for tests that satisfies those constraints?

[xlc]

it does. we had bad tests with AccountId = u64 because the type size is too small and causing unexpected behaviors. yes the test environment doesn’t need to match to prod env but let’s don’t introduce unnecessary differences for no benefits. it can be different, only if it helps.

[gupnik]

it does. we had bad tests with AccountId = u64 because the type size is too small and causing unexpected behaviors

It makes sense when the type size used in test env is smaller than the prod one. However, it's the opposite here and its difficult to imagine a missing scenario in this case.

let’s don’t introduce unnecessary differences for no benefits. it can be different, only if it helps.

Yeah, agreed to this one. @philoniare We might need to go back to u32 ones. Hopefully, it won't be that difficult now that you are already familiar with the process.

[philoniare]

@gupnik I'll unassign myself on this one. My old macbook is unable to handle a workspace level build, so I've been using the CI tests as a feedback loop, which results in a pretty bad dev experience. I'd rather focus on individual pallet level tickets that I can build locally.

[gupnik]

@gupnik I'll unassign myself on this one. My old macbook is unable to handle a workspace level build, so I've been using the CI tests as a feedback loop, which results in a pretty bad dev experience. I'd rather focus on individual pallet level tickets that I can build locally.

Understandable. Thanks for looking into it!

You might choose to do it in separate PRs with a few pallets at a time, if you would still want to take it to completion.

[gupnik]

As discussed, assigning this to you @Gilt0

[Gilt0]

@gupnik Just to make sure I am on the right track

The command % grep -ri BlockNumber * | grep 'type BlockNumber = ' | grep -v u32 outputs

alliance/src/mock.rs:type BlockNumber = u64;
authority-discovery/src/lib.rs: pub type BlockNumber = u64;
democracy/src/migrations/unlock_and_unreserve_all_funds.rs:             type BlockNumber = BlockNumberFor<Test>;
election-provider-multi-phase/src/lib.rs:       type BlockNumber = BlockNumberFor<T>;
election-provider-multi-phase/src/mock.rs:pub(crate) type BlockNumber = u64;
election-provider-multi-phase/src/mock.rs:      type BlockNumber = BlockNumber;
election-provider-multi-phase/src/mock.rs:      type BlockNumber = BlockNumber;
election-provider-support/src/onchain.rs:       type BlockNumber = frame_system::pallet_prelude::BlockNumberFor<T::System>;
election-provider-support/src/onchain.rs:       type BlockNumber = u64;
election-provider-support/src/onchain.rs:                       type BlockNumber = BlockNumber;
election-provider-support/src/lib.rs://!         type BlockNumber = BlockNumber;
election-provider-support/src/lib.rs://!         type BlockNumber = BlockNumber;
election-provider-support/src/lib.rs:   type BlockNumber = BlockNumber;
fast-unstake/src/mock.rs:pub type BlockNumber = u64;
fast-unstake/src/mock.rs:       type BlockNumber = BlockNumber;
merkle-mountain-range/src/tests.rs:type BlockNumber = frame_system::pallet_prelude::BlockNumberFor<Test>;
nomination-pools/benchmarking/src/mock.rs:type BlockNumber = u64;
nomination-pools/test-staking/src/mock.rs:type BlockNumber = u64;
nomination-pools/src/mock.rs:pub type BlockNumber = u64;
root-offences/src/mock.rs:type BlockNumber = u64;
safe-mode/src/lib.rs:   type BlockNumber = BlockNumberFor<T>;
staking/src/mock.rs:pub(crate) type BlockNumber = u64;
staking/src/pallet/impls.rs:    type BlockNumber = BlockNumberFor<T>;
support/test/compile_pass/src/lib.rs:pub type BlockNumber = u64;
support/test/tests/construct_runtime_ui/use_undefined_part.rs:pub type BlockNumber = u64;
support/test/tests/construct_runtime_ui/both_use_and_excluded_parts.rs:pub type BlockNumber = u64;
support/test/tests/construct_runtime_ui/exclude_undefined_part.rs:pub type BlockNumber = u64;
support/test/tests/composite_enum.rs:pub type BlockNumber = u64;
support/test/tests/issue2219.rs:pub type BlockNumber = u64;
support/test/tests/construct_runtime.rs:pub type BlockNumber = u64;
support/test/tests/runtime_metadata.rs:pub type BlockNumber = u64;
support/procedural/src/lib.rs:///     type BlockNumber = u64;
support/procedural/src/lib.rs:///     type BlockNumber = <TestDefaultConfig as DefaultConfig>::BlockNumber;
system/src/lib.rs:      type BlockNumber = BlockNumberFor<T>;
tips/src/migrations/unreserve_deposits.rs:              type BlockNumber = BlockNumberFor<Test>;
utility/src/tests.rs:type BlockNumber = u64;

In cases where u64 appears I change to u32? And in less straight forward definitions (eg type BlockNumber = BlockNumberFor<T>;) I dig a little further to ensure that u32 is indeed used - I can get inspiration from pull request #3285 by @philoniare?

Thank you

[xlc]

if possible, fix it one pallet at a time. unless the fix is trivial that only requires search and replace. otherwise it is going to take very long time to review and you will keep having merge conflicts

[philoniare]

Yep @Gilt0, you can just perform the reverse operation on my PR. Please note that you'll need to update the BlockHashCount whenever necessary as well

[Gilt0]

if possible, fix it one pallet at a time. unless the fix is trivial that only requires search and replace. otherwise it is going to take very long time to review and you will keep having merge conflicts

Are you suggesting that I create a pull request for each pallet? Or can I make only one? In any case, I was planning on fixing one pallet at time, build, then move on to the next one if it is successful.

Thank you for your help 🤗

[Gilt0]

Yep @Gilt0, you can just perform the reverse operation on my PR. Please note that you'll need to update the BlockHashCount whenever necessary as well

Duely noted thank you 🤗

[xlc]

One PR for each chunk of work, which could be one big pallet or a few small pallets

[gupnik]

@Gilt0 You are on the right track 👍

[Gilt0]

@gupnik @xlc @philoniare Thank you!

[Gilt0]

FYI

I will modify these pallets

% grep -ri 'type BlockNumber = ' * | grep -v 'u32' | awk -F/ '{print $1}' | sort -u                 
alliance
authority-discovery
democracy
election-provider-multi-phase
election-provider-support
fast-unstake
merkle-mountain-range
nomination-pools
root-offences
safe-mode
staking
support
system
tips
utility

And following @xlc 's advice, I will create pull requests according to his logic.

[Gilt0]

FYI

Using the list above I managed to change some files - see below for my notes.

alliance                            DONE
authority-discovery                 DONE
democracy                           DONE -- Changed `type Block = frame_system::mocking::MockBlock<Test>;` to `type Block = frame_system::mocking::MockBlockU32<Test>;` -- did not touch `type BlockNumber = BlockNumberFor<Test>;` as test do not seem to run
election-provider-multi-phase       UNSURE -- When changing `pub(crate) type BlockNumber = u64;` there seem to be a lot of dependencies I am not entirely sure of as of now
election-provider-support           DONE
fast-unstake                        DONE
merkle-mountain-range               DONE -- Compiles but tests fail because hardcoded hashes seem off (expected with a change of type) -- if changes approved, I will change the hardcoded hashes to fix tests
nomination-pools                    DONE -- tests not going in test-staking/src/mock.rs or benchmarking/src/mock.rs
root-offences                       DONE
safe-mode                           NOT DONE -- Deep dependencies
staking                             DONE
support                             NOTHING TO DO -- tests not going in test/tests/versioned_migration.rs where `Block = frame_system::mocking::MockBlock<Test>;` is found
system                              NOT DONE -- Deep dependencies
tips                                DONE -- Tests not going in src/migrations/unreserve_deposits.rs where 'type BlockNumber = BlockNumberFor<Test>;' is found
utility                             DONE

I did not touch system (as already discussed) nor safe-mode. In the latter, I found deeper dependencies that I did not felt comfortable changing in the current PR ( #3437 ), it will have its own too. Also I am uncertain of the dependencies in election-provider-multi-phase so same thing, it will have its own PR.

In support, I did not see how the tests that could be changed where actually being run so I discarded it.

The tests in merkle-mountain-range fail because the hard coded hashes do not match the computed value which, I believe, could be expected since the type has changed. @gupnik Please try and have a look if the changes are ok and if I can modify the hard coded hash values with the new computed ones - they are indicated in the panic message.

Finally, I have also tried to look for MockBlock<Test> in the frame folder and low and behold... There are a lot of occurrences (maybe even the whole frame folder)! Here is the output.

% grep -ri 'MockBlock<Test>' * | awk -F/ '{print $1}' | sort -u
asset-conversion
asset-rate
assets
atomic-swap
aura
authorship
babe
balances
beefy
beefy-mmr
benchmarking
bounties
broker
child-bounties
contracts
conviction-voting
core-fellowship
examples
glutton
grandpa
identity
indices
insecure-randomness-collective-flip
lottery
membership
message-queue
nft-fractionalization
nfts
nis
node-authorization
paged-list
preimage
proxy
ranked-collective
recovery
referenda
remark
safe-mode
salary
sassafras
scheduler
scored-pool
session
society
statement
sudo
support
system
timestamp
transaction-storage
treasury
tx-pause
uniques
vesting
whitelist

If I am not mistaken, MockBlock<Test> mentions should be changed to MockBlockU32<Test>. So it will have its own PR too. 😆

Therefore: (i) Please let me know if I can change the hash codes in the failing tests of merkle-mountain-range pallet (ii) Once I have changed I will make the PR non draft, so please accept it. (iii) Then I will move on with the MockBlock<Test> mentions to be changed as MockBlockU32<Test>, the other individual pallets next, finishing with system pallet.

[gupnik]

Thanks @Gilt0!

(i) Please let me know if I can change the hash codes in the failing tests of merkle-mountain-range pallet

Let's do this in a separate PR.

[Gilt0]

Thanks @Gilt0!

(i) Please let me know if I can change the hash codes in the failing tests of merkle-mountain-range pallet

Let's do this in a separate PR.

You're welcome, thank you for the guidance.

Here is a pull request I created on my fork: https://github.com/Gilt0/polkadot-sdk/pull/1

Not sure how to share it with you otherwise as the PR cannot be merged unless all test pass, right?

[gupnik]

Shall we first remove changes in merkle-mountain-range, try to fix all tests in https://github.com/paritytech/polkadot-sdk/pull/3437 and get it merged? We can then come back to it in the second PR?

[Gilt0]

@gupnik Noted, thanks for the pointer, it makes a lot of sense. I was busy these past two days, but I shall get back into it tomorrow at the latest.

[Gilt0]

@gupnik

I hope this message will find you well.

Here is the result of this first straight forward pass of changes, using the grep command.

grep -ri 'MockBlock<Test>' * | awk -F/ '{print $1}' | sort -u

asset-conversion                    DONE
asset-rate                          DONE
assets                              DONE
atomic-swap                         DONE
aura                                DONE
authorship                          DONE
babe                                DONE -- Aliased type Header = generic::Header<u32, BlakeTwo256> instead of sourcing from sp_runtime::testing
balances                            DONE
beefy                               DONE
beefy-mmr                           NOT DONE -- Hashes to be modified for tests to be validated
benchmarking                        DONE -- ignored pov/src/benchmarking.rs, pov/src/tests.rs, src/baseline.rs (no tests depend on it)
bounties                            DONE
broker                              DONE
child-bounties                      DONE
contracts                           NOT DONE -- WASM code to be updated
conviction-voting                   DONE
core-fellowship                     DONE
examples                            NOTHING TO DO -- Tests are not triggered
glutton                             DONE 
grandpa                             NOT DONE -- Tests fail
identity                            DONE
indices                             DONE
insecure-randomness-collective-flip DONE
lottery                             DONE
membership                          DONE
message-queue                       DONE
nft-fractionalization               DONE
nfts                                DONE
nis                                 DONE
node-authorization                  DONE
paged-list                          DONE                       
preimage                            DONE
proxy                               DONE
ranked-collective                   DONE -- PollStatus::Completed(*when, *succeeded) => PollStatus::Completed(*when as u32, *succeeded) -- there may be a loss of data with the conversion but it is better this way?
recovery                            DONE
referenda                           NOT DONE -- test referendum_status_v0 needs some further work as there is a hardcoded hex value to updated
remark                              DONE
safe-mode                           NOT DONE -- I have errors like trait `From<RawOrigin<u32>>` is not implemented for `mock::RuntimeOrigin
salary                              DONE
sassafras                           DONE -- using a lot of 'epoch_length as u32' as Slot is mainly defined as u64 and I do not want to touch to primitives in this PR
scheduler                           NOT DONE -- two tests fails: `migration_to_v4_works` and `test_migrate_origin`
scored-pool                         DONE
session                             DONE
society                             NOT DONE - requires modification outside of test files (in society/src/migrations.rs)
statement                           DONE
sudo                                DONE
support                             NOTHING TO DO -- tests do not use MockBlock
system                              NOT DONE -- will have its own PR
timestamp                           DONE
transaction-storage                 DONE
treasury                            DONE
tx-pause                            DONE
uniques                             DONE
vesting                             NOT DONE -- trait `sp_runtime::traits::Convert<u32, u64>` is not implemented for `sp_runtime::traits::Identity
whitelist                           DONE

To summarize, they all went pretty smoothly except for (including the first pass above)

• safe-mode
• election-provider-multi-phase
• merkle-mountain-range
• beefy-mmr
• contracts
• grandpa
• referenda
• safe-mode
• scheduler
• society
• system
• vesting
• system

Therefore, if you can, have a look at the current PR and let me know if anything is not good for you (I'll switch it to not draft).

From now on, I will work on the pallets above, starting a PR for each of them. I'll probably be constantly working on two or three at a time. If you are ok, I will allow myself with questions on the pallet for each respective PR.

Thank you again for this learning opportunity. I look forward to completely resolving this issue with your help. :smiley:

[gupnik]

Thanks @Gilt0 ! Will go through the PR soon and looking forward to the subsequent ones.

[bkchr]

@bkontur I think we can close this with https://github.com/paritytech/polkadot-sdk/pull/4543? As there is no real need to move all tests to u32 or u64.