-
Notifications
You must be signed in to change notification settings - Fork 602
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
Conversation
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 |
This looks good to me |
I’ll review and provide comments in the next 1-2 days. Thanks for the tag. |
There was a problem hiding this 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.
This is for users to list their own files only |
Ah, I missed |
@v0l For the listing route, please check my review comment above. You are exposing fields that are already part of the nip94_event object. |
|
||
### Query args | ||
|
||
- `page` page number (`offset=page*count`) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
No one is counting much on my opinion but it's fine with me 😀😅🤓 nostrcheck-server will implement it. |
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 When you upload a file to some server and it gets compressed then that file should be the original hash. |
@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. |
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. |
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? |
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 |
The title of the NIP is "File Storage Integration" |
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. |
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 |
@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. |
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. |
@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
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. |
@sant0s12 @nostrband @fishcakeday this is completely off topic for this PR btw, i was going to make another PR to add the |
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 |
I was directed here to talk about no transform, so here I am :)
Two NBs, very confusing. Ok, I think "no_transform" is in good hands now, will not interfere your discussions any further. |
Yes, let's discuss this in a new PR. @v0l do you want to open one? |
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 😅🤓) If someone wants to open a debate there I'll be happy to adapt the idea to what the consensus seems appropriate. |
I will just take the opportunity too xD (sorry for the PR offtopic) to explain why the About the different (image) versions like thumbnails, there is the For videos, i don't know what similar query param could be useful to hint server could send a lower quailty version, maybe |
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. |
@arthurfranca I understand the use of the |
@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 |
@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 ^^ |
@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 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>
@sant0s12 I mean just that if the user adds Though yeah user could be adding bogus Adding |
@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 |
@v0l |
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 |
imo ready to merge |
added to void-cat-rs: https://git.v0l.io/Kieran/void-cat-rs/commit/5547673331a8ffebe73df7139eca2bfaef5357c4 |
@nostrband I've added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Hello @nostrband @v0l @sant0s12 @fishcakeday @Semisol @vitorpamplona Added list files API and no_transform to nostrcheck.me, the other instances (those installed by others) will have it implemented as soon as version 0.6.0 is released in 2-3 weeks. |
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