From 1ad8d7aad2273be61c5765484c2e8fd6a8b7bc24 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Thu, 26 Dec 2024 14:36:25 -0500 Subject: [PATCH] =?UTF-8?q?test:=20=E2=9C=85=20Fix=20race=20conditions=20i?= =?UTF-8?q?n=20integration=20tests=20(#299)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: :white_check_mark: Fix race condition in `skipTo` waiting for proofs * test: :technologist: Improve error log in `waitForTxInPool` * fix: :bug: Remove `break` from `for` loop counting pending proof submissions * test: :white_check_mark: Fix race condition of many payment streams charged events in last block * fix: :white_check_mark: Use correct event variable in test for expected value * fix: :white_check_mark: Change `sleep`s for waiting and polling functions avoiding race conditions * fix: :white_check_mark: Fix `waitForTxInPool` * style: :art: Apply TS formatting * fix: :white_check_mark: Increase timeout to 10s in `waitForBspStoredWithoutSealing` and `waitForMspResponseWithoutSealing` * fix: :white_check_mark: Increase default timeout in `waitForTxInPool` * fix: :white_check_mark: Clarity logic for waiting for proofs in `skipTo` * fix: :adhesive_bandage: Remove `only: true` from test * test: :white_check_mark: Simplify `bsp-thresholds` test to avoid race conditions * revert: :rewind: Rollback runtime change to speed up CI * fix: :white_check_mark: Change name of parameter `watchForBspProofs` in `skipTo` to match `advanceToBlock`, making it pass down the parameter * fix: :white_check_mark: Fix submit-proofs tests race conditions * revert: :rewind: Rollback `only: true` flag * fix: :white_check_mark: Fix race condition of sending transaction with BSP key when there's another in the pool * fix: :white_check_mark: Replace `sleep` for `mspApi.wait.fileStorageComplete` * fix: :white_check_mark: Wait for new signed up BSP to catch up to chain tip --- test/scripts/generateBenchmarkProofs.ts | 2 +- .../integration/bsp/bsp-thresholds.test.ts | 99 ++++++-------- .../integration/bsp/debt-collection.test.ts | 22 +--- .../integration/bsp/multiple-delete.test.ts | 4 +- .../integration/bsp/reorg-proof.test.ts | 2 +- .../integration/bsp/single-volunteer.test.ts | 2 +- .../integration/bsp/storage-capacity.test.ts | 30 +++-- .../integration/bsp/submit-proofs.test.ts | 23 ++-- .../integration/msp/debt-collection.test.ts | 25 +++- .../msp/respond-multi-requests.test.ts | 44 ++----- .../msp/stop-storing-bucket.test.ts | 6 +- test/util/bspNet/block.ts | 66 +++++----- test/util/bspNet/test-api.ts | 31 +++-- test/util/bspNet/waits.ts | 124 ++++++++++++------ 14 files changed, 253 insertions(+), 227 deletions(-) diff --git a/test/scripts/generateBenchmarkProofs.ts b/test/scripts/generateBenchmarkProofs.ts index 0d20e2967..0c8be578c 100644 --- a/test/scripts/generateBenchmarkProofs.ts +++ b/test/scripts/generateBenchmarkProofs.ts @@ -190,7 +190,7 @@ async function generateBenchmarkProofs() { fileKeys.push(fileMetadata.fileKey); await userApi.wait.bspVolunteer(1); - await bspApi.wait.bspFileStorageComplete(fileMetadata.fileKey); + await bspApi.wait.fileStorageComplete(fileMetadata.fileKey); await userApi.wait.bspStored(1); } diff --git a/test/suites/integration/bsp/bsp-thresholds.test.ts b/test/suites/integration/bsp/bsp-thresholds.test.ts index 264d58f7f..b23c81e0e 100644 --- a/test/suites/integration/bsp/bsp-thresholds.test.ts +++ b/test/suites/integration/bsp/bsp-thresholds.test.ts @@ -114,7 +114,7 @@ describeBspNet( ); await userApi.sealBlock( - userApi.tx.sudo.sudo(userApi.tx.fileSystem.setGlobalParameters(null, 10)) + userApi.tx.sudo.sudo(userApi.tx.fileSystem.setGlobalParameters(null, 500)) ); // Create a new BSP and onboard with no reputation @@ -131,8 +131,8 @@ describeBspNet( await userApi.wait.bspCatchUpToChainTip(bspDownApi); const { fileKey } = await userApi.file.createBucketAndSendNewStorageRequest( - "res/smile.jpg", - "test/smile.jpg", + "res/whatsup.jpg", + "test/whatsup.jpg", "bucket-1" ); @@ -150,17 +150,27 @@ describeBspNet( ) ).asOk.toNumber(); - if ((await userApi.rpc.chain.getHeader()).number.toNumber() < lowReputationVolunteerTick) { - await userApi.block.skipTo(lowReputationVolunteerTick); - } + const currentBlockNumber = (await userApi.rpc.chain.getHeader()).number.toNumber(); + assert( + currentBlockNumber === normalReputationVolunteerTick, + "The BSP with high reputation should be able to volunteer immediately" + ); + assert( + currentBlockNumber < lowReputationVolunteerTick, + "The volunteer tick for the low reputation BSP should be in the future" + ); - if (normalReputationVolunteerTick < lowReputationVolunteerTick) { - await userApi.wait.bspVolunteer(); - } else { - await userApi.wait.bspVolunteer(2); - } + // Checking volunteering and confirming for the high reputation BSP + await userApi.wait.bspVolunteer(1); + await bspApi.wait.fileStorageComplete(fileKey); + await userApi.wait.bspStored(1); + + // Checking volunteering and confirming for the low reputation BSP + await userApi.block.skipTo(lowReputationVolunteerTick); + await userApi.wait.bspVolunteer(1); const matchedEvents = await userApi.assert.eventMany("fileSystem", "AcceptedBspVolunteer"); // T1 + // Check that it is in fact the BSP with low reputation that just volunteered const filtered = matchedEvents.filter( ({ event }) => (userApi.events.fileSystem.AcceptedBspVolunteer.is(event) && @@ -224,49 +234,23 @@ describeBspNet( ) ).asOk.toNumber(); - if (bsp1VolunteerTick < bsp2VolunteerTick) { - // If the first BSP can volunteer first, wait for it to volunteer and confirm storing the file - if ((await userApi.rpc.chain.getHeader()).number.toNumber() < bsp1VolunteerTick) { - await userApi.block.skipTo(bsp1VolunteerTick); - } - await userApi.wait.bspVolunteer(1); - await bspApi.wait.bspFileStorageComplete(fileKey); - await userApi.wait.bspStored(1); + assert(bsp1VolunteerTick < bsp2VolunteerTick, "BSP one should be able to volunteer first"); + const currentBlockNumber = (await userApi.rpc.chain.getHeader()).number.toNumber(); + assert( + currentBlockNumber === bsp1VolunteerTick, + "BSP one should be able to volunteer immediately" + ); - // Then wait for the second BSP to volunteer and confirm storing the file - if ((await userApi.rpc.chain.getHeader()).number.toNumber() < bsp2VolunteerTick) { - await userApi.block.skipTo(bsp2VolunteerTick); - } - await userApi.wait.bspVolunteer(1); - await bspTwoApi.wait.bspFileStorageComplete(fileKey); - await userApi.wait.bspStored(1); - } else if (bsp1VolunteerTick > bsp2VolunteerTick) { - // If the second BSP can volunteer first, wait for it to volunteer and confirm storing the file - if ((await userApi.rpc.chain.getHeader()).number.toNumber() < bsp2VolunteerTick) { - await userApi.block.skipTo(bsp2VolunteerTick); - } - await userApi.wait.bspVolunteer(1); - await bspTwoApi.wait.bspFileStorageComplete(fileKey); - await userApi.wait.bspStored(1); + await userApi.wait.bspVolunteer(1); + await bspApi.wait.fileStorageComplete(fileKey); + await userApi.wait.bspStored(1); - // Then wait for the first BSP to volunteer and confirm storing the file - if ((await userApi.rpc.chain.getHeader()).number.toNumber() < bsp1VolunteerTick) { - await userApi.block.skipTo(bsp1VolunteerTick); - } - await userApi.wait.bspVolunteer(1); - await bspApi.wait.bspFileStorageComplete(fileKey); - await userApi.wait.bspStored(1); - } else { - // If both BSPs can volunteer at the same time, advance to the tick where both can volunteer - if ((await userApi.rpc.chain.getHeader()).number.toNumber() < bsp1VolunteerTick) { - await userApi.block.skipTo(bsp1VolunteerTick); - } - // And wait for them to volunteer and confirm storing the file - await userApi.wait.bspVolunteer(2); - await bspApi.wait.bspFileStorageComplete(fileKey); - await bspTwoApi.wait.bspFileStorageComplete(fileKey); - await userApi.wait.bspStored(2); - } + // Then wait for the second BSP to volunteer and confirm storing the file + await userApi.block.skipTo(bsp2VolunteerTick); + + await userApi.wait.bspVolunteer(1); + await bspTwoApi.wait.fileStorageComplete(fileKey); + await userApi.wait.bspStored(1); await bspTwoApi.disconnect(); await userApi.docker.stopBspContainer("sh-bsp-two"); @@ -282,6 +266,7 @@ describeBspNet( bspStartingWeight: 800_000_000n }); const bspThreeApi = await BspNetTestApi.create(`ws://127.0.0.1:${rpcPort}`); + await userApi.wait.bspCatchUpToChainTip(bspThreeApi); // Wait for it to catch up to the top of the chain await userApi.wait.bspCatchUpToChainTip(bspThreeApi); @@ -319,11 +304,11 @@ describeBspNet( ); // Advance to the tick where the new BSP can volunteer - if ( - (await userApi.rpc.chain.getHeader()).number.toNumber() < highReputationBspVolunteerTick - ) { - await userApi.block.skipTo(highReputationBspVolunteerTick); - } + const currentBlockNumber = (await userApi.rpc.chain.getHeader()).number.toNumber(); + assert( + currentBlockNumber === highReputationBspVolunteerTick, + "BSP with high reputation should be able to volunteer immediately" + ); // Wait until the new BSP volunteers await userApi.wait.bspVolunteer(1); diff --git a/test/suites/integration/bsp/debt-collection.test.ts b/test/suites/integration/bsp/debt-collection.test.ts index 6a39a95e7..bf9663d81 100644 --- a/test/suites/integration/bsp/debt-collection.test.ts +++ b/test/suites/integration/bsp/debt-collection.test.ts @@ -1,13 +1,6 @@ import assert, { strictEqual } from "node:assert"; import { after } from "node:test"; -import { - bob, - describeBspNet, - fetchEvent, - ShConsts, - sleep, - type EnrichedBspApi -} from "../../../util"; +import { bob, describeBspNet, fetchEvent, ShConsts, type EnrichedBspApi } from "../../../util"; import { BN } from "@polkadot/util"; describeBspNet( @@ -262,9 +255,9 @@ describeBspNet( 3 // There are 3 running BSPs to fulfil the storage request ); await userApi.wait.bspVolunteer(3); - await bspApi.wait.bspFileStorageComplete(cloudFileMetadata.fileKey); - await bspTwoApi.wait.bspFileStorageComplete(cloudFileMetadata.fileKey); - await bspThreeApi.wait.bspFileStorageComplete(cloudFileMetadata.fileKey); + await bspApi.wait.fileStorageComplete(cloudFileMetadata.fileKey); + await bspTwoApi.wait.fileStorageComplete(cloudFileMetadata.fileKey); + await bspThreeApi.wait.fileStorageComplete(cloudFileMetadata.fileKey); await userApi.wait.bspStored(3); const adolphusFileMetadata = await userApi.file.createBucketAndSendNewStorageRequest( @@ -277,9 +270,9 @@ describeBspNet( 3 // There are 3 running BSPs to fulfil the storage request ); await userApi.wait.bspVolunteer(3); - await bspApi.wait.bspFileStorageComplete(adolphusFileMetadata.fileKey); - await bspTwoApi.wait.bspFileStorageComplete(adolphusFileMetadata.fileKey); - await bspThreeApi.wait.bspFileStorageComplete(adolphusFileMetadata.fileKey); + await bspApi.wait.fileStorageComplete(adolphusFileMetadata.fileKey); + await bspTwoApi.wait.fileStorageComplete(adolphusFileMetadata.fileKey); + await bspThreeApi.wait.fileStorageComplete(adolphusFileMetadata.fileKey); await userApi.wait.bspStored(3); // Check the payment stream info after adding the new files @@ -493,7 +486,6 @@ describeBspNet( // Seal a block to allow BSPs to charge the payment stream await userApi.sealBlock(); - await sleep(500); // Assert that event for the BSP charging its payment stream was emitted await userApi.assert.eventPresent("paymentStreams", "PaymentStreamCharged"); diff --git a/test/suites/integration/bsp/multiple-delete.test.ts b/test/suites/integration/bsp/multiple-delete.test.ts index 5c506f29e..e6ef25998 100644 --- a/test/suites/integration/bsp/multiple-delete.test.ts +++ b/test/suites/integration/bsp/multiple-delete.test.ts @@ -76,7 +76,7 @@ describeBspNet("Single BSP Volunteering", ({ before, createBspApi, it, createUse // Wait for the BSP to volunteer await userApi.wait.bspVolunteer(source.length); for (const fileKey of fileKeys) { - await bspApi.wait.bspFileStorageComplete(fileKey); + await bspApi.wait.fileStorageComplete(fileKey); } // Waiting for a confirmation of the first file to be stored @@ -214,7 +214,7 @@ describeBspNet("Single BSP Volunteering", ({ before, createBspApi, it, createUse // Wait for the BSP to volunteer await userApi.wait.bspVolunteer(source.length); for (const fileKey of fileKeys) { - await bspApi.wait.bspFileStorageComplete(fileKey); + await bspApi.wait.fileStorageComplete(fileKey); } // Waiting for a confirmation of the first file to be stored diff --git a/test/suites/integration/bsp/reorg-proof.test.ts b/test/suites/integration/bsp/reorg-proof.test.ts index a0271f15e..782b428e0 100644 --- a/test/suites/integration/bsp/reorg-proof.test.ts +++ b/test/suites/integration/bsp/reorg-proof.test.ts @@ -40,7 +40,7 @@ describeBspNet( await userApi.block.seal(); // To make sure we have a finalised head const nextChallengeTick = await getNextChallengeHeight(userApi); await userApi.block.skipTo(nextChallengeTick, { - waitForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID], + watchForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID], finalised: true }); diff --git a/test/suites/integration/bsp/single-volunteer.test.ts b/test/suites/integration/bsp/single-volunteer.test.ts index d73ea08e8..f9b2d143f 100644 --- a/test/suites/integration/bsp/single-volunteer.test.ts +++ b/test/suites/integration/bsp/single-volunteer.test.ts @@ -226,7 +226,7 @@ describeBspNet("Single BSP multi-volunteers", ({ before, createBspApi, createUse // Wait for the BSP to receive and store all files for (let i = 0; i < source.length; i++) { const fileKey = fileKeys[i]; - await bspApi.wait.bspFileStorageComplete(fileKey); + await bspApi.wait.fileStorageComplete(fileKey); } // The first file to be completed will immediately acquire the forest write lock diff --git a/test/suites/integration/bsp/storage-capacity.test.ts b/test/suites/integration/bsp/storage-capacity.test.ts index d2252e8af..9d247406c 100644 --- a/test/suites/integration/bsp/storage-capacity.test.ts +++ b/test/suites/integration/bsp/storage-capacity.test.ts @@ -1,24 +1,22 @@ import assert from "node:assert"; import { bspKey, describeBspNet, type EnrichedBspApi, ferdie, sleep } from "../../../util"; -describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, createBspApi }) => { +describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi }) => { let userApi: EnrichedBspApi; - let bspApi: EnrichedBspApi; before(async () => { userApi = await createUserApi(); - bspApi = await createBspApi(); }); it("Unregistered accounts fail when changing capacities", async () => { const totalCapacityBefore = await userApi.query.providers.totalBspsCapacity(); const bspCapacityBefore = await userApi.query.providers.backupStorageProviders( - bspApi.shConsts.DUMMY_BSP_ID + userApi.shConsts.DUMMY_BSP_ID ); assert.ok(bspCapacityBefore.unwrap().capacity.eq(totalCapacityBefore)); const { events, extSuccess } = await userApi.sealBlock( - userApi.tx.providers.changeCapacity(bspApi.shConsts.CAPACITY[1024]), + userApi.tx.providers.changeCapacity(userApi.shConsts.CAPACITY[1024]), ferdie ); assert.strictEqual(extSuccess, false); @@ -37,7 +35,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c const totalCapacityAfter = await userApi.query.providers.totalBspsCapacity(); const bspCapacityAfter = await userApi.query.providers.backupStorageProviders( - bspApi.shConsts.DUMMY_BSP_ID + userApi.shConsts.DUMMY_BSP_ID ); assert.ok(bspCapacityAfter.unwrap().capacity.eq(totalCapacityBefore)); assert.ok(totalCapacityAfter.eq(totalCapacityBefore)); @@ -50,7 +48,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c ); const capacityUsed = ( - await userApi.query.providers.backupStorageProviders(bspApi.shConsts.DUMMY_BSP_ID) + await userApi.query.providers.backupStorageProviders(userApi.shConsts.DUMMY_BSP_ID) ) .unwrap() .capacityUsed.toNumber(); @@ -59,6 +57,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c const newCapacity = Math.max(minCapacity, capacityUsed + 1); // Set BSP's available capacity to 0 to force the BSP to increase its capacity before volunteering for the storage request. + await userApi.wait.waitForAvailabilityToSendTx(bspKey.address.toString()); const { extSuccess } = await userApi.sealBlock( userApi.tx.providers.changeCapacity(newCapacity), bspKey @@ -101,9 +100,9 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c await userApi.sealBlock(); - const updatedCapacity = BigInt(bspApi.shConsts.JUMP_CAPACITY_BSP + newCapacity); + const updatedCapacity = BigInt(userApi.shConsts.JUMP_CAPACITY_BSP + newCapacity); const bspCapacityAfter = await userApi.query.providers.backupStorageProviders( - bspApi.shConsts.DUMMY_BSP_ID + userApi.shConsts.DUMMY_BSP_ID ); assert.strictEqual(bspCapacityAfter.unwrap().capacity.toBigInt(), updatedCapacity); @@ -113,16 +112,17 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c it("Total capacity updated when single BSP capacity updated", async () => { const newCapacity = - BigInt(Math.floor(Math.random() * 1000 * 1024 * 1024)) + bspApi.shConsts.CAPACITY_512; + BigInt(Math.floor(Math.random() * 1000 * 1024 * 1024)) + userApi.shConsts.CAPACITY_512; // Skip block height past threshold await userApi.block.skipToMinChangeTime(); + await userApi.wait.waitForAvailabilityToSendTx(bspKey.address.toString()); await userApi.sealBlock(userApi.tx.providers.changeCapacity(newCapacity), bspKey); const totalCapacityAfter = await userApi.query.providers.totalBspsCapacity(); const bspCapacityAfter = await userApi.query.providers.backupStorageProviders( - bspApi.shConsts.DUMMY_BSP_ID + userApi.shConsts.DUMMY_BSP_ID ); assert.strictEqual(bspCapacityAfter.unwrap().capacity.toBigInt(), newCapacity); assert.strictEqual(totalCapacityAfter.toBigInt(), newCapacity); @@ -140,6 +140,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c // Skip block height past threshold await userApi.block.skipToMinChangeTime(); + await userApi.wait.waitForAvailabilityToSendTx(bspKey.address.toString()); const { events, extSuccess } = await userApi.sealBlock( userApi.tx.providers.changeCapacity(2n), bspKey @@ -160,7 +161,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c it("Test BSP storage size increased twice in the same increasing period (check for race condition)", async () => { const capacityUsed = ( - await userApi.query.providers.backupStorageProviders(bspApi.shConsts.DUMMY_BSP_ID) + await userApi.query.providers.backupStorageProviders(userApi.shConsts.DUMMY_BSP_ID) ) .unwrap() .capacityUsed.toNumber(); @@ -169,6 +170,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c const newCapacity = Math.max(minCapacity, capacityUsed + 1); // Set BSP's available capacity to 0 to force the BSP to increase its capacity before volunteering for the storage request. + await userApi.wait.waitForAvailabilityToSendTx(bspKey.address.toString()); const { extSuccess } = await userApi.sealBlock( userApi.tx.providers.changeCapacity(newCapacity), bspKey @@ -196,7 +198,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c await sleep(500); // Assert BSP has sent a call to increase its capacity. - await bspApi.assert.extrinsicPresent({ + await userApi.assert.extrinsicPresent({ module: "providers", method: "changeCapacity", checkTxPool: true @@ -209,7 +211,7 @@ describeBspNet("BSPNet: Validating max storage", ({ before, it, createUserApi, c const updatedCapacity = BigInt(userApi.shConsts.JUMP_CAPACITY_BSP + newCapacity); const bspCapacityAfter = await userApi.query.providers.backupStorageProviders( - bspApi.shConsts.DUMMY_BSP_ID + userApi.shConsts.DUMMY_BSP_ID ); assert.strictEqual(bspCapacityAfter.unwrap().capacity.toBigInt(), updatedCapacity); }); diff --git a/test/suites/integration/bsp/submit-proofs.test.ts b/test/suites/integration/bsp/submit-proofs.test.ts index 255a27a6d..17bb892f1 100644 --- a/test/suites/integration/bsp/submit-proofs.test.ts +++ b/test/suites/integration/bsp/submit-proofs.test.ts @@ -322,7 +322,7 @@ describeBspNet( const currentBlock = await userApi.rpc.chain.getBlock(); const currentBlockNumber = currentBlock.block.header.number.toNumber(); await userApi.block.skipTo(currentBlockNumber + storageRequestTtl, { - waitForBspProofs: [ShConsts.DUMMY_BSP_ID] + watchForBspProofs: [ShConsts.DUMMY_BSP_ID] }); // Resume BSP-Two and BSP-Three. @@ -334,7 +334,11 @@ describeBspNet( }); // Wait for BSPs to resync. - await sleep(1000); + await userApi.wait.bspCatchUpToChainTip(bspTwoApi); + await userApi.wait.bspCatchUpToChainTip(bspThreeApi); + + // And give some time to process proofs. + await sleep(3000); // There shouldn't be any pending volunteer transactions. await assert.rejects( @@ -376,12 +380,11 @@ describeBspNet( if (nextChallengeTick > currentBlockNumber) { // Advance to the next challenge tick if needed - await userApi.block.skipTo(nextChallengeTick); + await userApi.block.skipTo(nextChallengeTick, { + watchForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] + }); } - // Wait for tasks to execute and for the BSPs to submit proofs. - await sleep(500); - // There should be at least one pending submit proof transaction. const submitProofsPending = await userApi.assert.extrinsicPresent({ module: "proofsDealer", @@ -464,7 +467,7 @@ describeBspNet( const currentBlock = await userApi.rpc.chain.getBlock(); const currentBlockNumber = currentBlock.block.header.number.toNumber(); await userApi.block.skipTo(currentBlockNumber + deletionRequestTtl, { - waitForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] + watchForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] }); // Check for a file deletion request event. @@ -481,7 +484,7 @@ describeBspNet( ); const nextCheckpointChallengeBlock = lastCheckpointChallengeTick + checkpointChallengePeriod; await userApi.block.skipTo(nextCheckpointChallengeBlock, { - waitForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] + watchForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] }); // Check that the event for the priority challenge is emitted. @@ -561,7 +564,7 @@ describeBspNet( if (firstBlockToAdvance !== currentBlockNumber) { // Advance to first next challenge block. await userApi.block.skipTo(firstBlockToAdvance, { - waitForBspProofs: [DUMMY_BSP_ID, BSP_TWO_ID, BSP_THREE_ID] + watchForBspProofs: [DUMMY_BSP_ID, BSP_TWO_ID, BSP_THREE_ID] }); } @@ -617,7 +620,7 @@ describeBspNet( if (secondBlockToAdvance !== currentBlockNumber) { // Advance to second next challenge block. await userApi.block.skipTo(secondBlockToAdvance, { - waitForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] + watchForBspProofs: [ShConsts.DUMMY_BSP_ID, ShConsts.BSP_TWO_ID, ShConsts.BSP_THREE_ID] }); } diff --git a/test/suites/integration/msp/debt-collection.test.ts b/test/suites/integration/msp/debt-collection.test.ts index 8b861bbc2..1bf002ca0 100644 --- a/test/suites/integration/msp/debt-collection.test.ts +++ b/test/suites/integration/msp/debt-collection.test.ts @@ -109,7 +109,7 @@ describeMspNet("Single MSP collecting debt", ({ before, createMsp1Api, it, creat assert(newStorageRequestDataBlob, "Event doesn't match NewStorageRequest type"); - await mspApi.wait.mspFileStorageComplete(newStorageRequestDataBlob.fileKey); + await mspApi.wait.fileStorageComplete(newStorageRequestDataBlob.fileKey); issuedFileKeys.push(newStorageRequestDataBlob.fileKey); } @@ -334,12 +334,12 @@ describeMspNet("Single MSP collecting debt", ({ before, createMsp1Api, it, creat "User account does not match" ); - // Advance many MSP charging periods to charge again, but this time with a known number of + // Advance one MSP charging period to charge again, but this time with a known number of // blocks since last charged. That way, we can check for the exact amount charged. // Since the MSP is going to charge each period, the last charge should be for one period. currentBlock = await userApi.rpc.chain.getHeader(); currentBlockNumber = currentBlock.number.toNumber(); - await userApi.block.skipTo(currentBlockNumber + 10 * MSP_CHARGING_PERIOD); + await userApi.block.skipTo(currentBlockNumber + MSP_CHARGING_PERIOD); // Calculate the expected rate of the payment stream and compare it to the actual rate. const valueProps = await userApi.call.storageProvidersApi.queryValuePropositionsForMsp( @@ -368,11 +368,26 @@ describeMspNet("Single MSP collecting debt", ({ before, createMsp1Api, it, creat // The expected amount to be charged is the rate of the payment stream times the charging period. const expectedChargedAmount = paymentStreamRate * MSP_CHARGING_PERIOD; - // Verify that it charged for the correct amount. - const paymentStreamChargedEvent = await userApi.assert.eventPresent( + // Getting the PaymentStreamCharged events. There could be multiple of these events in the last block, + // so we get them all and then filter the one where the Provider ID matches the MSP ID. + const paymentStreamChargedEvents = await userApi.assert.eventMany( "paymentStreams", "PaymentStreamCharged" ); + const paymentStreamChargedEventsFiltered = paymentStreamChargedEvents.filter((e) => { + const event = e.event; + assert(userApi.events.paymentStreams.PaymentStreamCharged.is(event)); + return event.data.providerId.eq(DUMMY_MSP_ID); + }); + + // There should be only one PaymentStreamCharged event for the MSP + assert( + paymentStreamChargedEventsFiltered.length === 1, + "Expected a single PaymentStreamCharged event" + ); + + // Verify that it charged for the correct amount. + const paymentStreamChargedEvent = paymentStreamChargedEventsFiltered[0]; assert(userApi.events.paymentStreams.PaymentStreamCharged.is(paymentStreamChargedEvent.event)); const paymentStreamChargedEventAmount = paymentStreamChargedEvent.event.data.amount.toNumber(); diff --git a/test/suites/integration/msp/respond-multi-requests.test.ts b/test/suites/integration/msp/respond-multi-requests.test.ts index 369fc7085..deecbe7a0 100644 --- a/test/suites/integration/msp/respond-multi-requests.test.ts +++ b/test/suites/integration/msp/respond-multi-requests.test.ts @@ -1,5 +1,5 @@ import assert, { strictEqual } from "node:assert"; -import { describeMspNet, shUser, sleep, type EnrichedBspApi } from "../../../util"; +import { describeMspNet, shUser, waitFor, type EnrichedBspApi } from "../../../util"; describeMspNet( "Single MSP accepting multiple storage requests", @@ -79,25 +79,13 @@ describeMspNet( `Expected ${source.length} NewStorageRequest events` ); - // Allow time for the MSP to receive and store the files from the user - // TODO: Ideally, this should be turned into a polling helper function. - await sleep(3000); - // Check if the MSP received the files. for (const e of matchedEvents) { const newStorageRequestDataBlob = userApi.events.fileSystem.NewStorageRequest.is(e.event) && e.event.data; - assert(newStorageRequestDataBlob, "Event doesn't match NewStorageRequest type"); - const result = await mspApi.rpc.storagehubclient.isFileInFileStorage( - newStorageRequestDataBlob.fileKey - ); - - assert( - result.isFileFound, - `File not found in storage for ${newStorageRequestDataBlob.location.toHuman()}` - ); + await mspApi.wait.fileStorageComplete(newStorageRequestDataBlob.fileKey); } // Seal block containing the MSP's first response. @@ -107,13 +95,6 @@ describeMspNet( await userApi.wait.mspResponseInTxPool(); await userApi.sealBlock(); - // Give time for the MSP to update the local forest root. - // TODO: Ideally, this should be turned into a polling helper function. - await sleep(1000); - - // Check that the local forest root is updated, and matches th on-chain root. - const localBucketRoot = await mspApi.rpc.storagehubclient.getForestRoot(bucketId); - const { event: bucketRootChangedEvent } = await userApi.assert.eventPresent( "providers", "BucketRootChanged" @@ -126,7 +107,12 @@ describeMspNet( "Expected BucketRootChanged event but received event of different type" ); - strictEqual(bucketRootChangedDataBlob.newRoot.toString(), localBucketRoot.toString()); + await waitFor({ + lambda: async () => { + const localBucketRoot = await mspApi.rpc.storagehubclient.getForestRoot(bucketId); + return localBucketRoot.toString() === bucketRootChangedDataBlob.newRoot.toString(); + } + }); // The MSP should have accepted exactly one file. // Register how many were accepted in the last block sealed. @@ -152,13 +138,6 @@ describeMspNet( await userApi.wait.mspResponseInTxPool(); await userApi.sealBlock(); - // Give time for the MSP to update the local forest root. - // TODO: Ideally, this should be turned into a polling helper function. - await sleep(1000); - - // Check that the local forest root is updated, and matches th on-chain root. - const localBucketRoot2 = await mspApi.rpc.storagehubclient.getForestRoot(bucketId); - const { event: bucketRootChangedEvent2 } = await userApi.assert.eventPresent( "providers", "BucketRootChanged" @@ -171,7 +150,12 @@ describeMspNet( "Expected BucketRootChanged event but received event of different type" ); - strictEqual(bucketRootChangedDataBlob2.newRoot.toString(), localBucketRoot2.toString()); + await waitFor({ + lambda: async () => { + const localBucketRoot2 = await mspApi.rpc.storagehubclient.getForestRoot(bucketId); + return localBucketRoot2.toString() === bucketRootChangedDataBlob2.newRoot.toString(); + } + }); // The MSP should have accepted at least one file. // Register how many were accepted in the last block sealed. diff --git a/test/suites/integration/msp/stop-storing-bucket.test.ts b/test/suites/integration/msp/stop-storing-bucket.test.ts index bbf679512..862e83b46 100644 --- a/test/suites/integration/msp/stop-storing-bucket.test.ts +++ b/test/suites/integration/msp/stop-storing-bucket.test.ts @@ -39,11 +39,7 @@ describeMspNet( const fileMetadata = await userApi.file.newStorageRequest(source, destination, bucketId); // Wait for MSP to download file from user - await sleep(2000); - - const result = await mspApi.rpc.storagehubclient.isFileInFileStorage(fileMetadata.fileKey); - - assert(result.isFileFound, "File not found in storage"); + mspApi.wait.fileStorageComplete(fileMetadata.fileKey); // Seal block containing the MSP's transaction response to the storage request await userApi.wait.mspResponseInTxPool(); diff --git a/test/util/bspNet/block.ts b/test/util/bspNet/block.ts index e83a0f77d..8afb1b46c 100644 --- a/test/util/bspNet/block.ts +++ b/test/util/bspNet/block.ts @@ -16,6 +16,7 @@ import * as ShConsts from "./consts"; import assert, { strictEqual } from "node:assert"; import * as Assertions from "../asserts"; import { waitForLog } from "./docker"; +import { waitForTxInPool } from "./waits"; export interface SealedBlock { blockReceipt: CreatedBlock; @@ -308,32 +309,6 @@ export const advanceToBlock = async ( verbose?: boolean; } ): Promise => { - // If watching for BSP proofs, we need to know the blocks at which they are challenged. - const challengeBlockNumbers: { nextChallengeBlock: number; challengePeriod: number }[] = []; - if (options.watchForBspProofs) { - for (const bspId of options.watchForBspProofs) { - // First we get the last tick for which the BSP submitted a proof. - const lastTickResult = - await api.call.proofsDealerApi.getLastTickProviderSubmittedProof(bspId); - if (lastTickResult.isErr) { - options.verbose && console.log(`Failed to get last tick for BSP ${bspId}`); - continue; - } - const lastTickBspSubmittedProof = lastTickResult.asOk.toNumber(); - // Then we get the challenge period for the BSP. - const challengePeriodResult = await api.call.proofsDealerApi.getChallengePeriod(bspId); - assert(challengePeriodResult.isOk); - const challengePeriod = challengePeriodResult.asOk.toNumber(); - // Then we calculate the next challenge tick. - const nextChallengeTick = lastTickBspSubmittedProof + challengePeriod; - - challengeBlockNumbers.push({ - nextChallengeBlock: nextChallengeTick, - challengePeriod - }); - } - } - const currentBlock = await api.rpc.chain.getBlock(); let currentBlockNumber = currentBlock.block.header.number.toNumber(); @@ -359,6 +334,7 @@ export const advanceToBlock = async ( const maxNormalBlockWeight = api.consts.system.blockWeights.perClass.normal.maxTotal.unwrap(); for (let i = 0; i < blocksToAdvance; i++) { + // Only for spamming! if (options.spam && i < blocksToSpam) { if (options.verbose) { console.log(`Spamming block ${i + 1} of ${blocksToSpam}`); @@ -395,18 +371,38 @@ export const advanceToBlock = async ( console.log(`Current tick: ${currentTick}`); } - // Check if we need to wait for BSP proofs. + // If watching for BSP proofs, we need to know if this block is a challenge block for any of the BSPs. if (options.watchForBspProofs) { - for (const challengeBlockNumber of challengeBlockNumbers) { - if (currentBlockNumber === challengeBlockNumber.nextChallengeBlock) { - // Wait for the BSP to process the proof. - await sleep(500); - - // Update next challenge block. - challengeBlockNumbers[0].nextChallengeBlock += challengeBlockNumber.challengePeriod; - break; + let txsToWaitFor = 0; + for (const bspId of options.watchForBspProofs) { + // TODO: Change this to a runtime API that gets the next challenge tick for a BSP. + // First we get the last tick for which the BSP submitted a proof. + const lastTickResult = + await api.call.proofsDealerApi.getLastTickProviderSubmittedProof(bspId); + if (lastTickResult.isErr) { + options.verbose && console.log(`Failed to get last tick for BSP ${bspId}`); + continue; + } + const lastTickBspSubmittedProof = lastTickResult.asOk.toNumber(); + // Then we get the challenge period for the BSP. + const challengePeriodResult = await api.call.proofsDealerApi.getChallengePeriod(bspId); + assert(challengePeriodResult.isOk); + const challengePeriod = challengePeriodResult.asOk.toNumber(); + // Then we calculate the next challenge tick. + const nextChallengeTick = lastTickBspSubmittedProof + challengePeriod; + + if (currentBlockNumber === nextChallengeTick) { + txsToWaitFor++; } } + + // Wait for all corresponding BSPs to have submitted their proofs. + await waitForTxInPool(api, { + module: "proofsDealer", + method: "submitProof", + checkQuantity: txsToWaitFor, + strictQuantity: false + }); } if (options.waitBetweenBlocks) { diff --git a/test/util/bspNet/test-api.ts b/test/util/bspNet/test-api.ts index f18e5abb5..0ec102d4d 100644 --- a/test/util/bspNet/test-api.ts +++ b/test/util/bspNet/test-api.ts @@ -34,11 +34,11 @@ export interface WaitForTxOptions { module: string; method: string; checkQuantity?: number; + strictQuantity?: boolean; shouldSeal?: boolean; expectedEvent?: string; - iterations?: number; - delay?: number; timeout?: number; + verbose?: boolean; } /** @@ -260,12 +260,12 @@ export class BspNetTestApi implements AsyncDisposable { Waits.waitForBspStoredWithoutSealing(this._api, expectedExts), /** - * Waits for a BSP to complete storing a file key. + * Waits for a Storage Provider to complete storing a file key. * @param fileKey - Param to specify the file key to wait for. * @returns A promise that resolves when a BSP has completed to store a file. */ - bspFileStorageComplete: (fileKey: H256 | string) => - Waits.waitForBspFileStorageComplete(this._api, fileKey), + fileStorageComplete: (fileKey: H256 | string) => + Waits.waitForFileStorageComplete(this._api, fileKey), /** * Waits for a BSP to complete deleting a file from its forest. @@ -300,12 +300,21 @@ export class BspNetTestApi implements AsyncDisposable { Waits.waitForMspResponseWithoutSealing(this._api, expectedExts), /** - * Waits for a MSP to complete storing a file key. - * @param fileKey - Param to specify the file key to wait for. - * @returns A promise that resolves when the MSP has completed to store a file. + * Waits for a block where the given address has no pending extrinsics. + * + * This can be used to wait for a block where it is safe to send a transaction signed by the given address, + * without risking it clashing with another transaction with the same nonce already in the pool. For example, + * BSP nodes are often sending transactions, so if you want to send a transaction using one of the BSP keys, + * you should wait for the BSP to have no pending extrinsics before sending the transaction. + * + * IMPORTANT: As long as the address keeps having pending extrinsics, this function will keep waiting and building + * blocks to include such transactions. + * + * @param address - The address of the account to wait for. + * @returns A promise that resolves when the address has no pending extrinsics. */ - mspFileStorageComplete: (fileKey: H256 | string) => - Waits.waitForBspFileStorageComplete(this._api, fileKey) + waitForAvailabilityToSendTx: (address: string) => + Waits.waitForAvailabilityToSendTx(this._api, address) }; /** @@ -446,7 +455,7 @@ export class BspNetTestApi implements AsyncDisposable { blockNumber: number, options?: { waitBetweenBlocks?: number | boolean; - waitForBspProofs?: string[]; + watchForBspProofs?: string[]; finalised?: boolean; spam?: boolean; verbose?: boolean; diff --git a/test/util/bspNet/waits.ts b/test/util/bspNet/waits.ts index 4ff544ead..27afbba3c 100644 --- a/test/util/bspNet/waits.ts +++ b/test/util/bspNet/waits.ts @@ -7,52 +7,59 @@ import type { Address, H256 } from "@polkadot/types/interfaces"; import type { WaitForTxOptions } from "./test-api"; /** - * Generic function to wait for a transaction in the pool + * Generic function to wait for a transaction in the pool. + * + * If the expected amount of extrinsics is 0, this function will return immediately. */ export const waitForTxInPool = async (api: ApiPromise, options: WaitForTxOptions) => { const { module, method, checkQuantity, + strictQuantity = false, shouldSeal = false, expectedEvent, - iterations = 100, - delay = 100, - timeout = 100 + timeout = 1000, + verbose = false } = options; + // Handle the case where the expected amount of extrinsics is 0 + if (checkQuantity === 0) { + // If the expected amount is 0, we can return immediately + verbose && + console.log( + `Expected 0 extrinsics for ${module}.${method}. Skipping wait for extrinsic in txPool.` + ); + return; + } // To allow node time to react on chain events - for (let i = 0; i < iterations; i++) { - try { - await sleep(delay); - const matches = await assertExtrinsicPresent(api, { - module, - method, - checkTxPool: true, - timeout - }); - if (checkQuantity) { - assert( - matches.length === checkQuantity, - `Expected ${checkQuantity} extrinsics, but found ${matches.length} for ${module}.${method}` - ); - } - - if (shouldSeal) { - const { events } = await sealBlock(api); - if (expectedEvent) { - assertEventPresent(api, module, expectedEvent, events); - } - } + try { + const matches = await assertExtrinsicPresent(api, { + module, + method, + checkTxPool: true, + timeout + }); + if (checkQuantity && strictQuantity) { + assert( + matches.length === checkQuantity, + `Expected ${checkQuantity} extrinsics, but found ${matches.length} for ${module}.${method}` + ); + } else if (checkQuantity && !strictQuantity) { + assert( + matches.length >= checkQuantity, + `Expected at least ${checkQuantity} extrinsics, but found ${matches.length} for ${module}.${method}` + ); + } - break; - } catch (e) { - if (i === iterations - 1) { - throw new Error( - `Failed to detect ${module}.${method} extrinsic in txPool after ${(i * delay) / 1000}s` - ); + if (shouldSeal) { + const { events } = await sealBlock(api); + if (expectedEvent) { + assertEventPresent(api, module, expectedEvent, events); } } + } catch (e) { + throw new Error(`Failed to detect ${module}.${method} extrinsic in txPool. Error: ${e}`); } }; @@ -193,9 +200,7 @@ export const waitForBspStoredWithoutSealing = async (api: ApiPromise, checkQuant module: "fileSystem", method: "bspConfirmStoring", checkQuantity, - iterations: 50, - delay: 200, - timeout: 300 + timeout: 10000 }); }; @@ -212,7 +217,7 @@ export const waitForBspStoredWithoutSealing = async (api: ApiPromise, checkQuant * * @throws Will throw an error if the file is not complete in the file storage after a timeout. */ -export const waitForBspFileStorageComplete = async (api: ApiPromise, fileKey: H256 | string) => { +export const waitForFileStorageComplete = async (api: ApiPromise, fileKey: H256 | string) => { // To allow time for local file transfer to complete (10s) const iterations = 10; const delay = 1000; @@ -277,8 +282,8 @@ export const waitForBspToCatchUpToChainTip = async ( syncedApi: ApiPromise, bspBehindApi: ApiPromise ) => { - // To allow time for BSP to catch up to the tip of the chain (10s) - const iterations = 100; + // To allow time for BSP to catch up to the tip of the chain (30s) + const iterations = 300; const delay = 100; for (let i = 0; i < iterations + 1; i++) { try { @@ -331,11 +336,50 @@ export const waitForMspResponseWithoutSealing = async (api: ApiPromise, checkQua module: "fileSystem", method: "mspRespondStorageRequestsMultipleBuckets", checkQuantity, - iterations: 41, - delay: 50 + timeout: 10000 }); }; +/** + * Waits for a block where the given address has no pending extrinsics. + * + * This can be used to wait for a block where it is safe to send a transaction signed by the given address, + * without risking it clashing with another transaction with the same nonce already in the pool. For example, + * BSP nodes are often sending transactions, so if you want to send a transaction using one of the BSP keys, + * you should wait for the BSP to have no pending extrinsics before sending the transaction. + * + * IMPORTANT: As long as the address keeps having pending extrinsics, this function will keep waiting and building + * blocks to include such transactions. + * + * @param api - The ApiPromise instance. + * @param address - The address of the account to wait for. + */ +export const waitForAvailabilityToSendTx = async ( + api: ApiPromise, + address: string, + iterations = 100, + delay = 500 +) => { + let isTxFromAddressPresent = false; + let its = iterations; + do { + await sleep(delay); + + // Check if the address has pending extrinsics + const result = await api.rpc.author.pendingExtrinsics(); + isTxFromAddressPresent = result.some((tx) => tx.signer.toString() === address); + if (isTxFromAddressPresent) { + // Build a block with the transactions from the address + await sealBlock(api); + } + } while (isTxFromAddressPresent && its-- > 0); + + if (isTxFromAddressPresent) { + // If the address still has pending extrinsics after the maximum number of iterations, throw an error + throw new Error(`Failed after ${iterations} iterations and ${(iterations * delay) / 1000}s`); + } +}; + /** * Options for the `waitFor` function. * @param lambda - The condition to wait for.