Download segment data only once for verification #1393
Conversation
yondonfu
force-pushed
the
mk/download-segment-data-once#1382
branch
from
0bb3a78
to
08842a3
Compare
yondonfu
dismissed
their stale review
Dismissing my own review because the latest changes I pushed resolves the request.
server/broadcast.go
Outdated
| renditionData := make([][]byte, len(segData)) | ||
| for i := range segData { | ||
| renditionData[i] = make([]byte, len(segData[i])) | ||
| copy(renditionData[i], segData[i]) |
There was a problem hiding this comment.
Can't we just do renditionData[i] = segData[i] ?
There was a problem hiding this comment.
I originally copied the data in segData[i] because changes to segData[i] would result in changes to params.Renditions[i] since they would point to the same slice header. But, segData is not used anywhere in server/broadcast.go besides in verify() so it should be safe to set params.Renditions to segData. In 75f9d11, I updated a few tests to allocate a new slice for segData whenever passing it as an argument to verify() (otherwise, some tests would fail because mutations to the same slice for segData would mutate the values for params.Renditions).
There was a problem hiding this comment.
Ahh I see, I would be okay with not mutating as a good practice here as well. I'll leave it up to you.
There was a problem hiding this comment.
Going to keep this change as avoiding the copy of segment data (which can be largeish) is nice.
There was a problem hiding this comment.
Ah yes didn't think about the size, good point.
|
Changes look good. I'll leave that one decision up to you whether you want to drop 75f9d11 or not. Can we rebase and resolve conflicts? |
Download the segment data if a verification policy is set or if the data needs to be uploaded to the broadcaster's own OS.
yondonfu
force-pushed
the
mk/download-segment-data-once#1382
branch
from
75f9d11
to
edde06d
Compare
|
Rebased. |
Broadcaster nodes should download segment data once for both sig verification and pixel count verification
closes #1382