Skip to content

Commit

Permalink
fix: partial revert of 1be7c1a; make traverser process identity CIDs
Browse files Browse the repository at this point in the history
It turns out that there are obscure cases where this matters, so we can't
as easily just ignore identity CIDs. Specifically, classic online Filecoin
deals that rely on Graphsync _and_ require identity CIDs be stored in the
CARv1 that is used to calculate CommP must see the identity CID pass through
the LinkSystem.

Unfortunately, the easiest way to deal with this is to send them over the wire
as if they are a normal block; which happens to be the "safe" backward
compatible way too. Less easy way would be to simulate it on both ends and
not send them, but we'll take the easy path for now.

Extension of tests in here to make sure that the full DAG is transferred even
in this case. Blockstore _must_ have identity CIDs in them, or be able to
respond properly to them.
  • Loading branch information
rvagg committed Sep 21, 2023
1 parent 46d19bd commit d136b82
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 31 deletions.
2 changes: 1 addition & 1 deletion impl/graphsync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,8 +383,8 @@ func TestGraphsyncIdentityCIDRoundTrip(t *testing.T) {
})
progressChan, errChan := requestor.Request(ctx, td.host2.ID(), identityDag.RootLink, selectorparse.CommonSelector_ExploreAllRecursively)

identityDag.VerifyWholeDAG(ctx, progressChan)
testutil.VerifyEmptyErrors(ctx, t, errChan)
identityDag.VerifyWholeDAG(ctx, progressChan)
require.Len(t, td.blockStore1, len(identityDag.AllLinks), "did not store all blocks")

// verify listener
Expand Down
19 changes: 0 additions & 19 deletions ipldutil/traverser.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package ipldutil

import (
"bytes"
"context"
"errors"
"io"
Expand All @@ -13,9 +12,7 @@ import (
"github.com/ipld/go-ipld-prime/node/basicnode"
"github.com/ipld/go-ipld-prime/traversal"
"github.com/ipld/go-ipld-prime/traversal/selector"
"github.com/multiformats/go-multihash"

"github.com/ipfs/go-cid"
"github.com/ipfs/go-graphsync/panics"
)

Expand Down Expand Up @@ -168,23 +165,7 @@ func (t *traverser) NBlocksTraversed() int {
return t.blocksCount
}

func asIdentity(c cid.Cid) (digest []byte, ok bool, err error) {
dmh, err := multihash.Decode(c.Hash())
if err != nil {
return nil, false, err
}
ok = dmh.Code == multihash.IDENTITY
digest = dmh.Digest
return digest, ok, nil
}

func (t *traverser) loader(lnkCtx ipld.LinkContext, lnk ipld.Link) (io.Reader, error) {
if digest, ok, err := asIdentity(lnk.(cidlink.Link).Cid); ok {
return io.NopCloser(bytes.NewReader(digest)), nil
} else if err != nil {
return nil, err
}

// A StorageReadOpener call came in; update the state and release the lock.
// We can't simply unlock the mutex inside the <-t.responses case,
// as then we'd deadlock with the other side trying to send.
Expand Down
32 changes: 21 additions & 11 deletions testutil/identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,17 @@ type TestIdentityDAG struct {
t testing.TB
loader ipld.BlockReadOpener

RootLink ipld.Link
AllLinks []ipld.Link
RootLink ipld.Link
AllLinks []ipld.Link
AllLinksNoIdent []ipld.Link
}

/* ugly, but it makes a DAG with paths that look like this but doesn't involved dag-pb or unixfs */
var identityDagPaths = []string{
var identityDagLinkPaths = []string{
"",
"a/!foo",
"a/b/!bar",
"a/b/c/!baz", // identity
"a/b/c/!baz/identity jump",
"a/b/c/!baz/identity jump/these are my children/blip",
"a/b/c/!baz/identity jump/these are my children/bloop",
Expand All @@ -49,13 +51,15 @@ var identityDagPaths = []string{
func SetupIdentityDAG(
ctx context.Context,
t testing.TB,
lsys ipld.LinkSystem) *TestIdentityDAG {

lsys ipld.LinkSystem,
) *TestIdentityDAG {
allLinks := make([]ipld.Link, 0)
allLinksNoIdent := make([]ipld.Link, 0)
store := func(lp datamodel.LinkPrototype, n datamodel.Node) datamodel.Link {
l, err := lsys.Store(linking.LinkContext{}, lp, n)
require.NoError(t, err)
allLinks = append(allLinks, l)
allLinksNoIdent = append(allLinksNoIdent, l)
return l
}

Expand Down Expand Up @@ -98,6 +102,11 @@ func SetupIdentityDAG(
identBytes := must(ipld.Encode(ident, dagjson.Encode))(t)
mh := must(multihash.Sum(identBytes, multihash.IDENTITY, len(identBytes)))(t)
bazIdent = cidlink.Link{Cid: cid.NewCidV1(cid.DagJSON, mh)}
w, wc, err := lsys.StorageWriteOpener(linking.LinkContext{})
require.NoError(t, err)
w.Write(identBytes)
require.NoError(t, wc(bazIdent))
allLinks = append(allLinks, bazIdent)
}
bar := store(djlp, basicnode.NewInt(2020202020202020))
foo := store(djlp, basicnode.NewInt(1010101010101010))
Expand All @@ -117,10 +126,11 @@ func SetupIdentityDAG(
}))(t))

return &TestIdentityDAG{
t: t,
loader: lsys.StorageReadOpener,
RootLink: root,
AllLinks: reverse(allLinks), // TODO: slices.Reverse post 1.21
t: t,
loader: lsys.StorageReadOpener,
RootLink: root,
AllLinks: reverse(allLinks), // TODO: slices.Reverse post 1.21
AllLinksNoIdent: reverse(allLinksNoIdent),
}
}

Expand Down Expand Up @@ -150,15 +160,15 @@ func (tid *TestIdentityDAG) checkResponses(responses []graphsync.ResponseProgres
for ii, response := range responses {
// only check the paths that have links, assume the rest are just describing
// the non-link nodes of the DAG
if response.Path.String() == identityDagPaths[pathIndex] {
if response.Path.String() == identityDagLinkPaths[pathIndex] {
if response.LastBlock.Link != nil {
expectedLink := tid.AllLinks[pathIndex]
require.Equal(tid.t, expectedLink.String(), response.LastBlock.Link.String(), fmt.Sprintf("response %d has correct link (%d)", ii, pathIndex))
}
pathIndex++
}
}
require.Equal(tid.t, len(identityDagPaths), pathIndex, "traverses all nodes")
require.Equal(tid.t, len(identityDagLinkPaths), pathIndex, "traverses all nodes")
}

func must[T any](v T, err error) func(t testing.TB) T {
Expand Down

0 comments on commit d136b82

Please sign in to comment.