keystore.has_keys behaviour should return the indicies found

[rakanalh]

Follow up to the comment here: paritytech/polkadot#1740 (comment)

The behaviour of the has_keys would be slow in cases such as:

https://github.com/paritytech/polkadot/blob/9ce9eccae4faa7935bbca9a4200e0978168a46f9/node/network/availability-distribution/src/lib.rs#L582-L586

where the list of keys is iterated so that the first key found is returned. However, in case of a remote signer, this loop would be quite slow.

has_keys behaviour should be modified so that the index of the first element found is returned so that this index can be used to return the public key.

[hirschenberger]

I'd like to work on this, how about these signatures?

async fn has_keys(&self, public_keys: &[(Vec<u8>, KeyTypeId)]) -> Option<usize>;
fn has_keys(&self, public_keys: &[(Vec<u8>, KeyTypeId)]) -> Option<usize>;

[bkchr]

@hirschenberger I think we actually should return a Vec<usize> which contains all the indices of all the keys we have in the store. Otherwise it looks okay.

[hirschenberger]

@bkchr I quickly scanned through the code and most of the time, this function is used in a boolean context, which would also be intuitive when returning an Option.

Maybe the design would be more clear if we add another function which returns an index-vec for that special usecase?

EDIT:

Hmm, although has_keys(...).is_empty() is also pretty clear.

[bkchr]

Hmm, although has_keys(...).is_empty() is also pretty clear.

Sounds good to me.

[hirschenberger]

Sorry I don't want to overcomplicate stuff, but having a function called has_xxx that does not return a bool-like type feels wrong.
Also the logic seems unintuitive as it has to be negated to get the old behaviour and is contrary to the has, this may lead to errors.

// what is really checked here?
if !keystore.has_keys(....).is_empty() { ...

I'd suggest either renaming the function to keys or wrap the returned Vec into an Option?

I also realized, that the current implementation is checking if all of the given public_keys are in the store. When we implement it in one of the above ways, the behaviour changes to if at least one of the keys is in the store.

[bkchr]

There is already a method keys.

Also the logic seems unintuitive as it has to be negated to get the old behaviour and is contrary to the has, this may lead to errors.

As we change the signature, it is not really that easy to mix this up on updating.

However, I'm open to a better name than has_keys.

[bkchr]

But wrapping Vec<> in an Option doesn't make any sense IMHO.

[hirschenberger]

Ok, maybe key_idxs or key_indices? In the stdlib there's std::str::char_indices().

How about the changed behaviour, not checking if all keys are present, do you think that may cause trouble?

[bkchr]

We check all, if the user wants to know if all are found they can check res.len() == requested.len().

Ok, maybe key_idxs or key_indices? In the stdlib there's std::str::char_indices().

This given function is not comparable to what we are doing here and key_indices also is wrong. We return the indices of input keys that match and not an index per key or similar.

[hirschenberger]

matching_key_indices ?

Idk, new to substrate and not so familiar with naming.

To be honest, the returning of indices works but also seems a little bit C-ish. IMHO, a cleaner solution would be to filter the input Vec and return a new Vec with matching keys.

It's your decision.