-
Notifications
You must be signed in to change notification settings - Fork 101
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
[StateService] get historical state #638
[StateService] get historical state #638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments left in GetState
I think returning state without proofs is not a very good thing. And to get a single KV pair we can already use
|
Given that we're not removing the ability to get proofs, what's the concern @roman-khimov? Are you worried about incenting developers to trust remote notes w/o proofs? Instead of a separate method for providing state w/o proof, could we have a method that returns the proof and the state, so that it can be retrieved in one round trip rather than requiring every key to require two round trips (getproof + verifyproof)
I love this idea. Since the data may span multiple pages, is it sufficient to return the boundary proofs for the first and last pairs on the page being returned? |
Yeah, it's not the end of the world thing, but still we better return things that can be checked when we can.
Yep, that should work, one can reconstruct a part of the MPT from a set of KV pairs and boundary proofs, per-page proofs work just fine for this too. |
OK, so let's change the new getstate method to return both the proof and the value. I'd rather add a new method and not break the existing getproof method (though I suspect the number of users of that API at this moment is very low) |
If you are suspicious of
Got it.
Now we have |
You don't need to return proof for every KV, only the first and the last element in a set (page) that you return, it's not that hard and StateService has all the data anyway. |
Why proofs of the first and last can prove all results reliable? |
So user can verify results only using the very first proof and the very last proof cross pages. We don't need to add proof in every page. We have |
Then consumer of this API will have to retrieve and store all pages before he can verify the result. It's not very productive IMO and some users of this API might as well stop iterating at some point. I'd prefer proving data as it comes (in pages). |
I still don't like both proofs and KV in one request. We have |
I'm personally fine with having
Part of the point is to retrieve information in as few round trips as possible. For |
@superboyiii Hi bro. Can you test this pr? |
It should be |
We can't. If the contract is not deployed, There is no contract id. If we really want to prove something, we can prove the contract inexistent in contract management contract. |
int count = Settings.Default.MaxFindResultItems; | ||
if (4 < _params.Count) | ||
count = int.Parse(_params[4].AsString()); | ||
if (Settings.Default.MaxFindResultItems < count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be returned as an exception because it could mislead the interpretation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? Can you describe this in detail? We have truncated
in response and the results are ordered by key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come ledger contract data is not included in state service?
Never mind
Primary reasons for that are described in #520. But we have |
Tested Pass. |
@shargon Could you review again? |
Merge? |
@shargon Merge? |
Changes:
* Add
GetState
FindStates
API to get historical storage or list storages by prefix* Add
README
@devhawk @shargon