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

core: voluntary exit #648

Merged
merged 19 commits into from
Jun 3, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
30 changes: 28 additions & 2 deletions core/bcast/bcast.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package bcast

import (
"context"
"encoding/json"

eth2client "github.com/attestantio/go-eth2-client"
eth2p0 "github.com/attestantio/go-eth2-client/spec/phase0"
Expand All @@ -32,6 +33,7 @@ import (
type eth2Provider interface {
eth2client.AttestationsSubmitter
eth2client.BeaconBlockSubmitter
eth2client.VoluntaryExitSubmitter
}

// New returns a new broadcaster instance.
Expand All @@ -49,8 +51,11 @@ type Broadcaster struct {
}

// Broadcast broadcasts the aggregated signed duty data object to the beacon-node.
func (b Broadcaster) Broadcast(ctx context.Context, duty core.Duty,
pubkey core.PubKey, aggData core.AggSignedData,
func (b Broadcaster) Broadcast(
ctx context.Context,
duty core.Duty,
pubkey core.PubKey,
aggData core.AggSignedData,
) (err error) {
ctx = log.WithTopic(ctx, "bcast")
defer func() {
Expand All @@ -72,6 +77,7 @@ func (b Broadcaster) Broadcast(ctx context.Context, duty core.Duty,
z.U64("slot", uint64(att.Data.Slot)),
z.U64("target_epoch", uint64(att.Data.Target.Epoch)),
z.Hex("agg_bits", att.AggregationBits.Bytes()),
z.Any("pubkey", pubkey.String()),
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove .String() since z.Any does that automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
}

Expand All @@ -86,13 +92,33 @@ func (b Broadcaster) Broadcast(ctx context.Context, duty core.Duty,
if err == nil {
log.Info(ctx, "Block proposal successfully submitted to beacon node",
z.U64("slot", uint64(duty.Slot)),
z.Any("pubkey", pubkey.String()),
)
}

return err
case core.DutyRandao:
// Randao is an internal duty, not broadcasted to beacon chain
return nil
case core.DutyVoluntaryExit:
// JSON decoding from the previous component
ve := new(eth2p0.SignedVoluntaryExit)
err := json.Unmarshal(aggData.Data, ve)
if err != nil {
return errors.Wrap(err, "json decoding voluntary exit")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow above pattern of extracting this to core.DecodeExitAggSignedData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


err = b.eth2Cl.SubmitVoluntaryExit(ctx, ve)

if err == nil {
log.Info(ctx, "Voluntary exit successfully submitted to beacon node",
z.U64("epoch", uint64(ve.Message.Epoch)),
z.U64("validator_index", uint64(ve.Message.ValidatorIndex)),
z.Any("pubkey", pubkey.String()),
)
}

return err
default:
return errors.New("unsupported duty type")
}
Expand Down
32 changes: 32 additions & 0 deletions core/bcast/bcast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package bcast_test

import (
"context"
"encoding/json"
"testing"

"github.com/attestantio/go-eth2-client/spec"
Expand Down Expand Up @@ -86,3 +87,34 @@ func TestBroadcastBeaconBlock(t *testing.T) {

<-ctx.Done()
}

func TestBroadcastVoluntaryExit(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
mock, err := beaconmock.New()
require.NoError(t, err)

ve := testutil.RandomSignedVoluntaryExit(t)

aggDataData, err := json.Marshal(ve)
require.NoError(t, err)

aggData := core.AggSignedData{
Data: aggDataData,
Signature: core.SigFromETH2(ve.Signature),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

extract this to core.EncodeExitAggSignedData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


mock.SubmitVoluntaryExitFunc = func(ctx context.Context, ve2 *eth2p0.SignedVoluntaryExit) error {
require.Equal(t, ve, ve2)
cancel()

return ctx.Err()
}

bcaster, err := bcast.New(mock)
require.NoError(t, err)

err = bcaster.Broadcast(ctx, core.Duty{Type: core.DutyVoluntaryExit}, "", aggData)
require.ErrorIs(t, err, context.Canceled)

<-ctx.Done()
}
43 changes: 43 additions & 0 deletions core/encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,22 @@ func EncodeAttestationParSignedData(att *eth2p0.Attestation, shareIdx int) (ParS
}, nil
}

// EncodeVoluntaryExitParSignedData encodes to json to pass between Go components losing typing,
// returns a ParSignedData that contains json.
// WARNING: using this method makes you lose Golang type safety features.
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove unprofessional comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is unprofessional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting personal style disagreements into the code as comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a personal style, this is a statement of fact, that other contributors should be aware of.

If you think this decision is shameful, we should decide about why we are doing it this way, when there are easier ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can find in Open Source software this type of comments very often, that using a method is dangerous for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

func EncodeVoluntaryExitParSignedData(ve *eth2p0.SignedVoluntaryExit, shareIdx int) (ParSignedData, error) {
data, err := json.Marshal(ve)
if err != nil {
return ParSignedData{}, errors.Wrap(err, "json marshal signed voluntary exit")
}

return ParSignedData{
Data: data,
Signature: SigFromETH2(ve.Signature),
ShareIdx: shareIdx,
}, nil
}

// DecodeAttestationParSignedData returns the attestation from the encoded ParSignedData.
func DecodeAttestationParSignedData(data ParSignedData) (*eth2p0.Attestation, error) {
att := new(eth2p0.Attestation)
Expand All @@ -113,6 +129,18 @@ func DecodeAttestationParSignedData(data ParSignedData) (*eth2p0.Attestation, er
return att, nil
}

// DecodeSignedVoluntaryExitParSignedData json decode signed voluntary exit from the previous
// Golang component.
func DecodeSignedVoluntaryExitParSignedData(data ParSignedData) (*eth2p0.SignedVoluntaryExit, error) {
ve := new(eth2p0.SignedVoluntaryExit)
err := json.Unmarshal(data.Data, ve)
if err != nil {
return nil, errors.Wrap(err, "json decoding signed voluntary exit")
}

return ve, nil
}

// EncodeAttestationAggSignedData returns the attestation as an encoded AggSignedData.
func EncodeAttestationAggSignedData(att *eth2p0.Attestation) (AggSignedData, error) {
data, err := json.Marshal(att)
Expand Down Expand Up @@ -275,3 +303,18 @@ func DecodeBlockAggSignedData(data AggSignedData) (*spec.VersionedSignedBeaconBl

return block, nil
}

// EncodeSignedVoluntaryExitAggSignedData encodes to json to pass between Go components losing typing,
// returns a AggSignedData that contains json.
// WARNING: using this method makes you lose Golang type safety features.
func EncodeSignedVoluntaryExitAggSignedData(ve *eth2p0.SignedVoluntaryExit) (AggSignedData, error) {
data, err := json.Marshal(ve)
if err != nil {
return AggSignedData{}, errors.Wrap(err, "json encoding voluntary exit")
}

return AggSignedData{
Data: data,
Signature: SigFromETH2(ve.Signature),
}, nil
}
20 changes: 20 additions & 0 deletions core/sigagg/sigagg.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,18 @@ func getAggSignedData(typ core.DutyType, data core.ParSignedData, aggSig *bls_si
}

return core.EncodeBlockAggSignedData(block)
case core.DutyVoluntaryExit:
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, use core.Encode/Decode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// JSON decode from previous component
ve, err := core.DecodeSignedVoluntaryExitParSignedData(data)
if err != nil {
return core.AggSignedData{}, errors.Wrap(err, "json decoding voluntary exit")
}

// change signature to TSS aggregated one
ve.Signature = eth2Sig

// JSON encode for next component
return core.EncodeSignedVoluntaryExitAggSignedData(ve)
default:
return core.AggSignedData{}, errors.New("unsupported duty type")
}
Expand Down Expand Up @@ -178,6 +190,14 @@ func getSignedRoot(typ core.DutyType, data core.ParSignedData) (eth2p0.Root, err
}

return block.Root()
case core.DutyVoluntaryExit:
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// JSON decode from previous component
ve, err := core.DecodeSignedVoluntaryExitParSignedData(data)
if err != nil {
return eth2p0.Root{}, errors.Wrap(err, "json decoding voluntary exit")
}

return ve.Message.HashTreeRoot()
default:
return eth2p0.Root{}, errors.New("unsupported duty type")
}
Expand Down
70 changes: 70 additions & 0 deletions core/sigagg/sigagg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package sigagg_test
import (
"context"
"crypto/rand"
"encoding/json"
"testing"

"github.com/attestantio/go-eth2-client/spec"
Expand Down Expand Up @@ -151,6 +152,75 @@ func TestSigAgg_DutyRandao(t *testing.T) {
require.NoError(t, err)
}

func TestSigAgg_DutyVoluntaryExit(t *testing.T) {
ctx := context.Background()

const (
threshold = 3
peers = 4
)

// Generate private shares
tss, secrets, err := tbls.GenerateTSS(threshold, peers, rand.Reader)
require.NoError(t, err)

uve := testutil.RandomVoluntaryExit(t)

msg, err := uve.MarshalSSZ()
require.NoError(t, err)

// Create partial signatures (in two formats)
var (
parsigs []core.ParSignedData
psigs []*bls_sig.PartialSignature
)
for _, secret := range secrets {
psig, err := tbls.PartialSign(secret, msg)
require.NoError(t, err)

sig := tblsconv.SigToETH2(tblsconv.SigFromPartial(psig))
ve := &eth2p0.SignedVoluntaryExit{
Message: uve,
Signature: sig,
}

data, err := json.Marshal(ve)
require.NoError(t, err)

parsig := core.ParSignedData{
Data: data,
Signature: core.SigFromETH2(ve.Signature),
ShareIdx: int(psig.Identifier),
}

psigs = append(psigs, psig)
parsigs = append(parsigs, parsig)
}

aggSig, err := tbls.Aggregate(psigs)
require.NoError(t, err)
expect := tblsconv.SigToCore(aggSig)

agg := sigagg.New(threshold)

// Assert output
agg.Subscribe(func(_ context.Context, _ core.Duty, _ core.PubKey, aggData core.AggSignedData) error {
require.Equal(t, expect, aggData.Signature)
sig, err := tblsconv.SigFromCore(aggData.Signature)
require.NoError(t, err)

ok, err := tbls.Verify(tss.PublicKey(), msg, sig)
require.NoError(t, err)
require.True(t, ok)

return nil
})

// Run aggregation
err = agg.Aggregate(ctx, core.Duty{Type: core.DutyVoluntaryExit}, "", parsigs)
require.NoError(t, err)
}

func TestSigAgg_DutyProposer(t *testing.T) {
ctx := context.Background()

Expand Down
11 changes: 6 additions & 5 deletions core/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ type DutyType int
const (
// DutyType enums MUST not change, it will break backwards compatibility.

DutyUnknown DutyType = 0
DutyProposer DutyType = 1
DutyAttester DutyType = 2
DutyRandao DutyType = 3
DutyUnknown DutyType = 0
DutyProposer DutyType = 1
DutyAttester DutyType = 2
DutyRandao DutyType = 3
DutyVoluntaryExit DutyType = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose we rename this to DutyExit, since there is only this type of exit duty, there are no non-voluntary or any other type of exit duty. This also aligns with the other duties above, which is just DutyProposer and DutyRandao instead of more verbose redundant versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't fell comfortable with this, there will be other Exits in the code, could we ask for feedback to @OisinKyne @dB2510 @xenowits ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Only ever append new types here...

dutySentinel DutyType = 4 // Must always be last
dutySentinel DutyType = 5 // Must always be last
)

func (d DutyType) Valid() bool {
Expand Down
3 changes: 2 additions & 1 deletion core/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ func TestBackwardsCompatability(t *testing.T) {
require.EqualValues(t, 1, core.DutyProposer)
require.EqualValues(t, 2, core.DutyAttester)
require.EqualValues(t, 3, core.DutyRandao)
require.EqualValues(t, 4, core.DutyVoluntaryExit)
// Add more types here.

const sentinel = core.DutyType(4)
const sentinel = core.DutyType(5)
for i := core.DutyUnknown; i <= sentinel; i++ {
if i == core.DutyUnknown || i == sentinel {
require.False(t, i.Valid())
Expand Down
26 changes: 25 additions & 1 deletion core/validatorapi/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type Handler interface {
eth2client.BeaconBlockSubmitter
eth2client.ProposerDutiesProvider
eth2client.ValidatorsProvider
eth2client.VoluntaryExitSubmitter
// Above sorted alphabetically.
}

Expand All @@ -71,6 +72,7 @@ func NewRouter(h Handler, beaconNodeAddr string) (*mux.Router, error) {
Name string
Path string
Handler handlerFunc
Methods []string
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is required, why not just support any method since it doesn't really matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we want to do this, we should probably be consistent and add methods for all endpoints (in a different PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we don't need methods since we use eth2-http package for handler which internally handles methods and stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only want to capture the POST, the GET should be proxied

}{
{
Name: "attester_duties",
Expand Down Expand Up @@ -112,12 +114,21 @@ func NewRouter(h Handler, beaconNodeAddr string) (*mux.Router, error) {
Path: "/eth/v1/beacon/blocks",
Handler: submitBlock(h),
},
{
Name: "submit_voluntary_exit",
Path: "/eth/v1/beacon/pool/voluntary_exits",
Handler: submitVoluntaryExit(h),
Methods: []string{"POST"},
},
// TODO(corver): Add more endpoints
}

r := mux.NewRouter()
for _, e := range endpoints {
r.Handle(e.Path, wrap(e.Name, e.Handler))
route := r.Handle(e.Path, wrap(e.Name, e.Handler))
if e.Methods != nil && len(e.Methods) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

if statement not required

r.Handle(e.Path, wrap(e.Name, e.Handler)).Methods(e.Methods...)

route.Methods(e.Methods...)
}
}

// Everything else is proxied
Expand Down Expand Up @@ -439,6 +450,19 @@ func submitBlock(p eth2client.BeaconBlockSubmitter) handlerFunc {
}
}

// submitVoluntaryExit receives the partially signed voluntary exit.
func submitVoluntaryExit(p eth2client.VoluntaryExitSubmitter) handlerFunc {
return func(ctx context.Context, params map[string]string, query url.Values, body []byte) (interface{}, error) {
ve := new(eth2p0.SignedVoluntaryExit)
err := ve.UnmarshalJSON(body)
if err != nil {
return nil, errors.New("invalid voluntary exit data")
}

return nil, p.SubmitVoluntaryExit(ctx, ve)
}
}

// proxyHandler returns a reverse proxy handler.
func proxyHandler(target string) (http.HandlerFunc, error) {
targetURL, err := url.Parse(target)
Expand Down
Loading