migrations: finalize migration_5 (share encoding GOB -> SSZ)

[iurii-ssv]

This PR finalizes the work started in #1837 essentially "completing" migration_5

it doesn't change migration_5 in a sense that it is still equivalent and backward-compatible with it's previous version, rather it just adds some sanity-checks.

[codecov]

Codecov Report

Attention: Patch coverage is 19.33702% with 146 lines in your changes missing coverage. Please review.

Project coverage is 47.7%. Comparing base (3e76188) to head (7bdae58).
Report is 9 commits behind head on stage.

Files with missing lines	Patch %	Lines
migrations/migration_5_share_gob_to_ssz.go	0.0%	129 Missing ⚠️
registry/storage/shares_encoding.go	33.3%	8 Missing ⚠️
migrations/migration_5_gob.go	0.0%	6 Missing ⚠️
registry/storage/shares.go	91.3%	2 Missing ⚠️
operator/storage/storage.go	90.9%	0 Missing and 1 partial ⚠️

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iurii-ssv]

I would like to rename Spec to Domain for FromSpecShare and ToSpecShare since we are not exactly converting to/from spectypes.Share (but rather to our own Domain representation of share which is types.SSVShare), wdyt ?

[oleg-ssvlabs]

I agree. Spec share is just type embedded into the domain Share

[iurii-ssv]

Applied in fdeb284

[y0sher]

I think it makes sense to use SSVShare since that's the current name of the struct. It's not the prettiest but it's better than having two names.

[iurii-ssv]

as per @moshe-blox recommendation I found litter to be super useful for debugging, what do you think of using it like that (for printing detailed error message) ?

Alternatively we could use logger to print the contents of shareGOB and shareSSZ, but since we also have to return error that means we essentially have to duplicate error message (and it will be logged twice)

[oleg-ssvlabs]

It seems to have pretty good score, although, would it be enough to just use the logger? I assume the challenge is that zap logger does not log pointer objects with its full properties if to use something like zap.Any(shareGOB) ?

[iurii-ssv]

I assume the challenge is that zap logger does not log pointer objects with its full properties if to use something like zap.Any(shareGOB) ?

I think zap logger should produce full object printout with zap.Any (although it will probably struggle if struct contains interfaces - #1981),

the main reason I don't want to use logger here is that we'll essentially be logging this GOB share doesn't match corresponding SSZ share, GOB: ... error twice:

• here like you suggest
• and the caller will also log error we return here (and we have to return error to signal migration failed)

and only one of these logged lines will contain full contents of shareGOB and shareSSZ printed out (which is even more confusing - could look like 2 separate/different errors unless you look super carefully)

[oleg-ssvlabs]

I see.
I’m just slightly hesitant about adding a new dependency and increasing the binary size, especially since it’s only used in one place within the DB migration, which clients would run just once. Although, if @moshe-blox thinks it's OK, then I do not mind

[oleg-ssvlabs]

lgtm. Great work!