fix: stamped putter support for streamed chunk endpoint #5256
Conversation
agazso
force-pushed
the
fix/chunk-stream-stamped-putter
branch
from
430a35a to
0f0c7c8
Compare
| default: | ||
| jsonhttp.BadRequest(w, nil) | ||
| // Create connection-level putter only if BatchID is provided | ||
| // If BatchID is not provided, per-chunk stamps must be used |
There was a problem hiding this comment.
I'm not sure I fully understand what per-chunk stamps are. Do you mean these are presigned stamps? If so, consider changing the comment to something like if ...., the API caller is expected to provide pre-signed stamps (and is also expected to keep track over stamp state over time)
pkg/api/chunk_stream.go
Outdated
| } | ||
|
|
||
| chunk, err := cac.NewWithDataSpan(msg) | ||
| logger.Debug("chunk upload stream", |
There was a problem hiding this comment.
do we really need this message?
pkg/api/chunk_stream.go
Outdated
| ) | ||
|
|
||
| // If message is large enough to contain a stamp + chunk data, try to extract the stamp | ||
| if len(msg) >= postage.StampSize+swarm.SpanSize { |
There was a problem hiding this comment.
Suggestion: reduce nesting here.
Compare:
if ... {
.....
if ....
}vs.
if condition {
return
}
....
if ...Personally I find the latter easier to read. In the context here: check once that the size constraints match, return if not, then continue execution, no need to branch out.
pkg/api/chunk_stream.go
Outdated
| potentialStamp := msg[:postage.StampSize] | ||
| potentialChunkData := msg[postage.StampSize:] | ||
|
|
||
| logger.Debug("chunk upload stream: attempting to extract stamp", |
There was a problem hiding this comment.
nit: probably can remove this once this is inching closer to where we want it to go
pkg/api/chunk_stream.go
Outdated
|
|
||
| // Try to unmarshal as a stamp | ||
| stamp := postage.Stamp{} | ||
| if err := stamp.UnmarshalBinary(potentialStamp); err == nil { |
There was a problem hiding this comment.
So here I have some doubts about the approach, I think doing guesswork here is probably not necessary.
Let's break down the possibilities:
- batch ID was provided - we will use 1 batch across the entire session and stamp all stamps. Chunks arrive without a stamp.
- batch ID was not provided - stamps can arrive in theory with any batch ID. Stamps are prepended to chunk.
My issue with this is:
- You have to guess whether the chunk is supposed to have a stamp or not (you might try to unmarshal the chunk as a stamp just because it falls, size-wise, as maybe a chunk with a stamp.
- Then the rest of the conditional logic happens if it's not a stamp+chunk.
But, you already know. In L61 you already know whether you should expect a stamp or not. This can be passed downstream either as a bool or not. Then have two functions that decode a message that have the same signature, e.g.:
func decodeWithStamp(msg []byte) (stamp, chunk, err) {...}
func decodeNoStamp(msg []byte) (stamp, chunk, err) {...}The latter will always return a nil stamp.
Then you still need to figure out which batch ID to use but that's not difficult.
I think that if you do a small changes here (including injecting the correct message decoder, the original code almost doesn't need to change, apart from the batch caching that you added. Hopefully this helps.
3 participants
Checklist
Description
This PR adds support for the WebSocket based chunk put endpoint to accept pre-signed (stamped) chunks.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
The WebSocket based chunk put endpoint exists as a performance optimization so that chunks can be uploaded with smaller overhead than on the HTTP chunk endpoint. However currently the WebSocket based endpoint does not support pre-signed chunks. The proposed change adds 113 extra bytes to the chunk, which is the size of the serialized postage stamp structure (see
postage.stampSize).At the moment the PR is not ready for merging, because although technically it is working it is quite slow, most likely because it creates a new
PutterSessionwith a pre-signed stamper for each chunk to be uploaded.