nips icon indicating copy to clipboard operation
nips copied to clipboard

[NIP-26] Fix for multiple `kind`s in delegation conditions

Open kdmukai opened this issue 2 years ago • 11 comments

Multiple delegation conditions are implicitly AND (created_at < X AND created_at > Y) so it's inconsistent to allow multiple separate kind conditions (e.g. kind=0&kind=1&kind=3000).

In my testing, relays reject delegation tokens with multiple kinds specified this way.

see: nostr-rs-relay

When given kind=0&kind=1&kind=3000, the relay is enforcing that kind must equal 0. And then it requires that kind must also equal 1. And so on.

Relays DO accept kind=0,1,3000 conditions: https://snort.social/e/note132uwuffvahjns77plwggtjjf9ssttl7wjagzgkw0mcs0djp0cdwqqf6lm5

{
  "id": "8ab8ee252cede5387bc1fb9085ca492c20b5ffce97502459cfde20f6c82fc35c",
  "pubkey": "d9023798840aa4172873c98c87f09493a35682da3538704727204c46c81f78fa",
  "created_at": 1675123680,
  "kind": 1,
  "tags": [
    [
      "delegation",
      "617fa8be17df3d708c2be0e300eaf7d21b91702a5161950d43e2f31d90acaa7a",
      "kind=1,3,7,3000&created_at>1675058400&created_at<1677650400",
      "6f33f71e92e8f9b6547929e16aa28062cfc54be14b622471033963261532b6591373abf142762130f3bf2f637d7a8c5128d98afff184a5079a5e8da98395fc6a"
    ]
  ],
  "content": "This is my second post! But I didn't sign it!! My NIP-26 delegated Nostr key (`npub1pr0xy`) signed it for me!\n\n#nip26",
  "sig": "d9cfe9f8012a82354d652e9cbdc43376c1162ead521c5b5b9a191a1a30d5a11a4e57caafeb4801466b0caa2fc85e8789212119b2bd43e033140c706be8c7419e",
  "relays": [
    "wss://nostr-pub.wellorder.net"
  ]
}

kdmukai avatar Jan 31 '23 04:01 kdmukai

Looks good to me. Any objections?

fiatjaf avatar Feb 01 '23 12:02 fiatjaf

Kind of related to this issue, I think the ranges might be more flexible than a comma separated list personally.

https://github.com/nostr-protocol/nips/issues/209

If some relays already support the comma list, would it make sense to allow both in the spec?

zackwy avatar Feb 01 '23 18:02 zackwy

I'm not quite following what the objection is to allowing kind=0&kind=1&kind=3000 so that the language remains consistent. I'm not diametrically opposed but it seems odd to me to bend the spec to fit an individual relay's implementation, so I'm curious if there are other limitations that are going over my head?

Backward compatibility is an interesting idea to me. If the idea is to make , a shorthand for multiple & operators in a chain though, it should also apply to other query string parameters for the sake of consistency created_at=>123,<234. This does however add extra tasks for any relays who have already, or are wanting to implement the NIP.

bhayward93 avatar Feb 01 '23 18:02 bhayward93

I'm not quite following what the objection is to allowing kind=0&kind=1&kind=3000 so that the language remains consistent.

@bhayward93 The problem is that the language as it is currently in the spec is not consistent in meaning.

kind=1&created_at>1675123680

Two conditions, both must be satisfied.

kind=1&created_at>1675123680&created_at<1675150000

Three conditions, all three must be satisfied.

kind=0&kind=1&created_at>1675123680&created_at<1675150000

Four conditions; the latter two must be satisfied but the first two are really an implicit OR.

it seems odd to me to bend the spec to fit an individual relay's implementation

It's more that this was a vague area in the spec until 5 days ago but I think that clarification was a poor choice.

If this was a more ossified spec, I'd feel differently. But the part I'm trying to change is literally just five days old.

kdmukai avatar Feb 01 '23 19:02 kdmukai

Kind of related to this issue, I think the ranges might be more flexible than a comma separated list personally.

@zackwy If ranges are supported, I think there should still only be one kind condition in the query string.

kind=0,1,3000,5000-5010

But I'd rather defer the question of ranges to its own discussion and ensuing PR.

kdmukai avatar Feb 01 '23 20:02 kdmukai

I'm not quite following what the objection is to allowing kind=0&kind=1&kind=3000 so that the language remains consistent.

@bhayward93 The problem is that the language as it is currently in the spec is not consistent in meaning.

kind=1&created_at>1675123680

Two conditions, both must be satisfied.

kind=1&created_at>1675123680&created_at<1675150000

Three conditions, all three must be satisfied.

kind=0&kind=1&created_at>1675123680&created_at<1675150000

Four conditions; the latter two must be satisfied but the first two are really an implicit OR.

it seems odd to me to bend the spec to fit an individual relay's implementation

It's more that this was a vague area in the spec until 5 days ago but I think that clarification was a poor choice.

If this was a more ossified spec, I'd feel differently. But the part I'm trying to change is literally just five days old.

Four conditions; the latter two must be satisfied but the first two are really an implicit OR.

I'm with you now - so the , delimiter is intended as an OR operand? In that case it makes much more sense - I thought you were proposing a shorthand AND operator that would be backwards compatible with a specific relay.

If this was a more ossified spec, I'd feel differently. But the part I'm trying to change is literally just five days old.

Understood - this had been the intended behaviour so I didn't realise it wasn't actually documented in the specificarion before then.

I'll think more on it but if its an OR operand then it makes much more sense. Seems I missed this line.

bhayward93 avatar Feb 01 '23 21:02 bhayward93

ACK

mikedilger avatar Feb 02 '23 01:02 mikedilger

Consider it an ACK from me at this point, but if you have any ideas on how to retain consistency between the AND and OR operands in terms of the number of conditions supplied per operator, It'd be good to hear. I haven't really came up with anything better, hence I'm happy to roll with this.

So for a more direct example

  • kind=1,2,3 // (specified once)
  • created_at=>123&created_at=<234 // (specified twice)

bhayward93 avatar Feb 02 '23 09:02 bhayward93

kind=1,2,3 // (specified once) created_at=>123&created_at=<234 // (specified twice)

This isn't inconsistent. You could say kind>1&kind<15 but it isn't very useful.

mikedilger avatar Feb 02 '23 20:02 mikedilger

kind=1,2,3 // (specified once) created_at=>123&created_at=<234 // (specified twice)

This isn't inconsistent. You could say kind>1&kind<15 but it isn't very useful.

Respectfully disagree - I think the use of created_at twice and kind once, feels inconsistent. My issue is not with any specific parameter, just the difference.

Not directly suggesting this, but for example kind=0,1,2;created_at>000&<111 or kind=0,1,2&created_at>000+<111 would feel consistent.

bhayward93 avatar Feb 02 '23 21:02 bhayward93

hmm, sorry for jumping on the discussion, and ignore me if it makes no sense, but instead of repeating created_at, why not use since and until instead, so there's no confusion?

eskema avatar Feb 02 '23 21:02 eskema

IMHO it is most consistent and comprehensible as it is in the NIP, as a standard logical formula.

Specifying the same variable twice has never been a problem in logical formulas: "x > 5 and x < 10" is perfectly comprehensible. Using 'since' and 'until' turns the testing of a single variable condition (created_at) into two variables which then have implicit meanings back to created_at. A computer could much more easily deal with the original logical formula, considering commas as ORs that bind more tightly than '&' does.

mikedilger avatar Feb 05 '23 21:02 mikedilger

We should merge this, but it needs rebasing.

fiatjaf avatar Feb 06 '23 15:02 fiatjaf

Lol I guess | doesn't exist in runes. How did this get merged without looking at the runes spec? Nostream won't implement this NIP cause nonway we'll have two ways of doing the same thing.

cameri avatar Feb 07 '23 14:02 cameri

What are the two ways of doing the same thing?

fiatjaf avatar Feb 07 '23 17:02 fiatjaf

What are the two ways of doing the same thing?

The rune-like way of doing OR is using the pipe character |. Nostream implements the full spec with the exception of considering _ as a special character because some properties like created_at use it.

We've gone our own way here. Is delegation rules limited to created_at and kind?

cameri avatar Feb 07 '23 18:02 cameri

Now I am confused. We can revert this if that is the best solution, but I am not following anything anymore.

fiatjaf avatar Feb 07 '23 21:02 fiatjaf

@Cameri, glad you spoke up. What @kdmukai is fixing needs fixing, but maybe not this way, fine.

If we don't allow kind=1,2,3 then should it be kind=1|kind=2|kind=3? It seems to me we need parenthesis or order-of-operations must more tightly bind or than and. Can you say something that means this in runes?

(kind=0|kind=1)&created_at>1675123680&created_at<1675150000

mikedilger avatar Feb 07 '23 21:02 mikedilger

@fiatjaf

Now I am confused. We can revert this if that is the best solution, but I am not following anything anymore.

kind=1|kind=2|kind=30000 is the rune-like way of allowing multiple alternatives for the same property. To combine it with created_at it will need to be repeated along with each kind condition OR we add support for parentheses which we don't have today.

Ref: https://pypi.org/project/runes/

cameri avatar Feb 07 '23 21:02 cameri

@Cameri, glad you spoke up. What @kdmukai is fixing needs fixing, but maybe not this way, fine.

If we don't allow kind=1,2,3 then should it be kind=1|kind=2|kind=3? It seems to me we need parenthesis or order-of-operations must more tightly bind or than and. Can you say something that means this in runes?


(kind=0|kind=1)

Yeah this will turn verbose:

kind=0 &created_at>1675123680&created_at<1675150000|kind=1 &created_at>1675123680&created_at<1675150000

I'm okay with adding support for commas just to make some rules simpler. I see benefit to it.

cameri avatar Feb 07 '23 21:02 cameri

I'm okay with adding support for commas just to make some rules simpler. I see benefit to it.

If we stayed with a logical formula though, with commas being an OR that binds more tightly than AND, we'd have:

kind=0,kind=1&created_at>1675123680&created_at<1675150000

rather than

kind=0,1&created_at>1675123680&created_at<1675150000

But to be totally honest, I'm just noticing things. I don't have much opinion on this stuff.

mikedilger avatar Feb 07 '23 21:02 mikedilger

I was working on this (using just the &) and just noticed this pull request.

I am a +1 on keeping it kind=1&kind=2&kind=5&created_at>1675150000&created_at<1675150000 where we can just say that & is a separator and should not be interpreted as a logical AND. Relays and clients should be able to easily implement this, just split at the & and loop over the pieces, interpreting each in a well-defined way. Perhaps it is not the most mathematical, but it seems simple enough.

Can say that delegation tokens must have AT MOST one created_at<N, created_at>N and arbitrarily many kind=N substrings, where N is an integer. The >= construction in created_at is not allowed, and ranges of kinds are not supported.

How fancy do we expect the delegation tokens to become? Keep in mind that people can just make a new one with the same delegatee if they somehow need to start signing events of a different kind... Also they can just not put any kind substrings to allow all kinds.

barkyq avatar Feb 12 '23 03:02 barkyq

If anyone complains we just say that && is AND and & is ampersAND

barkyq avatar Feb 12 '23 03:02 barkyq

How about we get rid of this rune stuff and use a different method for expressing the conditions?

fiatjaf avatar Feb 12 '23 11:02 fiatjaf

and ranges of kinds are not supported.

@barkyq some of the NIPs are reserving ranges of kind IDs; if any are accepted, it would be nice to be able to group authorizations for whole distinct purposes (e.g. NIP-X reserves kinds Y-Z).

kdmukai avatar Feb 12 '23 14:02 kdmukai

How about we get rid of this rune stuff and use a different method for expressing the conditions?

@fiatjaf Agreed. I had never heard of runes in this context before. It wasn't mentioned in the NIP so it was a surprise criteria/criticism to me.

I think the two created_at range boundaries are good enough as-is (though separate issue: maybe we make them required).

And then from there I just want the clearest, least verbose way to specify the allowed kinds.

Repeating "kind=" "kind=" "kind=" is just kind of silly and makes events a tiny bit heavier. If the NIPs keep going the way they are, a delegation token could have a TON of kinds specified.

So a single "kind=" entry that supports comma-separated values AND ranges is the most compact while still being easy to understand.

kind=0,1,3,110-120,3000,4550-4575,9000

Or if you prefer a wholesale format change: a completely stripped-down, json-friendly version could just specify the conditions as an array (with positions explicitly reserved for start/end timestamps):

nostr:delegation:<pubkey of publisher (delegatee)>:[<valid_from>,<valid_until>,"0,1,3,110-120,3000,4550-4575,9000"]

[
  "delegation",
  <pubkey of the delegator>,
  [<valid_from>,<valid_until>,"0,1,3,110-120,3000,4550-4575,9000"],
  <64-byte Schnorr signature of the sha256 hash of the delegation token>
]

kdmukai avatar Feb 12 '23 15:02 kdmukai

I don't think a created_at<N field should be required in the NIP. Perhaps we should say something like, app developers SHOULD add a created_at<N field to any delegation token they ask the user to sign.

In theory I support arrays of ranges of kinds. In practice it is a bit awkward to store as a DelegationToken object. The kinds field would need to be some sort of array of (start, end) 2-tuples. It's not so bad I suppose, and if other people support ranges I will not complain any further. I suppose there is a trade-off though: event byte size versus computational complexity of the verification process.

Also I feel like NIP developers / app developers should design future things so that such a complicated delegation token is not required. Like which app will reasonably ask for a delegation token with: kind=0,1,3,110-120,3000,4550-4575,9000. By not allowing ranges, we force good behavior.

I would certainly support "AT MOST one kind=n1,n2,..,nk substring where each ni is an integer." That would be easy to parse, and have a smaller byte size than the current implementation. I would half-heartedly support "AT MOST one kind=z1,z2,...,zk substring where each zk is either a single integer nk or an interval ak-bk where ak <= bk are integers."

barkyq avatar Feb 12 '23 16:02 barkyq

I agree with @barkyq; I don't think we need to create complex conditioning and ranges, since we're raising the bar for relays and clients to properly implement NIP-26 too much.

I think we should push these complex setups to the apps trying to use them by requesting multiple delegation tokens for different combinations, which also makes it easier for a user to understand what they are delegating.

pablof7z avatar Feb 14 '23 07:02 pablof7z

@pablof7z Yeah, even within the ranges for a particular NIP/use case, you'd almost definitely want more fine-grained permissions anyway.


I'd like to ask @fiatjaf to consider reverting 3f39a24 to restore this PR's proposed changes which are:

  • Single kind entry; multiple comma-separated values allowed.
  • Multiple separate kind=&kind=&kind= entries not allowed.
  • note: This PR makes no mention of support for ranges. Happy to drop that part of the discussion.

@mikedilger: "ACK / I don't have much opinion on this stuff." @bhayward93: "Consider it an ACK from me at this point" (albeit w/consistency misgivings) @Cameri: "I'm okay with adding support for commas just to make some rules simpler. I see benefit to it." @barkyq: "I would certainly support "AT MOST one kind=n1,n2,..,nk substring where each ni is an integer." " @pablof7z: ??? I think you and I just want this settled already!

kdmukai avatar Feb 14 '23 15:02 kdmukai

Commenting on this even if old because I see that the issue isn't really resolved, as @fiatjaf reverted 6a11f4d4cd0159c517414ffa6cfc646f8c7c9da3.

I think allowing multiple options is important. The specific syntax being used isn't important, in my opinion. Comma-separated values would already grant much more flexibility.

I agree that repeating the word "kind" makes no sense. It would just make it harder to read, and events heavier.

Some of this discussion seem to be due to the fact that:

  • Conditions are in AND
  • created_at appears more than once
  • There isn't a natural way to specify multiple possible values (let alone ranges) for kind.

None of this leads to any actual technical issue, but I guess it feels wrong in the rigid mind of us programmers, so let me suggest something:

  • This can be solved, and it wouldn't be hard to parse, by avoiding the equal sign, using a different predicate that means "in this set of values", making up a simple syntax for sets of values. For example, kind:{0,2...15,25} might allow events of kind 0, between 2 and 15 or 25.
  • We could also use the syntax suggested originally, accepting in our mind that the equality symbol doesn't signifies equality, but rather membership to a set (effectively the same as suggested before, but written differently) or, alternatively, that commas on the right side signify disjunction of conditions with the same left side and kind of predicate and that dashes are a shortened version of multiple such conditions.

Repeating created_at and not kind would be fully consistent, but one can absolutely use the same syntax to avoid repeating created_at, if we decide it's allowed.

I think going with prior art would make sense, but I don't care much what syntax is used, as long as multiple kinds are allowed.

Aspie96 avatar Jul 21 '24 22:07 Aspie96