Conversation
| // Validate attestation before processing | ||
| self.validateAttestation(signed_vote) catch |err| { | ||
| self.module_logger.debug("Gossip attestation validation failed: {any}", .{err}); | ||
| return; // Drop invalid gossip attestations |
There was a problem hiding this comment.
add a task to add it to add the head block to be pulled via req/resp if the validation fails because of the head root missing in forkchoice
| // Validate attestation before processing | ||
| self.validateAttestation(signed_vote) catch |e| { | ||
| self.module_logger.err("invalid attestation in block: validator={d} error={any}", .{ signed_vote.validator_id, e }); | ||
| // Skip invalid attestations but continue processing the block |
There was a problem hiding this comment.
add a task to determine and implement what to do if the attestation validation fails for the block attestations
| }; | ||
|
|
||
| const head_idx = self.forkChoice.protoArray.indices.get(vote.head.root) orelse { | ||
| self.module_logger.debug("Attestation validation failed: unknown head block root=0x{s}", .{ |
There was a problem hiding this comment.
check if the spec validates if head is in forkchoice, do a PR in the python spec for this change and put it up for discussion @bomanaps
There was a problem hiding this comment.
| const current_slot = self.forkChoice.fcStore.timeSlots; | ||
| const max_allowed_slot = if (is_from_block) | ||
| current_slot + constants.MAX_FUTURE_SLOT_TOLERANCE // Block attestations: allow +1 | ||
| else | ||
| current_slot; // Gossip attestations: no future slots allowed |
There was a problem hiding this comment.
where is this check in specs? can you share the link
There was a problem hiding this comment.
General check: leanSpec/src/lean_spec/subspecs/forkchoice/store.py#L140
Gossip-specific check: leanSpec/src/lean_spec/subspecs/forkchoice/store.py#L177
There was a problem hiding this comment.
seems incorrect to me (L140 one), raising it with others
| // Block attestations can be more lenient since the block itself was validated. | ||
| const current_slot = self.forkChoice.fcStore.timeSlots; | ||
| const max_allowed_slot = if (is_from_block) | ||
| current_slot + constants.MAX_FUTURE_SLOT_TOLERANCE // Block attestations: allow +1 |
There was a problem hiding this comment.
| current_slot + constants.MAX_FUTURE_SLOT_TOLERANCE // Block attestations: allow +1 | |
| current_slot - constants.MAX_FUTURE_SLOT_TOLERANCE // Block attestations: allow +1 |
this is the correct condition, i.e. an attestation/vote packed in a proposal block at a slot can't be any later than the previous slot
can you also do the PR in the spec to fix this if thats there
There was a problem hiding this comment.
In the spec where have this assert vote.slot <= Slot(current_slot + Slot(1)), "Attestation too far in future" which means it should be assert vote.slot <= Slot(current_slot - Slot(1)), "Attestation from current or future slot" Line https://github.com/leanEthereum/leanSpec/blob/161c0493e980c4f26b70c3f873d0f17b378f6268/src/lean_spec/subspecs/forkchoice/store.py#L178 gossip check would become redundant after this fix (since -1 is stricter than <= current_slot), so should I leave it as a redundant safety check?
There was a problem hiding this comment.
ok lets leave this here right now, @noopur23 create an issue to clarify and if needed patch the attestation validation slot condition
2 participants
Add attestation validation matching leanSpec specification with tests