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/v2 commp cid arbitrary size #6

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

aschmahmann
Copy link

Builds off of #5 with an alternative v2 cid representation that handles arbitrary sizes

Comment on lines +116 to +117
if size*128 < size {
return 0, fmt.Errorf("unsupported size: too big")
Copy link
Author

Choose a reason for hiding this comment

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

TODO: This was just easier for now so I didn't have to worry about integer overflow things to get something basic up, but something that should probably be dealt with

Comment on lines +138 to +139
if dataSize < 127 {
return 0, 0, fmt.Errorf("unsupported size: too small")
Copy link
Author

Choose a reason for hiding this comment

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

TODO: Needs some decision here filecoin-project/FIPs#808 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aschmahmann aschmahmann force-pushed the feat/v2-commp-cid-arbitrary-size branch from d86ce67 to ec6db9a Compare August 25, 2023 00:51
Copy link
Collaborator

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

Really dislike some of the naming/direction. Added a few comments to the effect.

// Fr32PaddedSizeToV1TreeHeight calculates the height of the piece tree given data that's been FR32 padded. Because
// pieces are only defined on binary trees if the size is not a power of 2 it will be rounded up to the next one under
// the assumption that the rest of the tree will be padded out (e.g. with zeros)
func Fr32PaddedSizeToV1TreeHeight(size uint64) uint8 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would drop this function entirely: what is its purpose...?

// tree before any FR32 padding is applied. Because pieces are only defined on binary trees of FR32 encoded data if the
// size is not a power of 2 after the FR32 padding is applied it will be rounded up to the next one under the assumption
// that the rest of the tree will be padded out (e.g. with zeros)
func UnpaddedSizeToV1TreeHeight(size uint64) (uint8, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop this entire function - just have the next one alone.

// encoded data if the size is not a power of 2 after the FR32 padding is applied it will be rounded up to the next one
// under the assumption that the rest of the tree will be padded out (e.g. with zeros). The amount of data padding that
// is needed to be applied is returned alongside the tree height.
func UnpaddedSizeToV1TreeHeightAndPadding(dataSize uint64) (uint8, uint64, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clearer? The entire FR32 "padding/unpadding" "expansion" business is an internal implementation detail anyway...

Suggested change
func UnpaddedSizeToV1TreeHeightAndPadding(dataSize uint64) (uint8, uint64, error) {
func PayloadSizeToV1TreeHeightAndPadding(dataSize uint64) (uint8, uint64, error) {

// - hash type: multihash.SHA2_256_TRUNC254_PADDED_BINARY_TREE
//
// The helpers UnpaddedSizeToV1TreeHeight and Fr32PaddedSizeToV1TreeHeight may help in computing tree height
func DataCommitmentV1ToPieceMhCID(commD []byte, unpaddedDataSize uint64) (cid.Cid, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Allow one to supply 0 as "don't care" ?

Suggested change
func DataCommitmentV1ToPieceMhCID(commD []byte, unpaddedDataSize uint64) (cid.Cid, error) {
func DataCommitmentV1ToPieceMhCID(commD []byte, optionalPayloadSize uint64) (cid.Cid, error) {

// piece multihash CID
//
// The helpers UnpaddedSizeToV1TreeHeight and Fr32PaddedSizeToV1TreeHeight may help in computing tree height
func ConvertDataCommitmentV1V1CIDtoPieceMhCID(v1PieceCid cid.Cid, unpaddedDataSize uint64) (cid.Cid, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too many V1s ? Also... the helper seems overly trivial, perhaps drop it? If kept: simpler name below:

Suggested change
func ConvertDataCommitmentV1V1CIDtoPieceMhCID(v1PieceCid cid.Cid, unpaddedDataSize uint64) (cid.Cid, error) {
func PieceCidV2FromV1(v1PieceCid cid.Cid, unpaddedDataSize uint64) (cid.Cid, error) {


// ConvertDataCommitmentV1PieceMhCIDToV1CID takes a piece multihash CID and produces a v1 piece CID along with the
// size of the unpadded data
func ConvertDataCommitmentV1PieceMhCIDToV1CID(pieceMhCid cid.Cid) (cid.Cid, uint64, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mixing "MH" into the names of things that do not return MHs ( but cid.Cid ) is... nasty.

Suggested change
func ConvertDataCommitmentV1PieceMhCIDToV1CID(pieceMhCid cid.Cid) (cid.Cid, uint64, error) {
func PieceCidV1FromV2(pcidV2 cid.Cid) (cid.Cid, uint64, error) {

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.

None yet

2 participants