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.

Sign up for GitHub

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

fix upgrade e2e test #1269

Merged
merged 1 commit into from
Jun 19, 2024
Merged

fix upgrade e2e test #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions tests/e2e/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@
"version": "1.0.0",
"description": "Astar end to end tests.",
"license": "MIT",
"type": "module",
"scripts": {
"test": "LOG_LEVEL=error vitest --silent --no-color",
"test:runtime-upgrade-shibuya": "RUNTIME=shibuya yarn test tests/runtime-upgrade.test.ts",
"test:runtime-upgrade-shiden": "RUNTIME=shiden yarn test tests/runtime-upgrade.test.ts",
"test:runtime-upgrade-astar": "RUNTIME=astar yarn test tests/runtime-upgrade.test.ts"
},
"dependencies": {
"@acala-network/chopsticks-testing": "^0.6.4",
"@polkadot/util-crypto": "^10.1.10",
"typescript": "^4.8.4",
"vitest": "^0.31.0"
"@acala-network/chopsticks-testing": "^0.12.2",
"@polkadot/util-crypto": "^12.6.2",
"typescript": "^5.3.0",
"vitest": "^1.4.0"
},
"prettier": {
"tabWidth": 2,
Expand Down
11 changes: 6 additions & 5 deletions tests/e2e/tests/runtime-upgrade.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,21 @@ describe('runtime upgrade', async () => {
const { alice } = testingPairs();

const runtime = process.env.RUNTIME || 'shibuya';
const cxt = await setupContext({ endpoint: endpoints[runtime] });
const { api, dev } = cxt;
const { api, dev, teardown } = await setupContext({ endpoint: endpoints[runtime] });

beforeAll(async () => {
await dev.setStorage({
Sudo: {
Key: alice.address,
},
System: {
Account: [[[alice.address], { data: { free: 100n * 10n ** 18n } }]],
Account: [[[alice.address], { providers: 1, data: { free: 1000n * 10n ** 18n } }]],
},
});
});

afterAll(async () => {
await cxt.teardown();
await teardown();
});

// Execution hook before runtime upgrade. To test storage migrations, set up the storage items
Expand Down Expand Up @@ -73,12 +72,14 @@ describe('runtime upgrade', async () => {
await api.tx.sudo
.sudoUncheckedWeight(
api.tx.system.setCode('0x' + code.toString('hex')),
0
{}
)
.signAndSend(alice);

// Do block production.
await dev.newBlock({ count: 2 });
// wait a bit for pjs/api to update
await new Promise((r) => setTimeout(r, 1000));
Comment on lines +81 to +82
Copy link
Member

@Dinonard Dinonard Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How come this is needed if you await for the block production above?
Doesn't it mean the newBlock function returns prematurely?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not for block production but for ApiPromise to become aware of the new block and storage changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alice is sending both tx and some how it needed a bit of time to sync nonce

Copy link
Member

Choose a reason for hiding this comment

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

Just to check if I understand correctly - remark is executed, followed by setCode, two new blocks are produced, then we wait for the internal chopsticks state to stabilize/sync, before proceeding forward?

I don't know how chopsticks works internally, but should it return from the newBlock if state isn't synced yet? Maybe it's a dumb question 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

once block is build it will resolve and announce storage changes async for listeners (like ApiPromise). But because we're immediately creating a new tx (aslo because everything is running on a single js thread event loop) nextNounce may not be reflected immediately so second tx was build with old nonce resulting in invalid tx.

Copy link
Member Author

Choose a reason for hiding this comment

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

even a 200ms will be enough but I added 1sec to be sure

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 appears to happen only when we perform an upgrade, this is making me think why 🤔, need to debug it

Copy link
Member Author

@ermalkaleci ermalkaleci Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Dinonard Initially I thought it was sending old nonce but it's not nonce, it's spec_version. When runtime upgrades happen, it takes a few millis for ApiPromise to receive metadata from chopsticks (because it extracts it from runtime itself, runtime call needs to happen) but pjs/api doesn't wait for the new metadata but instead uses what it has (old spec_version) and making an invalid tx


// The spec version is increased.
const curSpecVersion = await specVersion(api);
Expand Down
0 tests/e2e/vitest.config.ts → tests/e2e/vitest.config.mts
View file
Open in desktop
File renamed without changes.
Loading
Loading