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.

Sign up for GitHub

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

Jump to bottom

fix(cli): object add-link: do not allow blocks over BS limit #8414

Merged
merged 6 commits into from
Sep 28, 2021
Merged

fix(cli): object add-link: do not allow blocks over BS limit #8414

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2fff226
fix(cli): object add-link: do not allow blocks over BS limit
schomatis Sep 7, 2021
4e5e7f4
add force option
schomatis Sep 23, 2021
e16bc39
abstract check in function
schomatis Sep 23, 2021
6374c6d
add check to all patch commands
schomatis Sep 23, 2021
445b9d7
test
schomatis Sep 24, 2021
77f420e
refactor: allow-big-block
lidel Sep 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions core/commands/object/patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,21 @@ Use MFS and 'files' commands instead:
return err
}

// We do not allow producing blocks bigger than 1 MiB to avoid errors
// when transmitting them over BitSwap. The 1 MiB constant is an
// unenforced and undeclared rule of thumb hard-coded here.
modifiedNode, err := api.Dag().Get(req.Context, p.Cid())
if err != nil {
return err
}
modifiedNodeSize, err := modifiedNode.Size()
if err != nil {
return err
}
if modifiedNodeSize > 1024 * 1024 {
Copy link
Contributor

Choose a reason for hiding this comment

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

My guess would be that returning an error wouldn't suffice. The node is already too big and this has to be reverted. But reverting it here is hard, because the reversal can itself cause another error. So I would think this must be rejected at the ObjectAPI level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petar I see two places where we could potentially put this check deeper in the stack:

  • (api *ObjectAPI) AddLink: after the InsertNodeAtPath call (which has the actual 'insertion' logic) using a check similar to the one placed at the CLI level (fetch the modified node and check its size). We might also want to evaluate purging the modified copy from the repo.

  • (deeper) in the go-merkledag repo executing the actual insertion logic: it has the advantage that already has a permanent/temporary store division where rejecting the modified node wouldn't impact the go-ipfs repo. The downside is that we would be injecting an arbitrary restriction on a library based on the limitation of another (bitswap) just because both are consumed by the same go-ipfs logic.

Either is doable so I'll leave to you the decision of where to execute.

return fmt.Errorf("object API does not support HAMT-sharding. To create big directories, please use the files API (MFS)")
}

return cmds.EmitOnce(res, &Object{Hash: p.Cid().String()})
},
Type: Object{},
Expand Down