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/fetcher: add DutyProposer to fetcher #305

Merged
merged 4 commits into from
Mar 30, 2022
Merged

core/fetcher: add DutyProposer to fetcher #305

merged 4 commits into from
Mar 30, 2022

Conversation

dB2510
Copy link
Contributor

@dB2510 dB2510 commented Mar 29, 2022

Adds DutyProposer in Fetcher

category: feature
ticket: #222

@dB2510 dB2510 linked an issue Mar 29, 2022 that may be closed by this pull request
@@ -84,6 +89,10 @@ func (f *Fetcher) Fetch(ctx context.Context, duty core.Duty, argSet core.FetchAr
return nil
}

func (f *Fetcher) RegisterAggSigDB(fn func(context.Context, core.Duty, core.PubKey) (core.AggSignedData, error)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add godoc, include is should be called before usage since not thread safe

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!

Comment on lines 156 to 157
var graffiti [32]byte
_, _ = rand.Read(graffiti[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't be unique, aggregation will fail I think. Just do empty graffiti for now.

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!

Copy link
Contributor

@corverroos corverroos left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -139,6 +143,7 @@ func Wire(
) {
sched.Subscribe(fetch.Fetch)
fetch.Subscribe(cons.Propose)

Copy link
Contributor

Choose a reason for hiding this comment

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

remove new line

core/types.go Outdated
@@ -165,6 +165,13 @@ type AttestationData struct {
Duty eth2v1.AttesterDuty
}

// ProposerData wraps the eth2 beacon block data and adds the original duty.
// The original duty allows mapping the partial signed response from the VC.
type ProposerData struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This type isn't required. It was required for attestation, but it isn't for block proposal. The VC queries unsigneddata by slot, which is included in block.

core/encode.go Outdated
@@ -161,3 +161,24 @@ func EncodeRandaoAggSignedData(randao eth2p0.BLSSignature) AggSignedData {
func DecodeRandaoAggSignedData(data AggSignedData) eth2p0.BLSSignature {
return data.Signature.ToETH2()
}

// EncodeProposerUnsignedData returns the proposer data as an encoded UnsignedData.
func EncodeProposerUnsignedData(proData *ProposerData) (UnsignedData, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in comment below, remove ProposerData struct. Here you should de/encode eth2spec.VersionedBeaconBlock directly.

@@ -211,7 +213,7 @@ func WithSlotsPerEpoch(slotsPerEpoch int) Option {

// WithDeterministicDuties configures the mock to provide deterministic duties based on provided arguments and config.
// Note it depends on ValidatorsFunc being populated, e.g. via WithValidatorSet.
//nolint:revive
//nolint:revive,dupl
func WithDeterministicDuties(factor int) Option {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this if not used anymore

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 used in scheduler_test.go for dutyproposer and dutyattester both

testutil/beaconmock/options.go Show resolved Hide resolved
// TODO(dhruv): what to do with graffiti?
// passing empty graffiti since it is not required in API
var graffiti [32]byte
proData, err := f.eth2Cl.BeaconBlockProposal(ctx, eth2p0.Slot(uint64(slot)), randaoEth2, graffiti[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename block

@dB2510 dB2510 merged commit 9520aae into main Mar 30, 2022
@dB2510 dB2510 deleted the dhruv/dutypro_fetch branch March 30, 2022 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support DutyProposer in Fetcher
2 participants