Proposal: gRPC streaming API

[Mirko-von-Leipzig]

We should strongly consider replacing our request/response SyncXxx methods with streaming versions instead.

This has a few benefits and imo matches the current client sync process much better. As a short overview, the client syncs by:

1. Using SyncMmr to a specific block.
2. Using the other SyncXxx methods to sync up to the block from (1).

I've added @kkovaacs's AI summaries of the status quo and streaming changes. imo they make a pretty strong case for this proposal but also contains additional suggestions which are maybe less relevant. I'll add my own thoughts below.

status quo.md
streaming.md

Rollout

I suggest we roll this out as a non-breaking change for v0.16 by adding new gRPC streaming methods, either as a separate rpc.Streaming service, or simply by XxxStreaming.

The client can then implement and test these whenever they have time. This also means we can roll these out a method at a time without worrying about deploying updates.

If things go well, in v0.17 we yank the original methods.

Problems this addresses

Response sizes

We have several issues/bugs related to this, but effectively:

Response sizes are difficult to estimate accurately since we need the item's size in protobuf plus protobufs framing thereof. And we can't have that because the response is a struct until tonic encodes it so we never truly have access to the response size until after we are done constructing it.

This is technically possible, but would require re-implementing grpc's codec or at least figuring it out for our collection types.

Pagination

Response size limits means we need to paginate user requests. Our pagination uses blocks as boundaries which complicates implementations since we must first reach the size limit, then backtrack any incomplete block.

It also means we cannot easily elide redundant data, e.g. we send every account storage update since we cannot know when the response limit may be reached. We could use an alternative pagination strategy here, but it would be convenient to have a more standardize approach.

With streaming we don't need to paginate and can instead stream the final result at the request's endpoint. This addresses several issues regarding sending redundant data.

Complexity

imo streaming results in much simpler client side code since pagination is a fairly involved and painful process.

On the node side, we sidestep the entire problem of response sizes, to which we do not have a good solution.

Implementation

I think we should still paginate in the stream handler in the node. This avoids memory spikes and hogging the database connection. Handler becomes:

1. Load page
2. Stream page
3. Repeat

This should be standardized with a trait e.g. async fn Paginator::load_next_page(). Then SyncStream<Paginator> handles the orchestration.

A good follow-up optimisation is using a double-buffer for page loading so there is no downtime where we wait for the next page.

We may run into issues if this requires holding a long-lasting RocksDb snapshots but if that's the case, then its a problem with normal pagination as well.

I would also avoid a continuation token unless absolutely necessary. That means data from an interrupted stream should be considered broken and dropped. If this is deemed risky, then I would prefer the client shortens its request range to limit the database i.e. the client can "paginate".

A stream continuation token would require an absolute ordering which is not always trivially present.

Final thoughts

Imo the only downside is the API churn this late in the game. However I do think its well worth it (if the design survives contact with reality).

[Mirko-von-Leipzig]

cc @igamigo @bobbinth @kkovaacs @sergerad @SantiagoPittella for comments.

[igamigo]

Some general comments on the status quo:

Response sizes are difficult to estimate accurately since we need the item's size in protobuf plus protobufs framing thereof. And we can't have that because the response is a struct until tonic encodes it so we never truly have access to the response size until after we are done constructing it.

I agree, but I would note that estimating the size of a raw struct consistently should be doable. Protobuf framing should also be predictable (we can also drop the remainder right before sending the responses to be sure) and if we allow for an error margin we should have no issues. However, I agree this is error prone and not very easy to evolve and so I'm 100% on board with trying to avoid this whenever possible.

We may run into issues if this requires holding a long-lasting RocksDb snapshots but if that's the case, then its a problem with normal pagination as well.

On the sync endpoints, is there any data that gets read from RocksDB at all? I don't think we check nullifiers from the tree itself, and there are no account proofs that are sent either. I do agree that current "consistency-over-time" problems would still remain though, but I don't think there are any meaningful ones: SyncAccountVault/SyncAccountStorageMaps do explicitly allow sending and receiving stale data, and that's already handled from teh client side.

EDIT: ah, I think we sync forked public accounts (e.g. your local client's state differs from the network's) which implies using GetAccount

On the proposal:

I agree we should implement this soon. From the user perspective I think it would be faster and more efficient, although it's hard to tell by how much (especially in browser clients which I'm not sure how they implement this). It also shouldn't pose any threats to try it because the current mechanisms are already working well, so your rollout plan seems sensible.

I'm not entirely sure how the DDoS-related features should interact with this change: currently pagination naturally "inflates" the rate-limiting tokens, whereas a streaming request would cost a single token (I think?) and can command a very large amount of node and DB work, especially with current limits (I think like 1000 tags are allowed on the SyncNotes endpoint). Current limits are very permissive anyway AFAIK, so maybe there is not much of a practical difference but I'd say it would be worth reviewing some of these parameters.

I also don't know what the current deployment setup looks like, but I wonder if there is anything to take into account in relation to reverse proxies/LBs that might exist in front of the endpoint.

I think we should still paginate in the stream handler in the node. This avoids memory spikes and hogging the database connection.

Yeah, I think this would have to be a requirement. Otherwise you'd risk delaying checkpointing, exhausting the connection pool, memory spikes, etc.

Some caveats/questions:

I would also avoid a continuation token unless absolutely necessary. That means data from an interrupted stream should be considered broken and dropped. If this is deemed risky, then I would prefer the client shortens its request range to limit the database i.e. the client can "paginate".

Maybe you meant something else, but because SyncMMR always gets you up to the latest block, I'm not precisely sure a client can decide to divide a sync into multiple block ranges. Specifically, endpoints like SyncNotes return blocks with their authentication paths that you are supposed to start tracking in your client MMR. Because these blocks are opened at the block_to forest, you would get a truncated authentication path. This should be easy to fix, but also there are different ways to tackle it so it might warrant some discussion. I wonder if we want to go back to letting users decide the cutoff block for every endpoint (SyncMMR included). I think this could have some other benefits, but this is a separate discussion.
A client can try to divide request in some other ways (if you have multiple tags, you can send multiple requests for non-overlapping subsets of them) and this would help, but this is not always possible and you could potentially be retrieving a bunch of duplicate data unnecessarily.

A stream continuation token would require an absolute ordering which is not always trivially present.

But isn't this the case already? Because these endpoints paginate to the block level we still require ordering. But I guess this is strictly granular at the block level only. In any case I do agree we can do away with tokens and retrying should work fine. I do wonder if there are ways to grief more and try to exhaust connections, etc. in a way that harms other clients, but as briefly touched upon above I think some of the protection mechanisms shoudl be revised.

streaming.md states:

Returning one note per message deliberately duplicates the block header and MMR path when several matching notes share a block. The benefits are: ...

I would definitely not go for duplicating header and proof on each note. Ordering here should be trivial, right? The note tree is already absolutely ordered (not sure at the SQL level but this should be simple to solve) so the approach where you send either a block or a note should be better.

[Mirko-von-Leipzig]

Some general comments on the status quo:

<...snip...> agreed.

I'm not entirely sure how the DDoS-related features should interact with this change: currently pagination naturally "inflates" the rate-limiting tokens, whereas a streaming request would cost a single token (I think?) and can command a very large amount of node and DB work, especially with current limits (I think like 1000 tags are allowed on the SyncNotes endpoint). Current limits are very permissive anyway AFAIK, so maybe there is not much of a practical difference but I'd say it would be worth reviewing some of these parameters.

I think in general streams are more efficient for both server and client. So if the client requires this data in any case, then its best to just give it to them. We could for example limit the number of open streams per client instead.

I also don't know what the current deployment setup looks like, but I wonder if there is anything to take into account in relation to reverse proxies/LBs that might exist in front of the endpoint.

👍 also unclear to me; but if we had started with streams.. then we also wouldn't have considered this.

I think we should still paginate in the stream handler in the node. This avoids memory spikes and hogging the database connection.

Yeah, I think this would have to be a requirement. Otherwise you'd risk delaying checkpointing, exhausting the connection pool, memory spikes, etc.

👍

Some caveats/questions:

I would also avoid a continuation token unless absolutely necessary. That means data from an interrupted stream should be considered broken and dropped. If this is deemed risky, then I would prefer the client shortens its request range to limit the database i.e. the client can "paginate".

Maybe you meant something else, but because SyncMMR always gets you up to the latest block, I'm not precisely sure a client can decide to divide a sync into multiple block ranges. Specifically, endpoints like SyncNotes return blocks with their authentication paths that you are supposed to start tracking in your client MMR. Because these blocks are opened at the block_to forest, you would get a truncated authentication path. This should be easy to fix, but also there are different ways to tackle it so it might warrant some discussion. I wonder if we want to go back to letting users decide the cutoff block for every endpoint (SyncMMR included). I think this could have some other benefits, but this is a separate discussion. A client can try to divide request in some other ways (if you have multiple tags, you can send multiple requests for non-overlapping subsets of them) and this would help, but this is not always possible and you could potentially be retrieving a bunch of duplicate data unnecessarily.

Yes I was thinking we should let the client specify the target height so you can reliable paginate if you need to sync "all of history" for example. But maybe that isn't required, or we should provide a different method to sync that much data. We could ignore this for now and add the target block in a non-breaking schema update.

Though if you're syncing a massive amount from a potentially untrusted rpc, then you probably want to chunk to verify the data in reasonable step sizes.

A stream continuation token would require an absolute ordering which is not always trivially present.

But isn't this the case already? Because these endpoints paginate to the block level we still require ordering. But I guess this is strictly granular at the block level only. In any case I do agree we can do away with tokens and retrying should work fine. I do wonder if there are ways to grief more and try to exhaust connections, etc. in a way that harms other clients, but as briefly touched upon above I think some of the protection mechanisms shoudl be revised.

Its only the case if we're still streaming per block. If you're streaming just the final state then I'm unsure if its consistent, especially if a reset stream connects to a different node.

streaming.md states:

Returning one note per message deliberately duplicates the block header and MMR path when several matching notes share a block. The benefits are: ...

I would definitely not go for duplicating header and proof on each note. Ordering here should be trivial, right? The note tree is already absolutely ordered (not sure at the SQL level but this should be simple to solve) so the approach where you send either a block or a note should be better.

I think we can chalk this up to "hallucination" or "bad idea".