Substrate: Is storage value of `Option<()>` supported?

[xlc]

We have few storage value with double map X, Y => Option<()> type to simulate an unordered set.

However I found out the .iter(key1) is not able to iterate all the values for key1.
Using getStorage RPC is able to read that storage did exists. getKeys is not able to iterate as the key wasn't exists.

Seems like the trie db is not able to distinguish between a non-exist key and an exist key with no value in some cases?

Change the type to Option<bool> works around the issue for me.

Example code:

https://github.com/laminar-protocol/laminar-chain/blob/f53fb6044ffa431993e4467f320f93af1b042ce2/modules/margin-protocol/src/lib.rs#L79

https://github.com/laminar-protocol/laminar-chain/blob/f53fb6044ffa431993e4467f320f93af1b042ce2/modules/margin-protocol/src/lib.rs#L667

This is not able to iterate.

The interesting thing is it works in unit test so we wasn't able to catch this issue before. Maybe something to do with low level db? The in memory db is able to handle but the real rocks db is not?

[kianenigma]

cc @thiolliere.

[JoshOrndorff]

This is not an answer to your question, but a possible workaround. If you want to use Option<()>, which has two variants, Some(()) and None, you might get away with using bool.

true -> Some(())
false -> None

[thiolliere]

as far as I can understand TrieDbMut doesn't support inserting empty value https://github.com/paritytech/trie/blob/master/trie-db/src/triedbmut.rs#L1537

If I'm correct then I don't know why
@cheme @arkpar

Also I suspect getStorage RPC works because the key is found in the cache, but this is just a guess.

[thiolliere]

one can easily reproduce by modifying existing test: (the test will fail)

diff --git a/primitives/state-machine/src/trie_backend_essence.rs b/primitives/state-machine/src/trie_backend_essence.rs
index 125a823f5..9ff017d8e 100644
--- a/primitives/state-machine/src/trie_backend_essence.rs
+++ b/primitives/state-machine/src/trie_backend_essence.rs
@@ -445,18 +445,18 @@ mod test {
                let mut mdb = PrefixedMemoryDB::<Blake2Hasher>::default();
                {
                        let mut trie = TrieDBMut::new(&mut mdb, &mut root_1);
-                       trie.insert(b"3", &[1]).expect("insert failed");
-                       trie.insert(b"4", &[1]).expect("insert failed");
-                       trie.insert(b"6", &[1]).expect("insert failed");
+                       trie.insert(b"3", &[]).expect("insert failed");
+                       trie.insert(b"4", &[]).expect("insert failed");
+                       trie.insert(b"6", &[]).expect("insert failed");
                }
                {
                        let mut mdb = KeySpacedDBMut::new(&mut mdb, child_info.keyspace());
                        // reuse of root_1 implicitly assert child trie root is same
                        // as top trie (contents must remain the same).
                        let mut trie = TrieDBMut::new(&mut mdb, &mut root_1);
-                       trie.insert(b"3", &[1]).expect("insert failed");
-                       trie.insert(b"4", &[1]).expect("insert failed");
-                       trie.insert(b"6", &[1]).expect("insert failed");
+                       trie.insert(b"3", &[]).expect("insert failed");
+                       trie.insert(b"4", &[]).expect("insert failed");
+                       trie.insert(b"6", &[]).expect("insert failed");
                }
                {
                        let mut trie = TrieDBMut::new(&mut mdb, &mut root_2);

[arkpar]

Ethereum legacy. In EVM there's no distinction between a zeroed and non-existent storage slot. I guess for substrate we could lift that restriction. @gavofyork What do you think?

[bkchr]

@arkpar should be fixed AFAIK?

[arkpar]

I'd expect so. Just need to be careful maintaining compatibility with existing networks here.

[xlc]

@bkchr Is this fixed?

[bkchr]

I'd expect so. Just need to be careful maintaining compatibility with existing networks here.

[arkpar]

Definitely not fixed.

I'd expect so. Just need to be careful maintaining compatibility with existing networks here.

This was answering the question if it should be fixed

[bkchr]

Okay ;) I meant this as a question if this "should already be fixed".

[gavofyork]

I’m not sure it really should be fixed.

[thiolliere]

then maybe we should

• make const assert in decl_storage to prevent using type of size 0. (hmm not true, a zero size type can encode to [1])
• add in the documentation in sp-io that zero size are not handled:
  Because if somebody insert a zero size value and then get it it can have different result depending on some cache. (overlay changes consider a zero size value to be Some(value)), and some other cache also.
• change client so that setting an empty slice value actually removes it.

EDIT: After discussion we will fix it. Storage will support adding empty value