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

Electra attestation interfaces #13937

Merged
merged 36 commits into from
Apr 30, 2024
Merged

Electra attestation interfaces #13937

merged 36 commits into from
Apr 30, 2024

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Apr 30, 2024

No description provided.

rkapka and others added 24 commits April 15, 2024 16:21
This reverts commit 32ad5009537bc5ec0e6caf9f52143d380d00be85.
This reverts commit f39644ed3cb6f3964fc6c86fdf4bd5de2a9668c8.
…p-7549

# Conflicts:
#	config/params/config.go
#	consensus-types/blocks/types.go
#	consensus-types/mock/block.go
#	proto/prysm/v1alpha1/attestation.go
#	proto/prysm/v1alpha1/attestation.pb.go
#	proto/prysm/v1alpha1/attestation.proto
#	proto/prysm/v1alpha1/beacon_block.pb.go
#	proto/prysm/v1alpha1/beacon_block.proto
#	proto/prysm/v1alpha1/cloners.go
#	proto/prysm/v1alpha1/generated.ssz.go
#	proto/ssz_proto_library.bzl
@rkapka rkapka requested a review from a team as a code owner April 30, 2024 05:00
@rkapka rkapka changed the title Electra att interfaces Electra attestation interfaces Apr 30, 2024
@rkapka rkapka force-pushed the electra-att-interfaces branch 2 times, most recently from 05787e7 to d2628f3 Compare April 30, 2024 14:46
@rkapka rkapka force-pushed the electra-att-interfaces branch 3 times, most recently from 1122d7a to 05787e7 Compare April 30, 2024 15:00
@@ -671,5 +671,5 @@ func ValidatorMaxEffectiveBalance(val *ethpb.Validator) uint64 {
if HasCompoundingWithdrawalCredential(val) {
return params.BeaconConfig().MaxEffectiveBalanceElectra
}
return params.BeaconConfig().MinActivationBalance // TODO: Add test that MinActivationBalance == (old) MaxEffectiveBalance
return params.BeaconConfig().MinActivationBalance
Copy link
Contributor

Choose a reason for hiding this comment

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

was a test added?

Copy link
Member

Choose a reason for hiding this comment

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

I added this test in #13921 and forgot to remove the TODO

"attestationSlot1": slashing.GetFirstAttestation().GetData().Slot,
"beaconBlockRoot1": fmt.Sprintf("%#x", bytesutil.Trunc(slashing.GetFirstAttestation().GetData().BeaconBlockRoot)),
"sourceEpoch1": slashing.GetFirstAttestation().GetData().Source.Epoch,
"targetEpoch1": slashing.GetFirstAttestation().GetData().Target.Epoch,
Copy link
Contributor

@james-prysm james-prysm Apr 30, 2024

Choose a reason for hiding this comment

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

not for this pr but maybe we should rename this function as it can be easily misused and accidentally swapped with GetSecondAttestation

GetSignature() []byte
}

func (a *Attestation) Version() int {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have a check for these attestations/sub types using var _ = MyInterface(concrete type)

Copy link
Member

Choose a reason for hiding this comment

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

@james-prysm i think that would have the same circular dependency issue that is mentioned on line 10

prestonvanloon
prestonvanloon previously approved these changes Apr 30, 2024
}

// PbCapella --
func (executionPayloadHeaderElectra) PbCapella() (*enginev1.ExecutionPayloadCapella, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be removed again as part of kasey's changes

@@ -13,7 +13,7 @@ import (
)

// Proto converts the signed beacon block to a protobuf object.
func (b *SignedBeaconBlock) Proto() (proto.Message, error) {
func (b *SignedBeaconBlock) Proto() (proto.Message, error) { // nolint:gocognit
Copy link
Contributor

Choose a reason for hiding this comment

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

probably a todo to improve

Copy link
Member

Choose a reason for hiding this comment

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

I dont think we can really make this much less complex :(

@prestonvanloon prestonvanloon added this pull request to the merge queue Apr 30, 2024
Merged via the queue into develop with commit 2c5a2e8 Apr 30, 2024
16 of 17 checks passed
@prestonvanloon prestonvanloon deleted the electra-att-interfaces branch April 30, 2024 21:36
nisdas pushed a commit that referenced this pull request Jul 4, 2024
* config values

* block protos

* get_committee_indices

* proto and ssz

* attestation interface

* Revert "Auxiliary commit to revert individual files from deadb21"

This reverts commit 32ad5009537bc5ec0e6caf9f52143d380d00be85.

* todos

* get_attesting_indices

* Revert "Auxiliary commit to revert individual files from dd27897"

This reverts commit f39644ed3cb6f3964fc6c86fdf4bd5de2a9668c8.

* beacon spec changes

* Fix pending attestation. Build ok

* Electra: add electra version

* Electra: consensus types

* gocognit exclusion

* @potuz's suggestion

* build fix

* interfaces for indexed att and slashing

* indexed att usage

* BuildSignedBeaconBlockFromExecutionPayload

* slashing usage

* grpc stubs

* remove unused methods

* Electra attestation interfaces

* cleanup

* tests

* make linter happy

* simple casting

* test fixes

* Fix spectest failures

* Regen pb and ssz files

* Handle "not ok" type assertion cases

* Setters that check version should always return an error. SetAttesterSlashings and SetAttestations

* gofmt

* Fix TestMinSpanChunksSlice_CheckSlashable

---------

Co-authored-by: terence tsao <terence@prysmaticlabs.com>
Co-authored-by: Preston Van Loon <preston@pvl.dev>
This pull request was closed.
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.

4 participants