rpc-servers: Allow chainHead methods to be called from a single connection #3343

lexnv · 2024-02-15T17:44:06Z

This PR adds middleware for the JSON rpc servers to ensure that the chainHead methods are called from a single connection.

The middleware intercepts the subscription IDs generated by the chainHead_follow.
Any further chainHead function call (ie chainHead_storage) must provide a subscription ID that was generated by the middleware's connection.
A middleware per connection is created in this context.

Closes: #3207

cc @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <[email protected]>

substrate/client/rpc-servers/src/middleware/chain_head.rs

Signed-off-by: Alexandru Vasile <[email protected]>

…o lexnv/chainhead-connections

…nections

paritytech-cicd-pr · 2024-02-15T17:59:26Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5229414

niklasad1 · 2024-02-16T07:23:31Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+/// "{"jsonrpc":"2.0","result":"tfMQUZekzJLorGlR","id":0}"
+/// ```
+fn get_method_result(response: &MethodResponse) -> Option<String> {
+	if response.is_error() {


Suggested change

if response.is_error() {

if response.is_error() || !response.is_subscription() {

niklasad1 · 2024-02-16T07:42:58Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+	}
+
+	let result = response.as_result();
+	let Ok(value) = serde_json::from_str(result) else { return None };


yeah, this is awkward to decode the response from the final JSON string.

It would be cleaner and easier with a type that implements deserialize.

#[derive(serde::Deserialize)] struct SubscriptionPayload { result: String, }

I guess the rpc v2 requires to subscription ID to be a string but jsonrpsee accepts integers as well.

niklasad1 · 2024-02-16T07:49:55Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+/// This method handles positional and named `camelCase` parameters.
+fn get_subscription_id<'a>(params: Params<'a>) -> Option<String> {
+	// Support positional parameters.
+	if let Ok(follow_subscription) = params.sequence().next::<String>() {


you can call params.is_object() here to do either parse it as a sequence or map.

however, you use params.one::<String>() instead sequence here since you only care about one item

niklasad1 · 2024-02-16T07:51:34Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+	}
+
+	// Support named parameters.
+	let Ok(value) = params.parse::<serde_json::Value>() else { return None };


again custom type that implement Deserialize here would be cleaner

#[derive(serde::Deserialize)] struct FollowSubscriptionPayload { result: String, }

niklasad1 · 2024-02-16T07:54:51Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+#[derive(Default)]
+pub struct ConnectionData {
+	/// Active `chainHead_follow` subscriptions for this connection.
+	subscriptions: HashSet<String>,


I would probably add a type alias SubscriptionId here for readability.

niklasad1 · 2024-02-16T08:09:03Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+/// subscription ID as a first parameter.
+///
+/// This method handles positional and named `camelCase` parameters.
+fn get_subscription_id<'a>(params: Params<'a>) -> Option<String> {


subscription_id_from_request

niklasad1 · 2024-02-16T08:09:30Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+/// ```ignore
+/// "{"jsonrpc":"2.0","result":"tfMQUZekzJLorGlR","id":0}"
+/// ```
+fn get_method_result(response: &MethodResponse) -> Option<String> {


subscription_id_from_response

niklasad1 · 2024-02-16T08:14:14Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+							req.id(),
+							jsonrpsee::types::error::ErrorObject::owned(
+								-32602,
+								"Invalid subscription ID",


A JSON-RPC error with error code -32602 is generated if one of the parameters doesn't correspond to the expected type (similarly to a missing parameter or an invalid parameter type).

I would probably add another error for this, the subscription ID is decoded successfully but the subscription is not "active" on the connection....

niklasad1 · 2024-02-16T08:26:44Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+impl<F: Future<Output = MethodResponse>> Future for ResponseFuture<F> {
+	type Output = F::Output;
+
+	fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {


not sure but may be worth boxing the future for readability

niklasad1 · 2024-02-16T08:31:31Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+		match this {
+			ResponseFutureProj::Ready { response } =>
+				Poll::Ready(response.take().expect("Value is set; qed")),
+			ResponseFutureProj::Forward { fut } => fut.poll(cx),


why are you not removing subscriptions when chainHead_unstable_unfollow is called?

niklasad1 · 2024-02-16T08:43:26Z

substrate/client/rpc-servers/src/middleware/chain_head.rs

+
+	fn call(&self, req: Request<'a>) -> Self::Future {
+		const CHAIN_HEAD_FOLLOW: &str = "chainHead_unstable_follow";
+		const CHAIN_HEAD_CALL_METHODS: [&str; 8] = [


A random thought is whether we can fetch these method names from the rpc module itself i.e, to keep in sync if a method is removed, modified and so on.

Before the server start you could do:

let chain_head_api = Vec<_> = rpc_api.method_names().filter(|m| m.starts_with("chainHead")).collect(); let chain_middleware = ChainHeadMiddleware::new(service, connection_data, chain_api);

Perhaps, we could add some integration tests for it at some point.

lexnv · 2024-02-28T16:00:57Z

Replaced by: #3481

… context and limit connections (#3481) This PR ensures that the chainHead RPC class can be called only from within the same connection context. The chainHead methods are now registered as raw methods. - paritytech/jsonrpsee#1297 The concept of raw methods is introduced in jsonrpsee, which is an async method that exposes the connection ID: The raw method doesn't have the concept of a blocking method. Previously blocking methods are now spawning a blocking task to handle their blocking (ie DB) access. We spawn the same number of tasks as before, however we do that explicitly. Another approach would be implementing a RPC middleware that captures and decodes the method parameters: - #3343 However, that approach is prone to errors since the methods are hardcoded by name. Performace is affected by the double deserialization that needs to happen to extract the subscription ID we'd like to limit. Once from the middleware, and once from the methods itself. This PR paves the way to implement the chainHead connection limiter: - #1505 Registering tokens (subscription ID / operation ID) on the `RpcConnections` could be extended to return an error when the maximum number of operations is reached. While at it, have added an integration-test to ensure that chainHead methods can be called from within the same connection context. Before this is merged, a new JsonRPC release should be made to expose the `raw-methods`: - [x] Use jsonrpsee from crates io (blocked by: paritytech/jsonrpsee#1297) Closes: #3207 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]>

… context and limit connections (paritytech#3481) This PR ensures that the chainHead RPC class can be called only from within the same connection context. The chainHead methods are now registered as raw methods. - paritytech/jsonrpsee#1297 The concept of raw methods is introduced in jsonrpsee, which is an async method that exposes the connection ID: The raw method doesn't have the concept of a blocking method. Previously blocking methods are now spawning a blocking task to handle their blocking (ie DB) access. We spawn the same number of tasks as before, however we do that explicitly. Another approach would be implementing a RPC middleware that captures and decodes the method parameters: - paritytech#3343 However, that approach is prone to errors since the methods are hardcoded by name. Performace is affected by the double deserialization that needs to happen to extract the subscription ID we'd like to limit. Once from the middleware, and once from the methods itself. This PR paves the way to implement the chainHead connection limiter: - paritytech#1505 Registering tokens (subscription ID / operation ID) on the `RpcConnections` could be extended to return an error when the maximum number of operations is reached. While at it, have added an integration-test to ensure that chainHead methods can be called from within the same connection context. Before this is merged, a new JsonRPC release should be made to expose the `raw-methods`: - [x] Use jsonrpsee from crates io (blocked by: paritytech/jsonrpsee#1297) Closes: paritytech#3207 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]>

lexnv added 4 commits February 15, 2024 18:07

rpc-servers: Add chainHead middleware to capture subscription IDs

5989756

Signed-off-by: Alexandru Vasile <[email protected]>

rpc-servers: Capture the method result as subscription ID

21615a8

Signed-off-by: Alexandru Vasile <[email protected]>

rpc-servers: Cleanup the ResponseFuture

0a5e2dc

Signed-off-by: Alexandru Vasile <[email protected]>

rpc-servers: Use parkinglot instead of std mutex

5b6063f

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv self-assigned this Feb 15, 2024

lexnv commented Feb 15, 2024

View reviewed changes

substrate/client/rpc-servers/src/middleware/chain_head.rs Outdated Show resolved Hide resolved

lexnv and others added 4 commits February 15, 2024 19:44

Update substrate/client/rpc-servers/src/middleware/chain_head.rs

cb7b67a

rpc-servers: Add parking lot dependency

9ae9e9b

Signed-off-by: Alexandru Vasile <[email protected]>

Merge remote-tracking branch 'origin/lexnv/chainhead-connections' int…

a1a6a4b

…o lexnv/chainhead-connections

Merge remote-tracking branch 'origin/master' into lexnv/chainhead-con…

8e2b268

…nections

niklasad1 reviewed Feb 16, 2024

View reviewed changes

Bullrich mentioned this pull request Feb 22, 2024

rpc-servers: Allow chainHead methods to be called from a single connection paritytech-stg/polkadot-sdk#22

Closed

lexnv mentioned this pull request Feb 26, 2024

chainHead: Allow methods to be called from within a single connection context and limit connections #3481

Merged

1 task

lexnv closed this Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc-servers: Allow chainHead methods to be called from a single connection #3343

rpc-servers: Allow chainHead methods to be called from a single connection #3343

lexnv commented Feb 15, 2024 •

edited

Loading

paritytech-cicd-pr commented Feb 15, 2024

niklasad1 Feb 16, 2024

niklasad1 Feb 16, 2024

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024

niklasad1 Feb 16, 2024

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024 •

edited

Loading

lexnv commented Feb 28, 2024

	if response.is_error() {
	if response.is_error() \|\| !response.is_subscription() {

rpc-servers: Allow chainHead methods to be called from a single connection #3343

rpc-servers: Allow chainHead methods to be called from a single connection #3343

Conversation

lexnv commented Feb 15, 2024 • edited Loading

paritytech-cicd-pr commented Feb 15, 2024

niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

niklasad1 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

lexnv commented Feb 28, 2024

lexnv commented Feb 15, 2024 •

edited

Loading

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024 •

edited

Loading

niklasad1 Feb 16, 2024 •

edited

Loading