Do not return empty entries from state_queryStorage (#2906)

* do not return empty entries from state_queryStorage

* revert back None -> null change

* Revert "revert back None -> null change"

This reverts commit 318eb043e03b7aabbc3f176e02c15fd4595d16db.

substrate/core/client/src/client.rs

@@ -525,6 +525,8 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 
 	/// Get pairs of (block, extrinsic) where key has been changed at given blocks range.
 	/// Works only for runtimes that are supporting changes tries.
+	///
+	/// Changes are returned in descending order (i.e. last block comes first).
 	pub fn key_changes(
 		&self,
 		first: NumberFor<Block>,

substrate/core/rpc/src/state/mod.rs

@@ -258,9 +258,9 @@ impl<B, E, Block: BlockT, RA> State<B, E, Block, RA> where
 		&self,
 		range: &QueryStorageRange<Block>,
 		keys: &[StorageKey],
+		last_values: &mut HashMap<StorageKey, Option<StorageData>>,
 		changes: &mut Vec<StorageChangeSet<Block::Hash>>,
 	) -> Result<()> {
-		let mut last_state: HashMap<_, Option<_>> = Default::default();
 		for block in range.unfiltered_range.start..range.unfiltered_range.end {
 			let block_hash = range.hashes[block].clone();
 			let mut block_changes = StorageChangeSet { block: block_hash.clone(), changes: Vec::new() };
@@ -268,15 +268,19 @@ impl<B, E, Block: BlockT, RA> State<B, E, Block, RA> where
 			for key in keys {
 				let (has_changed, data) = {
 					let curr_data = self.client.storage(&id, key)?;
-					let prev_data = last_state.get(key).and_then(|x| x.as_ref());
-					(curr_data.as_ref() != prev_data, curr_data)
+					match last_values.get(key) {
+						Some(prev_data) => (curr_data != *prev_data, curr_data),
+						None => (true, curr_data),
+					}
 				};
 				if has_changed {
 					block_changes.changes.push((key.clone(), data.clone()));
 				}
-				last_state.insert(key.clone(), data);
+				last_values.insert(key.clone(), data);
+			}
+			if !block_changes.changes.is_empty() {
+				changes.push(block_changes);
 			}
-			changes.push(block_changes);
 		}
 		Ok(())
 	}
@@ -286,6 +290,7 @@ impl<B, E, Block: BlockT, RA> State<B, E, Block, RA> where
 		&self,
 		range: &QueryStorageRange<Block>,
 		keys: &[StorageKey],
+		last_values: &HashMap<StorageKey, Option<StorageData>>,
 		changes: &mut Vec<StorageChangeSet<Block::Hash>>,
 	) -> Result<()> {
 		let (begin, end) = match range.filtered_range {
@@ -298,17 +303,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();
+			for (block, _) in self.client.key_changes(begin, end, key)?.into_iter().rev() {
 				if last_block == Some(block) {
 					continue;
 				}
+
 				let block_hash = range.hashes[(block - range.first_number).saturated_into::<usize>()].clone();
 				let id = BlockId::Hash(block_hash);
 				let value_at_block = self.client.storage(&id, key)?;
+				if last_value == value_at_block {
+					continue;
+				}
+
 				changes_map.entry(block)
 					.or_insert_with(|| StorageChangeSet { block: block_hash, changes: Vec::new() })
-					.changes.push((key.clone(), value_at_block));
+					.changes.push((key.clone(), value_at_block.clone()));
 				last_block = Some(block);
+				last_value = value_at_block;
 			}
 		}
 		if let Some(additional_capacity) = changes_map.len().checked_sub(changes.len()) {
@@ -432,8 +444,9 @@ impl<B, E, Block, RA> StateApi<Block::Hash> for State<B, E, Block, RA> where
 	) -> Result<Vec<StorageChangeSet<Block::Hash>>> {
 		let range = self.split_query_storage_range(from, to)?;
 		let mut changes = Vec::new();
-		self.query_storage_unfiltered(&range, &keys, &mut changes)?;
-		self.query_storage_filtered(&range, &keys, &mut changes)?;
+		let mut last_values = HashMap::new();
+		self.query_storage_unfiltered(&range, &keys, &mut last_values, &mut changes)?;
+		self.query_storage_filtered(&range, &keys, &last_values, &mut changes)?;
 		Ok(changes)
 	}
 

substrate/core/rpc/src/state/tests.rs

@@ -167,12 +167,16 @@ fn should_query_storage() {
 
 		let add_block = |nonce| {
 			let mut builder = client.new_block(Default::default()).unwrap();
-			builder.push_transfer(runtime::Transfer {
-				from: AccountKeyring::Alice.into(),
-				to: AccountKeyring::Ferdie.into(),
-				amount: 42,
-				nonce,
-			}).unwrap();
+			// fake change: None -> None -> None
+			builder.push_storage_change(vec![1], None).unwrap();
+			// fake change: None -> Some(value) -> Some(value)
+			builder.push_storage_change(vec![2], Some(vec![2])).unwrap();
+			// actual change: None -> Some(value) -> None
+			builder.push_storage_change(vec![3], if nonce == 0 { Some(vec![3]) } else { None }).unwrap();
+			// actual change: None -> Some(value)
+			builder.push_storage_change(vec![4], if nonce == 0 { None } else { Some(vec![4]) }).unwrap();
+			// actual change: Some(value1) -> Some(value2)
+			builder.push_storage_change(vec![5], Some(vec![nonce as u8])).unwrap();
 			let block = builder.bake().unwrap();
 			let hash = block.header.hash();
 			client.import(BlockOrigin::Own, block).unwrap();
@@ -182,32 +186,31 @@ fn should_query_storage() {
 		let block2_hash = add_block(1);
 		let genesis_hash = client.genesis_hash();
 
-		let alice_balance_key = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into()));
-
 		let mut expected = vec![
 			StorageChangeSet {
 				block: genesis_hash,
 				changes: vec![
-					(
-						StorageKey(alice_balance_key.to_vec()),
-						Some(StorageData(vec![232, 3, 0, 0, 0, 0, 0, 0]))
-					),
+					(StorageKey(vec![1]), None),
+					(StorageKey(vec![2]), None),
+					(StorageKey(vec![3]), None),
+					(StorageKey(vec![4]), None),
+					(StorageKey(vec![5]), None),
 				],
 			},
 			StorageChangeSet {
 				block: block1_hash,
 				changes: vec![
-					(
-						StorageKey(alice_balance_key.to_vec()),
-						Some(StorageData(vec![190, 3, 0, 0, 0, 0, 0, 0]))
-					),
+					(StorageKey(vec![2]), Some(StorageData(vec![2]))),
+					(StorageKey(vec![3]), Some(StorageData(vec![3]))),
+					(StorageKey(vec![5]), Some(StorageData(vec![0]))),
 				],
 			},
 		];
 
 		// Query changes only up to block1
+		let keys = (1..6).map(|k| StorageKey(vec![k])).collect::<Vec<_>>();
 		let result = api.query_storage(
-			vec![StorageKey(alice_balance_key.to_vec())],
+			keys.clone(),
 			genesis_hash,
 			Some(block1_hash).into(),
 		);
@@ -216,7 +219,7 @@ fn should_query_storage() {
 
 		// Query all changes
 		let result = api.query_storage(
-			vec![StorageKey(alice_balance_key.to_vec())],
+			keys.clone(),
 			genesis_hash,
 			None.into(),
 		);
@@ -224,10 +227,9 @@ fn should_query_storage() {
 		expected.push(StorageChangeSet {
 			block: block2_hash,
 			changes: vec![
-				(
-					StorageKey(alice_balance_key.to_vec()),
-					Some(StorageData(vec![148, 3, 0, 0, 0, 0, 0, 0]))
-				),
+				(StorageKey(vec![3]), None),
+				(StorageKey(vec![4]), Some(StorageData(vec![4]))),
+				(StorageKey(vec![5]), Some(StorageData(vec![1]))),
 			],
 		});
 		assert_eq!(result.unwrap(), expected);

substrate/core/test-runtime/client/src/block_builder_ext.rs

@@ -25,6 +25,8 @@ use generic_test_client::client::block_builder::api::BlockBuilder;
 pub trait BlockBuilderExt {
 	/// Add transfer extrinsic to the block.
 	fn push_transfer(&mut self, transfer: runtime::Transfer) -> Result<(), client::error::Error>;
+	/// Add storage change extrinsic to the block.
+	fn push_storage_change(&mut self, key: Vec<u8>, value: Option<Vec<u8>>) -> Result<(), client::error::Error>;
 }
 
 impl<'a, A> BlockBuilderExt for client::block_builder::BlockBuilder<'a, runtime::Block, A> where
@@ -34,4 +36,8 @@ impl<'a, A> BlockBuilderExt for client::block_builder::BlockBuilder<'a, runtime:
 	fn push_transfer(&mut self, transfer: runtime::Transfer) -> Result<(), client::error::Error> {
 		self.push(transfer.into_signed_tx())
 	}
+
+	fn push_storage_change(&mut self, key: Vec<u8>, value: Option<Vec<u8>>) -> Result<(), client::error::Error> {
+		self.push(runtime::Extrinsic::StorageChange(key, value))
+	}
 }

substrate/core/test-runtime/src/lib.rs

@@ -106,6 +106,7 @@ pub enum Extrinsic {
 	AuthoritiesChange(Vec<AuthorityId>),
 	Transfer(Transfer, AccountSignature),
 	IncludeData(Vec<u8>),
+	StorageChange(Vec<u8>, Option<Vec<u8>>),
 }
 
 #[cfg(feature = "std")]
@@ -129,6 +130,7 @@ impl BlindCheckable for Extrinsic {
 				}
 			},
 			Extrinsic::IncludeData(_) => Err(runtime_primitives::BAD_SIGNATURE),
+			Extrinsic::StorageChange(key, value) => Ok(Extrinsic::StorageChange(key, value)),
 		}
 	}
 }

substrate/core/test-runtime/src/system.rs

@@ -238,6 +238,7 @@ fn execute_transaction_backend(utx: &Extrinsic) -> ApplyResult {
 		Extrinsic::Transfer(ref transfer, _) => execute_transfer_backend(transfer),
 		Extrinsic::AuthoritiesChange(ref new_auth) => execute_new_authorities_backend(new_auth),
 		Extrinsic::IncludeData(_) => Ok(ApplyOutcome::Success),
+		Extrinsic::StorageChange(key, value) => execute_storage_change(key, value.as_ref().map(|v| &**v)),
 	}
 }
 
@@ -273,6 +274,14 @@ fn execute_new_authorities_backend(new_authorities: &[AuthorityId]) -> ApplyResu
 	Ok(ApplyOutcome::Success)
 }
 
+fn execute_storage_change(key: &[u8], value: Option<&[u8]>) -> ApplyResult {
+	match value {
+		Some(value) => storage::unhashed::put_raw(key, value),
+		None => storage::unhashed::kill(key),
+	}
+	Ok(ApplyOutcome::Success)
+}
+
 #[cfg(feature = "std")]
 fn info_expect_equal_hash(given: &Hash, expected: &Hash) {
 	use primitives::hexdisplay::HexDisplay;