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

fix: block_event dedupe using linked list #423

Merged
merged 12 commits into from
Nov 14, 2024
Merged

Conversation

rebelArtists
Copy link
Contributor

@rebelArtists rebelArtists commented Nov 7, 2024

Description

  • store linked list of seen block hashes and corresponding timestamp withing p2p conn struct
  • helper funcs to add new hash to linked list and lookup seen hashes
  • automatic TTL deletion of stale hashes

Testing

smoke tests locally (i.e. no go errors)

prod tests w/ datastore also appear successful:
hash=0x49858d9863bd02bebec5805f0902e73342fa0846ca05e9c641292af00bc94a8f peer=enode://360811f7d432cc06f28b54ca09d57a374b94648d8eed6a8364190ec8e0bdbb43402cafda9f579a50a0fbc38448672a81ba1eaf5965728688ed0731993e920ae9@200.69.14.233:32303 2:43PM INF Skipping duplicate block hash hash=0x49858d9863bd02bebec5805f0902e73342fa0846ca05e9c641292af00bc94a8f peer=enode://360811f7d432cc06f28b54ca09d57a374b94648d8eed6a8364190ec8e0bdbb43402cafda9f579a50a0fbc38448672a81ba1eaf5965728688ed0731993e920ae9@200.69.14.233:32303 2:43PM INF Skipping duplicate block hash hash=0x49858d9863bd02bebec5805f0902e73342fa0846ca05e9c641292af00bc94a8f peer=enode://360811f7d432cc06f28b54ca09d57a374b94648d8eed6a8364190ec8e0bdbb43402cafda9f579a50a0fbc38448672a81ba1eaf5965728688ed0731993e920ae9@200.69.14.233:32303

datastore shows impact of deduped msgs by testing count of Key after change:
image

also mem has increased <5% locally after turning on sensor and letting run >10min:
image

if we further refine datastore query, we see only a single record for each unique Hash + PeerId combo:
image

@rebelArtists rebelArtists requested a review from minhd-vu November 7, 2024 14:06
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
p2p/protocol.go Outdated Show resolved Hide resolved
@rebelArtists rebelArtists marked this pull request as ready for review November 12, 2024 15:38
Copy link
Contributor

@minhd-vu minhd-vu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work Dan. Note that, this will reduce block events, but there is an edge case where a peer can send a NewBlock and BlockHashes message. A NewBlock message will also write a block event to Datastore. This shouldn't be a concern because a well behaving peer will never propagate the block hash to our sensor if it's already sent the full block. This is just something to be aware of. See the image below, since we recieve so few NewBlock messages, it should also not be a concern in terms of duplication.

Screenshot 2024-11-12 at 11 50 49 AM

@minhd-vu minhd-vu merged commit 2953761 into main Nov 14, 2024
6 checks passed
@minhd-vu minhd-vu deleted the dan/dedupe_llist_approach branch November 14, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants