Do not return empty entries from state_queryStorage #2906
Conversation
core/rpc/src/state/mod.rs
Outdated
| last_values.insert(key.clone(), data); | ||
| } | ||
|
|
||
| // always insert entry for the first block, because we're claiming to provide values of |
There was a problem hiding this comment.
The RPC method description claims that "This first returned result contains the initial state of storage for all keys.". But actually the first entry won't have a value for the key if the value is None:
{"jsonrpc":"2.0","result":[{"block":"0x61..0c","changes":[]}],"id":1}But if later the key has been changed to Some(_) and then back to None, the changes will look like that (mind null in the last block' changes):
{"jsonrpc":"2.0","result":[{"block":"0x61..0c","changes":[]},{"block":"0x81..8d","changes":[["0x05","0x06"]]},{"block":"0x91..9e","changes":[["0x05",null]]}],"id":1}So we are using both absence-of-key and null to mark None values. What do you think - does it worth to use only null for that? If so, I could revert last commit, that itself reverts this fix.
There was a problem hiding this comment.
It might be a bit confusing but it seems fine for me. The alternative would be to print out all the keys with null value in the initial block, right?
While we could do that if user subscribes to specific keys (still a bit wasteful), it would not be feasible in case of a wildcard.
I'm fine with both, maybe let's hear @jacogr's opinion on this.
There was a problem hiding this comment.
The alternative would be to print out all the keys with null value in the initial block, right?
Exactly
There was a problem hiding this comment.
Prefer consistency, so always showing "something" - currently on the middleware side we have to handle both undefined and null - either way, as long as the order is correct, should not break anything.
There was a problem hiding this comment.
Thanks, so it'll be null everywhere (speaking of this method only, of course)
core/rpc/src/state/mod.rs
Outdated
| last_values.insert(key.clone(), data); | ||
| } | ||
|
|
||
| // always insert entry for the first block, because we're claiming to provide values of |
There was a problem hiding this comment.
It might be a bit confusing but it seems fine for me. The alternative would be to print out all the keys with null value in the initial block, right?
While we could do that if user subscribes to specific keys (still a bit wasteful), it would not be feasible in case of a wildcard.
I'm fine with both, maybe let's hear @jacogr's opinion on this.
| @@ -298,17 +304,24 @@ impl<B, E, Block: BlockT, RA> State<B, E, Block, RA> where | |||
| let mut changes_map: BTreeMap<NumberFor<Block>, StorageChangeSet<Block::Hash>> = BTreeMap::new(); | |||
| for key in keys { | |||
| let mut last_block = None; | |||
| for (block, _) in self.client.key_changes(begin, end, key)? { | |||
| let mut last_value = last_values.get(key).cloned().unwrap_or_default(); | |||
There was a problem hiding this comment.
Do we have to clone here? Can't we compare with value_at_block.as_ref()?
There was a problem hiding this comment.
It is also used as storage for the last value (it is updated in the loop). I could use the same last_values map for this, but it is single clone() call vs multiple HashMap::get() + HashMap::insert() calls. Could change that if you feel this is better.
This reverts commit 318eb04.
* do not return empty entries from state_queryStorage * revert back None -> null change * Revert "revert back None -> null change" This reverts commit 318eb04.
closes #2826
The first problem occurs when CT are disabled. The result of
state_queryStorageincludes entries for every block in the range, even though changes are empty:{"jsonrpc":"2.0","result":[{"block":"0x610c","changes":[["0x05","0x05"],["0x0a","0x0a"]]}, {"block":"0x81..8d","changes":[]}],"id":1}I've added a check for that && now entry is only included when
changesfield is not empty.The second problem is linked to CT + fake changes (see #2865 ). When there's fake change, it is shown in
changes=> redundant entry appears.{"jsonrpc":"2.0","result":[{"block":"0x61..0c","changes":[["0x05","0x05"]]}, {"block":"0x81..8d","changes":[["0x05","0x05"]]}],"id":1}I've fixed that by adding a check that
prev_value != new_value.{"jsonrpc":"2.0","result":[{"block":"0x61..0c","changes":[["0x05","0x05"]]}, {"block":"0x81..8d","changes":[["0x05","0x06"]]}],"id":1}So after this PR, every entry in resulting
Vecis guaranteed to be inserted only when requested key(s) has actually been changed at linked block.