Add ChildTriePrefixIterator and methods

[KiChjang]

Fixes #8462.

This is my first substrate PR and I'm not sure if I've implemented everything correctly, reviewers please let me know of any changes I need to make. Thanks!

[KiChjang]

Pinging @thiolliere for review.

[gui1117]

needs some test but looks good

[shawntabrizi]

@KiChjang thanks for opening this! It would be great to get this in ASAP since I want to use it on Polkadot ;)

[KiChjang]

@thiolliere I'll need some help in trying to setup the test infrastructure to test my changes. I'm not even sure how I could begin testing this. Could you please provide some pointers?

[gui1117]

you can take a look at this test:

substrate/frame/support/src/storage/mod.rs

Lines 695 to 767 in f3b3e62

	#[test]
	fn prefixed_map_works() {
	TestExternalities::default().execute_with(|| {
	struct MyStorage;
	impl StoragePrefixedMap<u64> for MyStorage {
	fn module_prefix() -> &'static [u8] {
	b"MyModule"
	}
	fn storage_prefix() -> &'static [u8] {
	b"MyStorage"
	}
	}
	let key_before = {
	let mut k = MyStorage::final_prefix();
	let last = k.iter_mut().last().unwrap();
	*last = last.checked_sub(1).unwrap();
	k
	};
	let key_after = {
	let mut k = MyStorage::final_prefix();
	let last = k.iter_mut().last().unwrap();
	*last = last.checked_add(1).unwrap();
	k
	};
	unhashed::put(&key_before[..], &32u64);
	unhashed::put(&key_after[..], &33u64);
	let k = [twox_128(b"MyModule"), twox_128(b"MyStorage")].concat();
	assert_eq!(MyStorage::final_prefix().to_vec(), k);
	// test iteration
	assert!(MyStorage::iter_values().collect::<Vec<_>>().is_empty());
	unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u64);
	unhashed::put(&[&k[..], &vec![1, 1][..]].concat(), &2u64);
	unhashed::put(&[&k[..], &vec![8][..]].concat(), &3u64);
	unhashed::put(&[&k[..], &vec![10][..]].concat(), &4u64);
	assert_eq!(MyStorage::iter_values().collect::<Vec<_>>(), vec![1, 2, 3, 4]);
	// test removal
	MyStorage::remove_all();
	assert!(MyStorage::iter_values().collect::<Vec<_>>().is_empty());
	// test migration
	unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u32);
	unhashed::put(&[&k[..], &vec![8][..]].concat(), &2u32);
	assert!(MyStorage::iter_values().collect::<Vec<_>>().is_empty());
	MyStorage::translate_values(|v: u32| Some(v as u64));
	assert_eq!(MyStorage::iter_values().collect::<Vec<_>>(), vec![1, 2]);
	MyStorage::remove_all();
	// test migration 2
	unhashed::put(&[&k[..], &vec![1][..]].concat(), &1u128);
	unhashed::put(&[&k[..], &vec![1, 1][..]].concat(), &2u64);
	unhashed::put(&[&k[..], &vec![8][..]].concat(), &3u128);
	unhashed::put(&[&k[..], &vec![10][..]].concat(), &4u32);
	// (contains some value that successfully decoded to u64)
	assert_eq!(MyStorage::iter_values().collect::<Vec<_>>(), vec![1, 2, 3]);
	MyStorage::translate_values(|v: u128| Some(v as u64));
	assert_eq!(MyStorage::iter_values().collect::<Vec<_>>(), vec![1, 2, 3]);
	MyStorage::remove_all();
	// test that other values are not modified.
	assert_eq!(unhashed::get(&key_before[..]), Some(32u64));
	assert_eq!(unhashed::get(&key_after[..]), Some(33u64));
	});
	}

And instead of StoragePrefixedMap you use the ChildTriePrefixIterator
and instead of putting key/values in the main trie with unhashed you should call https://substrate.dev/rustdocs/v3.0.0/frame_support/storage/child/fn.put.html

[gui1117]

sorry to go back and forth about the API, I don't have a clear idea how to design it, I hope my suggestion make sense to you.

The ChildTriePrefixIterator should ideally be able to handle all usecase, thus being able to be created for an usage like iter_prefix_starting_at_key(prefix, key) in this latter case, the inner field make more sense to be renamed fetch_previous_key. and we don't need fetch_prefix method because it is the expected behavior.

What do you think ?

[KiChjang]

@thiolliere Honestly, I don't have much of an opinion because I'm not even sure how a child trie is accessed. Which also makes me a bit confused about how to write the tests, because e.g. child::put asks for a ChildInfo, and I've no idea how I can get one.

[gui1117]

API looks very good to me now.

to create a ChildInfo in tests you can use https://crates.parity.io/frame_support/storage/child/enum.ChildInfo.html#method.new_default

For the test we need to test that within a child trie (i.e. for one specific child info):

• the childtrieprefixiterator::with_prefix doesn't fetch other key those starting with prefix
• the childtrieprefixiterator::with_prefix drain correctly remove keys
• the childtrieprefixiterator::with_prefix correctly fetch the key at prefix when there is one.
• the childtrieprefixiterator::with_prefix_over_key correctly ignore keys that he can't decode
• the childtrieprefixiterator::with_prefix_over_key correctly decode keys he can decode

Those are basic tests to double check the implementation

[gui1117]

I pushed a test for the child trie stuff.

I think a similar test should be written for the newly introduced function storage_iter and storage_iter_with_suffix
Or we can remove them and not deprecate the other structure.
(My initial thought was to use PrefixIterator inside StorageIterator, in order not to break API, usually I prefer not to deprecate stuff without good reason, anyway here both are fine to me)

[gui1117]

looks good to me

master needs to be merged to make CI happy

[shawntabrizi]

@KiChjang can you respond with your Polkadot or Kusama address?

Thanks!

[shawntabrizi]

@KiChjang also try to avoid force pushing. git merge master should hopefully have been enough here :)

[shawntabrizi]

bot merge

[ghost]

Trying merge.

[KiChjang]

Sure! My Polkadot address is 14gKczWKAcEB8n6P9FeGttQhnTWvxFvxZr7RN4K63tKCfggR

…

On Thu, Apr 1, 2021, 7:21 AM parity-processbot[bot] < ***@***.***> wrote: Merged #8478 <#8478> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8478 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYZDS5MNK2WRDQX6DTJ3VDTGR6MXANCNFSM4Z6XUAIA> .

[shawntabrizi]

Thanks @KiChjang . I have opened a tip for you on the network.

[KiChjang]

Thanks @KiChjang . I have opened a tip for you on the network.

Dumb question: how does that work? Do I need to claim it somehow?

[shawntabrizi]

@KiChjang no. the DOTs will appear in your account once the council is finished voting for the tip. You can keep track on the Polkadot JS UI:

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

[KiChjang]

Got it, thanks!