fix: all nodes process attestations#718
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the node’s gossip handling so all nodes import/process incoming attestations (instead of only nodes with aggregation enabled), aligning the chain’s behavior with the spec expectation that on_gossip_attestation is run universally.
Changes:
- Remove the
is_aggregator_enabledgate for processing gossip attestations. - Always run
onGossipAttestationafter validation and record success metrics/logs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.logger.info("processed gossip attestation for slot={d} validator={d}{f}", .{ | ||
| slot, | ||
| validator_id, | ||
| validator_node_name, | ||
| }); |
There was a problem hiding this comment.
processed gossip attestation... is logged at info level for every successfully imported attestation. Now that all nodes will process gossip attestations (not just aggregators), this can dramatically increase log volume and I/O overhead in normal operation. Consider downgrading this to debug (or adding sampling / rate limiting) to keep info logs actionable.
| self.onGossipAttestation(signed_attestation) catch |err| { | ||
| zeam_metrics.metrics.lean_attestations_invalid_total.incr(.{ .source = "gossip" }) catch {}; | ||
| self.logger.err("attestation processing error: {any}", .{err}); | ||
| return err; | ||
| }; | ||
| self.logger.info("processed gossip attestation for slot={d} validator={d}{f}", .{ | ||
| slot, | ||
| validator_id, | ||
| validator_node_name, | ||
| }); | ||
| zeam_metrics.metrics.lean_attestations_valid_total.incr(.{ .source = "gossip" }) catch {}; | ||
| return .{}; |
There was a problem hiding this comment.
This change alters consensus-critical behavior: gossip attestations are now processed regardless of is_aggregator_enabled. There are existing tests for attestation validation/processing, but none exercise the onGossip(.attestation) path with is_aggregator_enabled=false to prevent regressions of the original bug. Please add a unit test that constructs a chain with ChainOpts.is_aggregator=false, feeds a valid GossipMessage.attestation through BeamChain.onGossip, and asserts forkchoice recorded the signature/vote.
g11tech
left a comment
g11tech
left a comment
There was a problem hiding this comment.
sorry a node who is not aggregator should not process/import attestations and even if they are aggregators, they are not supposed to import attestations from the subnets they are not listening to, ideally only use them for aggregation
This is a fix I had to make to make the zeam nodes process attestations as they were not aggregators.
I believe as per service.py in the spec all nodes call on_gossip_attestation.
There still may be some minor difference from spec on the is_aggregator logic.