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(apps/hermes): fix duplicate events for processed slots #2474

Merged
merged 10 commits into from
Mar 19, 2025

Conversation

cctdaniel
Copy link
Contributor

@cctdaniel cctdaniel commented Mar 12, 2025

Fix Duplicate Events for Processed Slots

Problem

The application was experiencing an issue where multiple events could be sent for the same slot under certain conditions:

  1. When a slot was processed out of order (e.g., slot 100, then 101, then 100 again), the system would send both a New event and an OutOfOrder event for the same slot.
  2. During concurrent processing of updates for the same slot, race conditions could lead to duplicate events being sent.

This resulted in downstream consumers (like SSE clients) receiving redundant events, which could cause unnecessary processing and potential bugs.

Solution

We implemented a more efficient mechanism to ensure that only one event is ever sent per slot:

  1. Modified store_wormhole_merkle_verified_message to check if a state already exists for the given slot before storing it, returning a boolean to indicate whether the state was newly stored.

  2. Modified store_update to check if accumulator messages already exist for a slot before storing them, returning early if they do.

This approach leverages the existing cache mechanisms (which already have size limits) to determine if a slot has been processed before. When either a VAA or accumulator message for a slot is received again, we detect this early in the processing pipeline and return immediately, preventing duplicate processing and event generation.

This solution is more memory-efficient as it eliminates the need for an unbounded tracking set while still ensuring that each slot will only generate a single event, either New or OutOfOrder, but never both.

Testing

Two tests were added to verify the fix:

  1. test_out_of_order_updates_send_single_event_per_slot: Verifies that when processing slots out of order (100 > 101 > 100), only one event is sent for each slot.
  2. test_concurrent_updates_same_slot_sends_single_event: Verifies that when 100 concurrent updates for the same slot are processed, only one event is sent.

Both tests confirm that our solution effectively prevents duplicate events while maintaining the correct application state.

Copy link

vercel bot commented Mar 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2025 0:38am
component-library ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2025 0:38am
entropy-debugger ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2025 0:38am
insights ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2025 0:38am
proposals ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2025 0:38am
staking ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 19, 2025 0:38am

@cctdaniel cctdaniel changed the title fix(apps/hermes): fix race condition that allows duplicate slots fix(apps/hermes): fix duplicate events for processed slots Mar 12, 2025
Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

my recommendation is that when you call store_wormhole_merkle_verified_message (and the other one) you return something like it was there or not, and if it was there you return immediately. Those states also have a limited size to make sure they don't grow much.

Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

Nice! can you bump the version before merging?

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