`SingleAttestation` implementation by eserilev · Pull Request #6488 · sigp/lighthouse

eserilev · 2024-10-15T04:31:32Z

Issue Addressed

Closes #6380

Proposed Changes

Spec: ethereum/consensus-specs#3900

Convert single attestation as early as possible to reduce code diff

Conga-lined off:

Electra spec changes for v1.5.0-beta.0 #6731

eserilev · 2024-10-15T19:27:10Z

@dapplion

eserilev · 2024-11-28T05:43:00Z

I've ran some kurtosis tests interoping against prysm and we are attesting correctly

EDIT: we are NOT interoping correctly at the moment. I am working on debugging the issue

…ngle_attestation

consensus/types/src/subnet_id.rs

validator_client/validator_services/src/attestation_service.rs

beacon_node/lighthouse_network/src/types/pubsub.rs

beacon_node/http_api/tests/fork_tests.rs

beacon_node/http_api/src/publish_attestations.rs

beacon_node/network/src/network_beacon_processor/mod.rs

consensus/types/src/attestation.rs

dapplion · 2025-01-15T05:32:00Z

consensus/types/src/attestation.rs

+        };
+
+        if committee.index != self.committee_index as u64 {
+            return Err(Error::InvalidCommittee);


Suggested change

return Err(Error::InvalidCommittee);

return Err(Error::UnexpectedCommittee(committee.index, self.committee_index));

I've updated the args for this method. Instead of passing in the full BeaconCommittee obj, I'm passing in the committee slice. Upstream from this method call I make a check for the existence of the specific committee for the slot and committee_index and return either a Error::NoCommitteeForSlotAndIndex or log a warn message if the committee isn't found

consensus/types/src/attestation.rs

michaelsproul

Approved modulo Lion's small tweaks

beacon_node/beacon_chain/src/attestation_verification.rs

dapplion · 2025-01-15T05:56:50Z

beacon_node/beacon_chain/src/events.rs

                .send(kind)
                .map(|count| log_count("attestation", count)),
+            EventKind::SingleAttestation(_) => self
+                .attestation_tx


This is not correct I believe. There's a new event name single_attestation, so you should have a new (tx,rx) and broadcast there. https://github.com/ethereum/beacon-APIs/blob/aa1be259e2d8a64f451dd304913e0310779f6f80/apis/eventstream/index.yaml#L70

Yeah otherwise calls into the event stream with error with unknown topic

I believe the attestation_tx is just for SSE which is for our own internal infra? We still emit a SingleAttestation event with the new single_attestation topic name, which I think is spec compliant. I could be wrong though

ok maybe I am wrong, ill double check, thanks

lighthouse/common/eth2/src/types.rs

Line 1267 in 0d90135

pub enum EventTopic {

this enum must include a SingleAttestation variant

Yep you were right, heres the fix
54844b3

michaelsproul · 2025-01-15T08:55:50Z

Looks like one of the SSE tests is failing (get_events)

eserilev · 2025-01-15T09:38:41Z

I believe i've fixed the test failure but I think I need to add some test coverage for the new single_attestation event topic

eserilev · 2025-01-16T04:58:48Z

i've added test coverage for the new single_attestation event topic. This PR should hopefully be good to go

dapplion · 2025-01-16T10:04:33Z

beacon_node/beacon_chain/src/beacon_chain.rs

+                        let current_fork = self
+                            .spec
+                            .fork_name_at_slot::<T::EthSpec>(v.attestation().data().slot);
+                        if current_fork.electra_enabled() {


Is it necessary to check the fork now? Can we have single attestations before electra?

dapplion

PR is good to go!

pawanjay176

LGTM as well

pawanjay176 · 2025-01-16T17:46:14Z

@mergify queue

mergify · 2025-01-16T17:46:33Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 06329ec

pawanjay176 added 10 commits August 29, 2024 16:11

First pass

25feedf

Add restrictions to RuntimeVariableList api

4dc6e65

Use empty_uninitialized and fix warnings

a9cb329

Fix some todos

60100fc

Merge branch 'unstable' into max-blobs-preset

13f9bba

Fix take impl on RuntimeFixedList

e71020e

cleanup

52bb581

Fix test compilations

d37733b

Fix some more tests

12c6ef1

Fix test from unstable

2fcb293

eserilev changed the title ~~Single attestation~~ SingleAttestation implementation Oct 15, 2024

eserilev added the work-in-progress PR is a work-in-progress label Oct 15, 2024

eserilev added the electra Required for the Electra/Prague fork label Oct 15, 2024

Merge branch 'unstable' into max-blobs-preset

21ecb58

eserilev added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Oct 24, 2024

This was referenced Nov 27, 2024

SingleAttestation full implementation #6616

Closed

Separate type for unaggregated network attestations #6380

Closed

eserilev added blocked and removed ready-for-review The code is ready for review labels Dec 2, 2024

michaelsproul added the v7.0.0-beta.0 New release c. Q1 2025 label Dec 3, 2024

SingleAttestation

366bed3

eserilev force-pushed the single_attestation branch from ba06501 to 366bed3 Compare December 4, 2024 09:29

eserilev added 2 commits December 13, 2024 21:51

Add post attestation v2 endpoint logic to attestation service

68292ad

Merge branch 'unstable' of https://github.com/sigp/lighthouse into si…

5a4cd3b

…ngle_attestation

michaelsproul self-requested a review December 19, 2024 06:23

michaelsproul added the electra-alpha10 Electra release for devnet 5 label Dec 19, 2024

Implement "Bugfix and more withdrawal tests"

9d5a3af

eserilev added 3 commits January 14, 2025 13:56

Merge branch 'unstable' of https://github.com/sigp/lighthouse into si…

fb5e378

…ngle_attestation

publish_attestations should accept Either<Attestation,SingleAttestation>

2d61b65

log an error when failing to convert to SingleAttestation

b6d80eb

pawanjay176 reviewed Jan 14, 2025

View reviewed changes

michaelsproul added 2 commits January 15, 2025 16:02

Use Cow to avoid clone

86ce323

Avoid reconverting to SingleAttestation

8cc43c1

michaelsproul mentioned this pull request Jan 15, 2025

Ensure SingleAttestation conversion happens on the BeaconProcessor #6805

Closed

michaelsproul and others added 4 commits January 15, 2025 16:28

Tweak VC error message

e900bb8

update comments

72da039

update comments

b9b9b99

pass in single attestation as ref to subnetid calculation method

4f2182e

dapplion reviewed Jan 15, 2025

View reviewed changes

michaelsproul approved these changes Jan 15, 2025

View reviewed changes

dapplion reviewed Jan 15, 2025

View reviewed changes

beacon_node/beacon_chain/src/attestation_verification.rs Show resolved Hide resolved

dapplion reviewed Jan 15, 2025

View reviewed changes

eserilev added 2 commits January 15, 2025 14:26

Improved API, new error variants and other minor tweaks

aa66b06

Fix single_attestation event topic boilerplate

54844b3

fix sse event failure

f7a0b37

Add single_attestation event topic test coverage

42c01c6

dapplion reviewed Jan 16, 2025

View reviewed changes

dapplion approved these changes Jan 16, 2025

View reviewed changes

pawanjay176 approved these changes Jan 16, 2025

View reviewed changes

mergify bot added a commit that referenced this pull request Jan 16, 2025

Merge of #6488

87d6012

mergify bot mentioned this pull request Jan 16, 2025

merge queue: embarking unstable (669932a) and #6488 together #6812

Closed

5 tasks

mergify bot merged commit 06329ec into sigp:unstable Jan 16, 2025
30 checks passed

	return Err(Error::InvalidCommittee);
	return Err(Error::UnexpectedCommittee(committee.index, self.committee_index));

Comments

Conversation

eserilev commented Oct 15, 2024 • edited by michaelsproul Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

eserilev commented Oct 15, 2024

Uh oh!

eserilev commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dapplion Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

eserilev Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dapplion Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

dapplion Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

eserilev Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eserilev Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

dapplion Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

eserilev Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Jan 15, 2025

Uh oh!

eserilev commented Jan 15, 2025

Uh oh!

eserilev commented Jan 16, 2025

Uh oh!

dapplion Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

dapplion left a comment

Choose a reason for hiding this comment

Uh oh!

pawanjay176 left a comment

Choose a reason for hiding this comment

Uh oh!

pawanjay176 commented Jan 16, 2025

Uh oh!

mergify bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

eserilev commented Oct 15, 2024 •

edited by michaelsproul

Loading

eserilev commented Nov 28, 2024 •

edited

Loading

eserilev Jan 15, 2025 •

edited

Loading

eserilev Jan 15, 2025 •

edited

Loading

mergify bot commented Jan 16, 2025 •

edited

Loading