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

feat: egress billing RFC #28

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: egress billing RFC #28

wants to merge 11 commits into from

Conversation

travis
Copy link
Member

@travis travis commented May 28, 2024

First writeup of the proposal for egress billing. Looking for feedback on the ideas and on the coherence of the writeup.

Preview

First writeup of the proposal for egress billing. Looking for feedback on the ideas and on the coherence of the writeup.
@travis travis marked this pull request as draft May 28, 2024 23:59
@travis travis self-assigned this May 28, 2024
}

async function handle(request){
const rateLimited = checkRedisToDetermineIfRateLimited(request)
Copy link
Member

Choose a reason for hiding this comment

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

I would just not reference the specific tool since it's an implementation detail.

Suggested change
const rateLimited = checkRedisToDetermineIfRateLimited(request)
const rateLimited = isRateLimited(request)

Copy link
Member Author

Choose a reason for hiding this comment

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

ah I hear you but I am trying to convey that only "Redis" should consulted here - I've come to think of "Redis" as an API not a product (because that seems to be the reality of it in 2024) - I do mention earlier that I'm using Redis as an example and I think this is consistent with that, so I'd like to keep it

Comment on lines +101 to +104
case No:
return { status: 200, body: readContentFromW3up(cid) }
case Maybe:
return { status: 200, body: readContentFromW3up(cid) }
Copy link
Member

Choose a reason for hiding this comment

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

To me this is clearer since it's saying to do the same thing in either case:

Suggested change
case No:
return { status: 200, body: readContentFromW3up(cid) }
case Maybe:
return { status: 200, body: readContentFromW3up(cid) }
case No:
case Maybe:
return { status: 200, body: readContentFromW3up(cid) }

Copy link
Member Author

Choose a reason for hiding this comment

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

ah I was worried that people who aren't JS-brained would misread that as "if No, do nothing" - I could definitely be convinced but imho the way I've done it makes it a little clearer that something should be done in each case, and since they are right next to eachother it's pretty easy to verify that they're the same thing...

}
```

[TODO: should I translate this into UCAN.current too?]
Copy link
Member

Choose a reason for hiding this comment

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

I would, as a <details/>

// multihash must match an uploaded blob
["==", ".content", { "/": { "bytes": "mEi...sfKg" } }],
// must be available from this url
["==", ".url", "https://asia.w3s.link/ipfs/bafk...7fi"],
Copy link
Member

Choose a reason for hiding this comment

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

I guess this will typically be a URL and a byte range? Like existing location claim: https://github.com/w3s-project/content-claims?tab=readme-ov-file#location-claim

Copy link
Member Author

Choose a reason for hiding this comment

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

that makes sense - I was cargo culting off of @Gozala so redirect this one over to him for confirmation...

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be a range yeah, but I think we made it optional if it's not a segment but the whole thing

"sub": "did:web:asia.web3.storage",
"pol": [
// Request origin header must be example.com
["==", ".headers['origin']", "example.com"],
Copy link
Member

Choose a reason for hiding this comment

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

What is the significance of ['origin'] over .origin here? Below .token is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, nothing that I know of - again just copying @Gozala, maybe he was trying to illustrate that either style can be used? I'm happy to use .origin!

}
```

For any given CID, the gateway will be able to look up the CID-specific content commitment as well as the
Copy link
Member

Choose a reason for hiding this comment

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

For any given CID

I think we're talking DAG root CID not any...right?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh that's a good point, I actually don't know how this will handle CIDs that aren't DAG roots - do those even get content commitments back from the blob upload service? @Gozala any thoughts on this? I'm not sure we accounted for these CIDs in our chats...

Copy link
Member

Choose a reason for hiding this comment

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

^^^ important point. I think there's an intersection where we need to understand how this relates to the indexing RFCs. Fine to just handle DAG root to start but having w3s.link ONLY work for DAG root is very frustrating and I see the indexing work heading in a direction where we're not limited in that way.


Note that `bafy..access` alone will not be enough to authorize retrieval
of the content - the accounting system will need to combine it with
a CID-specific content commitment that has been delegated to `Group`.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following how this works, I think you need to show the "CID-specific content commitment that has been delegated to Group"

Copy link
Member Author

Choose a reason for hiding this comment

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

oh that's the first one, bafy..group - I've updated the text to clarify that!

first implement a global per-CID rate-limit that is expected to account for N%
of _R_. This rate limit will be used for all content that has not been configured with another
rate limit, and for content that has been configured with other rate limits that have already
been reached.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to model this as "just another user that set a rate limt", where "another user" is us. Eventually, some entity will have to pay the egress from the nodes storing the content, so we do need to be able to allocate responsibility for that egress to some entity.

i.e. there's no magic "global" limit, it's just some user (us) that has set a rate limit that happens to apply to ALL content served by the gateway.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea completely agree!

Copy link
Contributor

Choose a reason for hiding this comment

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

🎯


We may want to consider using an open source alternative like https://github.com/nalgeon/redka if we
are concerned about using "source available" products - my inclination is to use Redis but consider products like
Redka as a fallback in case Redis no longer meets our needs. We may also want to consider using AWS or Cloudflare products with Redis-compatible APIs:
Copy link
Member

Choose a reason for hiding this comment

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

Cloudflare's KV is super fast since it's eventually consistent, which feels like a good fit.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah cool will add it to the list!

I will say that one of the core requirements here is the ability to implement efficient rate limits - you can see an example of what that could mean here: https://redis.io/glossary/rate-limiting/

I think (but am not sure) that one of the key things is support for the INCR operation, which I don't think KV implements, and expiring entries, which may be better supported?

The good news is that lots of stuff seems to implement the Redis query interface these days, including Cloudflare's Upstash and no less than two different AWS products - I think we'll have options!

system. We expect this to result in some amount of "unauthorized" read request service - for example, if two requests for
the same content arrive at nearly the same time they will always both be served even if the second request
goes over all established rate limits, since the rate limits will only be updated some time after a request is served.
This pattern is similar in some ways to the "stale while revalidate" pattern that is popular on the web today - stale
Copy link
Member

Choose a reason for hiding this comment

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

"fail open" also comes to mind...

Copy link
Member Author

Choose a reason for hiding this comment

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

yea love this!

Comment on lines +10 to +11
We need to start charging for data served by our gateway ("gateway egress"). Users who upload data using `w3up` should
be charged when we serve that data through the gateway that is currently hosted at https://w3s.link. Importantly, if
Copy link
Contributor

Choose a reason for hiding this comment

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

are we strictly tackling gateway, or do we also plan to bitswap? I think some of the ideas here could be applied there as well, even though with extra layer of indirection to figure out the blob CID where a block CID belongs to

Copy link
Member Author

Choose a reason for hiding this comment

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

ah I was thinking of the "gateway" as something that is also handling bitswap - imho the gateway and bitswap provider should both work off the same rate limits

Copy link
Contributor

Choose a reason for hiding this comment

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

probably I would make this extra specific and add it to spec then :) There are things like origin that are actually only specific for HTTP and as far as I know won't be available in bitswap.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yea agree, will do!

Copy link
Contributor

Choose a reason for hiding this comment

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

There are things like origin that are actually only specific for HTTP and as far as I know won't be available in bitswap.

I would expect that libp2p node will be delegated capability explicitly and constraints there may be different from the ones imposed on gateway (described here). I'm not sure what those constraints could be perhaps something like peer id that can perform a request.

@travis travis marked this pull request as ready for review May 30, 2024 16:39
Copy link
Member

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

I think I'm approving generally, but if we're going to work on Egress billing and indexing at once, we really need to sync on these two designs. I think we still have some confusion and I wonder if when we move beyond prototype we do the indexing first cause it's really essential to all this.


When the gateway receives a request, it will query the [content claims service] for claims about the requested
CID [TODO: and also other information like origin and token?]. If multiple claims establishing different rate limits
are found, one is chosen randomly and its request count is incremented, resulting in billing to the
Copy link
Member

Choose a reason for hiding this comment

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

There's an important clarification I hope I'm reading right.

When are multiple claims found?

If you have to match token and origin, why would you have multiple claims?

Or maybe you have the matching unlimited claim and the general open access rate limited claim? In which case I'd lean towards using the unlimited claim rather than picking at random.

travis added a commit to storacha/freeway that referenced this pull request Jun 13, 2024
Add a new middleware that checks a rate limiting service and returns a 429 if the CID is over a rate limit.

This sketches out an API for the rate limiting and accounting services suggested in storacha/RFC#28

This is not ready to merge, but should probably be the starting point for this work once we all agree that this is the right shape.
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.

5 participants