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 NIP 45 COUNT #842

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

staab
Copy link
Member

@staab staab commented Oct 26, 2023

  • No non-defunct relay implementations that I'm aware of
  • No active client implementations that I'm aware of (coracle has some code but doesn't use it)
  • There's no way to accurately reconcile counts between multiple relays
  • Increases complexity of relay implementations
  • Better handled outside relays (via centralized service or DVM)
  • I was the original author of COUNT

Copy link
Member

@cameri cameri left a comment

Choose a reason for hiding this comment

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

Thank you 😌

@cameri
Copy link
Member

cameri commented Oct 26, 2023

Should it be removed or just marked as deprecated?

@staab
Copy link
Member Author

staab commented Oct 26, 2023

We could deprecate it, but I honestly think adoption is so low no one will care. Removing it from the spec won't break anyone's implementation

@vitorpamplona
Copy link
Collaborator

Ack.

@staab
Copy link
Member Author

staab commented Oct 26, 2023

Ok, marked this as deprecated. We can remove it entirely in a few months.

@alexgleason
Copy link
Member

There's no way to accurately reconcile counts between multiple relays

Yes there is. You use a gossip table to find the best relay for the author of an event, and then take the count from that relay.

@cameri
Copy link
Member

cameri commented Oct 26, 2023

There's no way to accurately reconcile counts between multiple relays

Yes there is. You use a gossip table to find the best relay for the author of an event, and then take the count from that relay.

@alexgleason this gossip table stuff, is it part of the Nostr protocol?

@staab
Copy link
Member Author

staab commented Oct 26, 2023

Yes there is. You use a gossip table to find the best relay for the author of an event, and then take the count from that relay.

This only solves the subset of cases where you're filtering on a single pubkey. I'm currently working on a proof of concept of count implemented via DVMs which I think is much more promising.

@alexgleason
Copy link
Member

@staab In theory when clients like a post or reply to it, they should ensure the event gets sent to the post author's best relay so they will get a notification. Therefore taking the count from the post authors relay is the best general way to do it. It's not a subset of cases. For each event in a feed you grab the author's best relay to grab the counts for that event.

@pablof7z
Copy link
Member

ACK. This is good.

👏

@fiatjaf
Copy link
Member

fiatjaf commented Oct 26, 2023

I didn't have much against this NIP, but I'm all for deleting stuff.

@arthurfranca
Copy link
Contributor

arthurfranca commented Oct 26, 2023

@staab There's no way to accurately reconcile counts between multiple relays

@alexgleason Yes there is. You use a gossip table to find the best relay for the author of an event, and then take the count from that relay.

I coded NIP-45 on my client for this reason. It is much better than requesting a bunch of events just to count them client-side (reactions for example). We just need more relays implementing it.

@arthurfranca
Copy link
Contributor

@alexgleason this gossip table stuff, is it part of the Nostr protocol?

@cameri its NIP-65. The event author's "read" relays are supposed to receive all event reactions, for example, as far as I know.

@staab
Copy link
Member Author

staab commented Oct 26, 2023

I coded NIP-45 on my client for this reason. It is much better than requesting a bunch of events just to count them client-side (reactions for example). We just need more relays implementing it.

DVMs fixes this. I'm working on a talk for nostrasia that covers this, I'll release it as a blog post as well. Feature bloat on relays is very bad, and also not going to happen anyway because relays need to be interoperable. Features need a high level of adoption in order to be usable by clients.

@cameri
Copy link
Member

cameri commented Oct 26, 2023

@alexgleason this gossip table stuff, is it part of the Nostr protocol?

@cameri its NIP-65. The event author's "read" relays are supposed to receive all event reactions, for example, as far as I know.

Thx @arthurfranca !

@arthurfranca
Copy link
Contributor

@staab I don't agree on this one. Count for SQL databases for example is very easily achievable. My client code checks NIP-11 to know if the relay supports COUNT first, if it doesn't, ok let's see what else can I use.

By deleting NIP-45 it seems you are creating a problem just to later sell the solution (DVM sats-eaters ^^)

@vitorpamplona
Copy link
Collaborator

My client code checks NIP-11 to know if the relay supports COUNT first, if it doesn't, ok let's see what else can I use.

Which relays support COUNT?

@arthurfranca
Copy link
Contributor

@vitorpamplona wss://relay.nostr.band does

Recently I opened issues on strfry, nostream and nostr-rs-relay requesting NIP-45 support.

@weex
Copy link

weex commented Oct 26, 2023

If it's a draft, it probably shouldn't become final on the way to removal. deprecated seems more proper.

That being said, almost everything outside of a few NIPs are drafts and are being implemented. It doesn't set a good precedent to deprecate without at least rough consensus.

If you prefer a version of nostr in which relays are dumb-pipes COUNT adds more intelligence to them and it will be up to the market to decide whether to pay them for it.

@staab
Copy link
Member Author

staab commented Oct 26, 2023

Updated back to draft.

If you prefer a version of nostr in which relays are dumb-pipes COUNT adds more intelligence to them and it will be up to the market to decide whether to pay them for it.

I think the market has decided. I am 50% of the people who have implemented count in the client, and the author of the spec. So I'd say it's failed rather than pending.

@arthurfranca
Copy link
Contributor

arthurfranca commented Oct 26, 2023

Give it time. @hoytech said 3 days ago "This is on my TODO list, I'm hoping to do it for next release."

Damus relay and many others use strfry.
In the past I counted some small relay repos that implemented it although not much.

nostream and nostr-rs-relay use postgresql. the support is just a select count(*) away.

@alexgleason
Copy link
Member

nostream and nostr-rs-relay use postgresql. the support is just a select count(*) away.

Pretty much.

It's implemented in Ditto as well. https://gitlab.com/soapbox-pub/ditto/-/blob/main/src/db/events.ts?ref_type=heads#L181

@arthurfranca
Copy link
Contributor

which relay should the author use to do counts out of their multiple read relays?

Imo counts don't need to be accurate to be useful, so pick the first one and maybe fallback to the second one. Knowing just that there are some reactions and some replies is enough to help user decide if they want to read the full note and its replies.

It is a huge waste of the user mobile data plan to download all note's replies and reactions just to count them client side, for each of all the notes being previewed on feed. The user will never access and read most of these replies. Relay outbound transfer is also a cost to consider.

@fiatjaf
Copy link
Member

fiatjaf commented Nov 1, 2023

I think this can stay and maybe we change the text to make it sure it's not expected that relays have this. In my opinion this should be a service from "aggregator relays" like what we have today for NIP-50 search queries.

@jb55
Copy link
Contributor

jb55 commented Nov 2, 2023 via email

@staab
Copy link
Member Author

staab commented Nov 9, 2023

A DVM kind for count now exists here. You can find my (very basic) implementation here.

@jb55
Copy link
Contributor

jb55 commented Nov 22, 2023

The idea of relying on DVMs for this kind of thing is crazy to me

@staab
Copy link
Member Author

staab commented Nov 22, 2023

The idea of relying on DVMs for this kind of thing is crazy to me

Can you elaborate?

@staab staab merged commit 444ad28 into nostr-protocol:master Jul 30, 2024
@alexgleason
Copy link
Member

My gut reaction to this is NOOOOO, but then I remembered that I had to aggressively rate-limit and timeout COUNT messages to my relay because SQL's count(*) is not even possible without destroying database performance. So even though I implemented it, it's not actually useful.

@arthurfranca
Copy link
Contributor

👎

@cameri
Copy link
Member

cameri commented Jul 31, 2024

👏

@arthurfranca
Copy link
Contributor

No rough consensus paired with refuted arguments and silent PR merge. What a joke!

  • No non-defunct relay implementations? Give it more time. Some relays were listed.
  • No active client implementations? Hard to say. Have you really did a research on this? I myself said I am using it on my client soon to be released
  • There's no way to accurately reconcile counts between multiple relays? Not needed. First, some use cases doen't need accuracy. Also, when outbox is used, count from one of the read relays. When not, count from e.g. one of the chat relays.
  • Increases complexity of relay implementations? This ain't complex. The following is much more complex: ["REQ", <subscription_id>, <filters1>, <filters2>, ...] (multiple filters with distincts limits could instead be multiple subs) - though it is on NIP-01.
  • Better handled outside relays (via centralized service or DVM)? Dumb cause it is slow and a worse third-party to trust rather than the note author's read relay. The author themselves selected it. Downloading individual events just to count them client-side (like a dvm does) is just ridiculous for most use case
  • I was the original author of COUNT. Lol? This isn't an argument.
  • Bad DB performance? False. I've never read count is an unrecommended feature due to poor performance on any DB documentation. Use proper indexes? Limit the max counted records? Cache? Rate limit?

@staab why are you forcing this deprecation? Why do you want this so badly?

@Semisol
Copy link
Collaborator

Semisol commented Jul 31, 2024

Bad DB performance? False. I've never read count is an unrecommended feature due to poor performance on any DB documentation. Use proper indexes? Limit the max counted records? Cache? Rate limit?

A simple solution would have been to offer a boolean that indicates there's more results.

Increases complexity of relay implementations? This ain't complex. The following is much more complex: ["REQ", <subscription_id>, , , ...] (multiple filters with distincts limits could instead be multiple subs) - though it is on NIP-01.

Worst case you repurpose the REQ flow for conting.

There's no way to accurately reconcile counts between multiple relays? Not needed. First, some use cases doen't need accuracy. Also, when outbox is used, count from one of the read relays. When not, count from e.g. one of the chat relays.

This will lead to huge inaccuracies: people have multiple read/write relays and clients will at their choice write to only a subset to optimize connections for example and only try other relays if the tried relays fail.

Better handled outside relays (via centralized service or DVM)? Dumb cause it is slow and a worse third-party to trust rather than the note author's read relay.

Dedicated aggregators are not worse, but DVMs are a horrible idea thinking about it now.

@Semisol
Copy link
Collaborator

Semisol commented Jul 31, 2024

Approving this PR was a bad idea.

@alexgleason
Copy link
Member

alexgleason commented Jul 31, 2024

DVMs are not a good solution. But then again neither is running COUNT 3 times for every post.

@cameri
Copy link
Member

cameri commented Jul 31, 2024

Aggregating counts should be done by clients.

@mattn
Copy link
Member

mattn commented Jul 31, 2024

Hmm, I do not think there was sufficient consensus on this merge yet.

@staab
Copy link
Member Author

staab commented Jul 31, 2024

I merged this because general consensus appeared to me to be in favor of deprecation, with the exception of @arthurfranca, who doesn't have an implementation he's supporting as far as I know. I don't want to continue arguing about this, so I've reverted it in #1404. Someone else can sponsor the deprecation if they want. Count will never work or be widely supported, but that's ok, that's true of much of the protocol.

@staab staab deleted the remove-nip-45 branch July 31, 2024 15:57
@vitorpamplona
Copy link
Collaborator

Unless people can prove that there are 2 implementations actively using this, there is no point in keeping it.

Wishful thinking that we should keep something that maybe someday someone will implement it is terrible.

If COUNT is not a good solution, delete it. Let somebody else create a useful solution.

Everything that is not being used should be deleted from this repo, regardless if it's the right way of doing it or not.

Otherwise, this repo is just going to be a bunch of junk and wishful bullshit no one has implemented fully.

@Semisol
Copy link
Collaborator

Semisol commented Jul 31, 2024

Unless people can prove that there are 2 implementations actively using this, there is no point in keeping it.

Wishful thinking that we should keep something that maybe someday someone will implement it is terrible.

If COUNT is not a good solution, delete it. Let somebody else create a useful solution.

Everything that is not being used should be deleted from this repo, regardless if it's the right way of doing it or not.

Otherwise, this repo is just going to be a bunch of junk and wishful bullshit no one has implemented fully.

Already is.

Lots of incomplete and actively harmful solutions like giftwraps already exist, etc

@jb55
Copy link
Contributor

jb55 commented Jul 31, 2024

stop getting worked up over this repo folks, it has very little impact over actual implementations. if your relay or client uses this, then find and talk to other implementations to find interop opportunities. This is not a core spec so you won't be able to rely on it in general anyways.

@mattn
Copy link
Member

mattn commented Jul 31, 2024

This COUNT is certainly not accurate since it should be that every relay returns a list of IDs based on the filter, and the list of IDs must be merged and counted. The only way to resolve this is for the client to merge it.

However, this COUNT works well enough to know how many events a single relay has for a particular condition. I sometimes use this count to determine if a relay has many users.

@mattn
Copy link
Member

mattn commented Jul 31, 2024

If this method returns a list of IDs, it should not be named COUNT. It should be named something like IDS. And it could coexist with COUNT.

@alexgleason
Copy link
Member

@mattn If you are using outbox model, the COUNT should be accurate if you get it from a write relay of the author of the post.

@mattn
Copy link
Member

mattn commented Jul 31, 2024

For a given condition, when two relays return the same COUNT result of 1 as the number of different IDs, it is not accurate.

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.