feat: upload blobs (part I) by volmedo · Pull Request #13 · storacha/guppy

volmedo · 2025-04-09T17:15:42Z

WIP Disclaimer

I'd appreciate having the first eyes on this. It is rough, I know, but please be gentle 😄 and consider it still as WIP. The plan is to have parts II and III coming as follow up PRs so that it's not a single, huge PR to review.

I keep a (long) list of follow up tasks. Most notably, filecoin flow and index upload are not implemented yet (this is why the PR is titled "upload blobs" and not "implement blob protocol" 😄). Aside from that, error handling is so-so for now and I plan to refactor big chunks of code, along with UX and other usability enhancements, that I'll implement afterwards.

What are the changes?

This PR changes the old method of uploading blobs via store/add. This protocol is now deprecated so the corresponding code has been removed. Instead, it uses the main flow of the blob protocol. I'm calling it "main flow" because index creation and filecoin piece aggregation and offering are still missing. Also note that it currently focuses on uploading CARs. Adding files and directories will come later when we add DAG creation and UnixFS wrapping.

I also implemented a full test of the happy path using test UCAN and HTTP servers.

Dependencies

Checks are currently failing because I'm using local changes in go-libstoracha and go-ucanto libs. Before merging, I'll consolidate these changes, release new versions and point to them here. Currently, it depends on:

feat: added upload capability to capability package go-libstoracha#13
feat: a more convenient Result interface go-ucanto#42 (this one may end up closed; I'll adjust this PR if that's the case)

alanshaw

❤️ Looking good so far - some feedback in comments but nothing major!

alanshaw · 2025-04-28T10:27:54Z

-//
-// The `params` are caveats required to perform an `upload/list` invocation.
-func UploadList(issuer principal.Signer, space did.DID, params uploadlist.Caveat, options ...Option) (receipt.Receipt[*uploadlist.Success, *uploadlist.Failure], error) {
+func BlobAdd(content io.Reader, issuer principal.Signer, space did.DID, receiptsURL *url.URL, options ...Option) (ipld.Link, delegation.Delegation, error) {


receiptsURL should be an option.

I agree. In the follow up PR I'm working on right now I'm putting this, as well as other stuff, in a Client abstraction. If think this can wait till that follow up arrives, I'll leave it as is for now.

alanshaw · 2025-04-28T10:30:40Z

-//
-// The `params` are caveats required to perform an `upload/list` invocation.
-func UploadList(issuer principal.Signer, space did.DID, params uploadlist.Caveat, options ...Option) (receipt.Receipt[*uploadlist.Success, *uploadlist.Failure], error) {
+func BlobAdd(content io.Reader, issuer principal.Signer, space did.DID, receiptsURL *url.URL, options ...Option) (ipld.Link, delegation.Delegation, error) {


I would perhaps offer no illusion that this is streaming and require a []byte to be passed instead.

personal I'd push for io.Reader just to make for flexible input sources, even if there's no streaming in the uplaod.

yep, I like io.Reader too. I'd say in Go it's commonly used even when the vast majority of sources are []byte and don't support streaming.

alanshaw · 2025-04-28T10:40:01Z

+		return nil, nil, fmt.Errorf("computing content multihash: %w", err)
+	}
+
+	contentLink := cidlink.Link{Cid: cid.NewCidV1(0x0202, contentHash)}


I don't think we should assume this is a CAR. I would return contentHash instead. The caller should convert this to a CID.

what is 0x0202? Does a constant exist we can reference instead?

yeah it's in the multicodec table

more specifically here: https://github.com/multiformats/multicodec/blob/2f10269521a8f5d2c224a413d447bd7229b1e621/table.csv#L142

I agree we shouldn't assume a CAR here and that this constant in the go-multicodec package can be used. This slipped in when I transplanted the code from the previous storeShard function.

alanshaw · 2025-04-28T10:44:03Z

-//
-// The `params` are caveats required to perform an `upload/list` invocation.
-func UploadList(issuer principal.Signer, space did.DID, params uploadlist.Caveat, options ...Option) (receipt.Receipt[*uploadlist.Success, *uploadlist.Failure], error) {
+func BlobAdd(content io.Reader, issuer principal.Signer, space did.DID, receiptsURL *url.URL, options ...Option) (ipld.Link, delegation.Delegation, error) {


Would be good to have a comment explaining the delegation in the return.

alanshaw · 2025-04-28T10:47:30Z

+	return nil
+}
+
+func pollAccept(link ucan.Link, conn client.Connection, receiptsURL *url.URL) (receipt.AnyReceipt, error) {


link to what?

Suggested change

func pollAccept(link ucan.Link, conn client.Connection, receiptsURL *url.URL) (receipt.AnyReceipt, error) {

func pollAccept(task ucan.Link, conn client.Connection, receiptsURL *url.URL) (receipt.AnyReceipt, error) {

alanshaw · 2025-04-28T10:49:22Z

 func MustGetConnection() client.Connection {
 	// service URL & DID
-	serviceURL, err := url.Parse("https://up.web3.storage")
+	serviceURLStr := os.Getenv("RACHA_SERVICE_URL") // use env var preferably


Would be good if these were named the same as the JS CLI: https://github.com/storacha/upload-service/tree/main/packages/cli#environment-variables

Whilst shorter, I'm not sure everyone will instinctively make the connection that "ratcha" is the suffix of "storacha"...

yea RACHA is more for funzy names I prefer for STORACHA for all env vars

definitely! This was just me playing with the idea of calling our CLI client racha 😄

hannahhoward

Nice work. And apologies for the terrible DX of go-ucanto so far. We really need to do better.

The only hard change (pending confimation from @alanshaw) is to fix the correctness when handling blobs that have been uploaded once already, which should skip the put operation and just go right to conclude.

The other stuff is a bunch of "suggestions" that will make the code more readable.

hannahhoward · 2025-04-28T17:57:06Z

+		return nil, nil, fmt.Errorf("blob add failed: %v", fail)
+	}
+
+	var allocateTask, putTask, acceptTask invocation.Invocation


this method is super long, I might go ahead and extract parseBlobAddReceiptNext like in the JS client

absolutely, that's part of the refactor I'm working on for part II

hannahhoward · 2025-04-28T18:10:03Z

+				if err != nil {
+					return nil, nil, fmt.Errorf("bad allocate receipt in conclude fx: %w", err)
+				}
+			case w3sBlob.AllocateAbility:


There is a LOT of ceremony/complexity we're doing around distinguishing between legacy allocate vs allocate, same with accept / legacy accept.

These are actually identical structs. I wonder if the solution is to just modify go-libstoracha so that the receipts for legacy just uses the type from regular blob capabilities imported

alternatively, @alanshaw do we even need to support the web3.storage capabilities in the go-client? Is this for any other reason that the upload service is not in prod?

we still need to support web3.storage/blob because this is what legacy spaces use. But yeah, I was thinking of creating some common interface in go-libstoracha/capabilities to allow handling them indistinctly, but merging them makes sense to me and it's a more straightforward approach.

I captured this in storacha/go-libstoracha#19.

No because an old space will not go through warm storage nodes...

hannahhoward · 2025-04-28T18:12:00Z

+
+		address := allocateOk.Address
+		if address == nil {
+			return nil, nil, fmt.Errorf("blob allocation failed: no address")


I could be wrong, but I believe this is not correct

There is a legitimate reason address could be empty -- for a blob that is already uploaded. When this is the case, we should skip the upload entirely and right to accept.

@alanshaw can you confirm if I'm reading this right?

see: https://github.com/storacha/upload-service/blob/main/packages/upload-client/src/blob/add.js#L230

changed to follow the implementation in upload-client. If this is not correct we can change it back afterwards.

Yes, I can confirm, no address is the indication that the service already has the blob and no upload is necessary. It is not an error.

https://github.com/storacha/specs/blob/main/w3-blob.md#allocation-address

hannahhoward · 2025-04-28T18:15:46Z

+		return fmt.Errorf("invalid put facts, 'keys' field is not a node")
+	}
+
+	// TODO: define a schema and use bindnode.Rebind rather than doing this manually


yea that will make you a happier camper :P dealing with raw ipld.Node interfaces is a mess

hannahhoward · 2025-04-28T18:26:50Z

+		return fmt.Errorf("receipt not found: %s", httpPutConcludeInvocation.Link())
+	}
+
+	reader, err := receipt.NewReceiptReaderFromTypes[ucancap.ConcludeOk, fdm.FailureModel](ucancap.ConcludeOkType(), fdm.FailureType(), captypes.Converters...)


I might consider putting stuff like this outside the functions, a.l.a.

var receiptReader ReceiptReader[ucancap.ConcludeOk, fdm.FailureModel] func init() { receiptReader, err := receipt.NewReceiptReaderFromTypes[ucancap.ConcludeOk, fdm.FailureModel](ucancap.ConcludeOkType(), fdm.FailureType(), captypes.Converters...) if err != nil { panic(err) } }

In fact, I might go a step futher and just put these in go-libstoracha to avoid having to make them over and over, just like we do with the capabilities

I like the idea. The original capabilities implemented in go-w3up did this.

Captured in storacha/go-libstoracha#20

frrist · 2025-04-28T17:46:28Z

+	receiptsURL := util.MustGetReceiptsURL()
+
+	// Handle options
+	isCAR := cCtx.String("car") != ""


I suggest inferring whether a file is a car file based on its extension (e.g., thing.car would be treated as a car file, while thing.txt or thing would be treated as regular files). This approach eliminates the need for users to set an additional flag when issuing CLI commands and simplifies the upload process to just accepting whatever file is passed as an argument. What do you think?

I also see that this flags inclusion in the code pre-exists your change, but since we are in the early phase of building the repo out, might be worth getting this right now.

Yeah we've done that before, but then if folks want to pipe a CAR file in it will be treated as a file not a CAR. This is useful when it's being generated on the fly and not already a file. Also when users want to upload a CAR that doesn't have a .car suffix they have to rename and finally, if they want to upload it as a UnixFS file (FML IDK why) you end up having to add a flag that disables the behaviour. I had the same stance originally until this literally happened...

frrist · 2025-04-28T17:54:33Z

+
+	// Handle options
+	isCAR := cCtx.String("car") != ""
+	isJSON := cCtx.Bool("json")


In the future, we may want to support output formats beyond just json. At minimum, we should clearly indicate to users that this flag controls the output format. I suggest renaming this flag to output and having it accept values like json, console (or possibly text or plaintext).

I've been thinking about an issue with JSON output. When we add the output option to make command results machine-readable, we're creating an expectation that nothing else will be written to stdout. For example, when a user runs:
w3up upload file file.txt | jq .'Root' | <some other command>

The jq command will fail if anything besides pure JSON appears in stdout. Currently, methods like uploadCAR use fmt.Println for logging, which writes to stdout and would break JSON parsing.

To solve this, we should direct all logging to stderr while sending only the actual command output (JSON or plaintext) to stdout. Our current CLI library, github.com/urfave/cli/v2, doesn't handle this separation well. We have at least two options:

Replace fmt.Print* calls with a logger that writes to stderr

Switch to a more robust CLI library like https://github.com/spf13/cobra that better manages stdout/stderr separation

More generally, it seems beneficial to maintain a separation between data (stdout) and status messages/logs (stderr). This approach aligns with common practices in command-line tool design and would improve compatibility with tools that process our output.

frrist · 2025-04-28T18:01:06Z

+						Name:  "wrap",
+						Value: true,
+						Usage: "Wrap single input file in a directory. Has no effect on directory or CAR uploads. Pass --no-wrap to disable.",


I recommend setting the default value for this flag to false. This would allow users to include --wrap only when they want to wrap a file in a directory, or omit it when they don't. Generally, defaulting boolean flags to true creates user experience problems since it requires awkward syntax like w3up upload --wrap=false file.txt. A cleaner approach would be w3up upload file.txt (without wrapping) and w3up upload --wrap file.txt (with wrapping).

mmm, I'm not sure about this one. AFAICT wrap is what the user will want most of the times, so it may make sense that it defaults to true and some cumbersome syntax is required to disable. @alanshaw what are your thoughts on this?

Sorry to be pedantic about all this, but iff that is the case I'd suggest naming the flag --no-wrap (or similar/more descriptive). In this way the syntax becomes w3up --no-wrap files-n-stuff

yeah, I like that

Yes, in my experience wrap is what users want most of the time and we encourage --no-wrap in the CLI for when they don't.

In the JS CLI framework we're using it automatically enables --no-XXX for boolean flags but you have to define as wrap - this is maybe where this came from?

frrist · 2025-04-28T18:04:16Z

+func uploadCAR(path string, signer principal.Signer, conn uclient.Connection, space did.DID, proofs []delegation.Delegation, receiptsURL *url.URL) (ipld.Link, error) {
+	f0, err := os.Open(path)
+	if err != nil {
+		return nil, fmt.Errorf("opening file: %s", err)


lets be sure to wrap errors here, and else where. i.e.

return nil, fmt.Errorf("opening file: %w", err)

frrist · 2025-04-28T18:07:02Z

+					},
+					&cli.IntFlag{
+						Name:  "concurrent-requests",
+						Value: 5,


Would it be better to default this value to 1? This approach would require users to explicitly specify the flag when they want concurrency, rather than having concurrency built into the command by default.

yeah, I don't know. It is currently not used, so at this point I'd be more on the side of removing it altogether. In any case, we will need to think through this carefully when we optimize uploads and implement resumability, so I'm inclined to leave it as is for now as the CLI is in general not super usable right now.

at this point I'd be more on the side of removing it altogether

In favor of that, and re-introducing when it's required.

frrist · 2025-04-28T18:44:25Z

+		return nil, nil, fmt.Errorf("computing content multihash: %w", err)
+	}
+
+	contentLink := cidlink.Link{Cid: cid.NewCidV1(0x0202, contentHash)}


what is 0x0202? Does a constant exist we can reference instead?

frrist · 2025-04-28T18:48:16Z

+			return nil, nil, fmt.Errorf("reading ucan/conclude receipt: %w", err)
+		}
+
+		switch concludeRcpt.Ran().Link() {


Let's also include a default case, logging a warning or erroring in the event concludeFX doesn't match the any of our cases.

the original implementation ignores other effects and receipts and the spec doesn't state the invocation should fail if unexpected effects are included, but logging a warning is a good idea.

frrist · 2025-04-28T18:50:07Z

+	if err != nil {
+		return fmt.Errorf("uploading blob: %w", err)
+	}
+	io.ReadAll(resp.Body)


Do we need to ready the body here?

back in the days draining the body all the way to EOF was necessary to allow connections to be re-used and I got used to always read and close response bodies, even when I don't care about their contents.

That said, it looks like today Response.Body.Close() also drains the body if it wasn't read, so it's probably not required. The docs still state the body needs to be read fully, and I don't think doing it explicitly does any harm.

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

frrist · 2025-04-28T18:55:12Z

-//
-// The `params` are caveats required to perform an `upload/list` invocation.
-func UploadList(issuer principal.Signer, space did.DID, params uploadlist.Caveat, options ...Option) (receipt.Receipt[*uploadlist.Success, *uploadlist.Failure], error) {
+func BlobAdd(content io.Reader, issuer principal.Signer, space did.DID, receiptsURL *url.URL, options ...Option) (ipld.Link, delegation.Delegation, error) {


Although it won't be used yet, lets pass a context.Context as the first argument to this method. It will help us down the road whenever we need to implement cancellation of methods/flow control.

@volmedo of all review-comments I've left, aside from the ones you've 👍 I'd really like these context plumbing ones addressed before merging.

I agree, but I'd rather not adding it right now as a signal that context cancellation is actually not supported. Let's add it when it's actually needed.

I added my previous reply before the page was refreshed with your latest reply 😅. I'll add it in.

I added contexts and also created this ticket so that we add support for them in go-ucanto: storacha/go-ucanto#43

frrist · 2025-04-28T18:58:00Z

+	return nil
+}
+
+func pollAccept(link ucan.Link, conn client.Connection, receiptsURL *url.URL) (receipt.AnyReceipt, error) {


Similar comment wrt context.Context here, lets include one in the function arguments. We can wire it into the http request here via http.NewRequestWithContext

…is nil

volmedo · 2025-04-30T17:59:33Z

thank you all for the review and comments. I have addressed many of them in this PR, deferred another to a follow up PR and captured the rest in these tickets:

@frrist as you likely already guessed, I mostly took the existing code dealing with CLI flag management and CAR handling and used it as is, and that code was mostly at PoC level rather than being super usable. Sorry, I forgot to mention that in the PR description. I'm pushing most of the issues related to that to the future.

Let me know whether I missed any outstanding issues.

frrist

Nothing more to add on my end. Would be good to get an answer from @alanshaw wrt car files with multiple roots

frrist · 2025-04-30T19:40:05Z

+func uploadFile(ctx context.Context, path string, signer principal.Signer, conn uclient.Connection, space did.DID, proofs []delegation.Delegation, receiptsURL *url.URL) (ipld.Link, error) {
+	return nil, nil
+}
+
+func uploadDirectory(ctx context.Context, paths []string, signer principal.Signer, conn uclient.Connection, space did.DID, proofs []delegation.Delegation, receiptsURL *url.URL) (ipld.Link, error) {
+	return nil, nil
+}


nit: These should return errors or panic if they are not implemented

frrist · 2025-04-30T19:41:02Z

+replace github.com/storacha/go-libstoracha => ../go-libstoracha
+
+replace github.com/storacha/go-ucanto => ../go-ucanto


These ought to be removed.

yes. As I mentioned in the "Dependencies" section in the PR's description, I still need to merge a couple things in that other repos. Once they are in, I'll point to the right versions.

The checks won't pass until I do that, so it's unlikely I'll forget doing it before merging 😄

volmedo added 6 commits April 8, 2025 13:51

inital implementation

7446b55

use legacy blob capabilities

4b8f87b

fix HTTP PUT request not having ContentLength set

b9e9698

fix ucan/conclude with http/put receipt

8046cb8

wip: poll receipts endpoint for blob/accept receipt

94de2f8

point to new service in staging

27475df

volmedo self-assigned this Apr 9, 2025

volmedo added 7 commits April 17, 2025 17:35

get receipts URL

3a0e1f9

poll accept receipt

d638c08

use go-libstoracha upload/add capability

6f674c8

minimal UI feedback

da67388

first iteration of the test passing

733df42

refactor test server code

9da0fbf

remove store/add, which is deprecated

42eaf22

volmedo changed the title ~~feat: upload blobs~~ feat: upload blobs (part I) Apr 28, 2025

volmedo marked this pull request as ready for review April 28, 2025 07:35

volmedo marked this pull request as draft April 28, 2025 07:36

volmedo requested a review from a team April 28, 2025 07:43

alanshaw requested changes Apr 28, 2025

View reviewed changes

hannahhoward requested changes Apr 28, 2025

View reviewed changes

frrist requested changes Apr 28, 2025

View reviewed changes

volmedo added 4 commits April 29, 2025 18:18

feedback: BlobAdd should not assume CARs

046be51

feedback: add comment about what BlobAdd returns

fc7e520

feedback: better param name in pollAccept

a3cf50d

feedback: use STORACHA as prefix for env vars

009c23a

volmedo mentioned this pull request Apr 30, 2025

Go client CLI UX improvements #14

Open

volmedo added 4 commits April 30, 2025 11:49

feedback: wrap errors and avoid log.Fatal where inappropriate

1d7a458

feedback: check if path is a directory when uploading CARs

4b3b616

feedback: fail fast if CAR decoding fails

cef4ddd

feedback: alias capability packages consistently

a42c94c

volmedo added 3 commits April 30, 2025 12:40

feedback: add default cases to switch statements where appropriate

54ad2c8

feedback: add contexts

435e807

feedback: assume blob has already been uploaded if allocated address …

8f5e474

…is nil

volmedo marked this pull request as ready for review April 30, 2025 17:59

frrist approved these changes Apr 30, 2025

View reviewed changes

volmedo added 4 commits May 1, 2025 14:50

feedback: remove concurrent-requests flag, which is unused

7e92922

feedback: pass CAR roots to sharder

b864f3d

feedback: unimplemented functions return errors

7285d19

use proper releases of go-ucanto and go-libstoracha

919c416

volmedo merged commit c0ddf1c into main May 12, 2025

volmedo deleted the vic/feat/upload-blobs branch May 12, 2025 08:42

	func pollAccept(link ucan.Link, conn client.Connection, receiptsURL *url.URL) (receipt.AnyReceipt, error) {
	func pollAccept(task ucan.Link, conn client.Connection, receiptsURL *url.URL) (receipt.AnyReceipt, error) {

		replace github.com/storacha/go-libstoracha => ../go-libstoracha

		replace github.com/storacha/go-ucanto => ../go-ucanto

Uh oh!

Conversation

volmedo commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WIP Disclaimer

What are the changes?

Dependencies

Uh oh!

alanshaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannahhoward left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frrist Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanshaw May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volmedo commented Apr 9, 2025 •

edited

Loading

frrist Apr 28, 2025 •

edited

Loading

alanshaw May 1, 2025 •

edited

Loading