Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Don't send back empty proofs if light request fails #12372

Merged

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Sep 28, 2022

At the moment, if the node receives a so-called "light client request" (asking for a storage proof or a call proof), and this request cannot be served, we send back a message indicating that everything is ok, except that it contains an empty proof.
The reason why a request cannot be served is typically that the block has been pruned.

This PR modifies the protocol so that we send back a message without any proof instead.

To do an analogy, right now we more or less send back Some(Vec::new()), while this PR modifies this to send back None.

This makes it clearer from the point of view of the request sender that something bad has happened.

@tomaka tomaka added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Sep 28, 2022
@tomaka tomaka requested a review from cheme September 28, 2022 07:28
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

Change looks good (til I learn that oneof is not required).
I am wondering a bit about versioning, but will probably leave it like this.
Would need to be mentioned in release note as potentially breaking through.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 28, 2022

I am wondering a bit about versioning, but will probably leave it like this.

As far as I know, only smoldot sends these requests at the moment, and I'm ok with my own change.

@lamafab
Copy link

lamafab commented Oct 5, 2022

What about the Header and Changes responses (currently both still just return HandleRequestError::BadRequest("Not supported."))?

EDIT: Changes tries were deprecated, so I guess the Changes request/response messages will be removed anyway.

@bkchr
Copy link
Member

bkchr commented Oct 5, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 4cd3248 into paritytech:master Oct 5, 2022
@tomaka tomaka deleted the light-client-rq-proofs branch October 5, 2022 16:02
ordian added a commit that referenced this pull request Oct 5, 2022
* master:
  Implement `Clone` and `Default` for `Config` (#12397)
  Don't send back empty proofs if light request fails (#12372)
  MMR: impl `TypeInfo` for some structures (#12423)
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants