add statement store methods

[alexggh]

Add specification for statement RPCs, the RPC's are already implemented and exposed by polkadot-sdk nodes when running with --enable-statement-store and by smoldot.

Fixes: paritytech/polkadot-sdk#11601.

Note! I did a small mistake when naming this API's and they don't have 'v1' in their names, at this point adding "v1" would break downstream library, so I don't think it makes sense to try to support both names.

[josepot]

These methods are not compliant with the format of the JSON-RPC spec. Please, don't merge this PR as it is. 🙏

[josepot]

Note! I did a small mistake when naming this API's and they don't have 'v1' in their names, at this point adding "v1" would break downstream library, so I don't think it makes sense to try to support both names.

That's what happens when you first make an implementation, and then you try to make a spec from it. Please do not pollute the spec with this. Follow the standards, and then the libraries that consume this can handle the fact that the nodes expose garbage... We are already used to that anyways. So, please, stick to the standards on the spec, and then -whenever you have cycles- address the fix on the nodes.

[alexggh]

That's what happens when you first make an implementation, and then you try to make a spec from it.

My mistake here, I wasn't aware of this spec repo and its conventions, I tried matching the conventions already in polkadot-sdk, but somehow I missed that _v1_ is mandatory.

Follow the standards, and then the libraries that consume this can handle the fact that the nodes expose garbage... We are already used to that anyways. So, please, stick to the standards on the spec, and then -whenever you have cycles- address the fix on the nodes.

So your preference is that the specification here is respecting the standard naming with _v1_ ? And then nodes can serve both of them, like we do for a plethora of other RPC's.

I can go this route, if you think it is important, I did consider it when creating this PR, but decided against it because I can't remove the old names at this point, so de-facto you end up with 2.

[josepot]

So your preference is that the specification here is respecting the standard naming with _v1_

My preference is that it starts with unstable and that we discuss the API as if the implementation didn't exist yet. Once, we have tested it for some months and we have finished iterating, then we move to v1. I know that you guys have already been iterating on these APIs, but I'm not completely sure that they are stable enough yet to start with v1 from the get go.

[carlosala]

Hey! First of all, thanks for the effort on this. It is nice to keep spec-ing APIs and find the best for everyone.

I've got a few comments on the general idea of this:

1. The API should follow the conventions of this spec. It does not. Please read the whole Introduction, in particular the Grouping functions section.

   1. We need to find a name for the group (statement, for instance) and start by using unstable for all functions.
   2. We need to make sure that these functions are indeed implementable by any kind of node. There should be clear boundaries for what the implementor is expected to do in distress situations. For example, in chainHead_v1_follow, the implementor must guarantee that 2 subscriptions can be started per connection. More than that, it is not guaranteed. Transaction broadcast have similar limitations.
   3. It is important to differentiate between expected errors (i.e. known, dataTooLarge, etc) and spec errors (i.e. wrong encoding, etc). Errors from 1.2 should go here.

2. In my opinion, it'd be way better to have a single source of emissions per subscription, similar to what happens with chainHead_v1_follow and notifications for new blocks. In my head, the flow goes this way:

   1. statement_unstable_follow(all_statements: bool) -> result<token: str, err>
   2. statement_unstable_add(token: str, filter) -> result<id: str, err>
   3. I receive notifications for new statements matching filter.
   4. statement_unstable_remove(id: str) -> result<void,err>

   If I have filters that have an overlap, I'll only one single notification per statement; which simplifies it as well for the consumer.

   The notifications, therefore, come through the main subscription, and not through each individual subscription by filter. This way it is easier to define clear boundaries and limits, making it easier to resource-constrained environments (such as light clients) to implement successfully this spec.

3. I agree with your spec that statement_unstable_submit should be stateless, and not depend on the subscription mentioned above.

4. You mentioned:

   Note! I did a small mistake when naming this API's and they don't have 'v1' in their names, at this point adding "v1" would break downstream library, so I don't think it makes sense to try to support both names.

   Yes, it matters and it makes sense. If there is a previous implementation pursuing the same goals we are pursuing here, that only serves as context to build this one better. I don't think we should take into account if a previous implementation exists, and build a better spec that will help in the future to consume the data in an optimal and stable manner.

It'll open a PR in the coming days with a concrete proposition for this spec, and we can iterate from both PRs.

[alexggh]

Hey! First of all, thanks for the effort on this. It is nice to keep spec-ing APIs and find the best for everyone.

Yeah, my intention was to document the existence of this API's here. These API's had already been designed with feedback from the actual users of them and it satisfies their current needs. They are currently used on test networks and they will be used on Polkadot People Chain, hence, they will be supported until something better comes along.

The API should follow the conventions of this spec. It does not. Please read the whole Introduction, in particular the Grouping functions section.

I'll follow up these days with changing the name to mark it unstable.

In my opinion, it'd be way better to have a single source of emissions per subscription, similar to what happens with chainHead_v1_follow and notifications for new blocks. In my head, the flow goes this way:

Yeah, I was aware of your suggestions, that's why I have this paritytech/polkadot-sdk#10997, for future improvements, I still thinks the changes could be done in such a way to not affect the existing API's, since if I understand your suggestion we just need some extra RPC to allow to modify the filters of the existing subscription.

Yes, it matters and it makes sense. If there is a previous implementation pursuing the same goals we are pursuing here, that only serves as context to build this one better. I don't think we should take into account if a previous implementation exists, and build a better spec that will help in the future to consume the data in an optimal and stable manner.

This is kind of where I disagree, because in general once you have an API implementation and clients using them, you need to support the API's, so in practice even if you create a better API specification you still need to support both, until you have confirmation no-one else is still using the old one.

It'll open a PR in the coming days with a concrete proposition for this spec, and we can iterate from both PRs.

It is not clear to me what you are proposing here, are you suggesting we merge the current API's as they are with an unstable prefix, or do you want only the new one in this repo ?

Looking forward to your proposal, one thing I want to point out is also, that I'm not sure how much bandwidth we will have to implement the new version, but we can cross that bridge when we get there.

[carlosala]

I opened an alternative spec in #185. I would appreciate your input there!

[alexggh]

I opened an alternative spec in #185. I would appreciate your input there!

Thank you! Will have a look.

[alexggh]

Closing in favour of what comes out of #185 proposal.