Skip to content
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

Merged
merged 3 commits into from
May 24, 2024

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented May 3, 2024

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 cycles

PR

Noticed the issue first but there's another log that gets spammy if the peer is disconnecting here

debug!(self.log, "Ignoring unknown block request"; "block_root" => %block_root, "reason" => reason);

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

@dapplion dapplion force-pushed the debounce-sync-block-unknown branch from fceeed9 to bc5899a Compare May 3, 2024 08:42
@realbigsean
Copy link
Member

Q: Should we revert #5672 now that the log is not spammy?

Yes! down for that

@realbigsean realbigsean added ready-for-review The code is ready for review v5.2.0 Q2 2024 labels May 6, 2024
michaelsproul added a commit that referenced this pull request May 7, 2024
Squashed commit of the following:

commit a050d13
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Tue May 7 11:13:05 2024 +0900

    Re-add dropped comment

commit 729005f
Merge: bc5899a 5135a77
Author: realbigsean <sean@sigmaprime.io>
Date:   Mon May 6 15:39:13 2024 -0400

    Merge branch 'unstable' into debounce-sync-block-unknown

commit bc5899a
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Fri May 3 17:13:20 2024 +0900

    Debounce UnknownBlockHashFromAttestation events
@michaelsproul michaelsproul mentioned this pull request May 7, 2024
Copy link
Member

@AgeManning AgeManning left a 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?

@dapplion
Copy link
Collaborator Author

dapplion commented May 9, 2024

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

michaelsproul added a commit that referenced this pull request May 14, 2024
Squashed commit of the following:

commit a050d13
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Tue May 7 11:13:05 2024 +0900

    Re-add dropped comment

commit 729005f
Merge: bc5899a 5135a77
Author: realbigsean <sean@sigmaprime.io>
Date:   Mon May 6 15:39:13 2024 -0400

    Merge branch 'unstable' into debounce-sync-block-unknown

commit bc5899a
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Fri May 3 17:13:20 2024 +0900

    Debounce UnknownBlockHashFromAttestation events
michaelsproul added a commit that referenced this pull request May 16, 2024
Squashed commit of the following:

commit a050d13
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Tue May 7 11:13:05 2024 +0900

    Re-add dropped comment

commit 729005f
Merge: bc5899a 5135a77
Author: realbigsean <sean@sigmaprime.io>
Date:   Mon May 6 15:39:13 2024 -0400

    Merge branch 'unstable' into debounce-sync-block-unknown

commit bc5899a
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Fri May 3 17:13:20 2024 +0900

    Debounce UnknownBlockHashFromAttestation events
michaelsproul added a commit that referenced this pull request May 23, 2024
Squashed commit of the following:

commit a050d13
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Tue May 7 11:13:05 2024 +0900

    Re-add dropped comment

commit 729005f
Merge: bc5899a 5135a77
Author: realbigsean <sean@sigmaprime.io>
Date:   Mon May 6 15:39:13 2024 -0400

    Merge branch 'unstable' into debounce-sync-block-unknown

commit bc5899a
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Fri May 3 17:13:20 2024 +0900

    Debounce UnknownBlockHashFromAttestation events
michaelsproul added a commit that referenced this pull request May 24, 2024
Squashed commit of the following:

commit a050d13
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Tue May 7 11:13:05 2024 +0900

    Re-add dropped comment

commit 729005f
Merge: bc5899a 5135a77
Author: realbigsean <sean@sigmaprime.io>
Date:   Mon May 6 15:39:13 2024 -0400

    Merge branch 'unstable' into debounce-sync-block-unknown

commit bc5899a
Author: dapplion <35266934+dapplion@users.noreply.github.com>
Date:   Fri May 3 17:13:20 2024 +0900

    Debounce UnknownBlockHashFromAttestation events
@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented May 24, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a1271bc

mergify bot added a commit that referenced this pull request May 24, 2024
@mergify mergify bot merged commit a1271bc into sigp:unstable May 24, 2024
27 checks passed
@dapplion dapplion deleted the debounce-sync-block-unknown branch May 24, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review v5.2.0 Q2 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants