-
Notifications
You must be signed in to change notification settings - Fork 682
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
view_state: add include_proof argument to view state request #7603
Conversation
Add include_proof to QueryRequest::ViewState requests and view_state runtime function. If set to true the proof of the response will be populated with nodes which can be used to verify the values. Otherwise, the proof won’t be included. For JSON RPC, the default is for the parameter to be set to false.
cc: @blasrodri |
@posvyatokum, FYI, we’ll need this PR in the next release. |
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.
Codewise, LGTM!
Semantic wise:
- I didn't follow the design discussion here, but I assume we are OK with the way we structure the proofs
- From the changelog, I don't see any mentioning on how to verify the new proofs. I think it's relatively important to include docs into the release.
- The requset has now an optional `include_proof` argument. When set to | ||
`true`, response’s `proof` will be populated. | ||
- The `proof` within each value in `values` list of a `view_state` response is | ||
now deprecated and will be removed in the future. Client code should ignore | ||
the field. |
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.
Could we link some docs explaining how to verify the proof?
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.
Those docs currently don’t exist. It is on my TODO to add them. It’s possible that this might need to go into nomicon.
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.
Process-wise, I would slightly worrying about merging this without docs: docs seem like a phase where we can easily discover some design issues or what not.
I don't think this is blocking, but I would prefer if we did docs before "stabilizing" things.
- The requset has now an optional `include_proof` argument. When set to | ||
`true`, response’s `proof` will be populated. |
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.
Just to verify my own understanding, populating the proof
field isn’t a change made specifically in this PR, correct? state_viewer/mod.rs
seems to have had all of the relevant code in it already, and the relevant change is the inclusion of the argument.
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.
Correct. We’ve added proof recently in #7593. Priori to that commit (that is, up to and including current testnet release) we’ve always sent empty proof. With #7593 we’re always populating proof. In this PR I want to introduce include_proof filed in the request so that users who don’t care about proofs don’t induce unnecessary load on RPC node.
#[serde(default, skip_serializing_if = "is_false")] | ||
include_proof: bool, |
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.
So, this defaults to a false
if not provided, which is fine, but is also somewhat sketchy given the changelog says:
- The
proof
field directly withinview_state
response is currently always
sent even if proof has not been requested. In the future the field will be
skipped in those cases. Clients should accept responses with this field
missing (unless they setinclude_proof
).
Now, I was going to say it is unfortunate that we’re setting ourselves up for an inevitable breaking change, but from what I can tell the reason this disclaimer is there is because something about
let mut iter = state_update.trie().iter()?;
iter.remember_visited_nodes(false);
results in iter
remembering the visited nodes anyway. Is my understanding correct here? The integration tests would suggest that include_proof = false
is actually working the way intuition would suggest it should, though?
I would say that it would be safer to change this default to true
for now and then add a FIXME
on top of this explaining what needs to change in the future. But happy to be convinced otherwise.
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.
remember_visited_nodes
works correctly. If include_proof
is false than proof
field will be an empty vector. The reason for the disclaimer is that I want to remove the field in the future but want to keep it now for compatibility. Note that the proof
fields were defined and sent long before we had proof functionality so I want to avoid situation where a client expects the field to be present (even though it historically has always been an empty array) and suddenly breaks when we stop sending the field if its empty.
Add include_proof to QueryRequest::ViewState requests and view_state runtime function. If set to true the proof of the response will be populated with nodes which can be used to verify the values. Otherwise, the proof won’t be included. For JSON RPC, the default is for the parameter to be set to false. Furthermore, deprecate empty proof fields in the query response. Those were historically always sent and it’s possible that clients exist which expects them to exist even though they were always empty in the past. Plan for the future is to include the `proof` field only when proof was requested. Issue: #2076
Hi! Do I understand correctly, that
{
"jsonrpc": "2.0",
"result": {
"block_hash": "GooyTyYrCmfGQDu5CkEqTrrqQsrq5NCYhhmgyBAwVjg",
"block_height": 83239132,
"proof": [[3,1,0,0,0,...], ...],
"values": [
{
"key": "cnYAAAAAAAAAAA==",
"proof": [],
"value": "FwAAAGJpc29udHJhaWxzLnBvb2x2MS5uZWFyAQAAAAURAAAAZGVwb3NpdF9hbmRfc3Rha2UCAAAAe32Enuq0q8TIePjQAAAAAAAAANCY1K9xAAAhAAAAAO22mGQjajGJOySFQx2tMUfkpVXUDNXPTTKzyRBT+q1rqkEE+bWDxRY="
}
]
},
"id": "33"
} Here, there is a Given that, what is Do you plan to implement proof for specific Thanks in advance! |
|
Add include_proof to QueryRequest::ViewState requests and view_state
runtime function. If set to true the proof of the response will be
populated with nodes which can be used to verify the
values. Otherwise, the proof won’t be included. For JSON RPC, the
default is for the parameter to be set to false.
Furthermore, deprecate empty proof fields in the query response.
Those were historically always sent and it’s possible that clients
exist which expects them to exist even though they were always empty
in the past. Plan for the future is to include the
proof
field onlywhen proof was requested.
Issue: #2076