-
Notifications
You must be signed in to change notification settings - Fork 784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Debounce UnknownBlockHashFromAttestation events #5706
Conversation
fceeed9
to
bc5899a
Compare
Yes! down for that |
Squashed commit of the following: commit a050d13 Author: dapplion <[email protected]> Date: Tue May 7 11:13:05 2024 +0900 Re-add dropped comment commit 729005f Merge: bc5899a 5135a77 Author: realbigsean <[email protected]> Date: Mon May 6 15:39:13 2024 -0400 Merge branch 'unstable' into debounce-sync-block-unknown commit bc5899a Author: dapplion <[email protected]> Date: Fri May 3 17:13:20 2024 +0900 Debounce UnknownBlockHashFromAttestation events
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.
Looks good to me.
The reason we are using the (peer_id, root) tuple is because we want distinct peers to send us the same root to add them to current lookups right?
Yes, if a second peer claims to know about a root of an existing lookup it will be added to the pool of "peers that claim to know the block". If the initial request of the lookup fails, that second peer may be used as retry. It's also important for scoring. If the block ends up being invalid we want to penalize everyone that claims to have imported it |
Squashed commit of the following: commit a050d13 Author: dapplion <[email protected]> Date: Tue May 7 11:13:05 2024 +0900 Re-add dropped comment commit 729005f Merge: bc5899a 5135a77 Author: realbigsean <[email protected]> Date: Mon May 6 15:39:13 2024 -0400 Merge branch 'unstable' into debounce-sync-block-unknown commit bc5899a Author: dapplion <[email protected]> Date: Fri May 3 17:13:20 2024 +0900 Debounce UnknownBlockHashFromAttestation events
Squashed commit of the following: commit a050d13 Author: dapplion <[email protected]> Date: Tue May 7 11:13:05 2024 +0900 Re-add dropped comment commit 729005f Merge: bc5899a 5135a77 Author: realbigsean <[email protected]> Date: Mon May 6 15:39:13 2024 -0400 Merge branch 'unstable' into debounce-sync-block-unknown commit bc5899a Author: dapplion <[email protected]> Date: Fri May 3 17:13:20 2024 +0900 Debounce UnknownBlockHashFromAttestation events
Squashed commit of the following: commit a050d13 Author: dapplion <[email protected]> Date: Tue May 7 11:13:05 2024 +0900 Re-add dropped comment commit 729005f Merge: bc5899a 5135a77 Author: realbigsean <[email protected]> Date: Mon May 6 15:39:13 2024 -0400 Merge branch 'unstable' into debounce-sync-block-unknown commit bc5899a Author: dapplion <[email protected]> Date: Fri May 3 17:13:20 2024 +0900 Debounce UnknownBlockHashFromAttestation events
Squashed commit of the following: commit a050d13 Author: dapplion <[email protected]> Date: Tue May 7 11:13:05 2024 +0900 Re-add dropped comment commit 729005f Merge: bc5899a 5135a77 Author: realbigsean <[email protected]> Date: Mon May 6 15:39:13 2024 -0400 Merge branch 'unstable' into debounce-sync-block-unknown commit bc5899a Author: dapplion <[email protected]> Date: Fri May 3 17:13:20 2024 +0900 Debounce UnknownBlockHashFromAttestation events
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at a1271bc |
Issue Addressed
debounce duplicated
UnknownBlockHashFromAttestation
for the same root peer tuple. A peer may forward us thousands of attestations, each triggering an individual event. Only one event is useful, the rest generating log noise and wasted cyclesPR
Noticed the issue first but there's another log that gets spammy if the peer is disconnecting here
lighthouse/beacon_node/network/src/sync/manager.rs
Line 694 in ee974db
I think this fix is more complete as it also reduces CPU cycles downstream, as for every event it wants to find a lookup and insert the peer to it
Proposed Changes
Debounce UnknownBlockHashFromAttestation events