-
Notifications
You must be signed in to change notification settings - Fork 15
Initial E2E system tests (set_code, authorize_upgrade)
#411
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes introduce a comprehensive set of end-to-end tests for system upgrades (set_code, authorize_upgrade) for both relay chains and parachains. The implementation is well-structured, with most of the logic centralized in the new packages/shared/src/system.ts file. However, there is a copy-paste error in one of the test files, a typo in a test description, and inconsistent, hardcoded values are used for XCM weight, which should be addressed.
Co-authored-by: Alexandre R. Baldé <[email protected]>
rockbmb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only changes that really need addressing are the scheduleInlineCallWithOrigin calls, otherwise looks good!
packages/shared/src/system.ts
Outdated
| return runAuthorizeUpgradeScenario(client, { | ||
| call: client.api.tx.system.authorizeUpgrade, | ||
| expectedAfterApply: (hash) => [ | ||
| { type: client.api.events.system.RejectedInvalidAuthorizedUpgrade, args: { codeHash: hash } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not too much work, could you also check the error field of RejectedInvalidAuthorizedUpgrade?
A runtime upgrade to the same WASM failing is expected, but the reasons for the failure should be (and stay) the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be left for a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure currently how to do better than:
expectedAfterApply: (hash) => [
{ type: paraClient.api.events.system.RejectedInvalidAuthorizedUpgrade, args: {
codeHash: hash,
error: (r: SpRuntimeDispatchError) => r.eq({Module: { index: '0', error: '0x01000000' } })
} },
],since RejectedInvalidAuthorizedUpgrade has associated error of type SpRuntimeDispatchError and in this case it is actually SpRuntimeModuleError ... I believe I cannot "cast" this to anything nicer
@rockbmb , does it look ok for you ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, TIL I can do it like:
paraClient.api.errors.system.SpecVersionNeedsToIncrease.is(r.asModule)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed - I was unaware that was the issue, as otherwise I'd have pointed you to a few instances of this pattern in PEt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in eee2357
packages/shared/src/system.ts
Outdated
| ) { | ||
| const authorizeHash = async (someHash) => { | ||
| const call = params.call(someHash).method | ||
| await scheduleInlineCallWithOrigin(client, call.toHex(), { system: 'Root' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every instance of scheduleInlineCallWithOrigin needs to be given a block provider, which the test config type has:
polkadot-ecosystem-tests/packages/shared/src/helpers/index.ts
Lines 466 to 489 in ae7271e
| /** | |
| * Configuration for relay chain tests. | |
| */ | |
| export interface RelayTestConfig { | |
| testSuiteName: string | |
| addressEncoding: number | |
| blockProvider: 'Local' | |
| chainEd?: ChainED | |
| } | |
| /** | |
| * Configuration for parachain tests. | |
| * Async backing is relevant due to the step size of `parachainSystem.lastRelayChainBlockNumber`. | |
| * | |
| * Recall that with the AHM, the scheduler pallet's agenda will be keyed by this block number. | |
| * It is, then, relevant for tests to know whether AB is enabled. | |
| */ | |
| export interface ParaTestConfig { | |
| testSuiteName: string | |
| addressEncoding: number | |
| blockProvider: BlockProvider | |
| asyncBacking: AsyncBacking | |
| chainEd?: ChainED | |
| } |
The scheduler in Kusama AH (and soon PAH) keyes its agenda via a nonlocal block provider, which means these tests will fail on KAH.
See other E2E tests for how to do this, e.g. scheduler tests for KAH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing test are actually fine, because the scenarios I introduced are using Relay's scheduler pallet - and that is always a "local" block provider
Before AHM I think parachains didn't have their scheduler (maybe Collectives only?), with AHM we are introducing scheduler pallet to Asset Hubs - so I can possibly create one more scenario specifiacally for post AHM Asset Hub such that it could use its own scheduler to execute the runtime upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best left for a future PR, this one is 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant ⬆️ is that the extra scenario is best left for another PR.
scheduleInlineCallWithOrigin still needs to be given a testConfig.blockProvider param, otherwise, in asset hubs (Kusama now, and Polkadot next week), it'll be scheduling tasks with an incorrect agenda key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testConfig.blockProvider and extra scenario introduced in eee2357
packages/shared/src/system.ts
Outdated
| >(relayChain: Chain<TCustomRelay, TInitStoragesRelay>, paraChain: Chain<TCustomPara, TInitStoragesPara>) { | ||
| const [relayClient, paraClient] = await setupNetworks(relayChain, paraChain) | ||
| return runSetCodeScenarioViaRelay(relayClient, paraClient, { | ||
| call: paraClient.api.tx.system.setCodeWithoutChecks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says Tests 'setCode' flow — ensures runtime upgrade to the same WASM fails, but this uses setCodeWithoutChecks - won't it pass instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this code was not used actually because this is a scenario where we want to upgrade parachain, but the parachain doesn't have scheduler pallet so we need to use relay's scheduler - and that fails because XCM message (from relay to parachain) would contain WASM blob and its too big for XCM message.
I have skipped this scenario and just left the code but I think I can actually use this scenario to verify that this will indeed fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in eee2357
|
Will close #456 . |
| const testConfig: RelayTestConfig = { | ||
| testSuiteName: 'Kusama AssetHub System', | ||
| addressEncoding: 0, | ||
| blockProvider: 'Local', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be ParaTestConfig, and the block provider is non-local - asset hub scheduler agendas will use the relay chain's block number as their key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added both scenarios:
- using scheduler on relay --> local block provider (from relay perspective)
- using scheduler on AH (post-AHM) --> NonLocal provider in that case
| import { assetHubPolkadot, polkadot } from '@e2e-test/networks/chains' | ||
| import { type RelayTestConfig, registerTestTree, systemE2ETestsViaRelay } from '@e2e-test/shared' | ||
|
|
||
| const testConfig: RelayTestConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar as for Kusama, I have added both scenarios:
- using scheduler on relay --> Local block provider (from relay perspective)
- (to be uncommented post-AHM) using scheduler on AH --> NonLocal provider in that case
|
@rockbmb , few tests for Kusama are failing due to a bug where Preimage pallet is still filtered out post migration and PET scenarios here are extensively using Preimage pallet along with Scheduler to inject stuff for execution This is affecting current live Kusama Relay. It that has been fixed by polkadot-fellows/runtimes#957 but we will need to wait for Kusama Relay update |
set_code, authorize_upgrade)
|
|
|
||
| // registerTestTree(systemE2ETests(kusama, testConfig)) | ||
|
|
||
| const testConfigForAssetHub: ParaTestConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fine as it was, RelayTestConfig
|
|
||
| // TODO: Uncomment Post-AHM on Polkadot | ||
|
|
||
| // const testConfigForAssetHub: ParaTestConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to changed: RelayTestConfig.
I know it's commented out, but still.
Relates to: #298
Will close #456 .
This initial implementation provides
Scenarios for Polkadot and Kusama relays. These scenearios utilize local pallet Scheduler to execute upgrade related calls as Root:
set_codewith currently used WASM expecting error to happen (this path is checking egspecVersionfrom WASM and will not allow upgrade to pass if it hasn't been incremented)set_code_without_checkswith currently used WASM expecting success, basically upgrading to the same wasm but version checks are skipped on runtime level thus allowing to passauthorize_upgrade+apply_authorized_upgradewith currently used WASM expecting error to happenauthorize_upgrade_without_checks+apply_authorized_upgradewith currently used WASM expecting successauthorize_upgradeandauthorize_upgrade_without_checkscan override authorized hashScenarios for Polkadot/Kusama system chains (AH, BH, People, Coretime, Collectives, Encointer). These scenarios utilize Relay chain's pallet Scheduler to send XCM Transact go gain root on destination chain:
authorize_upgrade+apply_authorized_upgradewith currently used WASM expecting error to happenauthorize_upgrade_without_checks+apply_authorized_upgradewith currently used WASM expecting successauthorize_upgradeandauthorize_upgrade_without_checkscan override authorized hash