swarm/hash: Custom multihash implementation #633

nolash · 2018-05-26T07:09:15Z

zelig · 2018-05-27T00:07:53Z

swarm/hash/multihash.go

+	"fmt"
+)
+
+const (


calling this default is misleading. it is for bmt-sha3-128
not sure this should be here even

I think it would be helpful if you gave me an overview of which hashing functions, one or more, will be used in the application overall.

👍 no to defaults. ideally pull this into a configuration flag

zelig · 2018-05-27T00:11:10Z

swarm/hash/multihash.go

+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
+package swarmhash


lets not use swarm hash cos it is something else already

But the swarmhash will be here in the end, no?

👍 please no swarmhash. use multihash or hash. in general i think that in terms of package names - either use the filename or directory name

Sure, we can call it something else. But hash is not good, because that's the name of a package already in use in the stack. And multihash is misleading because the package will / is indended to cover all hashing functionality in swarm.

Any other good name suggestions? hashing ? hasher?

multihash is exactly the name since it concerns a self describing hash, hence it is a multihash. or am i seeing things the wrong way?

And multihash is misleading because the package will / is indended to cover all hashing functionality in swarm.

along the lines of:

hashing.Add("SHA3", SomeNewHashFunc(SHAHash)") shinynewhash, err := hashing.Hash("foo")

Have a look at: #634

I vote "swarm/multihash"

@fjl you would vote swarm/multihash even if it does more than just treating multihashes?

acud · 2018-05-29T09:26:21Z

swarm/storage/resource.go

@@ -725,7 +728,14 @@ func (self *ResourceHandler) parseUpdate(chunkdata []byte) (*Signature, uint32,
 	var intdatalength int
 	var multihash bool
 	if datalength == 0 {
-		intdatalength = isMultihash(chunkdata[cursor:])
+		var intheaderlength int


can you please extract this to a function?

because it was already?

~~sorry I read it wrong~~

the GetMultihashLength takes the place of the isMultihash .. that's a function..? What am I missing?

acud · 2018-05-29T09:29:14Z

swarm/hash/multihash.go

+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
+package swarmhash


👍 please no swarmhash. use multihash or hash. in general i think that in terms of package names - either use the filename or directory name

acud · 2018-05-29T09:29:55Z

swarm/hash/multihash.go

+	"fmt"
+)
+
+const (


👍 no to defaults. ideally pull this into a configuration flag

acud · 2018-05-29T09:30:08Z

swarm/hash/multihash.go

+	MultihashLength   = defaultMultihashLength
+)
+
+func init() {


remove and replace with a constructor.

acud · 2018-05-29T09:31:30Z

swarm/hash/multihash.go

+}
+
+// check if valid swarm multihash
+func isSwarmMultihashType(code uint8) bool {


as far as i know typecode 0x1b infers sha3 but does no infer swarm hash

0x1b is keccak256.

There is no "swarm hash" in mulithash (yet?)

But a swarm hash uses keccak256.

This is as close as we get for now, I think.

then for that matter swarm hash does not exist. i would get rid of as much of the multihash references as possible (except for the points where the implementation is indeed generalised) since we are just supporting 1 multihash format thus change the references to describe keccak256.

Hmm, I think this all warrants further discussion. The terms are confusing.

acud · 2018-05-29T10:07:40Z

swarm/storage/resource.go

-	if isMultihash(data) == 0 {
-		return nil, NewResourceError(ErrNothingToReturn, "Invalid multihash")
+	if _, _, err := swarmhash.GetMultihashLength(data); err != nil {
+		return nil, NewResourceError(ErrNothingToReturn, fmt.Sprintf("Invalid multihash: %v", err))


this error is not so crystal clear, why can't you just bubble the error from GetMultihashLength up?

the error texts in swarm/hash are pretty neutral, e.g "unreadable length field", not so easy to tell further up what that means.

Do you mean we should have typed error maybe?

acud · 2018-05-29T10:08:00Z

swarm/storage/resource_test.go

-	if err != nil {
-		t.Fatal(err)
-	}
+	sha1bytes := make([]byte, swarmhash.MultihashLength)


are we using sha1?

acud · 2018-05-29T10:08:11Z

swarm/storage/resource_test.go

-	if !bytes.Equal(swarmhashdecode.Digest, swarmhashbytes.Bytes()) {
-		t.Fatalf("Decoded SHA1 hash '%x' does not match original hash '%x'", swarmhashdecode.Digest, swarmhashbytes.Bytes())
+	if !bytes.Equal(swarmhashdecode, swarmhashbytes.Bytes()) {
+		t.Fatalf("Decoded SHA1 hash '%x' does not match original hash '%x'", swarmhashdecode, swarmhashbytes.Bytes())


acud · 2018-05-29T10:08:27Z

swarm/storage/resource_test.go

-	if !bytes.Equal(sha1decode.Digest, sha1bytes) {
-		t.Fatalf("Decoded SHA1 hash '%x' does not match original hash '%x'", sha1decode.Digest, sha1bytes)
+	if !bytes.Equal(sha1decode, sha1bytes) {
+		t.Fatalf("Decoded SHA1 hash '%x' does not match original hash '%x'", sha1decode, sha1bytes)


acud · 2018-05-29T10:08:39Z

swarm/storage/resource_test.go

-	if !bytes.Equal(sha1decode.Digest, sha1bytes) {
-		t.Fatalf("Decoded SHA1 hash '%x' does not match original hash '%x'", sha1decode.Digest, sha1bytes)
+	if !bytes.Equal(sha1decode, sha1bytes) {
+		t.Fatalf("Decoded SHA1 hash '%x' does not match original hash '%x'", sha1decode, sha1bytes)


more of this

nonsense · 2018-06-05T15:38:04Z

swarm/storage/mru/resource.go

+		intdatalength, intheaderlength, err = swarmhash.GetMultihashLength(chunkdata[cursor:])
+		if err != nil {
+			log.Error("multihash parse error", "err", err)
+			return nil, 0, 0, "", nil, false, fmt.Errorf("Corrupt multihash data: %v", err)


no need to wrap err.

nonsense · 2018-06-05T15:38:19Z

swarm/storage/mru/resource.go

@@ -286,6 +287,7 @@ func (self *Handler) SetStore(store *storage.NetStore) {
 func (self *Handler) Validate(addr storage.Address, data []byte) bool {
 	signature, period, version, name, parseddata, _, err := self.parseUpdate(data)
 	if err != nil {
+		log.Warn("foo", "err", err)


just log.Warn(err.Error())

nonsense · 2018-06-05T15:39:57Z

swarm/storage/mru/resource.go

-		return nil, NewError(ErrNothingToReturn, "Invalid multihash")
+	// \TODO perhaps this check should be in newUpdateChunk()
+	if _, _, err := swarmhash.GetMultihashLength(data); err != nil {
+		return nil, NewError(ErrNothingToReturn, fmt.Sprintf("Invalid multihash: %v", err))


no need to wrap err in fmt.Sprintf - it should be clear where it is coming from and what's wrong with it.

nonsense · 2018-06-05T15:41:41Z

swarm/hash/multihash.go

+	cursor += c
+	hashlength, c := binary.Uvarint(data[cursor:])
+	if c <= 0 {
+		return 0, 0, fmt.Errorf("unreadable length field")


No format here. Just errors.New...

nonsense · 2018-06-05T15:41:52Z

swarm/hash/multihash.go

+	cursor := 0
+	typ, c := binary.Uvarint(data)
+	if c <= 0 {
+		return 0, 0, fmt.Errorf("unreadable hashtype field")


No format here.

nonsense · 2018-06-05T15:42:00Z

swarm/hash/multihash.go

+	// we cheekily assume hashlength < maxint
+	inthashlength := int(hashlength)
+	if len(data[c:]) < inthashlength {
+		return 0, 0, fmt.Errorf("length mismatch")


nonsense · 2018-06-05T15:42:15Z

swarm/hash/multihash_test.go

+		t.Fatalf("expected header length %d, got %d", 2, hl)
+	}
+	if _, _, err := GetMultihashLength(expected[1:]); err == nil {
+		t.Fatalf("expected failure on corrupt header")


nonsense · 2018-06-05T15:42:22Z

swarm/hash/multihash_test.go

+		t.Fatalf("expected failure on corrupt header")
+	}
+	if _, _, err := GetMultihashLength(expected[:len(expected)-2]); err == nil {
+		t.Fatalf("expected failure on short content")


nolash · 2018-06-06T06:29:19Z

In the spirit of the "let's just merge this as is" conclusion of yesterday's meeting I hereby submit this to another review, albeit under a new name.

nonsense · 2018-06-06T10:24:43Z

swarm/api/api.go

 					apiGetInvalid.Inc(1)
 					status = http.StatusUnprocessableEntity
-					log.Warn(fmt.Sprintf("invalid resource multihash code: %x", decodedMultihash.Code))
+					log.Warn(fmt.Sprintf("invalid resource multihash: %v", err))


We have structured logs, no need to use fmt.Sprintf.

nonsense · 2018-06-06T10:27:55Z

Linter is failing and also compilation doesn't seem to work.

zelig

if linter and travis green i am fine with this

nolash added enhancement ready for review labels May 26, 2018

nolash added this to the master merge milestone May 26, 2018

nolash self-assigned this May 26, 2018

nolash requested review from nonsense and gbalint May 26, 2018 07:09

This was referenced May 26, 2018

swarm/hash: Centralize hasher functionality #634

Closed

swarm/hash: New package + custom multihash #601

Closed

zelig reviewed May 27, 2018

View reviewed changes

acud reviewed May 29, 2018

View reviewed changes

gbalint mentioned this pull request May 31, 2018

vendor: added vendor packages necessary for the swarm-network-rewrite merge ethereum/go-ethereum#16792

Merged

nolash force-pushed the multihash-only branch from 7732869 to c9ca72a Compare June 4, 2018 08:07

nolash added in progress and removed ready for review labels Jun 4, 2018

nonsense reviewed Jun 5, 2018

View reviewed changes

nolash added ready for another review and removed in progress labels Jun 6, 2018

nonsense reviewed Jun 6, 2018

View reviewed changes

nolash added 3 commits June 8, 2018 18:59

swarm/hash: Custom multihash implementation

d306ad6

swarm/hash, swarm/storage: Delint and fix comments

e4b7ce3

swarm/storage: Rebase cleanup

f10a403

nolash added 4 commits June 8, 2018 19:00

swarm/multihash: Rename package to multihash

7486fee

swarm/multihash, swarm/storage: Remove redundant error formatting

85a809e

swarm/multihash: Add missing files

64fcfb2

swarm/storage: Fix error string bug

5769ce1

nolash force-pushed the multihash-only branch from 7590b40 to 5769ce1 Compare June 8, 2018 17:01

swarm/api: Remove sprintf in error

20439d8

zelig approved these changes Jun 9, 2018

View reviewed changes

nolash force-pushed the multihash-only branch from 69b647c to 20439d8 Compare June 9, 2018 08:43

nolash merged commit 3acc595 into swarm-network-rewrite Jun 9, 2018

nolash deleted the multihash-only branch June 9, 2018 11:48

zelig mentioned this pull request Jun 11, 2018

swarm/multihash: Custom implementation #595

Closed

+              	"fmt"
+              )
+              const (

swarm/hash: Custom multihash implementation #633

swarm/hash: Custom multihash implementation #633

Conversation

nolash commented May 26, 2018

Choose a reason for hiding this comment

nolash May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash May 29, 2018 • edited Loading

Choose a reason for hiding this comment

fjl Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolash commented Jun 6, 2018

Choose a reason for hiding this comment

nonsense commented Jun 6, 2018

zelig left a comment

Choose a reason for hiding this comment

nolash May 27, 2018 •

edited

Loading

nolash May 29, 2018 •

edited

Loading

fjl Jun 1, 2018 •

edited

Loading

nolash May 29, 2018 •

edited

Loading