Fix distance evaluation client

• Fix owner key retrieval in distance client
  • I could not find how to decode the StorageMap cleanly so I used byte ranges with the help of this article. I'm not sure it can't break in the future.
• Fix modulo in evaluation result filename in distance oracle
• Fix evaluation result file deletion
  • If you want to name the files using pool indices instead of a strictly increasing sequence, you run into problems (this is the reason why it was not the case, btw). It should work now, but if the node can't publish (i.e. crash or network error for this block) after the file is deleted, the evaluation result is lost.

Fixes #261 (closed)

changed milestone to %client-0.9.0

added C-binary RN-binary bug labels

marked this merge request as ready

requested review from @HugoTrentesaux

There is a conflict between this MR which addresses #261 (closed) and !290 (merged) which addresses #262 (closed). I'll comment only in-scope modifications (key retrieval), because others belong to the use of pool index as filename.

Here you are doing this:

1. iterate over this storage map which has as many entries as validators (like 20-50):

/// The owner of a key. The key is the `KeyTypeId` + the encoded key.
#[pallet::storage]
pub type KeyOwner<T: Config> =
    StorageMap<_, Twox64Concat, (KeyTypeId, Vec<u8>), T::ValidatorId, OptionQuery>;

2. decode all raw data to sr25519::Public and AccountId types
3. for each of these, see if local owner_keys contain this public key

It seems to me that we should go the other way around:

1. iterate over all local keys (like 1-2 depending on how many times rotateKeys() was called)
2. for each of this key, query the storage map at session.keyOwner(babe, <key>)
3. if it is None, this key is not currently in use, if it is Some, we can stop because a node is not supposed to have multiple active owner keys locally

This way we limit the number of entries we have to iterate on and we do not have to decode manually the storage map.

Indeed I could transpose and decode fewer keys but I don't know how to get a nice typed API for the storage from the client. The getter session.key_owner seems accessible from the runtime only. I could not find a way to decode the StorageMap directly, as it's not a proper structure but a key prefix.

Edit: If we want to write pallet_session::Pallet::<T>::key_owner(sp_runtime::KeyTypeId(*b"babe"), key) we need T to impl Config.

What do you mean by "accessible from the runtime only"? In this post I showed a screenshot of the call in polkadotjs, so it should be available from outside, as any storage item.

And since the key of the StorageMap is (KeyTypeId, Vec<u8>), there is no "nice typed API". The map simply expects a Vec<u8> prefixed by babe. The return value is an AccountId and can simply be constructed with sp_runtime::AccountId32 as you did.

Not sure about the implementation details in this context. I would imagine that runtime apis would also be available client-side, but apparently not ?

Yes, subxt would give us a better API but that's not an option here. (this code is called by the smith node when building a block)

By "nice typed API" I meant a getter like we have in the runtime. Outside, we have to decode since we don't have Config.

Even the item's storage key, we would have to compute it ourselves (concatenate the item key's hash to the output of storage_prefix).

I could not find any helpful example of requesting the storage from the client in substrate's code. There probably are, but I don't know what keyword to search for.

I guess the name is "state call" https://paritytech.github.io/polkadot-sdk/master/sc_rpc/state/trait.StateBackend.html#tymethod.call

changed this line in version 2 of the diff

Finally I computed the storage key. I find it acceptably clean, given we have no other choice than encoding something manually anyway.

State call still needs encoding the call to bytes, and it seems overkill if we just want to fetch a value from the storage. Seems it's not much used.

This is tricky, current_pool_index is already % 3.

u32 is not Z/3Z, if current_pool_index == 2 we want 0 not 3. Unless I've missed something?

Why want 0 when having 2? The filename can simply correspond to current_pool_index, no need to offset it by one. We can chose whatever we want for the filename, but I guess the less operations we apply on it, the simpler it is.

In the runtime CurrentPoolIndex is set modulo 3:

let index = (CurrentPoolIndex::<T>::get() + 1) % 3;
CurrentPoolIndex::<T>::put(index);

and the distance client uses directly this value for the filename. We just need to have the same number in the client and the oracle.

added 1 commit

• d708eb56 - Avoid decoding storage by hand

Compare with previous version

added 3 commits

• 38111644 - 1 commit from branch master
• 4f0f3825 - Fix distance evaluation client
• 407b1d15 - Avoid decoding storage by hand

Compare with previous version

approved this merge request

Le pipeline n'a pas l'air de vouloir se lancer...

merged

mentioned in commit f73a957d