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 batched reqs #315

Merged
merged 9 commits into from
Oct 14, 2024
Merged

Fix batched reqs #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0135dcb
handle batched requests in middleware
apoorvsadana Oct 7, 2024
2003682
fix changelog
apoorvsadana Oct 7, 2024
94008b6
remove println
apoorvsadana Oct 7, 2024
e7f0148
broken rpcs should return with correct id
apoorvsadana Oct 8, 2024
77f7542
merge and fix
apoorvsadana Oct 8, 2024
e719650
Merge branch 'main' into fix_batched_reqs
Trantorian1 Oct 9, 2024
7772e09
merge main
apoorvsadana Oct 11, 2024
57eba3a
Merge branch 'main' into fix_batched_reqs
apoorvsadana Oct 14, 2024
1c39f2d
Merge branch 'main' into fix_batched_reqs
antiyro Oct 14, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## Next release

- fix(rpc): handle batched requests in middleware
- chore: padded devnet address display with 64 chars
- feat(script): added more capabilities to the launcher script
- fix(fgw): sync from other nodes and block signature
Expand Down
34 changes: 20 additions & 14 deletions crates/node/src/service/rpc/middleware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,6 @@ enum VersionMiddlewareError {
BodyReadError(#[from] hyper::Error),
#[error("Failed to parse JSON: {0}")]
JsonParseError(#[from] serde_json::Error),
#[error("Invalid request format")]
InvalidRequestFormat,
#[error("Invalid URL format")]
InvalidUrlFormat,
#[error("Invalid version specified")]
Expand Down Expand Up @@ -236,9 +234,6 @@ where
VersionMiddlewareError::InvalidVersion => {
ErrorObject::owned(-32600, "Invalid RPC version specified", None::<()>)
}
VersionMiddlewareError::InvalidRequestFormat => {
ErrorObject::owned(-32600, "Invalid JSON-RPC request format", None::<()>)
}
VersionMiddlewareError::UnsupportedVersion => {
ErrorObject::owned(-32601, "Unsupported RPC version specified", None::<()>)
}
Expand All @@ -248,7 +243,7 @@ where
let body = json!({
"jsonrpc": "2.0",
"error": error,
"id": 0
"id": null
Copy link
Member

Choose a reason for hiding this comment

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

ah so we can just null out that field, okay
i guess that makes sense

})
.to_string();

Expand All @@ -267,18 +262,29 @@ async fn add_rpc_version_to_method(req: &mut hyper::Request<Body>) -> Result<(),
let version = RpcVersion::from_request_path(&path)?;

let whole_body = hyper::body::to_bytes(req.body_mut()).await?;
let mut json: Value = serde_json::from_slice(&whole_body)?;

if let Some(method) = json.get_mut("method").as_deref().and_then(Value::as_str) {
let new_method = format!("starknet_{}_{}", version.name(), method.strip_prefix("starknet_").unwrap_or(method));
let json: Value = serde_json::from_slice(&whole_body)?;
Copy link
Member

Choose a reason for hiding this comment

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

i really don't like the fact that we're deserializing and reserializing the body like that 😔 this has a perf cost, especially with serde_json::Value
but that isnt the scope of this PR i guess..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya we can do that in a new PR probably, I can create an issue if you like. Do you've any specific suggestions?

Copy link
Member

@cchudant cchudant Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i think it would require us to handle versions very differently

the problem i have with parsing requests to a serde_json::Value is that it looks like this https://docs.rs/serde_json/latest/serde_json/enum.Value.html
it does one heap allocation per object, string and array, recursively for the whole request and it's really unnecessary
also, their Object(Map<... variant is a BTreeMap, which is really really pushing it - it does a ton of allocs, as you would expect

parsing a concrete struct is much cheaper since nested structs and enums only require one allocation as a whole unless there's a Box or Vec or such in between, so it's not that we're deserializing it twice for nothing, we're actually deserializing it first in the most inefficient way first and then deserializing it again in an okay form

(it's not a problem of the serde_json library, their serde_json::Value type is very sensible, it's just that we should avoid it in such a critical path)

Copy link
Member

Choose a reason for hiding this comment

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

let's make an issue, right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done #322


json["method"] = Value::String(new_method);
// in case of batched requests, the request is an array of JSON-RPC requests
let mut batched_request = false;
let mut items = if let Value::Array(items) = json {
batched_request = true;
items
} else {
return Err(VersionMiddlewareError::InvalidRequestFormat);
vec![json]
};
Comment on lines +268 to +274
Copy link
Member

Choose a reason for hiding this comment

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

Could we do an helper function for batched_request like:

fn is_batched_request(json: &Value) -> Option<&Vec<Value>> {
    match json {
        Value::Array(items) => Some(items),
        _ => None,
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Not mandatory but it would be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, although making is_batched_request return a optional vector looks non intuitive. I can maybe change it to get_items_from_request but then I would need to return a tuple - (items, is_batched_request)


for item in items.iter_mut() {
if let Some(method) = item.get_mut("method").as_deref().and_then(Value::as_str) {
let new_method =
format!("starknet_{}_{}", version.name(), method.strip_prefix("starknet_").unwrap_or(method));

item["method"] = Value::String(new_method);
}
// we don't need to throw an error here, the request will be rejected later if the method is not supported
}

let new_body = Body::from(serde_json::to_vec(&json)?);
*req.body_mut() = new_body;
let response = if batched_request { serde_json::to_vec(&items)? } else { serde_json::to_vec(&items[0])? };
*req.body_mut() = Body::from(response);

Ok(())
}
82 changes: 82 additions & 0 deletions crates/tests/src/rpc/read.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,88 @@ mod test_rpc_read_calls {
assert_eq!(result, 1);
}

/// Fetches the latest block hash and number.
///
/// Example curl command:
///
/// ```bash
/// curl --location 'https://free-rpc.nethermind.io/sepolia-juno/' \
/// --header 'Content-Type: application/json' \
/// --data '[
/// {
/// "jsonrpc": "2.0",
/// "method": "starknet_blockHashAndNumber",
/// "params": {},
/// "id": 0
/// },
/// {
/// "jsonrpc": "2.0",
/// "method": "starknet_getBlockTransactionCount",
/// "params": {
/// "block_id": {
/// "block_number": 2
/// }
/// },
/// "id": 1
/// }
/// ]'
/// ```
#[rstest]
#[tokio::test]
async fn test_batched_requests_work() {
let madara = get_shared_state().await;

// use reqwest to send a batch request to the madara rpc.
// TODO: use a jsonrpc client instead of reqwest when we move
// to starknet-providers 0.12.0
let client = reqwest::Client::new();
let res = client
.post(madara.rpc_url.clone())
.json(&[
serde_json::json!({
"jsonrpc": "2.0",
"method": "starknet_blockHashAndNumber",
"params": {},
"id": 0
}),
serde_json::json!({
"jsonrpc": "2.0",
"method": "starknet_getBlockTransactionCount",
"params": {
"block_id": {
"block_number": 2
}
},
"id": 1
}),
])
.send()
.await
.unwrap();

let result = res.json::<serde_json::Value>().await.unwrap();

assert_eq!(
result[0],
serde_json::json!({
"jsonrpc": "2.0",
"result": {
"block_hash": "0x4177d1ba942a4ab94f86a476c06f0f9e02363ad410cdf177c54064788c9bcb5",
"block_number": 19
},
"id": 0
})
);
assert_eq!(
result[1],
serde_json::json!({
"jsonrpc": "2.0",
"result": 1,
"id": 1
})
);
}

/// Fetches a block with its transactions and receipts.
///
/// Example curl command:
Expand Down
Loading