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

Keymanager APIs - get,post,delete graffiti #13474

Merged
merged 57 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
02d99b2
wip
james-prysm Jan 9, 2024
b3f2d52
adding set and delete graffiti
james-prysm Jan 16, 2024
6954e6c
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 16, 2024
a619275
fixing mock
james-prysm Jan 17, 2024
3a67b18
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 17, 2024
c4a68de
fixing mock linting and putting in scaffolds for unit tests
james-prysm Jan 17, 2024
9d09686
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 17, 2024
6163564
adding some tests
james-prysm Jan 18, 2024
8fa12b4
gaz
james-prysm Jan 18, 2024
33fbd8f
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 18, 2024
4fb7c0a
adding tests
james-prysm Jan 18, 2024
30204c1
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 18, 2024
2ba1d90
updating missing unit test
james-prysm Jan 18, 2024
357ecb2
fixing unit test
james-prysm Jan 18, 2024
77c2329
Update validator/rpc/handlers_keymanager.go
james-prysm Jan 18, 2024
4a82c2d
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 19, 2024
ab1ec5d
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 19, 2024
aa34ba5
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 22, 2024
a58f927
Update validator/client/propose.go
james-prysm Jan 22, 2024
5f28c9c
Update validator/rpc/handlers_keymanager.go
james-prysm Jan 22, 2024
feb1a98
Update validator/client/propose.go
james-prysm Jan 22, 2024
f01b04c
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 22, 2024
558fbee
radek's feedback
james-prysm Jan 22, 2024
ba26d89
fixing tests
james-prysm Jan 22, 2024
2df8437
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 23, 2024
0ef666f
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Jan 23, 2024
b83a12b
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Feb 15, 2024
e9d26e3
using wrapper for graffiti
james-prysm Feb 15, 2024
e9af279
fixing linting
james-prysm Feb 15, 2024
ec61a61
wip
james-prysm Feb 16, 2024
41e6cfb
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Feb 16, 2024
cafc2bc
fixing setting proposer settings
james-prysm Feb 16, 2024
0a76ad9
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Feb 29, 2024
74e2a01
partial fix to merging
james-prysm Mar 4, 2024
499b2bd
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 5, 2024
352cf4c
more partial fixes to tests
james-prysm Mar 6, 2024
0472a99
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 6, 2024
8fe24d6
gaz
james-prysm Mar 6, 2024
021ccdb
fixing tests and setting logic
james-prysm Mar 7, 2024
6a47352
changing keymanager
james-prysm Mar 7, 2024
94eb5d7
fixing tests and making graffiti optional in the proposer file
james-prysm Mar 8, 2024
0ff6520
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 8, 2024
8cff0ac
remove unneeded lines
james-prysm Mar 8, 2024
2e51b34
reverting unintended changes
james-prysm Mar 8, 2024
2d50abf
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 11, 2024
98b3712
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 11, 2024
9ad18fa
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 11, 2024
44b37db
Update validator/client/propose.go
james-prysm Mar 12, 2024
ca16050
addressing feedback
james-prysm Mar 12, 2024
d5e1f6d
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 12, 2024
0aaeea0
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 12, 2024
3341883
removing uneeded line
james-prysm Mar 12, 2024
46ec87c
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 12, 2024
c03f501
Merge branch 'develop' into graffiti-keymanager-api
james-prysm Mar 13, 2024
ebb136c
fixing bad merge resolution
james-prysm Mar 13, 2024
01278b1
gofmt
james-prysm Mar 13, 2024
752af27
gaz
james-prysm Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/validator/service/proposer_settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ func (ps *ProposerSettings) ToPayload() *validatorpb.ProposerSettingsPayload {
if option.BuilderConfig != nil {
p.Builder = option.BuilderConfig.ToPayload()
}
p.Graffiti = option.Graffiti
payload.ProposerConfig[hexutil.Encode(key[:])] = p
}
}
Expand All @@ -120,6 +121,7 @@ func (ps *ProposerSettings) ToPayload() *validatorpb.ProposerSettingsPayload {
if ps.DefaultConfig.BuilderConfig != nil {
p.Builder = ps.DefaultConfig.BuilderConfig.ToPayload()
}
p.Graffiti = ps.DefaultConfig.Graffiti
payload.DefaultConfig = p
}
return payload
Expand All @@ -134,6 +136,7 @@ type FeeRecipientConfig struct {
type ProposerOption struct {
FeeRecipientConfig *FeeRecipientConfig
BuilderConfig *BuilderConfig
Graffiti string
}

// Clone creates a deep copy of the proposer settings
Expand Down Expand Up @@ -211,5 +214,6 @@ func (po *ProposerOption) Clone() *ProposerOption {
if po.BuilderConfig != nil {
p.BuilderConfig = po.BuilderConfig.Clone()
}
p.Graffiti = po.Graffiti
return p
}
108 changes: 59 additions & 49 deletions proto/prysm/v1alpha1/validator-client/keymanager.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions proto/prysm/v1alpha1/validator-client/keymanager.proto
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ message SignResponse {
message ProposerOptionPayload {
string fee_recipient = 1;
BuilderConfig builder = 2;
string graffiti = 3;
}

// BuilderConfig is a property of ProposerOptionPayload
Expand Down
1 change: 1 addition & 0 deletions validator/accounts/testing/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ go_library(
"//validator:__subpackages__",
],
deps = [
"//config/fieldparams:go_default_library",
"//config/validator/service:go_default_library",
"//consensus-types/primitives:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
Expand Down
22 changes: 22 additions & 0 deletions validator/accounts/testing/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"sync"
"time"

fieldparams "github.com/prysmaticlabs/prysm/v4/config/fieldparams"
validatorserviceconfig "github.com/prysmaticlabs/prysm/v4/config/validator/service"
"github.com/prysmaticlabs/prysm/v4/consensus-types/primitives"
ethpb "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1"
Expand Down Expand Up @@ -90,6 +91,7 @@ func (_ *Wallet) InitializeKeymanager(_ context.Context, _ iface.InitKeymanagerC
type Validator struct {
Km keymanager.IKeymanager
proposerSettings *validatorserviceconfig.ProposerSettings
Graffiti string
Copy link
Contributor

Choose a reason for hiding this comment

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

The field should not be exported because we have a setter.

}

func (_ *Validator) LogSyncCommitteeMessagesSubmitted() {}
Expand Down Expand Up @@ -213,6 +215,26 @@ func (m *Validator) SetProposerSettings(_ context.Context, settings *validatorse
return nil
}

// GetGraffiti for mocking
func (m *Validator) GetGraffiti(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte) ([]byte, error) {
if m.Graffiti == "" {
return nil, errors.New("graffiti is missing ")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would not having a graffiti produce an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was just for the mock, but i'll think of a better way to represent

return []byte(m.Graffiti), nil
}

// SetGraffiti for mocking
func (m *Validator) SetGraffiti(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte, graffiti []byte) error {
m.Graffiti = string(graffiti)
return nil
}

// DeleteGraffiti for mocking
func (m *Validator) DeleteGraffiti(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte) error {
m.Graffiti = ""
return nil
}

func (_ *Validator) StartEventStream(_ context.Context) error {
panic("implement me")
}
Expand Down
3 changes: 3 additions & 0 deletions validator/client/iface/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ type Validator interface {
SignValidatorRegistrationRequest(ctx context.Context, signer SigningFunc, newValidatorRegistration *ethpb.ValidatorRegistrationV1) (*ethpb.SignedValidatorRegistrationV1, error)
ProposerSettings() *validatorserviceconfig.ProposerSettings
SetProposerSettings(context.Context, *validatorserviceconfig.ProposerSettings) error
GetGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte) ([]byte, error)
SetGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte, graffiti []byte) error
DeleteGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte) error
StartEventStream(ctx context.Context) error
EventStreamIsRunning() bool
NodeIsHealthy(ctx context.Context) bool
Expand Down
62 changes: 55 additions & 7 deletions validator/client/propose.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import (
"fmt"
"time"

"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/golang/protobuf/ptypes/timestamp"
"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/v4/async"
"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/signing"
fieldparams "github.com/prysmaticlabs/prysm/v4/config/fieldparams"
"github.com/prysmaticlabs/prysm/v4/config/params"
validatorserviceconfig "github.com/prysmaticlabs/prysm/v4/config/validator/service"
"github.com/prysmaticlabs/prysm/v4/consensus-types/blocks"
"github.com/prysmaticlabs/prysm/v4/consensus-types/interfaces"
"github.com/prysmaticlabs/prysm/v4/consensus-types/primitives"
Expand Down Expand Up @@ -64,7 +66,7 @@ func (v *validator) ProposeBlock(ctx context.Context, slot primitives.Slot, pubK
return
}

g, err := v.getGraffiti(ctx, pubKey)
g, err := v.GetGraffiti(ctx, pubKey)
if err != nil {
// Graffiti is not a critical enough to fail block production and cause
// validator to miss block reward. When failed, validator should continue
Expand Down Expand Up @@ -382,9 +384,24 @@ func signVoluntaryExit(
return sig.Marshal(), nil
}

// Gets the graffiti from cli or file for the validator public key.
func (v *validator) getGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte) ([]byte, error) {
// When specified, default graffiti from the command line takes the first priority.
// GetGraffiti Gets the graffiti from cli or file for the validator public key.
james-prysm marked this conversation as resolved.
Show resolved Hide resolved
func (v *validator) GetGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte) ([]byte, error) {
// Check proposer settings first
if v.proposerSettings != nil {
if v.proposerSettings.ProposeConfig != nil {
option, ok := v.proposerSettings.ProposeConfig[pubKey]
if ok && option.Graffiti != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why checking if option.Graffiti != ""?
The user may set an empty graffiti for this precise validator on purpose.

return []byte(option.Graffiti), nil
}
}
if v.proposerSettings.DefaultConfig != nil {
if v.proposerSettings.DefaultConfig.Graffiti != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here, even if I agree it has less sense...

return []byte(v.proposerSettings.DefaultConfig.Graffiti), nil
}
}
}

// When specified, default graffiti from the command line takes the second priority.
james-prysm marked this conversation as resolved.
Show resolved Hide resolved
if len(v.graffiti) != 0 {
return bytesutil.PadTo(v.graffiti, 32), nil
}
Expand All @@ -393,7 +410,7 @@ func (v *validator) getGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubk
return nil, errors.New("graffitiStruct can't be nil")
}

// When specified, individual validator specified graffiti takes the second priority.
// When specified, individual validator specified graffiti takes the third priority.
nalepae marked this conversation as resolved.
Show resolved Hide resolved
idx, err := v.validatorClient.ValidatorIndex(ctx, &ethpb.ValidatorIndexRequest{PublicKey: pubKey[:]})
if err != nil {
return nil, err
Expand All @@ -403,7 +420,7 @@ func (v *validator) getGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubk
return bytesutil.PadTo([]byte(g), 32), nil
}

// When specified, a graffiti from the ordered list in the file take third priority.
// When specified, a graffiti from the ordered list in the file take fourth priority.
if v.graffitiOrderedIndex < uint64(len(v.graffitiStruct.Ordered)) {
graffiti := v.graffitiStruct.Ordered[v.graffitiOrderedIndex]
v.graffitiOrderedIndex = v.graffitiOrderedIndex + 1
Expand All @@ -414,7 +431,7 @@ func (v *validator) getGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubk
return bytesutil.PadTo([]byte(graffiti), 32), nil
}

// When specified, a graffiti from the random list in the file take fourth priority.
// When specified, a graffiti from the random list in the file take Fifth priority.
if len(v.graffitiStruct.Random) != 0 {
r := rand.NewGenerator()
r.Seed(time.Now().Unix())
Expand All @@ -429,3 +446,34 @@ func (v *validator) getGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubk

return []byte{}, nil
}

func (v *validator) SetGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte, graffiti []byte) error {
if v.proposerSettings != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually handle error conditions first. I would rewrite the code as

if v.proposerSettings == nil {
  // error
}
if v.proposerSettings.ProposeConfig == nil {
  // error
}
ps := v.proposerSettings.Clone()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops yeah i should do it this way

ps := v.proposerSettings.Clone()
if ps.ProposeConfig != nil {
var option *validatorserviceconfig.ProposerOption
option, ok := ps.ProposeConfig[pubKey]
if ok && option != nil {
option.Graffiti = string(graffiti)
return v.SetProposerSettings(ctx, ps) // save the proposer settings
}
return fmt.Errorf("attempted to set graffiti but proposer settings are missing for pubkey:%s", hexutil.Encode(pubKey[:]))
}
}
return errors.New("attempted to set graffiti without proposer settings, graffiti will default to flag options")
}

func (v *validator) DeleteGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte) error {
if v.proposerSettings != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

ps := v.proposerSettings.Clone()
if ps.ProposeConfig != nil {
option, ok := ps.ProposeConfig[pubKey]
if ok && option != nil {
option.Graffiti = ""
return v.SetProposerSettings(ctx, ps) // save the proposer settings
}
return fmt.Errorf("graffiti not found in proposer settings for pubkey:%s", hexutil.Encode(pubKey[:]))
}
}
return errors.New("attempted to delete graffiti without proposer settings, graffiti will default to flag options")
}
Loading
Loading