Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

Add interface and correctness reference #1

Merged
merged 11 commits into from
Apr 8, 2020

Conversation

nolash
Copy link
Contributor

@nolash nolash commented Apr 1, 2020

This PR is the initial introduction of the BMT component for use in bee. It contains the interface implementation, along with a reference bmt hash generator largely copied from the bmt package of swarm. The latter does not implement the BMT interface, it is only intended to generate correct hashes using code that is as easy to understand as possible.

Part of ethersphere/bee#39

@nolash nolash self-assigned this Apr 1, 2020
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Thanks @nolash.

Beside comments below, I did not want to comment on every instance:

  • to add copyright header to files that are missing it
  • to finish all sentences with a dot so that they are nicely shown in godoc.org

reference/doc.go Outdated
// Copyright 2020 The Swarm Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
//
Copy link
Member

Choose a reason for hiding this comment

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

This should be a blank line.

reference/reference_test.go Show resolved Hide resolved
bmt.go Outdated
// BMT provides the necessary extension of the hash interface to add the length-prefix of the BMT hash
//
// Any implementation should make it possible to generate a BMT hash using the hash.Hash interface only. However, the limitation will be that the Span of the BMT hash always must be limited to the amount of bytes actually written.
type BMTHash interface {
Copy link
Member

Choose a reason for hiding this comment

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

As the package is called bmt, could this type be called just Hash?

reference/reference.go Show resolved Hide resolved
bmt.go Outdated
"hash"
)

// BMT provides the necessary extension of the hash interface to add the length-prefix of the BMT hash
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect type name in comment.

@nolash nolash force-pushed the lash/interface-and-correctness branch from 856dc17 to 14c63aa Compare April 3, 2020 06:23
@nolash
Copy link
Contributor Author

nolash commented Apr 3, 2020

Thanks @janos amended

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Thanks @nolash. Looks good. There are still a few sentences in exported identifiers comments that do not end with a dot.

@nolash
Copy link
Contributor Author

nolash commented Apr 3, 2020

@janos you're right I was a bit quick there. Is this better?

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

LGTM except the usual rant

bmt.go Outdated

// Hash provides the necessary extension of the hash interface to add the length-prefix of the BMT hash.
//
// Any implementation should make it possible to generate a BMT hash using the hash.Hash interface only. However, the limitation will be that the Span of the BMT hash always must be limited to the amount of bytes actually written.
Copy link
Member

Choose a reason for hiding this comment

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

maybe have this comment split into two lines?

reference/doc.go Outdated
// license that can be found in the LICENSE file.

// Package bmt is a simple nonconcurrent reference implementation for hashsize segment based
// Binary Merkle tree hash on arbitrary but fixed maximum chunksize
Copy link
Member

Choose a reason for hiding this comment

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

somewhat ambivalent.
maybe "on a bounded n number of bytes such that 0 ≤ n ≤ 4096 bytes`?

return rh.hash(d, rh.maxDataLength)
}

// data has length maxDataLength = segmentSize * 2^k
Copy link
Member

Choose a reason for hiding this comment

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

comment more go-doc-ishly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also for unexported?

// section contains two data segments (d)
section = data
} else {
// section contains hashes of left and right BMT subtreea
Copy link
Member

@acud acud Apr 6, 2020

Choose a reason for hiding this comment

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

i like your latin

@nolash
Copy link
Contributor Author

nolash commented Apr 6, 2020

@janos @acud the new linter is a bit more eager than the last one, so had to do a few more changes. Still shouldn't change functionality. But please have another browse to be safe. Thanks.

@nolash nolash requested review from janos and acud April 6, 2020 15:49
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

LGTM. Only one small comment.

}

// calculates the Keccak256 SHA3 hash of the data
func sha3hash(t *testing.T, data ...[]byte) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

It would be good this function as t.Helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That means just add t.Helper() as first line in the function? @janos

@nolash nolash merged commit e6b105c into master Apr 8, 2020
@nolash nolash deleted the lash/interface-and-correctness branch April 8, 2020 08:02
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.

None yet

3 participants