Add gossip validation spec tests for proposer/attester slashings#9323
Add gossip validation spec tests for proposer/attester slashings#9323jimmygchen wants to merge 5 commits into
Conversation
…tor `process_gossip_*_slashing` methods to return `MessageAcceptance` for testability.
jimmygchen
added
test improvement
Validation errors were returning `Ignore` instead of `Reject`, which is incorrect per the spec. Internal errors still return `Ignore`.
jimmygchen
force-pushed
the
ef-tests-gossip
branch
from
a1a58a3 to
921c1a5
Compare
jimmygchen
added
ready-for-review
| /// `MessageAcceptance` doesn't implement clone so we do a manual match here. | ||
| fn clone_message_acceptance(a: &MessageAcceptance) -> MessageAcceptance { | ||
| match a { | ||
| MessageAcceptance::Accept => MessageAcceptance::Accept, | ||
| MessageAcceptance::Reject => MessageAcceptance::Reject, | ||
| MessageAcceptance::Ignore => MessageAcceptance::Ignore, | ||
| } | ||
| } |
There was a problem hiding this comment.
Oof. I opened libp2p/rust-libp2p#6445, let's add a comment here to remove this fn once that is available.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| enum ExpectedOutcome { | ||
| Valid, | ||
| Ignore, | ||
| Reject, | ||
| } | ||
|
|
||
| impl PartialEq<MessageAcceptance> for ExpectedOutcome { | ||
| fn eq(&self, other: &MessageAcceptance) -> bool { | ||
| matches!( | ||
| (self, other), | ||
| (Self::Valid, MessageAcceptance::Accept) | ||
| | (Self::Ignore, MessageAcceptance::Ignore) | ||
| | (Self::Reject, MessageAcceptance::Reject) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Once libp2p/rust-libp2p#6445 is merged, we might use MessageAcceptance directly, with a serde(deserialize_with = ...) attribute. Though that is a matter of taste, so no objection to just keeping this.
There was a problem hiding this comment.
yeah i agree, the impact is pretty minimal - I would just keep it as it is, unless the Valid / Accept mismatch gets standardized.
| // Penalize peer slightly for invalids. | ||
| self.gossip_penalize_peer( | ||
| peer_id, | ||
| PeerAction::HighToleranceError, | ||
| "invalid_gossip_proposer_slashing", | ||
| ); |
There was a problem hiding this comment.
Is it intentional that this now only happens on ProposerSlashingValidationError?
My intuition would actually keep or remove a HighToleranceError for the ignore case but upgrade to a LowToleranceError for the reject case.
Dito for attestatiion slashings.
There was a problem hiding this comment.
My original intention was to not change any scoring logic in this PR unless it's not compliant with the spec. However, it doesn't really make sense to penalize peer if the node fails to load the head state at current slot (peer has nothing to do with it) - so I removed that but maintain the current HighToleranceError for potential invalids.
verify_proposer_slashing_for_gossip can return error from one of these sources:
BeaconChainErrorfromwall_clock_state(): Error loading head state at current slot - this is an internal error and can't be triggered by peer, so it doesn't make sense to penalize peersProposerSlashingValidationErrorare mostly caused by peers, how could also be internal errors likeBeaconStateError- we could try to categorise them but it gives us little gain - slashing processing is pretty low priority in the beacon processor queue so they just get dropped if malicious peers try to spam us with invalids, therefore I think the current high tolerance makes sense.
There was a problem hiding this comment.
FYI the penalty was originally introduced here a long while ago: #1602
2 participants
Issue Addressed
Addresses #9232 partially. This PR covers two topics only.
Wires up networking test vectors for
gossip_proposer_slashingandgossip_attester_slashingtopics.The tests also revealed minor spec non-compliance where invalid slashings were ignored rather than rejected.
Proposed Changes
process_gossip_proposer_slashingandprocess_gossip_attester_slashingto returnMessageAcceptance, so it can be verified in the testsGossipValidationtest case, handler, and test entriesRejectwhen the slashing is invalid and only penalise on invalid messagesAI Assistance Disclosure
Tools used (required — write
noneif no AI was used): claude for the test boilerplate. Rewrote most of the production changes manually.Attestation (required):