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

view_state: add include_proof argument to view state request #7603

Merged
merged 17 commits into from
Sep 19, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@
Instead of it aggregate `near_peer_message_received_by_type_total` metric.
For example, to get total rate of received messages use
`sum(rate(near_peer_message_received_by_type_total{...}[5m]))`.
* Few changes to `view_state` JSON RPC query:
- The requset has now an optional `include_proof` argument. When set to
`true`, response’s `proof` will be populated.
Comment on lines +30 to +31
Copy link
Collaborator

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.

Copy link
Contributor Author

@mina86 mina86 Sep 19, 2022

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.

- 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.
Comment on lines +30 to +34
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 `proof` field directly within `view_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 set `include_proof`).
matklad marked this conversation as resolved.
Show resolved Hide resolved
* Backtraces on panics are enabled by default, so you no longer need to set
`RUST_BACKTRACE=1` environmental variable. To disable backtraces, set
`RUST_BACKTRACE=0`.
Expand Down
6 changes: 5 additions & 1 deletion chain/client/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1542,7 +1542,11 @@ impl TestEnv {
last_block.header().prev_hash(),
last_block.header().hash(),
last_block.header().epoch_id(),
&QueryRequest::ViewState { account_id, prefix: vec![].into() },
&QueryRequest::ViewState {
account_id,
prefix: vec![].into(),
include_proof: false,
},
)
.unwrap();
match response.kind {
Expand Down
1 change: 1 addition & 0 deletions chain/jsonrpc/jsonrpc-tests/tests/rpc_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ fn test_query_state() {
request: QueryRequest::ViewState {
account_id: "test".parse().unwrap(),
prefix: vec![].into(),
include_proof: false,
},
})
.await
Expand Down
6 changes: 6 additions & 0 deletions core/primitives/src/views.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,8 @@ pub enum QueryRequest {
account_id: AccountId,
#[serde(rename = "prefix_base64", with = "base64_format")]
prefix: StoreKey,
#[serde(default, skip_serializing_if = "is_false")]
include_proof: bool,
Comment on lines +280 to +281
Copy link
Collaborator

@nagisa nagisa Sep 19, 2022

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 within view_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 set include_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.

Copy link
Contributor Author

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.

},
ViewAccessKey {
account_id: AccountId,
Expand All @@ -290,6 +292,10 @@ pub enum QueryRequest {
},
}

fn is_false(v: &bool) -> bool {
!*v
}

#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))]
#[derive(Debug, PartialEq, Eq, Clone)]
pub struct QueryResponse {
Expand Down
140 changes: 98 additions & 42 deletions integration-tests/src/tests/runtime/state_viewer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,74 @@ fn test_view_call_with_args() {
assert_eq!(view_call_result.unwrap(), 3u64.to_le_bytes().to_vec());
}

#[track_caller]
fn assert_proof(proof: &[Arc<[u8]>], want: &[&'static str]) {
let got = proof.iter().map(|bytes| to_base64(bytes)).collect::<Vec<_>>();
fn assert_view_state(
trie_viewer: &TrieViewer,
state_update: &near_store::TrieUpdate,
prefix: &[u8],
want_values: &[(&[u8], &[u8])],
want_proof: &[&'static str],
) -> ProofVerifier {
let alice = alice_account();
let alina = "alina".parse().unwrap();

let values = want_values
.iter()
.map(|(key, value)| StateItem { key: key.to_vec(), value: value.to_vec(), proof: vec![] })
.collect::<Vec<_>>();

// Test without proof
mina86 marked this conversation as resolved.
Show resolved Hide resolved
let view_state =
|include_proof| trie_viewer.view_state(&state_update, &alice, prefix, include_proof);

let result = view_state(false).unwrap();
assert_eq!(values, result.values);
assert_eq!(Vec::<Arc<[u8]>>::new(), result.proof);

// Test with proof included
let result = view_state(true).unwrap();
assert_eq!(values, result.values);
let got = result.proof.iter().map(|bytes| to_base64(bytes)).collect::<Vec<_>>();
let got = got.iter().map(String::as_str).collect::<Vec<_>>();
assert_eq!(want, &got[..]);
// The proof isn’t deterministic because the state contains test contracts
// which aren’t built hermetically. Fortunately, only the first two items
// in the proof are affected. First one is the state root which is an
// Extension("0x0", child_hash) node and the second one is child hash
// pointing to a Branch node which splits into four: 0x0 (accounts), 0x1
// (contract code; that’s what’s nondeterministic), 0x2 (access keys) and
// 0x9 (contract data; that’s what we care about).
assert_eq!(&want_proof[..], &got[2..]);

// Verify proofs for all the expected values.
let proof_verifier = ProofVerifier::new(result.proof).unwrap();
let root = state_update.get_root();
for (key, value) in want_values {
// Proof for known (key, value) should succeed.
assert!(
proof_verifier.verify(root, &alice, key, Some(value)),
"key: alice / {key:x?}; value: {value:x?}"
);
// The key exist, so proof for non-existence should fail.
assert!(
!proof_verifier.verify(root, &alice, key, None),
"key: alice / {key:x?}; value: None"
);
// Proof for different value should fail.
assert!(
!proof_verifier.verify(root, &alice, key, Some(b"bogus")),
"key: alice / {key:x?}; value: None"
);
// Proofs for different account should fail.
assert!(
!proof_verifier.verify(root, &alina, key, Some(value)),
"key: alice / {key:x?}; value: {value:x?}"
);
assert!(
!proof_verifier.verify(root, &alina, key, None),
"key: alice / {key:x?}; value: {value:x?}"
);
}

proof_verifier
}

#[test]
Expand Down Expand Up @@ -221,17 +284,8 @@ fn test_view_state() {

let state_update = tries.new_trie_update(shard_uid, new_root);
let trie_viewer = TrieViewer::default();
let result = trie_viewer.view_state(&state_update, &alice_account(), b"").unwrap();
// The proof isn’t deterministic because the state contains test contracts
// which aren’t built hermetically. Fortunately, only the first two items
// in the proof are affected. First one is the state root which is an
// Extension("0x0", child_hash) node and the second one is child hash
// pointing to a Branch node which splits into four: 0x0 (accounts), 0x1
// (contract code; that’s what’s nondeterministic), 0x2 (access keys) and
// 0x9 (contract data; that’s what we care about).
assert_proof(&result.proof[2..], &[
"AwEAAAAQjHWWT6rXAXqUm14fjfDxo3286ApntHMI1eK0aQAJZPfJewEAAAAAAA==",
"AQcCSXBK8DHIYBF47dz6xB2iFKLLsPjAIAo9syJTBC0/Y1OjJNvT5izZukYCmtq/AyVTeyWFl1Ei6yFZBf5yIJ0i96eYRr8PVilJ81MgJKvV/R1SxQuTfwwmbZ6sN/TC2XfL1SCJ4WM1GZ0yMSaNpJOdsJH9kda203WM3Zh81gxz6rmVewEAAAAAAA==",

let proof = [
"AwMAAAAWFsbwm2TFX4GHLT5G1LSpF8UkG7zQV1ohXBMR/OQcUAKZ3gwDAAAAAAAA",
"ASAC7S1KwgLNl0HPdSo8soL8sGOmPhL7O0xTSR8sDDR5pZrzu0ty3UPYJ5UKrFGKxXoyyyNG75AF9hnJHO3xxFkf5NQCAAAAAAAA",
"AwEAAAAW607KPj2q3O8dF6XkfALiIrd9mqGir2UlYIcZuLNksTsvAgAAAAAAAA==",
Expand All @@ -240,41 +294,37 @@ fn test_view_state() {
"AQoAVWCdny7wv/M1LvZASC3Fw0D/NNhI1NYwch9Ux+KZ2qRdQXPC1rNsCGRJ7nd66SfcNmRUVVvQY6EYCbsIiugO6gwBAAAAAAAA",
"AAMAAAAgMjMDAAAApmWkWSBCL51Bfkhn79xPuKBKHz//H6B+mY6G9/eieuNtAAAAAAAAAA==",
"AAMAAAAgMjEDAAAAjSPPbIboNKeqbt7VTCbOK7LnSQNTjGG91dIZeZerL3JtAAAAAAAAAA==",
][2..]);
assert_eq!(
result.values,
[
StateItem { key: b"test123".to_vec(), value: b"123".to_vec(), proof: vec![] },
StateItem { key: b"test321".to_vec(), value: b"321".to_vec(), proof: vec![] }
]
);
let result = trie_viewer.view_state(&state_update, &alice_account(), b"xyz").unwrap();
assert_eq!(result.values, []);
let result = trie_viewer.view_state(&state_update, &alice_account(), b"test123").unwrap();
assert_eq!(
result.values,
[StateItem { key: b"test123".to_vec(), value: b"123".to_vec(), proof: vec![] }]
);
assert_proof(
&result
.proof[2..],
];
let values = [(&b"test123"[..], &b"123"[..]), (&b"test321"[..], &b"321"[..])];
assert_view_state(&trie_viewer, &state_update, b"", &values, &proof);
assert_view_state(&trie_viewer, &state_update, b"test", &values, &proof);

assert_view_state(&trie_viewer, &state_update, b"xyz", &[], &[
"AwMAAAAWFsbwm2TFX4GHLT5G1LSpF8UkG7zQV1ohXBMR/OQcUAKZ3gwDAAAAAAAA",
"ASAC7S1KwgLNl0HPdSo8soL8sGOmPhL7O0xTSR8sDDR5pZrzu0ty3UPYJ5UKrFGKxXoyyyNG75AF9hnJHO3xxFkf5NQCAAAAAAAA",
"AwEAAAAW607KPj2q3O8dF6XkfALiIrd9mqGir2UlYIcZuLNksTsvAgAAAAAAAA==",
"AQhAP4sMdbiWZPtV6jz8hYKzRFSgwaSlQKiGsQXogAmMcrLOl+SJfiCOXMTEZ2a1ebmQOEGkRYa30FaIlB46sLI2IPsBAAAAAAAA",
"AwwAAAAWUubmVhcix0ZXN0PKtrEndk0LxM+qpzp0PVtjf+xlrzz4TT0qA+hTtm6BLlYBAAAAAAAA",
][..]);

let proof_verifier = assert_view_state(
&trie_viewer,
&state_update,
b"test123",
&[(&b"test123"[..], &b"123"[..])],
&[
"AwEAAAAQjHWWT6rXAXqUm14fjfDxo3286ApntHMI1eK0aQAJZPfJewEAAAAAAA==",
"AQcCSXBK8DHIYBF47dz6xB2iFKLLsPjAIAo9syJTBC0/Y1OjJNvT5izZukYCmtq/AyVTeyWFl1Ei6yFZBf5yIJ0i96eYRr8PVilJ81MgJKvV/R1SxQuTfwwmbZ6sN/TC2XfL1SCJ4WM1GZ0yMSaNpJOdsJH9kda203WM3Zh81gxz6rmVewEAAAAAAA==",
"AwMAAAAWFsbwm2TFX4GHLT5G1LSpF8UkG7zQV1ohXBMR/OQcUAKZ3gwDAAAAAAAA",
"ASAC7S1KwgLNl0HPdSo8soL8sGOmPhL7O0xTSR8sDDR5pZrzu0ty3UPYJ5UKrFGKxXoyyyNG75AF9hnJHO3xxFkf5NQCAAAAAAAA",
"AwEAAAAW607KPj2q3O8dF6XkfALiIrd9mqGir2UlYIcZuLNksTsvAgAAAAAAAA==",
"AQhAP4sMdbiWZPtV6jz8hYKzRFSgwaSlQKiGsQXogAmMcrLOl+SJfiCOXMTEZ2a1ebmQOEGkRYa30FaIlB46sLI2IPsBAAAAAAAA",
"AwwAAAAWUubmVhcix0ZXN0PKtrEndk0LxM+qpzp0PVtjf+xlrzz4TT0qA+hTtm6BLlYBAAAAAAAA",
"AQoAVWCdny7wv/M1LvZASC3Fw0D/NNhI1NYwch9Ux+KZ2qRdQXPC1rNsCGRJ7nd66SfcNmRUVVvQY6EYCbsIiugO6gwBAAAAAAAA",
"AAMAAAAgMjMDAAAApmWkWSBCL51Bfkhn79xPuKBKHz//H6B+mY6G9/eieuNtAAAAAAAAAA==",
][2..]
]
);

let root = state_update.get_root();
let account = alice_account();
let proof_verifier =
ProofVerifier::new(result.proof).expect("could not create a ProofVerifier");
for (want, key, value) in [
(true, b"test123".as_ref(), Some(b"123".as_ref())),
(false, b"test123".as_ref(), Some(b"321".as_ref())),
Expand All @@ -287,8 +337,14 @@ fn test_view_state() {
(false, b"test1234", Some(b"123")),
(true, b"test1234", None),
] {
let got = proof_verifier.verify(root, &account, key, value);
assert_eq!(want, got, "key: {key:x?}; value: {value:x?}");
let got = proof_verifier.verify(&root, &account, key, value);
assert_eq!(
want,
got,
"key: {:?}; value: {:?}",
std::str::from_utf8(key).unwrap(),
value.map(|value| std::str::from_utf8(value).unwrap())
);
}
}

Expand All @@ -302,7 +358,7 @@ fn test_view_state_too_large() {
&Account::new(0, 0, CryptoHash::default(), 50_001),
);
let trie_viewer = TrieViewer::new(Some(50_000), None);
let result = trie_viewer.view_state(&state_update, &alice_account(), b"");
let result = trie_viewer.view_state(&state_update, &alice_account(), b"", false);
assert!(matches!(result, Err(errors::ViewStateError::AccountStateTooLarge { .. })));
}

Expand All @@ -318,7 +374,7 @@ fn test_view_state_with_large_contract() {
);
state_update.set(TrieKey::ContractCode { account_id: alice_account() }, contract_code);
let trie_viewer = TrieViewer::new(Some(50_000), None);
let result = trie_viewer.view_state(&state_update, &alice_account(), b"");
let result = trie_viewer.view_state(&state_update, &alice_account(), b"", false);
assert!(result.is_ok());
}

Expand Down
1 change: 1 addition & 0 deletions integration-tests/src/user/rpc_user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ impl User for RpcUser {
let query = QueryRequest::ViewState {
account_id: account_id.clone(),
prefix: prefix.to_vec().into(),
include_proof: false,
};
match self.query(query)?.kind {
near_jsonrpc_primitives::types::query::QueryResponseKind::ViewState(
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/src/user/runtime_user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ impl User for RuntimeUser {
fn view_state(&self, account_id: &AccountId, prefix: &[u8]) -> Result<ViewStateResult, String> {
let state_update = self.client.read().expect(POISONED_LOCK_ERR).get_state_update();
self.trie_viewer
.view_state(&state_update, account_id, prefix)
.view_state(&state_update, account_id, prefix, false)
.map_err(|err| err.to_string())
}

Expand Down
13 changes: 10 additions & 3 deletions nearcore/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1524,9 +1524,15 @@ impl RuntimeAdapter for NightshadeRuntime {
block_hash: *block_hash,
})
}
QueryRequest::ViewState { account_id, prefix } => {
QueryRequest::ViewState { account_id, prefix, include_proof } => {
let view_state_result = self
.view_state(&shard_uid, *state_root, account_id, prefix.as_ref())
.view_state(
&shard_uid,
*state_root,
account_id,
prefix.as_ref(),
*include_proof,
)
.map_err(|err| {
near_chain::near_chain_primitives::error::QueryError::from_view_state_error(
err,
Expand Down Expand Up @@ -1943,9 +1949,10 @@ impl node_runtime::adapter::ViewRuntimeAdapter for NightshadeRuntime {
state_root: MerkleHash,
account_id: &AccountId,
prefix: &[u8],
include_proof: bool,
) -> Result<ViewStateResult, node_runtime::state_viewer::errors::ViewStateError> {
let state_update = self.tries.new_trie_update_view(*shard_uid, state_root);
self.trie_viewer.view_state(&state_update, account_id, prefix)
self.trie_viewer.view_state(&state_update, account_id, prefix, include_proof)
}
}

Expand Down
1 change: 1 addition & 0 deletions runtime/runtime/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,5 +64,6 @@ pub trait ViewRuntimeAdapter {
state_root: MerkleHash,
account_id: &AccountId,
prefix: &[u8],
include_proof: bool,
) -> Result<ViewStateResult, crate::state_viewer::errors::ViewStateError>;
}
3 changes: 2 additions & 1 deletion runtime/runtime/src/state_viewer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ impl TrieViewer {
state_update: &TrieUpdate,
account_id: &AccountId,
prefix: &[u8],
include_proof: bool,
) -> Result<ViewStateResult, errors::ViewStateError> {
match get_account(state_update, account_id)? {
Some(account) => {
Expand All @@ -143,7 +144,7 @@ impl TrieViewer {
let query = trie_key_parsers::get_raw_prefix_for_contract_data(account_id, prefix);
let acc_sep_len = query.len() - prefix.len();
let mut iter = state_update.trie().iter()?;
iter.remember_visited_nodes(true);
iter.remember_visited_nodes(include_proof);
iter.seek_prefix(&query)?;
for item in &mut iter {
let (key, value) = item?;
Expand Down