Subscription id too long about strfry HOT 12 CLOSED

It's better to chase the clients and ask them to shorten their ids.

from strfry.

Thanks for letting me know! Currently the subIds are restricted to 39 characters. This limit is an optimisation so that the subId can be stored inline in the subscription object I just checked snort and it's sending 45 characters.

I pushed an update to master that increases the limit to 63, please pull and rebuild.

BTW, thank you for your early beta testing! Just a warning: I have a change I'm working on that will require a DB rebuild. It saves 16 bytes per event and 16 bytes for every e/p tag. When I roll this out you'll have to strfry export and strfry import the DB.

from strfry.

This worked well, thank you!

from strfry.

@hoytech running my shiny new strfry instance (with sub id max-length now at 63) and looking at logs I periodically still see this:

[Ingester 1 ]INFO| sending error to [1318]: bad req: subscription id too long

Taking a look at NIP-01 which is the only place that defines them at all, it says this:

<subscription_id> is a random string that should be used to represent a subscription.

I think either strfry needs to accept arbitrarily long subscription id’s or better, submit a PR to NIP-01 that defines the max length of a <subscription_id> so all clients and relays can safely optimize.

cc @fiatjaf

from strfry.

The beta branch increases this limit to 71. How long are you seeing? (You can turn on verbose logging with relay.logging.dumpInReqs.

It's a shame that clients send such long subIds, since these are echoed back on every EVENT, so it adds up to a lot of wasted bandwidth.

There's a minor efficiency advantage to using a static sized buffer for this, but if really needed I can make it a configurable length.

from strfry.

@hoytech I turned on verbose logging for reqs and saw a 100 char sub id. It has an 81 char random hash, then an underscore and then the url of my relay.

what about sha256-ing the incoming sub id string before storing - would that be too big a performance hit?

It's better to chase the clients and ask them to shorten their ids.

Really? Why not specify the max length of a subscription ID so relays can legitimately reject a potential bad actor vector?

from strfry.

Unfortunately we can't hash the subIds because we need to echo it back to the client.

from strfry.

Unfortunately we can't hash the subIds because we need to echo it back to the client.

ah so.

Leaving the subscription_id so loosely defined in the protocol feels to me like a vulnerability to all kinds of potential bad client mischief.

from strfry.

FWIW, I did an nslookup on an IP address for a client that tried to connect and received the subId too long error and the IP resolved to api.nostr.watch.

from strfry.

Ya, so a little code spelunking in the nostr-watch github turns this up in many places:

subid = crypto.randomBytes(40).toString('hex')

…which yields an 81 character random string.

So @fiatjaf we should at this point ask @dskvr to make those subid’s shorter?

edit: also @hoytech : nostream is sending this to clients: "max_subid_length":256 in its NIP-11 info

see also: nostr-protocol/nips#240

from strfry.

Thanks for looking into this! Good to know where that's coming from.

Using crypto.randomBytes() for this is unnecessary, and I think indicates we need some better education for client authors.

The max_subid_length NIP-11 response is interesting but IMO also unnecessary. If I were making a client I would just use sequential per-connection integers for my subscription IDs and not worry about this limit at all (as long as it's at least, say, 10).

from strfry.

Nice job finding the culprit.

from strfry.