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

Add 2 prefixes, and define them all #1778

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mikedilger
Copy link
Contributor

Here I'm proposing 2 new prefixes:

deleted - for EVENT, when the relay rejects the event because it has been deleted
redacted - for CLOSED, when some matching events were not served due to the requester not being authenticated or authorized.

I'm also defining what the prefixes mean, and what contexts they are used in (either EVENT or CLOSED or both).

@vitorpamplona
Copy link
Collaborator

I like this

Copy link
Collaborator

@Semisol Semisol left a comment

Choose a reason for hiding this comment

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

redacted has the side effect of closing the subscription, meaning new events aren't received if the client wishes to do nothing.

If the client acts on redacted, it should reopen the REQ.

@staab
Copy link
Member

staab commented Feb 11, 2025

redacted has the side effect of closing the subscription, meaning new events aren't received if the client wishes to do nothing.

In this case I think a notice would make more sense. Redacted is going to be very common, it would be annoying to have subscriptions start flapping. I like deleted, since clients can follow up by requesting the delete to verify, remove the deleted event from their local database, and maybe pass the delete event along to the relay that served the deleted event.

@Semisol
Copy link
Collaborator

Semisol commented Feb 11, 2025

In this case I think a notice would make more sense. Redacted is going to be very common, it would be annoying to have subscriptions start flapping.

We may need better ways of communicating information to clients in general. NOTICE is too general.

I like deleted, since clients can follow up by requesting the delete to verify, remove the deleted event from their local database, and maybe pass the delete event along to the relay that served the deleted event.

Deleted is nice, agreed.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 11, 2025

NOTICE is too general.

Kill notice. It is useless (only good for debugging purposes)

@Semisol
Copy link
Collaborator

Semisol commented Feb 11, 2025

Kill notice. It is useless (only good for debugging purposes)

And that is exactly why it is useful. If a relay has issues, or a client is hitting issues with a relay (malformed query, specific rate-limits, etc), additional details can be sent in a NOTICE.

@mikedilger
Copy link
Contributor Author

mikedilger commented Feb 11, 2025

redacted has the side effect of closing the subscription, meaning new events aren't received if the client wishes to do nothing.

If the client acts on redacted, it should reopen the REQ.

I was considering adding a <message> to EOSE just for this case.

@mikedilger
Copy link
Contributor Author

redacted has the side effect of closing the subscription, meaning new events aren't received if the client wishes to do nothing.

In this case I think a notice would make more sense. Redacted is going to be very common, it would be annoying to have subscriptions start flapping. I like deleted, since clients can follow up by requesting the delete to verify, remove the deleted event from their local database, and maybe pass the delete event along to the relay that served the deleted event.

I'm a bit worried that clients who haven't updated to this PR (it always takes time for that to happen) won't understand redacted and may enter error handling code, either because they think the subscription errored or because they can't parse the machine-readable prefix. And what of it? Would they try again without comprehension? Or can we assume they will ignore what they don't understand?

I think redacted gives the client useful information. I've had multiple occasions where I was baffled that the event wasn't showing up... I was sure it was on the relay.. and it turned out to be redacted for one reason or another - my debugging effort would have been better directed if the relay gave me that hint.

@staab
Copy link
Member

staab commented Feb 11, 2025

Yeah, I agree redacted is useful. But overloading EOSE or CLOSED will cause problems (overloading EOSE would prevent using redacted for ongoing subscriptions). A new REDACTED verb seems like overkill, but I can't think of where else to fit it in if we don't want to use NOTICE.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 12, 2025

["AUTH", "<challenge>", "<some message goes here letting the client or user know that performing auth is not mandatory but will affect the results of their queries>"]

@fiatjaf
Copy link
Member

fiatjaf commented Feb 12, 2025

Also I think having clients send more granular subscriptions instead of trying to mash a bunch of unrelated stuff together in the same subscription because of arbitrary relay limits will go a long way, you can do CLOSED more freely for some subscriptions and fulfill others fully.

@Semisol
Copy link
Collaborator

Semisol commented Feb 12, 2025

["AUTH", "<challenge>", "<some message goes here letting the client or user know that performing auth is not mandatory but will affect the results of their queries>"]

This works.

@mleku
Copy link

mleku commented Feb 12, 2025

NOTICE is too general.

Kill notice. It is useless (only good for debugging purposes)

you can never un-declare a thing in an open protocol, you can only deprecate it

but i'm of the opinion like @Semisol that notice definitely has debugging use case and extremely important especially for people developing relays to be able to enable this kind of information and for it to not be rejected

and i also think that clients should show a notice history in the relay information page, anyway, this would also help with relay debugging

@mleku
Copy link

mleku commented Feb 12, 2025

Also I think having clients send more granular subscriptions instead of trying to mash a bunch of unrelated stuff together in the same subscription because of arbitrary relay limits will go a long way, you can do CLOSED more freely for some subscriptions and fulfill others fully.

i think if you split apart the filter structure and give them different names it would help

  • fetching IDs is exclusive to all the rest, so why allow the whole mess at all
  • search field is mostly not implemented except a few DVMs and such, mostly because laziness, but as a result most clients don't use it anyway... it should be a separate message
  • what remains, pubkeys, tags and timestamps, is a filter

because these things are jumbled together in the spec it is possible to write some really retarded queries, and like, what is the semantics anyway when you have a search term and filter fields? this has to be an AND operation, surely

and the last point i'm gonna make is that results from filters and search queries should not return the whole event but only a list of matching event IDs, this way the client has to handle query state and they can selectively fetch those matches in whatever way suits them, and this eliminates the complexity of specifying a pagination scheme, you can do whatever fits your client's requirements, it could even be based on number of lines that render on a display, for example, which is a different iteration than asking for segments of 10

the entire REQ protocol should be split into these three parts and results as lists of event IDs be available, the easiest way to do this is start a new naming scheme that provides context as well so encoders don't have to be prompted, they just unpack it according to reading the message header - first string field.

@SilberWitch
Copy link
Contributor

I second the ability to get a list of eventIDs as a result. Would make my life a lot easier. Important for OtherStuff implementations.

@staab
Copy link
Member

staab commented Feb 12, 2025

I believe that can be accomplished today with negentropy.

@Semisol
Copy link
Collaborator

Semisol commented Feb 12, 2025

I believe that can be accomplished today with negentropy.

Negentropy is computationally expensive, complicated to implement compared to an IDs-only filter on both the relay and client, and amplifies the effects of network latency.

@Semisol
Copy link
Collaborator

Semisol commented Feb 12, 2025

You also cannot limit the amount of events you scan, which allows DoS, otherwise the client will think you only have those events.

@mikedilger
Copy link
Contributor Author

["AUTH", "<challenge>", "<some message goes here letting the client or user know that performing auth is not mandatory but will affect the results of their queries>"]

This is unnecessary and doesn't distinguish the case when something was actually redacted versus when nothing was.

@Semisol
Copy link
Collaborator

Semisol commented Feb 13, 2025

["AUTH", "<challenge>", "<some message goes here letting the client or user know that performing auth is not mandatory but will affect the results of their queries>"]

This is unnecessary and doesn't distinguish the case when something was actually redacted versus when nothing was.

If any form of redaction is possible, the client should AUTH immediately, unless it is trying to access public content. Also, redacted gives you a side channel on REQs that had filtered events, and allowing you to narrow it down with more filters.

Public content in this context is any content that is trying to be read from a user's write relays in an outbox context, and things relating to that, such as reactions on read relays.

Unless it is explicitly signaled that certain content is expected to be public (such as listed as a write relay in outbox), it should be treated as possibly-AUTH-gated.

Also, I do not understand why AUTH is such a big concern:

  • For your own read and write relays, you already lack any privacy, and should AUTH immediately.
  • For outbox, you should be AUTHing immediately if you are trying to send DM events or similar restricted event types. Also rate-limits for gift wraps.
  • For most outbox reads, you do not need to AUTH.
  • If you get a rate-limit, you could prompt for AUTH.

There is only one case where you may want to prompt for it.

@mleku
Copy link

mleku commented Feb 18, 2025

Also I think having clients send more granular subscriptions instead of trying to mash a bunch of unrelated stuff together in the same subscription because of arbitrary relay limits will go a long way, you can do CLOSED more freely for some subscriptions and fulfill others fully.

separating subscription from query would be a good start...

@mikedilger
Copy link
Contributor Author

Today I'm not sure ["CLOSED", "redacted: ..."] is any more useful than ["CLOSED", "auth-required: ..."] and I'm leaning towards closing any subscription as soon as any event needs redacting, rather than giving partial results and silently or confusingly (in a new way that not all clients recognize) explaining such. Clients should all understand ["CLOSED", "auth-required: ..."] so that is safest.

The tangle of problems that arises going the other way is too much. Clients need to learn of 'redacted'. If a subscription stays open, EOSE needs a 'redacted' message too. Etc.

I'll push PR updates after discussion (if any).

@mikedilger
Copy link
Contributor Author

I'm not going to use "redacted". I went ahead and used "auth-required" (after serving the partial results).

I still like "deleted" but not enough to push this PR forward.

@mikedilger mikedilger closed this Feb 20, 2025
@fiatjaf
Copy link
Member

fiatjaf commented Feb 21, 2025

I still think we could use a way to notify clients they're missing events, but I'm not convinced "redacted" is the perfect solution. Or maybe this is too subjective and should be done more manually anyway.

@mikedilger
Copy link
Contributor Author

Ok I'm reopening. @yukibtc suggested something similar nostrability/nostrability#167 (comment)

@mikedilger mikedilger reopened this Feb 22, 2025
@vitorpamplona
Copy link
Collaborator

Question, couldn't we keep the sub open even with redacted? It could mean that new events might also be redacted.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 22, 2025

What about just adding a new message? ["REDACTED", "<subid>", "by not being authenticated you're missing some events"]

@mikedilger
Copy link
Contributor Author

What about just adding a new message? ["REDACTED", "<subid>", "by not being authenticated you're missing some events"]

I think that would be more compatible than adding a prefix to EOSE.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 22, 2025

Frankly, I think redacted should be the default relay behavior. So much that the notice doesn't need to exist. For clients, everything is redacted. We are always at the mercy of the relay. We make do with whatever comes down. The message helps to debug, but that's it.

It would be nice to understand how to up our user's permissions in the relay, if available, like:

["REDACTED", "<subid>", "Follow this key to get everything: npub.. "]
["REDACTED", "<subid>", "Auth to get everything: <challenge> "]
["REDACTED", "<subid>", "Pay this invoice to get everything: lnbc1.. "]

But maybe these types of "upgrades" should be on #901 instead. Because, in the end, it will be the user's action, not the client's.

@mikedilger
Copy link
Contributor Author

Yes, what I care about is knowing if I can get more results by AUTHing, but also if there is some other way I'd want to know that too.

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.

8 participants