feat: Update containers for Vote and Attestation #855

varun-doshi · 2025-10-16T09:31:10Z

What was wrong?

Fixes #840

Changes:

Vote -> Rename AttestationData
Add AggregatedAttestation, SignedAggregatedAttestation
SignedVote -> Rename to SignedAttestation and fix message type as Attestation
Fix existing attestation.rs
Remove a few unnecessary clones
Other changes to comply with the new types

How was it fixed?

Implement some changes fromm ongoing spec PR: leanEthereum/leanSpec#67

To-Do

Our SignedBlock is diverged from current ongoing spec change. This should be VariableList<SignedAttestation, U4096> but since we are yet to implement SignedBlockAndVote(these are still being reviewed). This will be done in a follow PR once the spec change has stabalized a bit. For context if we use Attestation the code wont compile because our db saves SignedAttestation

jihoonsong · 2025-10-16T11:12:54Z

Do we want to wait until the spec PR is merged?

KolbyML · 2025-10-16T11:13:57Z

Do we want to wait until the spec PR is merged?

yes

KolbyML

Hopefully they fix the upstream spec

syjn99 · 2025-10-22T15:36:25Z

crates/common/chain/lean/src/lean_chain.rs

    pub async fn on_attestation_from_block(
        &mut self,
-        signed_votes: impl IntoIterator<Item = SignedVote>,
+        signed_votes: impl IntoIterator<Item = SignedAttestation>,


Suggested change

signed_votes: impl IntoIterator<Item = SignedAttestation>,

signed_attestations: impl IntoIterator<Item = SignedAttestation>,

We're gonna drop all vote jargons, so I'd suggest to fix the parameter name as well.

Fixed; I've tried to remove most if not all vote jargons from the lean flow

Do you want to open a PR to the Lean spec to change gossip topic from vote to attestation? @syjn99

I can do it as well

@varun-doshi
Yes, I think this PR can change the gossip topic as well, along with a PR on leanSpec.

leanEthereum/leanSpec#78

crates/common/consensus/lean/src/vote.rs

jihoonsong

There are some variables that named after vote in lean_block.rs and state.rs. It's just name changes so having it in this PR would be simpler imo

jihoonsong · 2025-10-22T16:08:28Z

crates/common/consensus/lean/src/attestation.rs

 /// for detailed protocol information.
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash)]
-pub struct Attestation {
+pub struct AggregatedAttestation {


I think this should be AggregatedAttestations similar to AggregatedSignatures. I've left a comment in the spec PR, let's see.

syjn99

Overall good, I couldn't find any issues on this PR. Just some renaming stuffs are remaining.

syjn99 · 2025-10-22T16:42:33Z

crates/common/consensus/lean/src/state.rs

            if !self
                .justified_slots
-                .get(vote.source.slot as usize)
+                .get(vote.source().slot as usize)


Suggested change

.get(vote.source().slot as usize)

.get(attestation.source().slot as usize)

Can we rename this as well?

syjn99 · 2025-10-22T16:46:08Z

crates/common/chain/lean/src/lean_chain.rs

    pub async fn on_attestation_from_block(
        &mut self,
-        signed_votes: impl IntoIterator<Item = SignedVote>,
+        signed_votes: impl IntoIterator<Item = SignedAttestation>,


@varun-doshi
Yes, I think this PR can change the gossip topic as well, along with a PR on leanSpec.

syjn99

LGTM, just a small renaming left.

syjn99 · 2025-10-22T22:44:51Z

crates/common/consensus/lean/src/state.rs

+        for signed_attestations in attestations {
+            let attestations = &signed_attestations.message;


Suggested change

for signed_attestations in attestations {

let attestations = &signed_attestations.message;

for signed_attestation in attestations {

let attestation = &signed_attestation.message;

Minor plural issues

syjn99 · 2025-10-22T22:45:38Z

crates/common/consensus/lean/src/state.rs

        let root2 = B256::repeat_byte(2);

-        // root0 is voted by validator 0
+        // root0 is attestationsd by validator 0


Looks like this part is also changed by using IDE features

jihoonsong

It's not critical but some comments are outdated (they're using vote). Other than that, looks good!

KolbyML · 2025-10-25T09:40:10Z

I will review this when I wake up

KolbyML

Here is some feedback

KolbyML · 2025-10-28T07:20:40Z

crates/common/consensus/lean/src/attestation.rs

+/// TODO: Add link from spec once finalized.
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash)]
+pub struct AttestationData {
+    pub slot: u64,
+    pub head: Checkpoint,
+    pub target: Checkpoint,
+    pub source: Checkpoint,
+}
+
+/// Validator specific attestation wrapping shared attestation data.
+///
+/// TODO: Add link from spec once finalized.
+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash)]
+pub struct Attestation {
+    pub validator_id: u64,
+    pub data: AttestationData,
+}
+
+impl Attestation {
+    // Return the attested slot.
+    pub fn slot(&self) -> u64 {
+        self.data.slot
+    }
+
+    // Return the attested head checkpoint.
+    pub fn head(&self) -> Checkpoint {
+        self.data.head
+    }
+
+    // Return the attested target checkpoint.
+    pub fn target(&self) -> Checkpoint {
+        self.data.target
+    }
+
+    // Return the attested source checkpoint.
+    pub fn source(&self) -> Checkpoint {
+        self.data.source
+    }
+}
+
+/// Validator attestation bundled with its signature.
+///
+/// TODO: Add link from spec once finalized.


I don't see the value in linking the spec here, this isn't a niche complex example, so I think this is an anti pattern

KolbyML · 2025-10-28T07:21:01Z

crates/common/consensus/lean/src/attestation.rs

+    // Return the attested slot.
+    pub fn slot(&self) -> u64 {
+        self.data.slot
+    }
+
+    // Return the attested head checkpoint.
+    pub fn head(&self) -> Checkpoint {
+        self.data.head
+    }
+
+    // Return the attested target checkpoint.
+    pub fn target(&self) -> Checkpoint {
+        self.data.target
+    }
+
+    // Return the attested source checkpoint.


these should be doc comments ///

KolbyML · 2025-10-28T07:22:21Z

crates/common/consensus/lean/src/attestation.rs

+#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Encode, Decode, TreeHash)]
+pub struct SignedAttestation {
+    pub message: Attestation,
+    // signature over attestaion message only as it would be aggregated later in attestation


this should be a doc comment

KolbyML · 2025-10-28T07:23:28Z

crates/common/consensus/lean/src/block.rs

+    // Diverged from current ongoing spec change. This should be
+    // `VariableList<ValidatorAttestation, U4096>` but since we are yet to implement
+    // `SignedBlockAndVote`(these are still being reviewed).


this should be a doc comment, and should have a todo tag and link to the PR in question

or issue etc, this is a todo which isn't clearly marked

varun-doshi changed the title ~~Varun/lean attestation~~ feat: Update containers for Vote and Attestation Oct 16, 2025

varun-doshi marked this pull request as ready for review October 16, 2025 09:35

varun-doshi requested review from Kayden-ML, KolbyML, syjn99 and unnawut as code owners October 16, 2025 09:35

KolbyML added the Blocked label Oct 16, 2025

KolbyML requested changes Oct 16, 2025

View reviewed changes

syjn99 removed the Blocked label Oct 22, 2025

varun-doshi mentioned this pull request Oct 22, 2025

Update containers for SignedBlock and Block #856

Open

varun-doshi force-pushed the varun/lean-attestation branch from cb70798 to 61082d7 Compare October 22, 2025 15:27

syjn99 reviewed Oct 22, 2025

View reviewed changes

crates/common/consensus/lean/src/vote.rs Outdated Show resolved Hide resolved

varun-doshi requested a review from syjn99 October 22, 2025 16:08

jihoonsong reviewed Oct 22, 2025

View reviewed changes

syjn99 reviewed Oct 22, 2025

View reviewed changes

syjn99 approved these changes Oct 22, 2025

View reviewed changes

varun-doshi enabled auto-merge October 23, 2025 03:23

jihoonsong approved these changes Oct 23, 2025

View reviewed changes

varun-doshi requested a review from KolbyML October 25, 2025 08:07

KolbyML approved these changes Oct 28, 2025

View reviewed changes

varun-doshi added this pull request to the merge queue Oct 28, 2025

KolbyML removed this pull request from the merge queue due to a manual request Oct 28, 2025

varun-doshi added 4 commits October 28, 2025 19:00

feat: ValidatorAttestation and Attestation

8f07ba9

fix: use new containers in the codebase

4cac5c1

fix: doc comments

b516e91

fix: fmt

4280b6c

varun-doshi added 7 commits October 28, 2025 19:05

fix: update attestation nomenclature

52072d3

fix: typo

5e5a06c

fix: remove Vote from lean codebase

0cbd3d9

fix: rename gossip topic to Attestation

069035f

fix: AggregatedAttestation -> AggregatedAttestations

8ef3e9b

fix: minor nits

7a50978

fix: doc comments

1e662d6

varun-doshi force-pushed the varun/lean-attestation branch from 0d2235b to 1e662d6 Compare October 28, 2025 13:39

fix: make clippy happy

2508030

varun-doshi added this pull request to the merge queue Oct 28, 2025

Merged via the queue into ReamLabs:master with commit fc63bed Oct 28, 2025
15 checks passed

	signed_votes: impl IntoIterator<Item = SignedAttestation>,
	signed_attestations: impl IntoIterator<Item = SignedAttestation>,

	.get(vote.source().slot as usize)
	.get(attestation.source().slot as usize)

		for signed_attestations in attestations {
		let attestations = &signed_attestations.message;

feat: Update containers for Vote and Attestation #855

feat: Update containers for Vote and Attestation #855

Uh oh!

Conversation

varun-doshi commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong?

How was it fixed?

To-Do

Uh oh!

jihoonsong commented Oct 16, 2025

Uh oh!

KolbyML commented Oct 16, 2025

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 Oct 22, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jihoonsong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syjn99 Oct 22, 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!

syjn99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonsong left a comment

Choose a reason for hiding this comment

Uh oh!

KolbyML commented Oct 25, 2025

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

varun-doshi commented Oct 16, 2025 •

edited

Loading

syjn99 Oct 22, 2025 •

edited

Loading

syjn99 Oct 22, 2025 •

edited

Loading