Improvements to the networking storage read requests

[tomaka]

Reopening #5

cc @cheme

Compared to #5, I've added a new onlyKeysAfterIgnoreLastNibble field.

[tomaka]

One thing I'm not sure about is:
What happens if you query :child_storage:default:foo with includeDescendants equal to true? Should the response contain the child trie entries? I would say no, and the response should only contain a node with the root hash of the child trie.

[cheme]

Totally agree about not including child trie descendant under prefix, query stay at a single trie level this way.

Actually this makes me think again that the query for child trie could be :

message ChildTrieInfo {
       enum ChildTrieNamespace {
               DEFAULT = 1;
       }

       required bytes root = 1;
       required bytes hash = 2;
       required ChildTrieNamespace namespace = 3;
}

Which let us directly get proof from root and not include the top trie proof content in the reply: the request and reply are strongly bounded to the child trie state only.
In consequence content of the proof will only be from a single trie, which makes things way simplier.
(I kept the hash field for technical reason)

[Noc2]

Feel free to ping me once I should merge it ;-)

[cheme]

I am currently implementing it, will probably have a few other question.

One that I just see: should we allow stopping between a node and a value node, in this case the restart at a node key/prefix will not work since node and value share the same location.
The direction I plan to take is to just not allow it (check size on node only, not on value node (value node size will be part of its parent node obviously).

[tomaka]

Yeah I would also not allow it.

In an ideal world the values would be Merkle-ized, and you could resume within the value as well, but we're clearly not there.

[cheme]

I did a mvp of this in the trie crate (only trie level).

Basically what I did is follow the query at node level (either following a value or iterating).
Use some simple limit (either number of node or total size of encoded nodes), and whenever the limit is reached I exit.

More precisely the exit is only done at the next hash of node I try to access from a branch (if there is hash of a value, the value is fetched and added to the proof).
Then to restart the information is the node key to the content pointed by this hash (the node key of the last recorded node ++ the last recorded node partial key ++ the child index of the next existing and non recorded children).
This means that when we restart we always are in a state where next operation is to descend in a children.

@tomaka writing this I understand better why you mention that the lower bound limit is actually inclusive, as it is the node key of the next non value node queried from a branch.
So I propose to remove the equal in the PPP.

[cheme]

An idea here would be to allow:
optional bytes usingRoot = 5;
This will be the state root, and could allow server to skip all block hash/number checks.

[tomaka]

That seems like an implementation detail to me.
An implementation needs to find the trie/storage that is requested in its database. If it uses the trie root hash as its database index, then it's indeed faster to provide this trie root hash, but that's really not something the networking should be aware of.

This would also raise the question: what happens if the request is invalid?
If an implementation uses the block hash as its database index, then what should it do if the provided root hash doesn't match?

It means that some implementations would accept the request and some others would reject the request, unless you make it mandatory for all implementations to check that the root hash matches the block hash, which would kill the point of providing this root hash.

[cheme]

If an implementation uses the block hash as its database index, then what should it do if the provided root hash doesn't match?

If root hash is present the block hash is not checked, same for header.
In fact it should be make mandatory that block hash / number are not checked when this hash is provided and the client support using it.
I got this idea as a way to restore processing (since it is a network protocol we can be a bit technical).
The issue is that the implementer should not be force to implement it and could refer to block number/hash if unsupported.
Probably a cleaner design would be to pass it in place of the block hash/number (like an enum) and return unsupported if client do not implement or allow direct state access from root. (I initially put it next to OnlyKeys as the intention is also to resume a query).

[tomaka]

I'm very much in favor of not doing this, in the name of simplicity/purity. This really feels like introducing a wart in the design just to save a few hundred microseconds.

[cheme]

Depends on the implementation, and for child trie it skip a full top state query, but yeah caching can solve this in a pretty non deterministic way in client side.

[cheme]

An alternative would be to just use a state root instead of the block hash/number (since we are at network protocol level).

[cheme]

An alternative would be to just use a state root instead of the block hash/number (since we are at network protocol level).

Well actually I cannot implement it due to internals with rocksdb, maybe as a V3 :(

[cheme]

For info, I manage to implement this with Compact Proof encoding (the one we use in pov).
It was way more involve than I expected. Drawback are that you can only check state at the end of each reply. Similarily running with size limit low will means that we need to include more hashes (so less space saved).
Also when producing proof, the nodes cannot be streamed (ordering does not allow it and all proof must stay in memory), but with current protobuf schema it does not matter.
Considering the implementation effort I am not sure it is worth it. Probably rather start with the full encoded node design and maybe later switch to something like w3f/PSPs#56 (got also the drawback that we can only invalidate reply after reading all nodes but should not need to put everything in memory when recording).

[tomaka]

I'm not familiar enough with your PSP to comment on this, but I agree that compact proofs aren't really interacting well with this proposal and IMO it makes more sense to stick to non-compact proofs.

[Noc2]

We will archive this repository in favor of the new RFCs repository by the Fellowship. So, I'm closing the issue here. I recommend opening a PR at the new repository.