feat: msp check previous block #337

undercover-cactus · 2025-01-22T21:26:36Z

In this PR, we add a check that allow msp to answer to NewStorageRequests when catching up to the block chain.

When closing up to the tip of the chain, a function will be called that will check for all the storage requests through a runtime api. If the storage request as the MSP id in it it will triggered a NewStorageRequest event. The MSP will then proceed the volunteer to it.

snowmead · 2025-01-23T13:46:01Z

client/blockchain-service/src/handler.rs

                // TODO: Send events to check that this node has a Forest Storage for each Bucket this MSP manages.
                // TODO: Catch up to Forest root writes in the Bucket's Forests.
+
+                let storage_requests: Vec<(H256, StorageRequestMetadata)> = self


Even though it is technically impossible for the storage_requests_by_msp to return duplicate storage requests, I would still make this a map instead to match runtime storage and have it be impossible on client side to emit more than a single event for the same file key

I agree with the above comment ⬆️. Making this a map instead of a vec would be ideal.

Hey @undercover-cactus! Just a quick tip for PR comments in general: please don’t resolve comments if no action has been taken. If there’s nothing to do, it’s best to leave a reply explaining why instead of resolving it.
Ideally, the reviewer should be the one to resolve their own comments, unless it’s something simple and directly addressed. This helps a lot when re-reviewing to track what’s actually been handled. Thanks!

Yep sorry.

I was going for it at first but I realize that HashMap need the key type to have the trait Hash which is in the std. And std is not allow in the runtime. In theory, having duplicate is not possible so it doesn't add much. I still convert it in the handler.

You don't need to use HashMap, you can use BTreeMap instead, which is available in the runtime, under the sp_std crate.

pallets/file-system/src/utils.rs

client/blockchain-service/src/handler.rs

pallets/file-system/src/utils.rs

links234

Left some preliminary comments.

links234 · 2025-02-07T11:03:09Z

client/blockchain-service/src/handler.rs

+                    .unresponded_storage_requests_by_msp(block_hash, msp_id)
+                    .unwrap();
+
+                for (file_key, sr) in storage_requests {


maybe ignore since it's personal preference

Can you rename sr to storage_request?

It is hard to differentiate storage_requests and storage_request when browsing the code. I realize that sr is not a good name but we can quickly understand what it is by looking at the loop. It is a trade off and I guess linked to programming habits.

Maybe to improve readability I could add a comment ?

Adding a comment is fine by me, btw.

node/src/tasks/bsp_charge_fees.rs

pallets/file-system/src/utils.rs

ffarall · 2025-02-11T17:31:12Z

client/blockchain-service/src/handler.rs

                // TODO: Send events to check that this node has a Forest Storage for each Bucket this MSP manages.
                // TODO: Catch up to Forest root writes in the Bucket's Forests.
+
+                let storage_requests: Vec<(H256, StorageRequestMetadata)> = self


I agree with the above comment ⬆️. Making this a map instead of a vec would be ideal.

ffarall · 2025-02-11T17:34:00Z

client/blockchain-service/src/handler.rs

+                    .client
+                    .runtime_api()
+                    .pending_storage_requests_by_msp(block_hash, msp_id)
+                    .unwrap();


Avoid unwrapping here, as it would panic the whole node. From what I see this runtime API call can fail if there is an internal error while calling the runtime API. So I'd handle the error logging that there was an internal error while trying to call the runtime API. Ideally, make it noticeable!

ffarall · 2025-02-11T18:13:45Z

client/blockchain-service/src/handler.rs

+                    .unresponded_storage_requests_by_msp(block_hash, msp_id)
+                    .unwrap();
+
+                for (file_key, sr) in storage_requests {


Adding a comment is fine by me, btw.

ffarall · 2025-02-11T18:14:42Z

pallets/file-system/runtime-api/src/lib.rs

@@ -45,19 +46,27 @@ pub enum QueryConfirmChunksToProveForFileError {
    ChallengedChunkToChunkIdError,
 }

+/// Error type for the `pending_storage_requests_by_msp`.
+#[derive(Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo)]
+pub enum StorageRequestsByMSPError {


Suggested change

pub enum StorageRequestsByMSPError {

pub enum StorageRequestsByMspError {

ffarall · 2025-02-11T18:15:50Z

pallets/file-system/src/utils.rs

+    pub fn pending_storage_requests_by_msp(
+        msp_id: ProviderIdFor<T>,
+    ) -> Vec<(MerkleHash<T>, StorageRequestMetadata<T>)> {
+        // Get the storeage requests for a specific MSP


Suggested change

// Get the storeage requests for a specific MSP

// Get the storage requests for a specific MSP

node/src/tasks/bsp_charge_fees.rs

test/suites/integration/msp/catch-up-storage.test.ts

ffarall · 2025-02-11T18:19:47Z

test/suites/integration/msp/catch-up-storage.test.ts

+    it("Network launches and can be queried", async () => {
+      const userNodePeerId = await userApi.rpc.system.localPeerId();
+      strictEqual(userNodePeerId.toString(), userApi.shConsts.NODE_INFOS.user.expectedPeerId);
+
+      const mspNodePeerId = await mspApi.rpc.system.localPeerId();
+      strictEqual(mspNodePeerId.toString(), userApi.shConsts.NODE_INFOS.msp1.expectedPeerId);
+    });


We can remove this one, it's everywhere 😂

Why can we remove it ?

It's testing the connections to the nodes, and we are testing this in pretty much every test. It doesn't hurt that it's there, but at this point it's not necessary really.

ffarall · 2025-02-11T18:20:43Z

test/suites/integration/msp/catch-up-storage.test.ts

+      // We need to wait so it won't try to answer the request storage
+      await sleep(5000);


Why do you need to wait here? It won't respond if it is paused.

I had the behavior that if even if the container was paused I had a NewStorageRequest event from the MSP. So I added a delay to be sure it is paused before creating the new storage request.

You could pause the container even before creating the bucket, or even add a check to see that it is not responding. But waiting 5s will unnecessarily slow down the integration test suite.

ffarall · 2025-02-11T18:24:36Z

test/suites/integration/msp/catch-up-storage.test.ts

+      // Advancing 10 blocks to see if MSP catchup
+      await userApi.block.skip(10);
+
+      await userApi.docker.restartBspContainer({ containerName: "docker-sh-msp-1" });
+
+      // need to wait for the container to be up again
+      // await sleep(5000);
+
+      // NOTE:
+      // We shouldn't have to recreate an API but any other attempt to reconnect failed
+      // Also had to guess for the port of MSP 1
+      // await using newMspApi = await createApi("ws://127.0.0.1:9777");
+
+      // Required to trigger out of sync mode
+      // await userApi.rpc.engine.createBlock(true, true);
+
+      // await waitFor({
+      //   lambda: async () =>
+      //     (await newMspApi.rpc.storagehubclient.isFileInFileStorage(event.data.fileKey)).isFileFound
+      // });
+
+      // await userApi.assert.eventPresent("fileSystem", "MspAcceptedStorageRequest");
+    });


What happened here? This is not really testing the right outcome 😂.

The MSP being offline the user cannot send it the file and the MSP can never finish the task because it doesn't have the file.

#361 will solve this by downloading the file from a BSP in this particular case. The commented part in this test should then pass. It is commented so the CI would pass.

ffarall

Commenting and not approving due to the missing test. But good job.

ffarall · 2025-02-13T15:36:06Z

test/suites/integration/msp/catch-up-storage.test.ts

@@ -0,0 +1,97 @@
+import assert, { strictEqual } from "node:assert";
+import { describeMspNet, shUser, type EnrichedBspApi, sleep } from "../../../util";


The CI is complaining about this unused sleep import.

ffarall · 2025-02-13T15:37:54Z

runtime/src/apis.rs

+use sp_std::collections::btree_map::BTreeMap;
 use sp_std::prelude::Vec;


Ideally, merge these two use statements into one.

undercover-cactus force-pushed the feat/msp-check-previous-block branch from 3757114 to 74d4946 Compare January 23, 2025 11:24

undercover-cactus marked this pull request as ready for review January 23, 2025 12:39

undercover-cactus requested review from ffarall, TDemeco and snowmead January 23, 2025 12:39

snowmead reviewed Jan 23, 2025

View reviewed changes

snowmead reviewed Jan 30, 2025

View reviewed changes

pallets/file-system/src/utils.rs Outdated Show resolved Hide resolved

undercover-cactus requested a review from snowmead February 7, 2025 09:19

links234 reviewed Feb 7, 2025

View reviewed changes

undercover-cactus requested a review from links234 February 7, 2025 12:40

ffarall requested changes Feb 11, 2025

View reviewed changes

undercover-cactus requested a review from ffarall February 13, 2025 14:26

ffarall reviewed Feb 13, 2025

View reviewed changes

undercover-cactus marked this pull request as draft February 17, 2025 16:19

undercover-cactus added 14 commits February 19, 2025 13:25

wip

ef2b3f9

needed file key value and some minors fixes

734490f

added an integration test but it is failing

1ad93fa

failing integration test

a0c3303

fmt

4754a8d

change name of the rpc function; incomplete integration test

0630fb1

linter

afdb7a3

typecheck

6fc1a37

remove sleep

100b10b

update comment; change unresponded_ to pending_ function name;

ccadab9

convert Vec of storage requests to HashMap; minor changes;

a4b450c

use BTreeMap from the sp-std lib; remove sleep;

0b3a072

typegen

84d69b4

remove unused imports

6c17fb7

undercover-cactus force-pushed the feat/msp-check-previous-block branch from ca38439 to 6c17fb7 Compare February 19, 2025 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: msp check previous block #337

feat: msp check previous block #337

undercover-cactus commented Jan 22, 2025 •

edited

Loading

snowmead Jan 23, 2025

ffarall Feb 11, 2025

santikaplan Feb 11, 2025

undercover-cactus Feb 12, 2025

ffarall Feb 13, 2025

links234 left a comment

links234 Feb 7, 2025

undercover-cactus Feb 7, 2025

ffarall Feb 11, 2025

ffarall Feb 11, 2025

ffarall Feb 11, 2025

ffarall Feb 11, 2025

ffarall Feb 11, 2025

ffarall Feb 11, 2025

ffarall Feb 11, 2025

undercover-cactus Feb 12, 2025

ffarall Feb 13, 2025

ffarall Feb 11, 2025

undercover-cactus Feb 12, 2025

ffarall Feb 13, 2025

ffarall Feb 11, 2025

undercover-cactus Feb 12, 2025

ffarall left a comment

ffarall Feb 13, 2025

ffarall Feb 13, 2025

	pub enum StorageRequestsByMSPError {
	pub enum StorageRequestsByMspError {

	// Get the storeage requests for a specific MSP
	// Get the storage requests for a specific MSP

		// We need to wait so it won't try to answer the request storage
		await sleep(5000);

		@@ -0,0 +1,97 @@
		import assert, { strictEqual } from "node:assert";
		import { describeMspNet, shUser, type EnrichedBspApi, sleep } from "../../../util";

		use sp_std::collections::btree_map::BTreeMap;
		use sp_std::prelude::Vec;

feat: msp check previous block #337

Are you sure you want to change the base?

feat: msp check previous block #337

Conversation

undercover-cactus commented Jan 22, 2025 • edited Loading

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

links234 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

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

ffarall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

undercover-cactus commented Jan 22, 2025 •

edited

Loading