FIX: Critical Filter Logic Bugs Preventing Event Propagation - #37
Open
PatMulligan wants to merge 3 commits into
Open
FIX: Critical Filter Logic Bugs Preventing Event Propagation#37PatMulligan wants to merge 3 commits into
PatMulligan wants to merge 3 commits into
Conversation
- Fix inverted logic in _can_add_filter() method that was preventing new subscription filters from being added - Fix inverted condition check when validating filter addition capacity - Fix REQ message handling to properly clear existing filters before adding new ones - Add debug logging to track filter matching and broadcast failures These bugs were causing customer order events (NIP-04/NIP-15) to be received by the relay but not forwarded to nostrclient/nostrmarket, requiring server restarts or manual refresh to process orders. The fix ensures proper event propagation: Customer → Relay → nostrclient → nostrmarket → Invoice. Root cause: The _can_add_filter() method returned true when filters >= max instead of when filters < max, and the validation check used the wrong conditional, effectively blocking all new filter subscriptions after initial connection. Additionally, REQ messages weren't clearing existing filters, causing state corruption.
- Added info logging when an event does not match the subscription filter, improving traceability of event handling. - Introduced warning logging for cases where no broadcast_event callback is available, ensuring better error visibility during event broadcasting.
Collaborator
Author
|
FYI |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
I used AI to help create this and have read it over carefully and confirmed problem and solution ! 👌🫡🤖
Problem
During end-to-end testing of the nostrmarket checkout flow, it was discovered that customer order events (NIP-15) were being received by the nostrrelay but not propagating to nostrclient and subsequently to nostrmarket. This resulted in:
Root Cause Analysis & Solution
1. Inverted Filter Addition Logic
The
_can_add_filter()method had inverted logic. This method should returnTruewhen a filter can be added, but was instead returningTruewhen the filter limit was already exceeded:def _can_add_filter(self) -> bool: return ( - self.config.max_client_filters != 0 - and len(self.filters) >= self.config.max_client_filters # Wrong: >= should be < + self.config.max_client_filters == 0 # No limit + or len(self.filters) < self.config.max_client_filters # Under limit )2. Inverted Validation Check
The validation in
_handle_request()was also inverted, causing the error message to be shown when filters could be added, and allowing filters when they shouldn't be added:3. Improper REQ Message Handling
The REQ message handler wasn't clearing existing filters before adding new ones, causing filter state corruption when clients sent multiple subscriptions with the same ID:
if message_type == "REQ": subscription_id = data[1] + # First remove existing filters for this subscription_id + self._remove_filter(subscription_id) # Then process new filters for filter_data in data[2:]: response = await self._handle_request(subscription_id, NostrFilter.parse_obj(filter_data))4. Added Debug Logging (feel free to strike the commit for these if undesirable!)
Added logging to help diagnose future filter matching issues:
async def notify_event(self, event: NostrEvent) -> bool: for nostr_filter in self.filters: if nostr_filter.matches(event): resp = event.serialize_response(nostr_filter.subscription_id) await self._send_msg(resp) return True + else: + logger.info(f"[NOSTRRELAY CLIENT] ❌ Filter didn't match for event {event.id}") return False async def _broadcast_event(self, e: NostrEvent): if self.broadcast_event: await self.broadcast_event(self, e) + else: + logger.warning(f"[NOSTRRELAY CLIENT] ❌ No broadcast_event callback available for event {e.id}")Evidence from Logs
❌ Before Fix: Customer Orders Stuck at Relay (16:28:54)
A customer order (NIP-04 encrypted message, kind 4) arrives at the relay but fails to propagate:
Result: The event never reaches nostrclient or nostrmarket. No invoice is generated. The customer sees no response.
After Server Restart: Same Event Finally Propagates (16:29:49)
After restarting the server, which re-establishes subscriptions with correct filters:
Note: This is the exact same event ID (
291bb282c3...) that failed at 16:28:54, now successfully processed after server restart.✅ After Fix: Customer Orders Process Automatically (16:38:34)
With the filter logic fixed, new customer orders now flow seamlessly through the entire chain:
Result: Complete automated flow: Customer Order → Relay → nostrclient → nostrmarket → Invoice Generation → Response to Customer
Impact
Before Fix
After Fix
Testing Verification
The fix was verified through end-to-end testing:
As shown in the logs at timestamp
23:40:43.70:Files Changed
relay/client_connection.py: Fixed filter logic and REQ message handlingRelated Issues
Backwards Compatibility
These changes maintain full backwards compatibility with existing clients while fixing the critical bugs that prevented proper operation.