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

NIP-96: List files / rewording, no_transform #1236

Merged
merged 4 commits into from
Jun 6, 2024
Merged

NIP-96: List files / rewording, no_transform #1236

merged 4 commits into from
Jun 6, 2024

Conversation

v0l
Copy link
Member

@v0l v0l commented May 14, 2024

Adding an endpoint to list uploads

Also included a bit of rewording, this NIP is far too long.

read: https://github.com/nostr-protocol/nips/blob/nip96-sync/96.md

@v0l v0l requested a review from vitorpamplona May 14, 2024 13:02
96.md Outdated Show resolved Hide resolved
@arthurfranca
Copy link
Contributor

Much better than before for sure!

Regarding the listing route, the response is JSON which may eat too much memory when generating it if user has uploaded too many files.

One way to solve this is to let the server optionally add a "nextPage": "<url-to-list-remaining-files>" to the response if it wants to split response in pages.

@vitorpamplona
Copy link
Collaborator

This looks good to me

96.md Outdated Show resolved Hide resolved
@fiatjaf
Copy link
Member

fiatjaf commented May 16, 2024

@fishcakeday

@Semisol Semisol self-requested a review May 17, 2024 19:01
@fishcakeday
Copy link

I’ll review and provide comments in the next 1-2 days. Thanks for the tag.

Copy link

@fishcakeday fishcakeday left a comment

Choose a reason for hiding this comment

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

Love the simplification and corrections, not a fan of the listing thing for many reasons that could expose the service to DDoS, bandwidth overconsumption, extra complexity and lack of consideration for large lists that are not always safe to share. Due to users' privacy concerns and potential harmful (CSAM, etc.) images that are not yet filtered/monitored, and are not safe to list.

96.md Outdated Show resolved Hide resolved
@Semisol
Copy link
Collaborator

Semisol commented May 19, 2024

Love the simplification and corrections, not a fan of the listing thing for many reasons that could expose the service to DDoS, bandwidth overconsumption, extra complexity and lack of consideration for large lists that are not always safe to share. Due to users' privacy concerns and potential harmful (CSAM, etc.) images that are not yet filtered/monitored, and are not safe to list.

This is for users to list their own files only

@fishcakeday
Copy link

Love the simplification and corrections, not a fan of the listing thing for many reasons that could expose the service to DDoS, bandwidth overconsumption, extra complexity and lack of consideration for large lists that are not always safe to share. Due to users' privacy concerns and potential harmful (CSAM, etc.) images that are not yet filtered/monitored, and are not safe to list.

This is for users to list their own files only

Ah, I missed Returns a list of files linked to the authenticated users pubkey. line completely. That makes much more sense now.

@arthurfranca
Copy link
Contributor

@v0l For the listing route, please check my review comment above. You are exposing fields that are already part of the nip94_event object.

96.md Show resolved Hide resolved

### Query args

- `page` page number (`offset=page*count`)
Copy link

Choose a reason for hiding this comment

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

Can we just do it in a simpler way? You request count and look for the 'cursor' field in the returned array, and if it is present, you copy that into the next request with count again. If no count provided, service will return what it does and gives cursor if more found. With this approach, you at least allow for a more efficient implementation. If the service wants to go with page based implementation, it will return a number, if something else, it maybe a string that is a 'cursor' for whatever backend DB they use.

Copy link
Member Author

@v0l v0l May 22, 2024

Choose a reason for hiding this comment

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

Yea i think this makes sense, every time cursor is present you know there are more results. But it doesnt actually work well for when you actually want page numbers on your UI

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, UI loads page 0 on load, response should contain total number of pages (on in the nip yet) and UI can display page numbers

Choose a reason for hiding this comment

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

I think the first request could be without page, just the count. And then the cursor is returned, it should be passed back with the count. This allows to have flexible cursors and not just page counts, e.g., DDB cursor or some other KV store.

@quentintaranpino
Copy link

No one is counting much on my opinion but it's fine with me 😀😅🤓

nostrcheck-server will implement it.

@nostrband
Copy link
Collaborator

Could we add an optional param to request "no transformation" from the server, so that GET requests would always serve the original file by default? And if it's not supported by the server it could reply with some 40x code. It seems like this single addition would make this nip supercede blossom.

@v0l
Copy link
Member Author

v0l commented May 27, 2024

Could we add an optional param to request "no transformation" from the server, so that GET requests would always serve the original file by default? And if it's not supported by the server it could reply with some 40x code. It seems like this single addition would make this nip supercede blossom.

I think i would rather drop the entire "original file" concept and just allow uploads with no compression, i think the ox tag was a big mistake.

When you upload a file to some server and it gets compressed then that file should be the original hash.

@sant0s12
Copy link
Contributor

sant0s12 commented May 27, 2024

@v0l Perhaps the sever could reply with a hash for each of the variants it intends to store so that the client can verify that they have not changed.

@fishcakeday
Copy link

Could we add an optional param to request "no transformation" from the server, so that GET requests would always serve the original file by default? And if it's not supported by the server it could reply with some 40x code. It seems like this single addition would make this nip supercede blossom.

I think i would rather drop the entire "original file" concept and just allow uploads with no compression, i think the ox tag was a big mistake.

When you upload a file to some server and it gets compressed then that file should be the original hash.

Then why are you running imgproxy on snort and not serving the original media? I think you are mixing file server and media server and nip-96 is mainly for media.

@fishcakeday
Copy link

@v0l Perhaps the sever could reply with a hash for each of the variants it intends to store so that the client can verify that they have not changed.

What if variant computed on-demand as it should be? Did you take into account the time it will take to compute and the resources it will require to do it? Did you consider video?

@v0l
Copy link
Member Author

v0l commented May 27, 2024

Could we add an optional param to request "no transformation" from the server, so that GET requests would always serve the original file by default? And if it's not supported by the server it could reply with some 40x code. It seems like this single addition would make this nip supercede blossom.

I think i would rather drop the entire "original file" concept and just allow uploads with no compression, i think the ox tag was a big mistake.
When you upload a file to some server and it gets compressed then that file should be the original hash.

Then why are you running imgproxy on snort and not serving the original media? I think you are mixing file server and media server and nip-96 is mainly for media.

My imgproxy does hashcheck on every image, which is why images were broken for a time from nostr.build because of CF Polish, nostr.build was not serving the original images

@v0l
Copy link
Member Author

v0l commented May 27, 2024

nip-96 is mainly for media.

The title of the NIP is "File Storage Integration"

@fishcakeday
Copy link

Could we add an optional param to request "no transformation" from the server, so that GET requests would always serve the original file by default? And if it's not supported by the server it could reply with some 40x code. It seems like this single addition would make this nip supercede blossom.

I think i would rather drop the entire "original file" concept and just allow uploads with no compression, i think the ox tag was a big mistake.
When you upload a file to some server and it gets compressed then that file should be the original hash.

Then why are you running imgproxy on snort and not serving the original media? I think you are mixing file server and media server and nip-96 is mainly for media.

My imgproxy does hashcheck on every image, which is why images were broken for a time from nostr.build because of CF Polish

The point is that the user does not see the original, regardless where it is modified. And imgproxy does a fairly poor optimization to begin with. In the end, asking a media host to serve immutable file is very single-sided argument that benefits only few clients and not the user. Plus, users are very bad at sanitizing their media and leave all kind of identifying metadata in them, hence the need to strip, re-compress, and resize.

@fishcakeday
Copy link

nip-96 is mainly for media.

The title of the NIP is "File Storage Integration"

Does it matter what the title is when the "spirit" of the NIP is towards media? Sure it can be expanded to other media, hence the request to add "immutability" flag from the @nostrband

@v0l
Copy link
Member Author

v0l commented May 27, 2024

@fishcakeday i dont think anybody was arguing about removing the resize/compress step, i think its vital, not sure where you are going with these comments.

@fishcakeday
Copy link

Perhaps I misread "I think i would rather drop the entire "original file" concept and just allow uploads with no compression, i think the ox tag was a big mistake." part to think that we want to stop compressing and leaving the file as-is. Now reading it again, I see that the proposal is opposite, and to just drop the original hash. But, we do compute the original hash for the purposes of naive deduplication, so it is still there. Sorry if I misunderstood you.

@v0l
Copy link
Member Author

v0l commented May 27, 2024

This assumes that the first server will not change the file again or serve different versions

Once the file is compressed, the hash is returned, you put this hash in your posts as the x tag and so clients check when they fetch this file that the hash is correct, the server cannot modify it again or it will break.

This is how i have implemented it in void-cat-rs i don't return the ox or use it at all, there is only 1 hash, and it is the hash of the file after compression.

@sant0s12
Copy link
Contributor

This assumes that the first server will not change the file again or serve different versions

Once the file is compressed, the hash is returned, you put this hash in your posts as the x tag and so clients check when they fetch this file that the hash is correct, the server cannot modify it again or it will break.

This is how i have implemented it in void-cat-rs i don't return the ox or use it at all, there is only 1 hash, and it is the hash of the file after compression.

As far as I understand (please correct me if I'm wrong), @fishcakeday would like to serve dynamic variants of the same file, which I can see why it would be useful (especially for video). Your approach only works if the server only compresses the file exactly once and serves that forever.

@v0l
Copy link
Member Author

v0l commented May 27, 2024

@sant0s12 on the subject of versions, i don't really see how this can work, to me each version is its own upload, if you want multiple versions then you need to upload using the type or another spec should be created to ask the server to resize the image for you.

Your approach only works if the server only compresses the file exactly once and serves that forever.

Yes this is how it should work, you cannot modify it after the fact as some sort of optimization because the hash will not be the same, the point of this spec is to avoid the media rugpull, you cannot serve whatever you want it has to be the same media that the user uploaded in the first place.

@v0l
Copy link
Member Author

v0l commented May 27, 2024

@sant0s12 @nostrband @fishcakeday this is completely off topic for this PR btw, i was going to make another PR to add the no_transform option later but here we are lol

@fishcakeday
Copy link

fishcakeday commented May 27, 2024

@sant0s12 @nostrband @fishcakeday this is completely off topic for this PR btw, i was going to make another PR to add the no_transform option later but here we are lol

LOL! I was thinking the same, we complete this PR and open a new one for the no_transformation purposes.

As for the variants, NB (nostr.build) already shares the links to the transformed versions that are not widely used by present in the response. And for immutable content, you can still redirect the user to the optimized version and offer the original. Let say a user uploads video.mov (MOV is shit for streaming as a container but here we are) and we offer video.mov/optimized.mp4 and video.mov/poster.jpg video.mov/thumbnails.png video.mov/stream.m3u8 video.mov/stream.mpd etc. Note that original is still available for download if needed, with disposition for that purpose.

https://github.com/nostrbuild/nostr.build/blob/52c1b78aa8b67c1b70231abb8c4e5e0f6fac179e/api/v2/helper_functions.php#L192 - is where the specs are described

@nostrband
Copy link
Collaborator

LOL! I was thinking the same, we complete this PR and open a new one for the no_transformation purposes.

I was directed here to talk about no transform, so here I am :)

As for the variants, NB already shares the links...

Two NBs, very confusing.

Ok, I think "no_transform" is in good hands now, will not interfere your discussions any further.

@sant0s12
Copy link
Contributor

Yes, let's discuss this in a new PR. @v0l do you want to open one?

@quentintaranpino
Copy link

I take this opportunity to show you another PR I did some time ago, where I tried to unify the URL's as blossom (before blossom 😅🤓)

#1097

If someone wants to open a debate there I'll be happy to adapt the idea to what the consensus seems appropriate.

@arthurfranca
Copy link
Contributor

I will just take the opportunity too xD (sorry for the PR offtopic) to explain why the ox tag concept exists. It is simply that from the user point of view, they have a file that when sent to multiple file servers can then be represented by different x tags. It is easier for the user to anchor all these variants by a single ox tag, cause for the user it represents the same one thing they intended to upload. With the no-transform PR, ox and x will be the same, no problem.

About the different (image) versions like thumbnails, there is the ?w=<width> param on NIP-96. Server should return the version it has (or generate it on the fly) that is closest to the asked width.

For videos, i don't know what similar query param could be useful to hint server could send a lower quailty version, maybe ?w=<width> too, but now representing not the video container's width, but the max device width (for cellphones it would be its height, cause of portrait mode).

@fishcakeday
Copy link

I will just take the opportunity too xD (sorry for the PR offtopic) to explain why the ox tag concept exists. It is simply that from the user point of view, they have a file that when sent to multiple file servers can then be represented by different x tags. It is easier for the user to anchor all these variants by a single ox tag, cause for the user it represents the same one thing they intended to upload. With the no-transform PR, ox and x will be the same, no problem.

About the different (image) versions like thumbnails, there is the ?w=<width> param on NIP-96. Server should return the version it has (or generate it on the fly) that is closest to the asked width.

For videos, i don't know what similar query param could be useful to hint server could send a lower quailty version, maybe ?w=<width> too, but now representing not the video container's width, but the max device width (for cellphones it would be its height, cause of portrait mode).

For video, it is not as simple as "give me a smaller version" since video can be an hour-long thing that will take a lot of time to transcode. There is also codec, bitrate, resolution, frame rate, etc. And there is also HLS and DASH and poster, and thumbnails, and subtitles, etc. etc.

@sant0s12
Copy link
Contributor

@arthurfranca I understand the use of the ox tag, however, I don't see the use of a separate x tag unless the server is required to never modify the file again (which I think is unwise unless the user asks for immutability).

@arthurfranca
Copy link
Contributor

video can be an hour-long thing that will take a lot of time to transcode

@fishcakeday hmm I guess file server may want to pre-generate (I see on-the-fly is just for images) some even lesser quality versions than the main one that it could serve depending on the ?w=<devide-width> used when client asks for download?

@arthurfranca
Copy link
Contributor

@sant0s12 while the server is free to modify further the file after the upload (no one can stop it) and even should if it wants to free up some space, the user wants to have a way to detect that, just in case the server is swapping their flower image to a porn one ^^

@sant0s12
Copy link
Contributor

@arthurfranca Client side checks only make sense under the assumption of immutability. If the server is free to change the image, then the client should not check (and vice versa, i.e., only check when the user previously specified no_transform).

I just think that this should be explicit in the NIP.

* no_transform

* Update 96.md

Co-authored-by: Santos <34815293+sant0s12@users.noreply.github.com>

---------

Co-authored-by: Santos <34815293+sant0s12@users.noreply.github.com>
@arthurfranca
Copy link
Contributor

@sant0s12 I mean just that if the user adds x tag(s) to the event and if the served image doesn't match it/them, the user can say "I'm not accountable for the image content".

Though yeah user could be adding bogus x tag to the event on purpose 🤔️. Well you are right, it doesn't make sense because of that.

Adding x tag (or some simple flag tag like ["immutable", "1"], as event may already have the ox with the same value) just in case of asking for no_transform seems correct.

@v0l
Copy link
Member Author

v0l commented May 28, 2024

@arthurfranca @sant0s12 its important to remember that NIP-96 was born out of NIP-94 which was a desire to have immutable files, i dont think its ever acceptable for a file to be modified by the server after it was uploaded.

If the server implements some sort of compression it needs to be done at the time of upload and the hash returned so that the user can attest to the content that the server produces, otherwise having the hash is completely useless and we should just switch to guid's

@sant0s12
Copy link
Contributor

sant0s12 commented May 28, 2024

@v0l
While I disagree that expecting immutability when not specifying no_transform makes little sense, I don't care too much. My main issue is that I think that what you just said is not clearly codified in the NIP and should be included to avoid confusion. Perhaps in the "File compression" section.

@arthurfranca
Copy link
Contributor

Yeah the transformed file hash has its uses. Sometimes I forget important details and jump to conclusions. It is just that it is not exactly a proof that the server changed the file after the upload because user can lie when setting the x tag to the event.

@arthurfranca
Copy link
Contributor

imo ready to merge

@v0l
Copy link
Member Author

v0l commented May 29, 2024

@fishcakeday
Copy link

fishcakeday commented Jun 2, 2024

@nostrband I've added no_transform directive to nostr.build, it is deployed now so you can test.

Copy link
Collaborator

@Semisol Semisol left a comment

Choose a reason for hiding this comment

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

lgtm

@Semisol Semisol changed the title NIP-96: List files / rewording NIP-96: List files / rewording, no_transform Jun 2, 2024
@v0l v0l merged commit ff2e05d into master Jun 6, 2024
@v0l v0l deleted the nip96-sync branch June 6, 2024 10:58
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.

None yet

9 participants