Let mentions() return all 'e' tags

bu5hm4nn commented

2023-03-15 13:00:01 +13:00

(Migrated from github.com)

Currently mentions() discards the first and last unmarked 'e' tag (due to legacy NIP-10 positional mentions logic). As the result of this function is used to fetch related notes in gossip client, this results in reposts not getting fetched where some clients use a single unmarked 'e' tag as a mention to create a repost.

This pull request discards different treatment of marked and unmarked 'e' tags and just returns all 'e' tags.

Currently mentions() discards the first and last unmarked 'e' tag (due to legacy NIP-10 positional mentions logic). As the result of this function is used to fetch related notes in gossip client, this results in reposts not getting fetched where some clients use a single unmarked 'e' tag as a mention to create a repost. This pull request discards different treatment of marked and unmarked 'e' tags and just returns all 'e' tags.

bu5hm4nn commented

2023-03-15 13:08:33 +13:00

(Migrated from github.com)

To see an example of a note that benefits from this change, build the latest from this branch https://github.com/mikedilger/gossip/pull/310 and search for this note (provided you only follow the reposter and not the original poster): note1ts55uz7kn7a5vu97hj27a04xjqakf2uc26zfunmp0mmvhq2exaqq2365g4

bu5hm4nn commented

2023-03-16 04:42:22 +13:00

(Migrated from github.com)

@fiatjaf I think you would be the right person to review this. This function is used in two locations in gossip code. One is to build the list of related posts that should be fetched and the other is to store post relationships in the database. I believe based on current use that I have seen in my feed is that unmarked 'e' tags are used (by some users/some clients) for no-comment mentions and those would not be included if we are discarding first and last unmarked 'e' tags.

fiatjaf commented

2023-03-16 07:49:54 +13:00

(Migrated from github.com)

Shouldn't we exclude e tags marked with "reply" or "root" here?

Shouldn't we exclude `e` tags marked with `"reply"` or `"root"` here?

mikedilger commented

2023-03-16 07:54:59 +13:00

(Migrated from github.com)

Presuming you want a function that returns all 'e' tags irrespective of their position (root, mention, reply), I think you should create a new function for that.

bu5hm4nn commented

2023-03-16 08:18:32 +13:00

(Migrated from github.com)

Ok, please understand what I am trying to fix, so you can advise me on how you want it fixed. We are getting notes that have a single unmarked 'e' tag and have a single mention "#[0]" as the content.
Those currently do not get fetched by the overlord because this function mentions() will not return that unmarked mention. So there are 3 solutions that I see:

modify mentions()
create a new function get_etags() that returns all 'e' tags. Then replace both uses of mention() in gossip to use this new function. Gossip would not use mentions() function anymore.
Ignore the issue, don't fetch those mentionend posts, users are out of luck (they do not show, even if you click on them because overlord doesn't know about them)

Ok, please understand what I am trying to fix, so you can advise me on how you want it fixed. We are getting notes that have a single unmarked 'e' tag and have a single mention "#[0]" as the content. Those currently do not get fetched by the overlord because this function `mentions()` will not return that unmarked mention. So there are 3 solutions that I see: 1. modify `mentions()` 2. create a new function `get_etags()` that returns all 'e' tags. Then replace both uses of `mention()` in gossip to use this new function. Gossip would not use `mentions()` function anymore. 3. Ignore the issue, don't fetch those mentionend posts, users are out of luck (they do not show, even if you click on them because overlord doesn't know about them)

mikedilger commented

2023-03-16 15:39:53 +13:00

(Migrated from github.com)

I just updated nostr-types with referred_events() that you can use which brings in the data from all e tags.

I also went looking for this call to event.mentions() in gossip that you are referring to which signals the minions to subscribe to these and I couldn't find it. Granted I'm working very fast and spending very little time here.

In the process of searching for it, I found two references that were buggy, so I fixed those bugs. But neither of those fixes uses this new referred_events() function.

So I'm curious where you would make the change in gossip.

I just updated nostr-types with `referred_events()` that you can use which brings in the data from all `e` tags. I also went looking for this call to `event.mentions()` in gossip that you are referring to which signals the minions to subscribe to these and I couldn't find it. Granted I'm working very fast and spending very little time here. In the process of searching for it, I found two references that were buggy, so I fixed those bugs. But neither of those fixes uses this new `referred_events()` function. So I'm curious where you would make the change in gossip.

mikedilger commented

2023-03-16 15:42:30 +13:00

(Migrated from github.com)

Ok .replies_to_ancestors() was already doing this (but was enforcing TextNote). Let's go back to that, but take out the enforcement. I can't do it, gotta run.

Ok `.replies_to_ancestors()` was already doing this (but was enforcing TextNote). Let's go back to that, but take out the enforcement. I can't do it, gotta run.

bu5hm4nn commented

2023-03-16 16:36:34 +13:00

(Migrated from github.com)

The call to mentions() I was referring to was at src/overlord/mod.rs line 1198 where it adds the mentions to the list of missing ancestors. You are of course the author and expert on this code, but I did notice that when I modified the mentions() function to return that first unmarked mention note id, then it would show up in GLOBALS.events.get(id). So in some way it's adding it to the list of stuff to fetch.

The call to `mentions()` I was referring to was at `src/overlord/mod.rs` line 1198 where it adds the mentions to the list of missing ancestors. You are of course the author and expert on this code, but I did notice that when I modified the `mentions()` function to return that first unmarked mention note id, then it would show up in `GLOBALS.events.get(id)`. So in some way it's adding it to the list of stuff to fetch.

mikedilger commented

2023-03-19 12:17:52 +13:00

(Migrated from github.com)

but I did notice...

Yes I can see how that would happen. The particular call to mentions() was wrong however, if you read the comment above it, this block of code was supposed to look for recommended_relay_urls in 'p' tags. That was a suggestion from fiatjaf long ago (so this was a bug for a long time apparently).

The block just above at line 1188 is doing the heavy lifting. It now calls .referred_events() which looks at all the 'e' tags, whether root or mention or reply, and uses all the recommended_relay_url fields it can find to try to find the ancestor event. It used to only use the mentions.

> but I did notice... Yes I can see how that would happen. The particular call to `mentions()` was wrong however, if you read the comment above it, this block of code was supposed to look for recommended_relay_urls in 'p' tags. That was a suggestion from fiatjaf long ago (so this was a bug for a long time apparently). The block just above at line 1188 is doing the heavy lifting. It now calls `.referred_events()` which looks at all the 'e' tags, whether root or mention or reply, and uses all the recommended_relay_url fields it can find to try to find the ancestor event. It used to only use the mentions.

bu5hm4nn commented

2023-03-22 06:26:50 +13:00

(Migrated from github.com)

Thanks for addressing this issue, the only time I don't see a mention in the cache now is if the tag doesn't include a relay url.

Perhaps at some point in the future we could offer searching on other relays to the user.

I now also actually believe your implementation of mentions is the correct way to handle it while there still are unmarked tags out there.

Thanks for addressing this issue, the only time I don't see a mention in the cache now is if the tag doesn't include a relay url. Perhaps at some point in the future we could offer searching on other relays to the user. I now also actually believe your implementation of mentions is the correct way to handle it while there still are unmarked tags out there.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

Let mentions() return all 'e' tags #7

Pull request closed