-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Punish peers for duplicate GRANDPA neighbor messages #12462
Punish peers for duplicate GRANDPA neighbor messages #12462
Conversation
@dmitry-markin please merge master to make CI happy ;) And also please wait for @andresilva approval. |
// allow duplicate neighbor packets if enough time has passed | ||
peers.inner.get_mut(&id).unwrap().view.last_update = | ||
Some(Instant::now() - NEIGHBOR_REBROADCAST_AFTER); | ||
check_update(&mut peers, update4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this test:
- It modifies internal structure that can change, and the test must be updated in this case.
- It rolls clock backwards: on systems with unsigned time representation and 0 time corresponding to the system boot, if the test is run less than two minutes after boot, it'll fail.
The alternative I see is modifying Peers::update_peer_state()
to accept time instant as an argument, but this will require measuring time on the calling side. This seems to be even worse.
Do you see any better options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to solve this cleanly we'd have to mock the time source but it seems overkill just for this test case. To address 2) we could take neighbor_rebroadcast_period
as a constructor argument to Peers
(and I guess NeighborPacketWorker
as well), and then we could use a shorter interval in the test.
// allow duplicate neighbor packets if enough time has passed | ||
peers.inner.get_mut(&id).unwrap().view.last_update = | ||
Some(Instant::now() - NEIGHBOR_REBROADCAST_AFTER); | ||
check_update(&mut peers, update4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to solve this cleanly we'd have to mock the time source but it seems overkill just for this test case. To address 2) we could take neighbor_rebroadcast_period
as a constructor argument to Peers
(and I guess NeighborPacketWorker
as well), and then we could use a shorter interval in the test.
Co-authored-by: André Silva <[email protected]>
bot merge |
* Decrease peer reputation for duplicate GRANDPA neighbor messages. * Fix comparison * Fix update_peer_state() validity condition * Add negative test * Rework update_peer_state() validity condition, add tests * update_peer_state() validity condition: invert comparison * Split InvalidViewChange and DuplicateNeighborMessage misbehaviors * Enforce rate-limiting of duplicate GRANDPA neighbor packets * Update client/finality-grandpa/src/communication/gossip.rs Co-authored-by: André Silva <[email protected]> * Make rolling clock back in a test safer Co-authored-by: André Silva <[email protected]>
Resolves #12191.
The code already contains logic that doesn't allow sending GRANDPA neighbor messages with the same or decreasing set ID / round within a set / finalized number. This PR only enforces this upon reception and reports misbehaving peers.
Follow-up issue for a generalized case: #12463.