Implement options to handle `IDENTITY` CIDs gracefully by masih · Pull Request #239 · ipld/go-car

masih · 2021-09-17T20:58:33Z

Implement two additional options that allow a CARv2 file to 1) include
IDENTNTIY CIDs, and 2) specify a maximum allowed CID length with default
of 2KiB as a sufficiently large default.

Configure ReadWrite blockstore to persist given blocks with IDENTITY
CIDs.

Introduce a new Characteristics filed that signalls whether an index in
a CAR file contains a full catalog of CIDs for backward compatibility
purposes. Note, this is a new addition and will need to be added to the
spec in a separate PR.

Relates to #215

v2/car.go

v2/errors.go

v2/index_gen.go

v2/index/index.go

mvdan · 2021-09-17T21:09:45Z

v2/blockstore/readwrite_test.go

+	// Assert the id is indexed.
+	r, err := carv2.OpenReader(path)
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, r.Close()) })


no need to check read-only close errors 😉

v2/blockstore/readwrite_test.go

rvagg · 2021-09-20T03:57:31Z

v2/index_gen.go

-		records = append(records, index.Record{Cid: c, Offset: uint64(sectionOffset)})
+
+		if c.Prefix().MhType != multihash.IDENTITY || o.IncludeIdentityCIDs {
+			if uint64(cidLen) > o.MaxIndexCidSize {


coverage of this case might be nice - it could even fit in one of the existing tests here without needing a special test just for it

There is a test for large CIDs already in this PR here. Do you think it's missing something?

yeah, it's not using the default (common) path but setting an unrealistic max to break, that was my thought
but this is a nice to have, not a demand for busy-work if you're comfortable with this as is

Thanks for pointing this out; captured #241

rvagg · 2021-09-20T04:02:27Z

The maximum of 2K is somewhat arbitrary isn't it? Not disagreeing with it, it feels arbitrary no matter how you slice it, but if we have a rationale then that might be good to include in a note.

By locking it in a default here as 2K we're probably going to be pushing that fairly broadly through the stack if the CARv2 writer interface becomes the default for creating CARs. Which is probably good we have a proper limiting condition, just with possible ramifications.

I might have to consider getting something similar in to the JavaScript CAR because we now have a lot of user data being generated that way, it's possible we get large identity CIDs slipping in there and only being caught if/when they hit this code in Filecoin.

masih · 2021-09-20T10:47:20Z

The maximum of 2K is somewhat arbitrary isn't it? Not disagreeing with it, it feels arbitrary no matter how you slice it, but if we have a rationale then that might be good to include in a note.

By locking it in a default here as 2K we're probably going to be pushing that fairly broadly through the stack if the CARv2 writer interface becomes the default for creating CARs. Which is probably good we have a proper limiting condition, just with possible ramifications.

I might have to consider getting something similar in to the JavaScript CAR because we now have a lot of user data being generated that way, it's possible we get large identity CIDs slipping in there and only being caught if/when they hit this code in Filecoin.

It is mostly arbitrary indeed; the rationale was to pick something that is somewhat future proof and unlikely to be hit in reasonable use cases today. Any suggestions for a better value is more than welcomed. I know @aschmahmann pointed out a limit of 80 in KAD DHT? The key thing to point out here though, is that, this is an option default value and not a hard limit; i.e. option can be overridden and set to all sorts of unreasonable values.

Thoughts?

mvdan

Nice work!

ribasushi

Solid wok overall, left a number of comments regarding naming/clarifications

None of the remarks are showstoppers, take them or leave them: it's good either way.

v2/options.go

v2/index/index.go

v2/blockstore/readwrite_test.go

v2/blockstore/readwrite.go

Implement two additional options that allow a CARv2 file to 1) include IDENTNTIY CIDs, and 2) specify a maximum allowed CID length with default of 2KiB as a sufficiently large default. Configure ReadWrite blockstore to persist given blocks with IDENTITY CIDs. Introduce a new Characteristics filed that signalls whether an index in a CAR file contains a full catalog of CIDs for backward compatibility purposes. Note, this is a new addition and will need to be added to the spec in a separate PR. Relates to #215

Update CARv2 specification to include a format for MultihashIndexSorted and fully-indexed characteristic bitfield. Relates to: - ipld/go-car#239 - ipld/go-car#217

masih force-pushed the masih/graceful-id-cid-opts branch 2 times, most recently from f1622e4 to 9f9f687 Compare September 17, 2021 21:09

mvdan approved these changes Sep 17, 2021

View reviewed changes

rvagg reviewed Sep 20, 2021

View reviewed changes

v2/blockstore/readwrite_test.go Show resolved Hide resolved

rvagg reviewed Sep 20, 2021

View reviewed changes

masih force-pushed the masih/graceful-id-cid-opts branch 2 times, most recently from bdfd5f3 to 92e3c89 Compare September 20, 2021 11:53

mvdan mentioned this pull request Sep 20, 2021

v2: use a byte array for the characteristics bitfield #240

Open

masih force-pushed the masih/graceful-id-cid-opts branch from 92e3c89 to 023c842 Compare September 20, 2021 12:59

mvdan approved these changes Sep 20, 2021

View reviewed changes

masih marked this pull request as ready for review September 20, 2021 13:13

masih requested a review from ribasushi September 20, 2021 13:15

ribasushi approved these changes Sep 20, 2021

View reviewed changes

v2/options.go Show resolved Hide resolved

v2/index/index.go Show resolved Hide resolved

v2/blockstore/readwrite_test.go Show resolved Hide resolved

v2/blockstore/readwrite.go Outdated Show resolved Hide resolved

masih force-pushed the masih/graceful-id-cid-opts branch from 023c842 to 29405e4 Compare September 20, 2021 14:40

masih merged commit f35d88c into master Sep 20, 2021

masih deleted the masih/graceful-id-cid-opts branch September 20, 2021 14:44

masih mentioned this pull request Sep 20, 2021

Update CARv2 MultihashIndexSorted and fully-indexed characteristic spec ipld/ipld#138

Merged

masih mentioned this pull request Sep 21, 2021

Add a test for max CID size with default options #241

Open

masih mentioned this pull request Jan 11, 2022

Import ux slow / needs improvment ipni/index-provider#118

Closed

rvagg mentioned this pull request Sep 1, 2025

fix(verifcid): enforce size limit for identity CIDs ipfs/boxo#1018

Merged

2 tasks

Comments

Conversation

masih commented Sep 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvdan Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rvagg Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

masih Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

rvagg Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

masih Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

rvagg commented Sep 20, 2021

Uh oh!

masih commented Sep 20, 2021

Uh oh!

mvdan left a comment

Choose a reason for hiding this comment

Uh oh!

ribasushi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants