Add http server to subspace-gateway. #3286

shamil-gadelshin · 2024-12-04T17:18:55Z

This PR adds an HTTP endpoint to subspace-gateway as an update to AutoDrive Gateway project. It reuses the object fetcher and keeps the existing RPC-server. The new API serves files reconstructed from DSN by object fetcher using object mappings received from the object mapping indexer.

Changes

Refactor run command to rpc command and move shared code
Add http command to serve requests on http://<IP>:<PORT>/data/{object_hash} using actix-web framework.

Code contributor checklist:

I have read, understood and followed contributing guide

teor2345

This is a good start, but it is missing remote input validation, which could introduce a security vulnerability. It’s also missing object data hash validation, but I don’t think that’s a security issue.

I also have a few suggestions to avoid code duplication, and a bunch of comment updates. Feel free to ignore the comment stuff for now.

Let me know if you’d like me to help with any of these changes.

crates/subspace-gateway/Cargo.toml

crates/subspace-gateway/src/commands/http.rs

crates/subspace-gateway/src/commands/rpc.rs

crates/subspace-gateway/src/commands/http.rs

crates/subspace-gateway/src/commands/http/server.rs

shamil-gadelshin

Thanks for the review!

crates/subspace-gateway/Cargo.toml

crates/subspace-gateway/src/commands/http/server.rs

crates/subspace-gateway/src/main.rs

crates/subspace-gateway/src/commands/http.rs

crates/subspace-gateway/src/commands/http/server.rs

shamil-gadelshin · 2024-12-05T16:34:01Z

crates/subspace-gateway/src/commands/http/server.rs

+    #[serde(rename = "pieceIndex")]
+    piece_index: u64,
+    #[serde(rename = "pieceOffset")]
+    piece_offset: u32,


It seems like spreading the dependency across contexts. I would prefer changing types.

teor2345

Looks good! Once we’ve got the dependencies and the hash type cleaned up, we should be ready to go.

crates/subspace-gateway/src/commands/http/server.rs

teor2345 · 2024-12-06T04:09:25Z

crates/subspace-gateway/src/commands/http/server.rs

+    #[serde(rename = "pieceIndex")]
+    piece_index: u64,
+    #[serde(rename = "pieceOffset")]
+    piece_offset: u32,


Changing to Blake3Hash here might simplify some of the code below.

Changes mostly applied

clostao · 2024-12-07T17:07:22Z

crates/subspace-gateway/src/commands/http/server.rs

+
+async fn request_object_mappings(endpoint: String, key: String) -> anyhow::Result<ObjectMapping> {
+    let client = reqwest::Client::new();
+    let object_mappings_url = format!("http://{}/objects/{}", endpoint, key,);


Hardcoding the HTTP protocol would avoid the usage of indexers with HTTPS endpoints? For example, using the currenthttps://indexer.taurus.autonomys.xyz.

Using this indexer could be useful for debugging and testing purposes

I’d suggest making the protocol part of the endpoint URL, like we do for RPC clients:

subspace/crates/subspace-gateway/src/commands/run/network.rs

Lines 20 to 21 in 07a3f4e

#[arg(long, value_hint = ValueHint::Url, default_value = "ws://127.0.0.1:9944")]

node_rpc_url: String,

Note: using HTTP isn’t a security issue here, because:

the raw object data is already public on the chain

we check the hash of the returned data, so it can’t be modified in transit

HTTP might be a minor privacy issue, because the hash and timing of each object fetch is unencrypted. It’s up to Carlos if that’s a significant issue in our use case though.

Allowing HTTPS in this PR would make that a deployment decision, not a developer coding decision.

I agree with this suggestion. I'll create and test a separate PR.

teor2345

Looks good, I made PR #3293 to simplify the code further.

teor2345 · 2024-12-09T00:22:23Z

crates/subspace-gateway/src/commands/http/server.rs

+
+async fn request_object_mappings(endpoint: String, key: String) -> anyhow::Result<ObjectMapping> {
+    let client = reqwest::Client::new();
+    let object_mappings_url = format!("http://{}/objects/{}", endpoint, key,);


I’d suggest making the protocol part of the endpoint URL, like we do for RPC clients:

subspace/crates/subspace-gateway/src/commands/run/network.rs

Lines 20 to 21 in 07a3f4e

#[arg(long, value_hint = ValueHint::Url, default_value = "ws://127.0.0.1:9944")]

node_rpc_url: String,

Note: using HTTP isn’t a security issue here, because:

the raw object data is already public on the chain

we check the hash of the returned data, so it can’t be modified in transit

HTTP might be a minor privacy issue, because the hash and timing of each object fetch is unencrypted. It’s up to Carlos if that’s a significant issue in our use case though.

Allowing HTTPS in this PR would make that a deployment decision, not a developer coding decision.

crates/subspace-gateway/src/commands/http/server.rs

teor2345 · 2024-12-09T04:29:33Z

crates/subspace-gateway/src/commands/http/server.rs

+    #[serde(rename = "pieceIndex")]
+    piece_index: u64,
+    #[serde(rename = "pieceOffset")]
+    piece_offset: u32,


Looks good, but I wanted to go a bit further, see 02c0ca4 (#3293) in #3293.

nazar-pc · 2024-12-18T02:03:04Z

crates/subspace-gateway/src/commands/http/server.rs

+    #[serde(deserialize_with = "string_to_u32")]
+    block_number: BlockNumber,


This looks like a bug to me. Why would you want to receive a number as a string in JSON instead of an actual number? This should be a number, just like all the others in this very struct.

Yeah I thought that was a bit weird! The block number provided by the node in the mapping subscription is a number:

subspace/crates/sc-consensus-subspace-rpc/src/lib.rs

Lines 230 to 237 in 3d5483e

pub struct ObjectMappingResponse {

/// The block number that the object mapping is from.

pub block_number: BlockNumber,

/// The object mappings.

#[serde(flatten)]

pub objects: GlobalObjectMapping,

}

(Specifically a u32.)

Seems to be a problem on my end. I expected it to be a u64/BigInt so postgres automatically parsed to string.

shamil-gadelshin added 2 commits December 4, 2024 20:52

Refactor run command

2c8e70f

Add http command.

821eb3f

shamil-gadelshin requested review from teor2345 and clostao December 4, 2024 17:18

shamil-gadelshin requested review from nazar-pc and rg3l3dr as code owners December 4, 2024 17:18

clostao previously approved these changes Dec 4, 2024

View reviewed changes

teor2345 previously requested changes Dec 4, 2024

View reviewed changes

clostao mentioned this pull request Dec 5, 2024

Adds file retriever to monorepo autonomys/auto-files-gateway#3

Merged

shamil-gadelshin dismissed clostao’s stale review via eb2e985 December 5, 2024 16:49

shamil-gadelshin commented Dec 5, 2024

View reviewed changes

shamil-gadelshin marked this pull request as draft December 5, 2024 16:58

Refactor subspace-gateway

135f043

shamil-gadelshin force-pushed the gateway-http-server branch from eb2e985 to 135f043 Compare December 5, 2024 17:50

shamil-gadelshin marked this pull request as ready for review December 5, 2024 17:50

teor2345 reviewed Dec 6, 2024

View reviewed changes

shamil-gadelshin added 2 commits December 6, 2024 14:38

Update subspace-gateway dependencies

3f810b9

Merge branch 'main' into gateway-http-server

fa4ef90

shamil-gadelshin requested a review from teor2345 December 6, 2024 10:56

Merge fixes

aaa6d7e

clostao reviewed Dec 7, 2024

View reviewed changes

teor2345 approved these changes Dec 9, 2024

View reviewed changes

shamil-gadelshin added this pull request to the merge queue Dec 9, 2024

Merged via the queue into main with commit ac3d5e5 Dec 9, 2024
10 checks passed

shamil-gadelshin deleted the gateway-http-server branch December 9, 2024 11:41

shamil-gadelshin mentioned this pull request Dec 9, 2024

Add protocol to indexer endpoint CLI option #3296

Merged

1 task

nazar-pc reviewed Dec 18, 2024

View reviewed changes

teor2345 mentioned this pull request Dec 18, 2024

Take BlockNumber as a JSON number in subspace gateway HTTP #3323

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add http server to subspace-gateway. #3286

Add http server to subspace-gateway. #3286

shamil-gadelshin commented Dec 4, 2024

teor2345 left a comment

shamil-gadelshin left a comment

shamil-gadelshin Dec 5, 2024

teor2345 left a comment

teor2345 Dec 6, 2024

clostao Dec 7, 2024

clostao Dec 7, 2024

teor2345 Dec 9, 2024

shamil-gadelshin Dec 9, 2024

teor2345 left a comment

teor2345 Dec 9, 2024

teor2345 Dec 9, 2024

nazar-pc Dec 18, 2024 •

edited

Loading

teor2345 Dec 18, 2024

clostao Dec 18, 2024

	#[arg(long, value_hint = ValueHint::Url, default_value = "ws://127.0.0.1:9944")]
	node_rpc_url: String,

		#[serde(deserialize_with = "string_to_u32")]
		block_number: BlockNumber,

	pub struct ObjectMappingResponse {
	/// The block number that the object mapping is from.
	pub block_number: BlockNumber,

	/// The object mappings.
	#[serde(flatten)]
	pub objects: GlobalObjectMapping,
	}

Add http server to subspace-gateway. #3286

Add http server to subspace-gateway. #3286

Conversation

shamil-gadelshin commented Dec 4, 2024

Changes

Code contributor checklist:

teor2345 left a comment

Choose a reason for hiding this comment

shamil-gadelshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 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

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazar-pc Dec 18, 2024 •

edited

Loading