Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

docs: update block api documentation #4124

Merged
merged 1 commit into from
Jun 10, 2022
Merged

docs: update block api documentation #4124

merged 1 commit into from
Jun 10, 2022

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jun 3, 2022

Closes #3913.

I have some inline notes and questions.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@@ -73,19 +74,18 @@ An optional object which may have the following keys:
| ---- | ---- | ------- | ----------- |
| format | `String` | `'dag-pb'` | The codec to use to create the CID |
| mhtype | `String` | `sha2-256` | The hashing algorithm to use to create the CID |
| mhlen | `Number` | | |
| mhlen | `Number` | `undefined` | The hash length (only relevant for `go-ipfs`) |
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-core-types/src/block/index.ts does not include this option. However, the go-ipfs CLI seems to support this option and found this mentions in this repository:

mhlen: {
describe: 'multihash hash length',
default: undefined
},

  • Is this option relevant? Should it be removed?
  • Is it supported by js-ipfs or only go-ipfs?

Copy link
Member

Choose a reason for hiding this comment

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

It's not supported by js-ipfs and I'm not clear on what it even does for go-ipfs. It defaults to -1 which means.. 🤷

If you could find out the what and the why maybe we could even remove it entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it seems to be literally the multihash length, but doesn't seem to be very useful looking at the very limited search results. I tried figuring out if this was useful when opening this PR, but couldn't find anything that explained why it may be needed.

Copy link
Member

@lidel lidel Jun 6, 2022

Choose a reason for hiding this comment

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

I had to refactor relevant go-ipfs code around this recently and can provide some details.

afaik the only place where this is used, is when mhlen gets passed deep into go-multihash code responsible for digert (Sum) calculation: https://github.com/multiformats/go-multihash/blob/75ae3688857d036ea15947be9df68d3172b19470/sum.go

tldr:

  • -1 is the implicit default which means "set length based on hashing function output"
    • same applies to any value <=0
  • setting custom mhlen allows for truncating longer digests produces by alternative hash functions
  • setting it to longer than the default output of specific hash function will produce error

@hacdias hacdias marked this pull request as ready for review June 3, 2022 09:10
@hacdias hacdias requested review from achingbrain and lidel June 3, 2022 09:10
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, have responded inline to the queries, we can merge once they are resolved.

@hacdias hacdias requested a review from achingbrain June 6, 2022 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ipfs.block API is out of sync with the implementation
3 participants