core/validatorapi: add SubmitBeaconBlock to validatorapi #409

dB2510 · 2022-04-09T20:23:32Z

Adds support for submit Partially signed beacon block to validatorapi.

category: feature
ticket: #225

codecov · 2022-04-09T20:32:12Z

Codecov Report

❗ No coverage uploaded for pull request base (main@2c477b3). Click here to learn what that means.
The diff coverage is 65.51%.

@@           Coverage Diff           @@
##             main     #409   +/-   ##
=======================================
  Coverage        ?   56.98%           
=======================================
  Files           ?       63           
  Lines           ?     5605           
  Branches        ?        0           
=======================================
  Hits            ?     3194           
  Misses          ?     1992           
  Partials        ?      419

Impacted Files	Coverage Δ
core/types.go	`41.66% <0.00%> (ø)`
testutil/beaconmock/beaconmock.go	`66.66% <0.00%> (ø)`
core/encode.go	`58.08% <41.66%> (ø)`
core/validatorapi/validatorapi.go	`51.43% <65.57%> (ø)`
core/validatorapi/router.go	`64.70% <97.56%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c477b3...4a97cc2. Read the comment docs.

leolara · 2022-04-10T04:24:09Z

core/validatorapi/validatorapi.go

+	if err != nil {
+		return errors.Wrap(err, "getting slots per epoch")
+	}
+	epoch := eth2p0.Epoch(uint64(slot) / slotsPerEpoch)


@dB2510 we are doing this in several places, perhaps we should create an issue to generalise it

Yeah, a function in this package called epochFromSlot is probably not a bad idea. We could also do PR to add eth2client.EpochFromSlotProvider to upstream client repo.

leolara · 2022-04-10T04:29:25Z

core/types.go

+//   core.Duty{Slot: slot, Type: core.DutyProposer}
+//   vs
+//   core.NewProposerDuty(slot)
+func NewProposerDuty(slot int64) Duty {


@dB2510 slot is an uint64 or eth2p0.Slot

Added this to align with the other similar types: NewAttesterDuty and NewRandaoDuty in the same file.

Maybe we can address this later as part of this issue: #383

yeah, it can also be done as big refactor when we do that issue

leolara · 2022-04-10T07:32:05Z

core/encode.go

@@ -184,3 +184,49 @@ func DecodeProposerUnsignedData(unsignedData UnsignedData) (*spec.VersionedBeaco

 	return proData, nil
 }
+
+// EncodeBlockParSignedData returns the partially signed block data as an encoded ParSignedData.
+func EncodeBlockParSignedData(block *spec.VersionedSignedBeaconBlock, shareIdx int) (ParSignedData, error) {


@dB2510 wouldn't it make sense if EncodeBlockParSignedData would be an accesor of spec.VersionedSignedBeaconBlock and DecodeBlockParSignedData a constructor of ParSignedData ?

We prefer to keep data and logic separate. Pure functions and pure data.

As per golang guidelines: Functions over methods.

Also, spec.VersionedSignedBeaconBlock isn't our type. So we cannot add methods to it.

yeah right, spec.VersionedSignedBeaconBlock is go-eth2-client's type that we are using.

leolara

approved with comments

corverroos

Looks good! Just need to figure out the block version thing

corverroos · 2022-04-11T05:27:13Z

core/encode.go

@@ -184,3 +184,49 @@ func DecodeProposerUnsignedData(unsignedData UnsignedData) (*spec.VersionedBeaco

 	return proData, nil
 }
+
+// EncodeBlockParSignedData returns the partially signed block data as an encoded ParSignedData.
+func EncodeBlockParSignedData(block *spec.VersionedSignedBeaconBlock, shareIdx int) (ParSignedData, error) {


Also, spec.VersionedSignedBeaconBlock isn't our type. So we cannot add methods to it.

corverroos · 2022-04-11T05:33:50Z

core/validatorapi/router.go

+func submitBlock(p eth2client.BeaconBlockSubmitter) handlerFunc {
+	return func(ctx context.Context, params map[string]string, query url.Values, body []byte) (interface{}, error) {
+		var phase0Block *eth2p0.SignedBeaconBlock
+		err := phase0Block.UnmarshalJSON(body)


problem: decoding json isn't a good way to detect block type, since json is very flexible, especially if some fields overlap and the general structure is the same. This first phase0 check will probably succeed for all block types. Suggest adding a test for other block types.

We should probably look at what the differences are between the versions/types. I'm guessing fields were added mostly and the newer types are backwards compatible with older data. That means we should probably just use the latest version always.

I think using latest version will create problems for us testing with old testnets like prater/kiln, have to check what's block version there. Latest one would be bellatrix though (the name of merge hard fork)

Also to check what gnosis is having

Note: prater has altair beacon block version.

using latest version should work

corverroos · 2022-04-11T05:36:15Z

core/validatorapi/validatorapi.go

+	if err != nil {
+		return errors.Wrap(err, "getting slots per epoch")
+	}
+	epoch := eth2p0.Epoch(uint64(slot) / slotsPerEpoch)


Yeah, a function in this package called epochFromSlot is probably not a bad idea. We could also do PR to add eth2client.EpochFromSlotProvider to upstream client repo.

corverroos · 2022-04-11T05:38:04Z

core/validatorapi/validatorapi.go

+	}
+	epoch := eth2p0.Epoch(uint64(slot) / slotsPerEpoch)
+
+	var sig eth2p0.BLSSignature


structure: can probably extract this as function getBlockSignature

or rather extract the whole verifyBlockSignature as a function

corverroos · 2022-04-11T05:39:49Z

core/validatorapi/validatorapi.go

+	if err != nil {
+		return err
+	}
+	pubkey, _, err := c.awaitBlockFunc(ctx, int64(slot))


Since there is a TODO to remove pubkey from awaitBlockFunc (see above), you should rather call awaitProposerFunc.

corverroos

LGTM

dB2510 added 3 commits April 9, 2022 01:19

add submit beacon block

046a8fd

add tests

01c4554

add encode test

636369c

leolara reviewed Apr 10, 2022

View reviewed changes

leolara approved these changes Apr 10, 2022

View reviewed changes

corverroos reviewed Apr 11, 2022

View reviewed changes

corverroos added the do not merge Indicate to bulldozer bot that this PR should not be merged label Apr 11, 2022

add tests and address comments

4a97cc2

corverroos approved these changes Apr 11, 2022

View reviewed changes

corverroos removed the do not merge Indicate to bulldozer bot that this PR should not be merged label Apr 11, 2022

dB2510 added the merge when ready Indicates bulldozer bot may merge when all checks pass label Apr 11, 2022

obol-bulldozer bot merged commit 023f894 into main Apr 11, 2022

obol-bulldozer bot deleted the dhruv/dutypro_vapis branch April 11, 2022 18:46

dB2510 mentioned this pull request Apr 11, 2022

Support DutyProposer in validatorapi (submit phase) #225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/validatorapi: add SubmitBeaconBlock to validatorapi #409

core/validatorapi: add SubmitBeaconBlock to validatorapi #409

dB2510 commented Apr 9, 2022

codecov bot commented Apr 9, 2022 •

edited

Loading

leolara Apr 10, 2022

corverroos Apr 11, 2022

leolara Apr 10, 2022

dB2510 Apr 11, 2022

dB2510 Apr 11, 2022

corverroos Apr 11, 2022

leolara Apr 10, 2022

corverroos Apr 11, 2022 •

edited

Loading

corverroos Apr 11, 2022

dB2510 Apr 11, 2022

leolara left a comment

corverroos left a comment

corverroos Apr 11, 2022

corverroos Apr 11, 2022

dB2510 Apr 11, 2022

dB2510 Apr 11, 2022

dB2510 Apr 11, 2022

dB2510 Apr 11, 2022

corverroos Apr 11, 2022

corverroos Apr 11, 2022

corverroos Apr 11, 2022

dB2510 Apr 11, 2022

corverroos Apr 11, 2022

dB2510 Apr 11, 2022

corverroos left a comment

core/validatorapi: add SubmitBeaconBlock to validatorapi #409

core/validatorapi: add SubmitBeaconBlock to validatorapi #409

Conversation

dB2510 commented Apr 9, 2022

codecov bot commented Apr 9, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leolara left a comment

Choose a reason for hiding this comment

corverroos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 9, 2022 •

edited

Loading

corverroos Apr 11, 2022 •

edited

Loading