Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

Infer multihash resource update content when datalength header is 0#214

Merged
gbalint merged 12 commits intoswarm-network-rewritefrom
swarm-mutableresources-multihash
Feb 20, 2018
Merged

Infer multihash resource update content when datalength header is 0#214
gbalint merged 12 commits intoswarm-network-rewritefrom
swarm-mutableresources-multihash

Conversation

@nolash
Copy link
Copy Markdown
Contributor

@nolash nolash commented Jan 24, 2018

This PR enables a convenience method for using multihash content in resource updates.

The corresponding method sets the datalength header to 0 in the chunk data. When retrieved, and a datalength header 0 is encountered, the data is resolved as a multihash.

A data integrity check is added to ensure that the first two values in the data are varints, and that the second varint corresponds to the length of the data (or data + signature).

@nolash nolash requested review from gbalint and janos January 24, 2018 18:45
@nolash nolash self-assigned this Jan 24, 2018
@lmars
Copy link
Copy Markdown

lmars commented Jan 25, 2018

@nolash this also looks like the same changes as #204?

@nolash nolash force-pushed the swarm-mutableresources-multihash branch from a6fcb8c to 97b5f59 Compare January 25, 2018 13:56
Comment thread swarm/storage/resource.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

c can be <0 too

Comment thread swarm/storage/resource_test.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need to PR the swarm BMT hash to multihash?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know, do we? Let's discuss separately.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should, along with a spec of its exact construction. We currently have a hack in go-meta to register it ourselves.

Comment thread swarm/storage/resource.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Any reason not to use multihash.Decode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the package is not actually in use now in the repo. Until it is necessary to add it, let's do away with the extra bloat. This check is perfectly valid according to the spec, afaics.

Comment thread swarm/storage/resource_test.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This context usage is ineffective. Since cancel is being deferred, it will only be called after the call to NewResource returns, therefore nothing will get cancelled.

We should just pass context.Background() directly to NewResource.

Comment thread swarm/storage/resource_test.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should, along with a spec of its exact construction. We currently have a hack in go-meta to register it ourselves.

Copy link
Copy Markdown

@gbalint gbalint left a comment

Choose a reason for hiding this comment

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

Some minor comments

Comment thread swarm/storage/resource.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this be a ResourceError?

Comment thread swarm/storage/resource.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Isn't this a little bit too much logging? They are fine during development, but probably they could be removed, or at least set to Debug. On the other hand the error cases are only logged with Debug level, although those could be Warns or Errors maybe.

Comment thread swarm/storage/resource_test.go Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

teardownTest() should be right after the setupTest, call, otherwise it will not run if setupTest gives back an error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

setupTest will only return Error if it can't make tempdir, in which case the teardownTest will be nil.

@nolash nolash force-pushed the swarm-mutableresources-multihash branch from 69e935f to 2e4da30 Compare February 1, 2018 14:56
@nonsense nonsense force-pushed the swarm-mutableresources-multihash branch from 7026bfb to 05e09eb Compare February 20, 2018 16:23
@gbalint gbalint merged commit a375d95 into swarm-network-rewrite Feb 20, 2018
@gbalint gbalint deleted the swarm-mutableresources-multihash branch February 20, 2018 20:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants