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

shuffle new relays #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

shuffle new relays #41

wants to merge 4 commits into from

Conversation

ksfi
Copy link

@ksfi ksfi commented Oct 23, 2023

Added a function to use more active relays via the nostr watch API

Copy link
Owner

@0xBEEFCAF3 0xBEEFCAF3 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!!
However, I can't entirely agree with parts of these changes. Here are the reasons

  • This requires a level of interaction from the user. This means it's going to be harder to auto-provision the server
  • There should be a strong preference for a set of nostr nodes that work well.

I think the best course of action here is for the user to be able to provide the node they want to connect to. And in the read me we can have a example start command that uses the nostr.watch API.

Lmk what you think

@ksfi
Copy link
Author

ksfi commented Oct 30, 2023

I made this available via argument parsing:

  • --add allows the user to give a a list of new relays to add before running
  • --show displays some active relays and exit

I don't know if exiting right after running the --show command is good, maybe I should just let the --add option and the user has to find new relays by himself.

Copy link
Owner

@0xBEEFCAF3 0xBEEFCAF3 left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review. This looks good. I do have a open question

def add_relays():
def show_relays():
r = (requests.get(NOSTR_WATCH, {})).json()
new_relays = [x for x in r[:20] if 'damus' not in x and 'nostr-pub.wellorder' not in x]
Copy link
Owner

Choose a reason for hiding this comment

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

Why not show damus and the nostr-pub.wellorder

Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain why r[:20] ?

Copy link
Owner

Choose a reason for hiding this comment

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

Is that just to skip protocol portion of the url?

Copy link
Author

Choose a reason for hiding this comment

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

  • First question: that’s because I’ve let the original relays in the NOSTR_RELAY global list, so I’m excluding them here to avoid potential duplicates
  • Second question: sorry for the magic number, should be corrected, that’s an arbitrary number bc the list of active relays is quite long, originally I was asking the user if he wanted to reshuffle so that it shows 20 other relays and so on

The relay selection could be improved based on relay location or other informations

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this make the service dependent on Nostr Watch? If that service goes down would that be a single point of failure? We may be able to achieve the same thing with an async websocket relay pool unless there are other advantages to using Nostr Watch.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah the show option relies on the nostr_watch API , the advantage to me is that it's dynamically updated which could be desirable given how relays are prone to instability.
Moreover it seems pretty canonical.
If by websocket relay pool you mean a statically maintained pool, it could lead to extra continuous work to maintain if but if you know how to get relays that are online and dynamically update the pool, it would indeed be a better idea. I could peep the nostr_watch repo to see how they get the relays under the hood.
Lmk what you think

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.

3 participants