-
Notifications
You must be signed in to change notification settings - Fork 5
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
Reject filters implicitly rather that failing a whole request #13
base: master
Are you sure you want to change the base?
Conversation
The actual issue is with the batching. Why batch? It makes no sense to batch these requests. Allowing multiple filters per request is a bug in the protocol that we should get rid of, but its initial intention was to allow multiple filters that had the same "purpose" to be grouped together (for example, "give me all events p-tagging myself and also those that e-tag my events" so I can display them in my "inbox" screen -- notice I had to come up with an unrealistic example because there are no real use cases for batching). Here you are batching things that belong to entirely different logic parts of the app, for what? Just to overcomplicate your client code? If we are to allow all filters even those that we disallow just because they might be grouped together with other filters that might be allowed that means the |
I can only imagine the reason why you have this unnecessary batching in your code is because strfry has a limit of 20 open subscriptions per connection, but one that you can easily bypass as long as you sweat enough to overcomplicate your client code, this is a sad state of affairs and one that I imagine wasn't intended by anyone. @hoytech can we stop this madness and start limiting subscriptions by number of filters active? If strfry treated each filter in a batched request as a different subscription and then increased the default limit the world would be much saner. @pablof7z then you can stop batching too, please? |
Sorry for the tone in the comment above, I was too sleepy. |
I think this is possible. I will take a look soon. I'm pretty sure strfry can handle a much higher number of filters in most cases. |
Another small benefit of batching is that events that match multiple filters aren't redundantly transferred (at least in strfry). But overall yeah, I think the simplicity of 1 REQ == 1 filter would be preferable. |
I'm onboard with the "don't send multiple filters" change, but I still think this PR is good, because the general principle of not being over-prescriptive is a good one. I ran into another use case that isn't currently covered, which is listening for deletes. You can't listen for deletes on a event id basis, without passing a huge filter and updating it every time an event gets added to the list of things you want to be notified about. You might be able to listen for deletes for a room, e.g. |
Flotilla (and NDK) batch requests, which results in valid filters being rejected along with invalid ones. An example:
This results in flaky loading of valid chat messages. It doesn't seem to follow that nip 29 support is incompatible with support of regular nostr events. Relay level authentication and policy is the correct tool for protecting a relay from spam, not feature support, which should be orthogonal (this isn't a commentary on whether nip 29 relays should accept other relays, but that should be up to the instance, not the implementation).
If a relay only accepts h-tagged events, returning no events for the first two filters is to be expected. Also, in the case that there are events with h tags that match the first two filters, it's reasonable to return only events that the user has access to based on the relay's prerogative to authorize users.
This PR might have many problems with it. I didn't fully understand the logic in NormalEventQuery, but I think my version is clearer (if maybe not quite as performant).