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

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

Merged
merged 40 commits into from
Apr 2, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Feb 26, 2024

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.

  • server: Register raw method with connection ID 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:

This PR paves the way to implement the chainHead connection limiter:

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:

Closes: #3207

cc @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). R0-silent Changes should not be mentioned in any release notes I5-enhancement An additional feature request. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Feb 26, 2024
@lexnv lexnv self-assigned this Feb 26, 2024
@lexnv lexnv requested review from andresilva and a team as code owners February 26, 2024 13:35
let _ = tx.send(result);
};

executor.spawn_blocking("substrate-rpc-subscription", Some("rpc"), blocking_fut.boxed());
Copy link
Member

Choose a reason for hiding this comment

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

yeah right, spawn_blocking doesn't return a handle for the future that's why you need the extra oneshot channel

/// Limit the RPC functionality to a single connection.
#[derive(Default, Clone)]
pub struct RpcConnections {
data: Arc<Mutex<HashMap<ConnectionId, HashSet<String>>>>,
Copy link
Member

Choose a reason for hiding this comment

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

could also be Arc<Mutex<HashSet<(ConnectionId, SubscriptionId)>>> I guess but perhaps you have plan for it to more generic

Copy link
Contributor

@jsdw jsdw Mar 15, 2024

Choose a reason for hiding this comment

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

mmm since ConnectionId is just a small u64, I'd just combine to Arc<Mutex<HashSet<(ConnectionId, SubscriptionId)>>> and that would be more efficient & slightly simpler code below, and probably small or no real extra space cost :)

lexnv and others added 5 commits March 5, 2024 18:25
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@@ -110,6 +110,8 @@ pub struct ChainHead<BE: Backend<Block>, Block: BlockT, Client> {
/// The maximum number of items reported by the `chainHead_storage` before
/// pagination is required.
operation_max_storage_items: usize,
/// Limit the RPC functionality to a single connection.
Copy link
Member

Choose a reason for hiding this comment

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

I would re-phrase this, I think you are trying to say that each connection has its own state and not that the RPC functionality is limited to a single connection?

This connection state is then used to determine whether a certain call is permitted on a particular connection such as "unstable_storage calls" or something like that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rephrased the documentation a bit, let me know if it makes more sense now 🙏

lexnv and others added 7 commits March 20, 2024 17:55
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
/// For transactionBroadcast, this represents the operation ID.
/// For transaction, this is empty and the number of active calls is tracked by
/// [`Self::num_identifiers`].
identifiers: HashSet<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right to assume that we are not worries about subscription IDs and operation IDs colliding at all in our implementation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, IIRC, we'll either use this for chainHead with subscriptions ID only; or for the tx with operation ID only

let mut data = self.data.lock();

let entry = data.entry(connection_id).or_insert_with(ConnectionData::default);
entry.num_identifiers = entry.num_identifiers.saturating_sub(1);
Copy link
Contributor

@jsdw jsdw Mar 26, 2024

Choose a reason for hiding this comment

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

Do we ever clear out entries completely when the number goes to 0? Else we might eventually leak memory

connection_data.num_identifiers = connection_data.num_identifiers.saturating_sub(1);

if connection_data.num_identifiers == 0 {
data.remove(&connection_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess this is whewre we clean up :)

Copy link
Contributor

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks good to me; only very small comments in the end :)

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv enabled auto-merge March 27, 2024 16:17
@lexnv lexnv disabled auto-merge April 2, 2024 09:49
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv enabled auto-merge April 2, 2024 13:03
@lexnv lexnv added this pull request to the merge queue Apr 2, 2024
Merged via the queue into master with commit 7430f41 Apr 2, 2024
126 of 133 checks passed
@lexnv lexnv deleted the lexnv/single-chain-head-connection branch April 2, 2024 13:45
Ank4n pushed a commit that referenced this pull request 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 <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request 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 <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I5-enhancement An additional feature request. R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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