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

Remove NOTICE relay messages from NIP-01 #1783

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexgleason
Copy link
Member

@alexgleason alexgleason commented Feb 13, 2025

NOTICE is practically useless. I argue it has been superseded by a combination of NIP-11 and relay CLOSED messages that were introduced 2 years ago in #902.

I am implementing a relay. There are 2 beautiful flows of Nostr:

  • Getting events: the client sends a REQ, to which the relay responds to with EVENT, EOSE, and/or CLOSED.
  • Posting events: the client sends an EVENT, to which the relay responds with an OK (which can be true or false).

These are request/response flows, which map very well to async functions in code. But NOTICE exists outside of that - it's a callback with no useful destination.

  • In some legacy relays (Strfry should send CLOSED for invalid messages hoytech/strfry#135), NOTICE was used to respond to subscription errors. However, it's impossible to parse, and therefore useless because it doesn't contain a subscription ID. The correct thing to use today is a CLOSED message, with the subscription ID and reason.

  • In the past, NOTICE may have been useful to tell users certain things about the relay. However, this was superseded by NIP-11, which is a superior way to find out relay information.

  • Relays are not consistent in how they use NOTICE, so it cannot even be trusted by clients to display prominently to the user. Even for display in logs, there is basically nothing useful a NOTICE message could actually say that wouldn't be communicated better with a CLOSED message or through NIP-11.

I've built multiple relay server and client implementations, and none of them have ever supported NOTICE. There is no clear place for it because it's not part of any flow. It's not useful enough to display to users, and not even useful enough to display in logs. It has been totally superseded by far superior solutions. Since it's useless, there's no point in promoting it to people. So let's remove it.

@alexgleason alexgleason requested a review from fiatjaf February 13, 2025 00:43
@vitorpamplona
Copy link
Collaborator

Yes, kill it.

@quentintaranpino
Copy link

I disagree that NOTICE is useless. While most Nostr flows are request/response, there are cases where a relay needs to notify a client asynchronously. Payment-based relays could be a good example. If a user’s balance runs out, the relay needs a way to inform them, and NOTICE is the natural choice. CLOSED isn’t meant for this, and NIP-11 is static, not real-time.

The issue isn’t that NOTICE has no purpose, but that its use is inconsistent. Instead of removing it, we should refine its definition to make it more reliable. Eliminating it would only make communication between relays and clients harder. IMHO.

@alexgleason
Copy link
Member Author

Payment based relays should use CLOSED.

@quentintaranpino
Copy link

I agree that a payment-based relay should use CLOSE when there is no remaining balance, but how do notify the client beforehand?

I still think we could structure NOTICE without breaking backward compatibility. Ultimately, it would achieve the same purpose you’re proposing, since NOTICE without a homogeneous behavior across relays is not very useful. However, I wouldn’t deprecate it, no doubt.

@vitorpamplona
Copy link
Collaborator

Use Notify instead. #901

So we can display your message to the user and they can either pay or remove your relay from their relay list.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 13, 2025

NOTICE is very good for debugging and development and they help me all the time.

I still think its messages should be displayed to users directly. Perhaps not in their faces, but somewhere.

In any case it's just a way for the relay to tell the developer something or anything. You don't have to use them, apps can ignore them if they wish, but we shouldn't remove them.

@vitorpamplona
Copy link
Collaborator

In any case it's just a way for the relay to tell the developer something or anything.

If the goal is to debug, we need to make that clear. And then Relays need to overuse it.

Right now we do display them to users in the relay info screen. But it is just a bunch of junk. There is no point in showing any of the current messages to users.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 13, 2025

Also, the messages are way too short/missing details and don't point to the action or request that triggered them. We don't even know which sub the notice is referring to.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 13, 2025

For instance, many relays send "error: too fast, slow down please" without actually saying which sub I am going too fast, or what are the rate limits for each action in the relay, before and after AUTH. I have no actionable information.

Others will say: "unexpected character in from_hex 118"... as if I knew what are they even talking about. I don't even know which file in their software is that 118 line referring to much less which event triggered that.

Then the only other two notices I get are "Failed to receive" because lack of payment and WOT. Which are fine, but also irrelevant because I don't have an invoice for the user to pay and I can't add WOT to the user.

I can't find any other NOTICE in over 2 days using Amethyst.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 13, 2025

Also, the messages are way too short/missing details and don't point to the action or request that triggered them. We don't even know which sub the notice is referring to.

Depending on what you're talking about this is not a protocol issue, this is relays being clueless.

For instance, many relays send "error: too fast, slow down please" without actually saying which sub I am going too fast, or what are the rate limits for each action in the relay, before and after AUTH. I have no actionable information.

If you're sending too many subscriptions and being rate-limited because of that I don't see how the relay could say something different. The problem isn't in a specific subscription. Maybe you want the relay to say how many subscriptions you're sending and how many you're allowed to send per minute or something like that? That's something that can be improved at the specific relay codebase, but also some relays may not want to reveal that to an attacker.

Others will say: "unexpected character in from_hex 118"... as if I knew what are they even talking about. I don't even know which file in their software is that 118 line referring to much less which event triggered that.

This is something that should be moved to a CLOSED or OK message and a better error message could be used instead, but that's irrelevant here.

Then the only other two notices I get are "Failed to receive" because lack of payment and WOT. Which are fine, but also irrelevant because I don't have an invoice for the user to pay and I can't add WOT to the user.

Same for these two.

I can't find any other NOTICE in over 2 days using Amethyst.

Great, that means things are working as expected? Maybe once we get to a point in which relays only return NOTICEs for very rare relevant human-readable information we can more decently display those to users.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 13, 2025

not a protocol issue, this is relays being clueless.

No. It's lack of guidance from the protocol. No one knows what NOTICE is for and how to use it. It's way too abstract.

This is something that should be moved to a CLOSED or OK.... Same for these two.

Great. You just validated the point that NOTICE should not exist (or at least, it should not be a mandatory requirement of implementing Nostr) and the messages should be moved to places where they can add more actionable information.

@alexgleason
Copy link
Member Author

NOTICE is very good for debugging and development and they help me all the time.

Can you give an example of how you use it?

@mikedilger
Copy link
Contributor

mikedilger commented Feb 13, 2025

I don't think anybody here has made the case that removing NOTICE makes things better. You might argue that it is confusing developers, or that it is confusing users, or pointless as things can be done in a different way. But none of those arguments leads to the conclusion that removing NOTICE makes nostr better. It makes it strictly worse because a client may then ignore them while some relay still expects them to be read.

In general. nostr will have "cruft" (NIP-26 is another example), but cleaning up the "cruft" is not actually beneficial and could be harmful.

The best I would support is deprecating NOTICE message creation on relays, but clients SHOULD still display them to users if they ever see them.

@alexgleason
Copy link
Member Author

I just want to see any examples of a useful NOTICE.

@mikedilger
Copy link
Contributor

Chorus uses it for the following cases:

  • Command unrecognized
  • Rate limit exceeded (handled prior to parsing the command, so not using OK or CLOSED)
  • Unexpected errors which aren't handled in any other way are shown to the user via Notice
  • On receipt of a binary websocket message: "Binary messages are not processed by this relay"

Sure I could put "Binary messages are not processed by this relay" in the NIP-11, but when someone is debugging their client it is far more immediate and useful for them to see that as a message rather than spending hours and then noticing it in the NIP-11.

@alexgleason
Copy link
Member Author

@mikedilger The WebSocket close method supports a code and reason that can be parsed by the client:

https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/close

https://www.rfc-editor.org/rfc/rfc6455.html#section-7.4.1

For the binary issue, you'd want to close with a 1003:

   1003

      1003 indicates that an endpoint is terminating the connection
      because it has received a type of data it cannot accept (e.g., an
      endpoint that understands only text data MAY send this if it
      receives a binary message).

For rate limiting, I would use 1008 with a custom message, like Rate limited, try again in 30 seconds:

   1008

      1008 indicates that an endpoint is terminating the connection
      because it has received a message that violates its policy.  This
      is a generic status code that can be returned when there is no
      other more suitable status code (e.g., 1003 or 1009) or if there
      is a need to hide specific details about the policy.

For "unexpected errors", I would like to see an example that wouldn't be solved by either CLOSED or a WebSocket close code.

"Command unrecognized" is probably the best argument I've heard in favor of NOTICE. Although it's only useful in a CLI - in production NIP-11 would be used.

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.

NOTICE is useful for relay debugging, and for providing more information to clients about why an OK/CLOSED message was returned.

@alexgleason
Copy link
Member Author

Several people say NOTICE is good for debugging, but still no examples. I have never seen it be useful in that way.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 19, 2025

Mike Dilger has provided multiple examples, they're pretty close to what khatru does.
strfry sends notices every time something is broken in the request that prevents it from being parsed too.

@mikedilger
Copy link
Contributor

@alexgleason Thank you, I could use the Close message codes for rate limiting. But not all errors are significant enough to close the connection. Binary messages for example, I'm not going to close I'm just not going to handle them and the notice is a nicety for developers who might wonder why their new binary-nostr client isn't working with chorus.

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.

6 participants