updater: fix static id hashes initialization

[ordian]

Might help with #10085 (comment).

The bug was introduced in #10670.
Previously we used to pad short bytes with zeros when converting them to H256:
https://github.com/paritytech/parity-common/blob/e16441a8421d2e39a2e91a40f30a6b01cbaee00e/fixed-hash/src/hash.rs#L105-L111
But in the latest version of fixed-hash there is a length check and a panic.

I'm not really sure if there are other places in our codebase where we rely on the hash string being resized automatically when converting to H256.

[ngotchac]

Can't H256::from_slice only accepts 32-lenth slices?
Otherwise, LGTM. I couldn't find any other places where this bug could occur

[ordian]

Can't H256::from_slice only accepts 32-lenth slices?

It currently does and that is the problem, since CLIEND_ID ("parity") and PLATFORM are less then 32 bytes.

[ngotchac]

Yeah, I meant as the argument type? So that we could catch this at compile time.

[ordian]

Can't H256::from_slice only accepts 32-lenth slices?

Ah, you mean &[u8; 32]? That would probably make it harder to use this API when we generate a Vec from a hex string or use a subslice of larger type like data[..32].

[ngotchac]

Yeah sure, but we could also have a from_unknown_slice method or so for such cases that would pad it as it used to do

[ordian]

Yeah sure, but we could also have a from_unknown_slice method or so for such cases that would pad it as it used to do

Fair enough, but you can create it from &[u8; 32] slice already by H256(*slice) :P
Maybe this is what we should use instead though.

[ngotchac]

Yeah sure.. But what I meant is that it would be better to not have a possible panic as we do now :p Maybe renaming from_slice to from_unsafe_slice then :p

[ordian]

I get what you're trying to say, but it would also require changing conversions to uint for consistency, which would in turn imply a bunch of breaking changes and new releases, which I'm not sure is justified atm :p

[dvdplm]

The change to panic with the wrong size is pretty new and before it was resizing the slice to the proper size, so old code might expect that behaviour. I agree with the change, but runtime panics like this are bad. Presumably any new code written will have tests in place to catch the panics but that's far from satisfying. The name from_slice is not helping here, it looks pretty innocent and "safe" (and I'm the first to admit I rarely read the rust docs for our own code where this is indeed mentioned).

We can:

1. rename it to something "dangerous", like from_slice_but_watchout_it_panics()
2. make it safe on the type level, like @ngotchac suggests
3. revert from_slice to behave like before, silently taking care of padding/truncating

Personally I'm tending to 3) tbh, but I'd like to hear more voices.

[ngotchac]

But why did we change the behavior in the first place?

[dvdplm]

But why did we change the behavior in the first place?

paritytech/parity-common@ef4a8d8

EDIT: and before that it was paritytech/parity-common@9dec8ff#diff-058ba728d97e352f0db6a91ab02dc64bR86

[ngotchac]

Yeah, I would vote for having from_fixed_bytes and from_bytes_padding/from_padded_bytes or something similar, so we'll have to update each references and be sure that it's correctly used.