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

CID length and identity hashes #21

Open
2 tasks
Stebalien opened this issue Jun 8, 2018 · 17 comments
Open
2 tasks

CID length and identity hashes #21

Stebalien opened this issue Jun 8, 2018 · 17 comments
Assignees

Comments

@Stebalien
Copy link
Member

Stebalien commented Jun 8, 2018

Moved from: ipfs/kubo#4918 as this isn't go-ipfs specific and will affect the spec.

Basically, we'd like to allow inlining small blocks into CIDs (using the identity hash function) for performance reasons. However, the larger the block we allow to be inlined, the less user friendly CIDs get. Unfortunately, we have to pick a "default inlined size" up front or we'll end up changing a bunch of hashes later.

Open questions:

  • Do we have a hard limit. That is, do we say that all CIDs must be shorter than X?
  • What should be the maximum size of CIDs created by default?

@whyrusleeping @kevina @diasdavid @vmx @kyledrake

@Stebalien
Copy link
Member Author

@kevina's options:

  1. Don't set a hard limit on CID digest size, but by default id hashes will have a maxium digest length (and thus content length) of 64 bytes
  2. Set a hard limit of 128 bytes on digest length (to keep things from getting to out of hand, but also to not artificially limit our options) but limit id hashes to 64 bytes by default.
  3. Set a hard limit of 64 bytes on digest length and thus limit id hashes to this length

@Stebalien's additional options:

  1. Soft limit of 38 bytes for the entire CID. That'll allow a base32 encoded CID to fit in a domain name segment.
  2. Soft limit of 42 bytes for the entire CID. That's what we use for inlining peer IDs (although we may want to reduce this to 38 given the DNS restriction.

Unfortunately, my options limit the utility of this feature. However, they do increase the usability.

@kevina
Copy link

kevina commented Jun 8, 2018

It is important to note that the 64 bytes comes from the maximum size modern crypro. hashes output (512 bits). If we set the limit lower than this will we prevent the option of using the all the output bits.

@Stebalien
Copy link
Member Author

We definitely can't set a lower hard limit, but we could set a lower auto-inline limit. That really depends on how likely we feel we are to move to a larger hash sometime soon.

@vmx
Copy link
Member

vmx commented Jun 13, 2018

I don't have a strong opinion on the limits. Though I'm not really sure about this whole data inlining. It will make the whole stack more complicated. Currently it's always "CID + data" and then if will become "CID + maybe data, depending on the CID". This is a huge change a lot of components need to learn about.

@whyrusleeping
Copy link
Member

@vmx i'm not entirely sure what you mean. Conceptually, its pretty simple. We're just allowing the 'hash' function of the CID to be f(x) = x. Everything else works exactly the same. The thing this enables though, is a cool optimization where we don't have to actually store data for CIDs using this particular 'hash' function.

@vmx
Copy link
Member

vmx commented Jun 13, 2018

@whyrusleeping I'm thinking in in code. Currently it's:

  1. get request via CID
  2. ask storage for this CID
  3. return the thing the storage returned

Then a new step between 1 and 2 is introduced:

  • check if there's inlined data
    • if yes, return that
    • else go on with 2.

I'm not sure how bad this really is. If you all think that's not really a big deal, that's fine for me :)

@Stebalien
Copy link
Member Author

So, our plan is to just modify the block service to "do the right thing". That is, when you try to put a block with a CID that uses the identity hash, it'll just throw it away. When you try to get the block, it'll extract it from the CID.

Currently we have to create indirect, large CIDs even for really tiny objects, files, and directories.

@richardschneider
Copy link

richardschneider commented Jun 14, 2018

It was easy to change my block service to support getting an inlined CID.

However, I have some concerns when putting.

  • this should be optional/experimental behaviour. Otherwise, pre-existing tests fail because the CID is different
  • as others have commented, a default limit for inlining is needed

@kevina
Copy link

kevina commented Jun 14, 2018

this should be optional/experimental behaviour. Otherwise, pre-existing tests fail because the CID is different

Why?

@richardschneider
Copy link

@kevina putting a small block in a test, blockService.Put(byte[] { 0x01 }), generates a different CID if CID inlining is enabled. By definition CID v1 must be used, whereas without CID inlining, CID v0 can be used.

@kevina
Copy link

kevina commented Jun 14, 2018

@richardschneider the automatic use of identity hashes will require a command line flags that is not enabled by default. See ipfs/kubo#4910 for a proof-of-concept implementation.

@richardschneider
Copy link

@kevina Thanks, did not know about --id-hash-limit option. So question 1 is answered.

Does the limit specify the number bytes in (1) the data block size or (2) the identity hash digest size or (3) the CID binary length?

Also, is id an alias for the identity hash algorithm?

@kevina
Copy link

kevina commented Jun 14, 2018

Does the limit specify the number bytes in (1) the data block size or (2) the identity hash digest size or (3) the CID binary length?

(2) the identity hash digest size

Also, is id an alias for the identity hash algorithm?

Yes

@richardschneider
Copy link

Thanks @kevina. You have provided enough info for me to implement the putting. Cheers!

@richardschneider
Copy link

What should be the behavior when the block service remove is called with an inline Cid?

I'm thinking this a no-op and no error is returned.

@Stebalien
Copy link
Member Author

I'm thinking this a no-op and no error is returned.

I agree. Personally, I'd prefer it if moved to being idempotent over deletes for both performance and usability reasons.

@richardschneider
Copy link

@kevina Would you mind commenting on richardschneider/net-ipfs-engine#20

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

No branches or pull requests

6 participants