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

Add function to get recorded keys from proof recorder instance #8

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

ParthDesai
Copy link

@ParthDesai ParthDesai commented Nov 29, 2023

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context,
including:

  • What does this PR do?
    This PR adds function to get recorded keys from proof recorder instance
  • Why are these changes needed?
    This change is required to get the keys accessed by the trie backend during the runtime execution. The keys are already tracked by proof recorder, just aren't exposed publicly.
  • How were these changes implemented and what do they affect?
    The changes are implemented by adding a public function in proof recorder that simply clones the recorded_keys field.

@nazar-pc I am not sure if this can be upstreamed. Since this change is specific to our use case where we want to get the keys accessed instead of directly getting the proof (this functionality is already there.). Please correct me if I am wrong.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

First of all, excellent PR description!

I am not sure if this can be upstreamed

Have you tried to open a conversation upstream? Better yet, since this is a trivial change just submit a PR there and you might get the first approval within the first hour.

Overall I'm fine with this change, but since it is a bit painful to upgrade Substrate, I think it'd be valuable to pull more stuff in, at least paritytech#2155, but ideally full upgrade if time allows.

substrate/primitives/trie/src/recorder.rs Outdated Show resolved Hide resolved
@ParthDesai
Copy link
Author

First of all, excellent PR description!

I am not sure if this can be upstreamed

Have you tried to open a conversation upstream? Better yet, since this is a trivial change just submit a PR there and you might get the first approval within the first hour.

Overall I'm fine with this change, but since it is a bit painful to upgrade Substrate, I think it'd be valuable to pull more stuff in, at least paritytech#2155, but ideally full upgrade if time allows.

Make sense. I will open a PR in upstream. Will keep this PR open till we receive any feedback from upstream folks.

@ParthDesai
Copy link
Author

PR is opened here: paritytech#2561

Co-authored-by: Nazar Mokrynskyi <nazar@mokrynskyi.com>
@ParthDesai ParthDesai merged commit 4c82ce2 into subspace-v1 Dec 1, 2023
2 of 5 checks passed
@ParthDesai ParthDesai deleted the add-fn-to-get-recorded-keys branch December 1, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants