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

Traitify Substrate client #2129

Merged
merged 40 commits into from
May 19, 2023
Merged

Traitify Substrate client #2129

merged 40 commits into from
May 19, 2023

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented May 12, 2023

related to #1820

What this PR does:

  • introduces the new Client trait, that will expose all node RPC methods, used by the relay;
  • renames old Client structure to RpcClient;
  • introduces CachingClient, backed by the RpcClient. This implementation caches result of some RpcClient method calls. Additionally, it allows its clients to share the GRANDPA and BEEFY justification subscriptions - i.e. we'll only have one subscription between node and relay, but justifiications may be delivered to any number of subscribers in the relay;
  • all relay code now uses the CachingClient<RpcClient> where the old Client has been used.

What is left in this PR:

What is left for future PRs:

  • make the relay code generic over underlying client type. Right now we are using CachingClient<RpcClient> everywhere, but instead we shall be able to use anything that implements Client trait;
  • introduce TestClient;
  • use TestClient in tests where right now we have (or must have) "environment" traits;
  • more cleanup - e.g. errors with context.

Cache performance in Rialto<>Millau (~30 mins running):

Client HeaderHashByNumber HeaderByHash BlockByHash RawStorageValue StateCall
RialtoClient 754 hits, 336 misses (70%) 1499 hits, 351 misses (81%) 205 hits, 314 misses (40%) 2081 hits, 2113 misses (50%) 551 hits, 518 misses (51%)
MillauClient 583 hits, 316 misses (65%) 1394 hits, 337 misses (80%) 140 hits, 276 misses (33%) 2011 hits, 2178 misses (50) 576 hits, 524 misses (52%)

Cache performance in RialtoParachain<>Millau (~30 mins running):

Client HeaderHashByNumber HeaderByHash BlockByHash RawStorageValue StateCall
RialtoClient 308 hits, 327 misses (50%) 904 hits, 343 misses (72%) 131 hits, 244 misses (35%) 654/305 (68%)
RialtoParachainClient 304 hits, 111 misses (73%) 990 hits, 150 misses (87%) 1666 hits, 1254 misses (87%) 632 hits, 347 misses (65%)
MillauClient 959 htis, 392 misses (71%) 1638 hits, 388 misses (81%) 307 hits, 364 misses (46%) 2205 htis, 2233 misses (50%) 1203 hits, 852 misses (56%)

@svyatonik svyatonik marked this pull request as ready for review May 15, 2023 11:21
@svyatonik
Copy link
Contributor Author

Additional changes in this PR:

  • I've tried to make relay-substrate-client errors more detailed - it isn't always clear which call e.g. has lead to the error from current logs. Ideally we need to use some errors with context in the future;
  • I've removed all "default" at-block arguments from the Client trait - now you always need to specify the block you want e.g. to do some runtime call at. In previous version you may have specified None and it would have ran at the best known block. But this may lead to different issues - e.g. if you do multiple calls and the best block advances, you may get conflicting results.

@serban300 serban300 self-requested a review May 15, 2023 12:23
}

/// Return subscription factory for this subscription.
pub fn factory(&self) -> SharedSubscriptionFactory<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I find it counter-intuitive that the subscription can return a SharedSubscriptionFactory. I would expect only the SharedSubscriptionFactory to be able to create Subscriptions. Would it make sense to remove this method and the subscribers_sender from Subscription ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to return Subscription from the Client methods, not the factory. So I need some adapter to be able to share the subscribction. I started to make this adapter and found that both adapter and subscription are almost the same (some background task that reads elements from one stream and sends to 1 or multiple receivers). So I've made this tightly coupled subscription and factory. Regarding your suggestion - we can't simple remove the factory method. I think we have following options:

  1. just make the method private - it'll only be available within the crate and won't cause any confusion for other users;
  2. return factory from fn subscribe()
  3. revert this factory and reintroduce multiplexing adapter.

WDYT is better

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm I'm not sure which one would be best. Anyway it's not a blocker. We can merge it as it is, and I'll spend some more time thinking about it later.

@svyatonik svyatonik merged commit 7e38a89 into master May 19, 2023
@svyatonik svyatonik deleted the new-client branch May 19, 2023 07:18
svyatonik added a commit that referenced this pull request May 24, 2023
svyatonik added a commit that referenced this pull request May 24, 2023
* Revert "Make client code generic over Client type (#2135)"

This reverts commit 1a95216.

* Revert "Traitify Substrate client (#2129)"

This reverts commit 7e38a89.
@bkontur bkontur mentioned this pull request Jun 5, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants