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

Light client state iteration proof. #8237

Closed
wants to merge 19 commits into from
Closed

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Mar 1, 2021

This PR implements state iteration for light client.
It should also provide an implementation for 'state_getKeysPaged' rpc for light clients.

There is a lot of boilerplate code but most logic is in prove and check function in state-machine.

Also depends on paritytech/trie#127 (could be implemented without but would fetch and additional node in all proofs).

@cheme cheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 1, 2021
@cheme cheme added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 1, 2021
@cheme
Copy link
Contributor Author

cheme commented Mar 2, 2021

Also related to paritytech/smoldot#576 CC\ @tomaka

@cheme cheme 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. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 9, 2021
@cheme cheme added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Mar 19, 2021
@cheme
Copy link
Contributor Author

cheme commented Mar 19, 2021

I did put the PR under 'D5-nice-to-have-audit', because the changes to state-machine, even if pretty trivial are exposing an api which breaks on error while the common api does skip error, so even if it looks rather trivial, this is related to some existing assumption of state-machine assertion.

@cheme
Copy link
Contributor Author

cheme commented Mar 19, 2021

This PR should not change any existing behavior for runtime or externalities function. In this sense runtime audit should just be checking that point, it is actually pretty trivial so I switch back to 'D1-trivial'.

@cheme cheme added D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 19, 2021
@gnunicorn gnunicorn added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label May 19, 2021
@cheme cheme mentioned this pull request May 24, 2021
@arkpar
Copy link
Member

arkpar commented May 27, 2021

Limiting response by key count rather than byte size may cause trouble. You never now if given 1000 keys are over the hardcoded packet limit, which will result in error. If this is used to reconstruct the whole state, the syncing algorithm would need to account for such an error and reduce requested count.

@@ -461,6 +461,21 @@ impl<H: Hasher> StateBackend<H> for GenesisOrUnavailableState<H>
}
}

fn apply_to_key_values_while<A: FnMut(&[u8], &[u8]) -> bool>(
Copy link
Member

Choose a reason for hiding this comment

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

Could be FnMut(Vec<u8>, Vec<u8>) to avoid copying data

@paritytech paritytech deleted a comment from cla-bot-2021 bot Jun 3, 2021
pub count: u32,
/// Maximum total size of value to query.
/// Acts as a threshold.
pub value_size: u32,
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest this to be called max_value_size or value_size_limit here and everywhere.

@stale stale bot closed this Jul 7, 2021
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. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants