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.

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

[RPC-Spec-V2] Allow chainHead operations to be made from a single connection #3207

Closed
Tracked by #1516
lexnv opened this issue Feb 5, 2024 · 2 comments · Fixed by #3481
Closed
Tracked by #1516

[RPC-Spec-V2] Allow chainHead operations to be made from a single connection #3207

lexnv opened this issue Feb 5, 2024 · 2 comments · Fixed by #3481

Comments

@lexnv
Copy link
Contributor

lexnv commented Feb 5, 2024

The chainHead-cli tool proves that we can have:

  • one connection to the chainHead_folow
  • a separate connection (on a separate process, on a different socket) that calls chainHead_storage with the subscription ID of the first connection; and produces the result in the first connection

To restrict this case, ensure that the subscription ID provided by the chainHead_follow is valid within the context of that connection only.

Repro case

This issue can be reproduced if the user makes 2 separate connections to the same RPC server.

For the common scenario, the user would establish 2 connections via rpc-polkadot.io, which would most likely generate 2 connections on different servers.
However, if the user knows the IP address of the server (or targets a local-development node for testing) this could be reproduced.

cc @paritytech/subxt-team

@jsdw
Copy link
Contributor

jsdw commented Feb 5, 2024

To restrict this case, ensure that the subscription ID provided by the chainHead_follow is valid within the context of that connection only.

Sounds good to me; I'd previously assumed that subscription IDs were scoped to the connection already, and it's def safer to ensure that they are :)

For the production environment use-case, this requires the user to make 2 separate connections to the same RPC server, which is unlikely due to our reverse-proxies. However, this is a valid use-case for testing environments.

I don't understand this sentence really?

@lexnv
Copy link
Contributor Author

lexnv commented Feb 5, 2024

For this case to happen, the user needs 2 separate connections to the same server.

From the user perspective, this is unlikely to happen for our production nodes. For example, connecting 2 times to rpc-polkadot.io would most probably generate 2 connections on different nodes.

However, if the user targets a localhost node / development node, or a node with a known IP address this behavior could be reproduced. Indeed the misleading of my issue is testing environments which is not entirely true; since the target node could still be in production :D will change the description a bit, thanks for pointing this out! 🙏

github-merge-queue bot pushed a commit that referenced this issue Apr 2, 2024
… 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]>
Ank4n pushed a commit that referenced this issue Apr 9, 2024
… 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]>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this issue Apr 9, 2024
… 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment