Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Jan 26, 2025
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ require (
github.com/prysmaticlabs/go-bitfield v0.0.0-20240328144219-a1caa50c3a1e
github.com/prysmaticlabs/prysm/v4 v4.0.8
github.com/rs/zerolog v1.32.0
github.com/sanity-io/litter v1.5.6
github.com/sourcegraph/conc v0.3.0
github.com/spf13/cobra v1.8.1
github.com/ssvlabs/eth2-key-manager v1.4.2
Expand All @@ -56,8 +57,6 @@ require (
tailscale.com v1.72.0
)

require github.com/felixge/httpsnoop v1.0.4 // indirect

require (
github.com/BurntSushi/toml v1.4.1-0.20240526193622-a339e1f7089c // indirect
github.com/DataDog/zstd v1.5.2 // indirect
Expand Down Expand Up @@ -91,6 +90,7 @@ require (
github.com/ethereum/c-kzg-4844 v1.0.0 // indirect
github.com/ethereum/go-verkle v0.1.1-0.20240306133620-7d920df305f0 // indirect
github.com/fatih/color v1.17.0 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/flynn/noise v1.1.0 // indirect
github.com/francoispqt/gojay v1.2.13 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
Expand Down
5 changes: 5 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ github.com/crate-crypto/go-kzg-4844 v1.0.0/go.mod h1:1kMhvPgI0Ky3yIa+9lFySEBUBXk
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/d4l3k/messagediff v1.2.1 h1:ZcAIMYsUg0EAp9X+tt8/enBE/Q8Yd5kzPynLyKptt9U=
github.com/d4l3k/messagediff v1.2.1/go.mod h1:Oozbb1TVXFac9FtSIxHBMnBCq2qeH/2KkEQxENCrlLo=
github.com/davecgh/go-spew v0.0.0-20161028175848-04cdfd42973b/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=
Expand Down Expand Up @@ -619,6 +620,7 @@ github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pmezard/go-difflib v0.0.0-20151028094244-d8ed2627bdf0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
Expand Down Expand Up @@ -683,6 +685,8 @@ github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR
github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk=
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/sanity-io/litter v1.5.6 h1:hCFycYzhRnW4niFbbmR7QKdmds69PbVa/sNmEN5euSU=
github.com/sanity-io/litter v1.5.6/go.mod h1:9gzJgR2i4ZpjZHsKvUXIRQVk7P+yM3e+jAF7bU2UI5U=
github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0=
github.com/sergi/go-diff v1.0.0/go.mod h1:0CfEIISq7TuYL3j771MWULgwwjU+GofnZX9QAmXWZgo=
github.com/shirou/gopsutil v3.21.11+incompatible h1:+1+c1VGhc88SSonWP6foOcLhvnKlUeu/erjjvaPEYiI=
Expand Down Expand Up @@ -745,6 +749,7 @@ github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
github.com/stretchr/testify v0.0.0-20161117074351-18a02ba4a312/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
Expand Down
177 changes: 153 additions & 24 deletions migrations/migration_5_share_gob_to_ssz.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package migrations

import (
"bytes"
"context"
"fmt"
"github.com/sanity-io/litter"
opstorage "github.com/ssvlabs/ssv/operator/storage"
"github.com/ssvlabs/ssv/registry/storage"
"github.com/ssvlabs/ssv/storage/basedb"
"go.uber.org/zap"
)
Expand All @@ -12,32 +16,51 @@
// data to be targeted - so that SSV node with "fresh" DB can operate just fine.
var migration_5_change_share_format_from_gob_to_ssz = Migration{
Name: "migration_5_change_share_format_from_gob_to_ssz",
Run: func(ctx context.Context, logger *zap.Logger, opt Options, key []byte, completed CompletedFunc) error {
// storagePrefix is a base prefix we use when storing shares
var storagePrefix = []byte("operator/")
Run: func(ctx context.Context, logger *zap.Logger, opt Options, key []byte, completed CompletedFunc) (err error) {
var sharesGOBTotal int

Check warning on line 20 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L19-L20

Added lines #L19 - L20 were not covered by tests

// sets is a bunch of updates this migration will need to perform, we cannot do them all in a
// single transaction (because there is a limit on how large a single transaction can be) so
// we'll use SetMany func that will split up the data we want to update into batches committing
// each batch in a separate transaction. I guess that makes this migration non-atomic.
sets := make([]basedb.Obj, 0)
defer func() {
if err != nil {
return // cannot complete migration successfully
}

Check warning on line 25 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L22-L25

Added lines #L22 - L25 were not covered by tests
// complete migration, this makes sure migration applies only once
if err = completed(opt.Db); err != nil {
err = fmt.Errorf("complete transaction: %w", err)
return
}
logger.Info("migration completed", zap.Int("gob_shares_total", sharesGOBTotal))

Check warning on line 31 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L27-L31

Added lines #L27 - L31 were not covered by tests
}()

// sharesSSZEncoded is a bunch of updates this migration will need to perform, we cannot do them
// all in a single transaction (because there is a limit on how large a single transaction can be)
// so we'll use SetMany func that will split up the data we want to update into batches committing
// each batch in a separate transaction. I guess that makes this migration non-atomic, but since
// this migration is idempotent atomicity isn't required (we can re-apply it however many times
// we like without "breaking" anything)
sharesSSZEncoded := make([]basedb.Obj, 0)

Check warning on line 40 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L40

Added line #L40 was not covered by tests

err := opt.Db.GetAll(append(storagePrefix, sharesPrefixGOB...), func(i int, obj basedb.Obj) error {
sharesGOB := make(map[string]*storageShareGOB)
err = opt.Db.GetAll(append(opstorage.OperatorStoragePrefix, sharesPrefixGOB...), func(i int, obj basedb.Obj) error {

Check warning on line 43 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L42-L43

Added lines #L42 - L43 were not covered by tests
shareGOB := &storageShareGOB{}
if err := shareGOB.Decode(obj.Value); err != nil {
return fmt.Errorf("decode gob share: %w", err)
}
sID := shareID(shareGOB.ValidatorPubKey)
if _, ok := sharesGOB[sID]; ok {
return fmt.Errorf("have already seen GOB share with the same share ID: %s", sID)
}
sharesGOB[sID] = shareGOB

Check warning on line 52 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L48-L52

Added lines #L48 - L52 were not covered by tests
share, err := storageShareGOBToSpecShare(shareGOB)
if err != nil {
return fmt.Errorf("convert storage share to spec share: %w", err)
}
shareSSZ := specShareToStorageShareSSZ(share)
key := storageKeySSZ(share.ValidatorPubKey[:])
shareSSZ := storage.FromSpecShare(share)
key := storage.SharesDBKey(shareSSZ.ValidatorPubKey[:])

Check warning on line 58 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L57-L58

Added lines #L57 - L58 were not covered by tests
value, err := shareSSZ.Encode()
if err != nil {
return fmt.Errorf("encode ssz share: %w", err)
}
sets = append(sets, basedb.Obj{
sharesSSZEncoded = append(sharesSSZEncoded, basedb.Obj{

Check warning on line 63 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L63

Added line #L63 was not covered by tests
Key: key,
Value: value,
})
Expand All @@ -47,23 +70,129 @@
return fmt.Errorf("GetAll: %w", err)
}

if err := opt.Db.SetMany(storagePrefix, len(sets), func(i int) (basedb.Obj, error) {
return sets[i], nil
sharesGOBTotal = len(sharesGOB)
if sharesGOBTotal == 0 {
return nil // we won't be creating any SSZ shares
oleg-ssvlabs marked this conversation as resolved.
Show resolved Hide resolved
}

Check warning on line 76 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L73-L76

Added lines #L73 - L76 were not covered by tests

if err := opt.Db.SetMany(opstorage.OperatorStoragePrefix, len(sharesSSZEncoded), func(i int) (basedb.Obj, error) {
return sharesSSZEncoded[i], nil

Check warning on line 79 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L78-L79

Added lines #L78 - L79 were not covered by tests
}); err != nil {
return fmt.Errorf("SetMany: %w", err)
}

// TODO - do not complete this migration for now, we'll complete it once
// additional sanity-checks are added here
//if err := opt.Db.DropPrefix(append(storagePrefix, sharesPrefixGOB...)); err != nil {
// return fmt.Errorf("DropPrefix: %w", err)
//}
//
//// This makes sure migration applies only once.
//if err := completed(opt.Db); err != nil {
// return fmt.Errorf("complete transaction: %w", err)
//}
sharesSSZTotal := 0
if err := opt.Db.GetAll(storage.SharesDBPrefix(opstorage.OperatorStoragePrefix), func(i int, obj basedb.Obj) error {
oleg-ssvlabs marked this conversation as resolved.
Show resolved Hide resolved
shareSSZ := &storage.Share{}
err := shareSSZ.Decode(obj.Value)
if err != nil {
return fmt.Errorf("decode ssz share: %w", err)
}
sID := shareID(shareSSZ.ValidatorPubKey)
shareGOB, ok := sharesGOB[sID]
if !ok {
// this shouldn't really happen & we should probably return error if it does, but
// on stage since we already have some SSV nodes that migrated to SSZ format and
// potentially added new validators (new SSZ shares) erroring would prevent migration
// from completing, so we don't return error here
return nil
}
oleg-ssvlabs marked this conversation as resolved.
Show resolved Hide resolved
if !matchGOBvsSSZ(shareGOB, shareSSZ) {
return fmt.Errorf(
"GOB share doesn't match corresponding SSZ share, GOB: %s, SSZ: %s",
litter.Sdump(shareGOB),
litter.Sdump(shareSSZ),
)
}
Comment on lines +101 to +107
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Contributor Author

@iurii-ssv iurii-ssv Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sharesSSZTotal++
return nil
}); err != nil {
return fmt.Errorf("GetMany: %w", err)
}

Check warning on line 111 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L84-L111

Added lines #L84 - L111 were not covered by tests

if sharesSSZTotal != sharesGOBTotal {
return fmt.Errorf("total SSZ shares count %d doesn't match GOB shares count %d", sharesSSZTotal, sharesGOBTotal)
}

Check warning on line 115 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L113-L115

Added lines #L113 - L115 were not covered by tests

if err = opt.Db.DropPrefix(append(opstorage.OperatorStoragePrefix, sharesPrefixGOB...)); err != nil {
err = fmt.Errorf("DropPrefix (GOB shares): %w", err)
return
}

Check warning on line 120 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L117-L120

Added lines #L117 - L120 were not covered by tests

return nil
},
}

func shareID(validatorPubkey []byte) string {
return string(validatorPubkey)

Check warning on line 127 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L126-L127

Added lines #L126 - L127 were not covered by tests
}

func matchGOBvsSSZ(shareGOB *storageShareGOB, shareSSZ *storage.Share) bool {
// note, ssz share no longer has OperatorID field
if !bytes.Equal(shareGOB.ValidatorPubKey, shareSSZ.ValidatorPubKey) {
return false
}
if !bytes.Equal(shareGOB.SharePubKey, shareSSZ.SharePubKey) {
return false
}
if len(shareGOB.Committee) != len(shareSSZ.Committee) {
return false
}
for i, committeeGOB := range shareGOB.Committee {
committeeSSZ := shareSSZ.Committee[i]
if committeeGOB.OperatorID != committeeSSZ.OperatorID {
return false
}
if !bytes.Equal(committeeGOB.PubKey, committeeSSZ.PubKey) {
return false
}

Check warning on line 148 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L130-L148

Added lines #L130 - L148 were not covered by tests
}
if shareGOB.Quorum != shareSSZ.Quorum {
return false
}
if shareGOB.PartialQuorum != shareSSZ.PartialQuorum {
return false
}
if shareGOB.DomainType != shareSSZ.DomainType {
return false
}
if shareGOB.FeeRecipientAddress != shareSSZ.FeeRecipientAddress {
return false
}
if !bytes.Equal(shareGOB.Graffiti, shareSSZ.Graffiti) {
return false
}

Check warning on line 164 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L150-L164

Added lines #L150 - L164 were not covered by tests

if shareGOB.OwnerAddress != shareSSZ.OwnerAddress {
return false
}
if shareGOB.Liquidated != shareSSZ.Liquidated {
return false
}

Check warning on line 171 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L166-L171

Added lines #L166 - L171 were not covered by tests

// finally, check Beacon metadata matches
if shareGOB.BeaconMetadata == nil {
if shareSSZ.ValidatorIndex != 0 {
return false
}
if shareSSZ.Status != 0 {
return false
}
if shareSSZ.ActivationEpoch != 0 {
return false
}

Check warning on line 183 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L174-L183

Added lines #L174 - L183 were not covered by tests
// note, ssz share no longer has Balance field
return true

Check warning on line 185 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L185

Added line #L185 was not covered by tests
}
if uint64(shareGOB.BeaconMetadata.Index) != shareSSZ.ValidatorIndex {
return false
}
if int(shareGOB.BeaconMetadata.Status) != int(shareSSZ.Status) {
return false
}
if uint64(shareGOB.BeaconMetadata.ActivationEpoch) != shareSSZ.ActivationEpoch {
return false
}

Check warning on line 195 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L187-L195

Added lines #L187 - L195 were not covered by tests

return true

Check warning on line 197 in migrations/migration_5_share_gob_to_ssz.go

View check run for this annotation

Codecov / codecov/patch

migrations/migration_5_share_gob_to_ssz.go#L197

Added line #L197 was not covered by tests
}
Loading
Loading