fix(pdp): remove unpadding transformation from root size #534
Conversation
Co-authored-by: zenground0 <[email protected]>
| @@ -801,7 +801,7 @@ func (p *PDPService) handleAddRootToProofSet(w http.ResponseWriter, r *http.Requ | |||
| } | |||
|
|
|||
| prevSubrootSize = subrootInfo.PieceInfo.Size | |||
| totalSize += uint64(subrootInfo.PieceInfo.Size.Unpadded()) | |||
| totalSize += uint64(subrootInfo.PieceInfo.Size) | |||
There was a problem hiding this comment.
Can't we do this conditionally for when size is less than 2048 instead of affecting all pieces regardless of size?
There was a problem hiding this comment.
I think this bug exists because of a quick in the Go version of this calculation, and it has to be consistent.
In the Pandora contract we're now doing authorization signing so that clients have to sign a blob that Curio passes on up through PDPVerifier to Pandora (as the referee contract) which verifies the signature, that signature has both the commp digest and the size. So what Curio reports has to be identical to what the client signs. If we introduce a weird <2048 rule in here because of an implementation quirk then we have to special case it for the client too. So the easiest route is to just report consistently the fully padded size since we all know what that should be: FR32 expand + zero pad to next power of two.
But also, the Unpadded is just wrong anyway, it doesn't bear a useful relationship to the original size that's being proven. It just takes off the FR32 expansion but that doesn't give you original, raw size, it just gives you something less than power of two size, it's more like "maximum possible raw size of this piece", which isn't terrible, but it's not right.
What we really need is to have, and track, the raw size when we do this reporting. Hopefully with the CommPv2 plumbing work we'll have that because it's included in the multihash and we should be able to extract it, or we just track it in the db somewhere so we can reuse it.
There was a problem hiding this comment.
@rvagg filecoin-project/go-fil-commcid#6 has what we need. Can you review it please? For using rawSize, parked_pieces table should be recording it as well. So, we can use the raw size from there for this.
There was a problem hiding this comment.
Can't we do this conditionally for when size is less than 2048 i
No this is a bug for every case it just fails silently (worse) when size > 2048 because there's no failure but the whole piece is not guaranteed to be proven (depending on how much padding ther is)
|
Any blockers for getting this merged? |
|
not from me, but I think @LexLuthr would prefer to just skip this and jump straight to the proper size and use commpv2, but that's a lift from where we are right now on synapse |
From the
synapsebranch. A bugfix that allowed us to get smaller pieces onboarded. The current reporting of the raw->padded->unpadded size is a bit off, and this is one of the side effects. Due to the way it gets calculated, pieces less than 2048 bytes get reported badly.This isn't quite the final fix here, because what this does is tell PDP to prove the whole, padded piece, which is not what the user really wants, but Curio can deal with it, it'll just be proving fake-zeros on the end of the piece sometimes. The ideal is to report the raw size, but PDP needs it to be multiples of 32. I think the final fix here would be to take the raw size, pad it to 32 and report that. But that's not as easy as just fixing this for now.