Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TMHASH is 32 bytes. Closes #1990 #2732

Merged
merged 4 commits into from Oct 31, 2018
Merged

TMHASH is 32 bytes. Closes #1990 #2732

merged 4 commits into from Oct 31, 2018

Conversation

ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Oct 30, 2018

Closes #1990

Change TMHASH to 32-bytes.

Include TMHASHTruncated as TMHASH[:20]

Add crypto.AddressSize = TMHASHTruncated.Size

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md


// Sum returns the SHA256 of the bz.
func Sum(bz []byte) []byte {
h := sha256.Sum256(bz)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We can probably just return sha256.Sum(bz) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't because it's a [32]byte and not []byte

Copy link
Contributor

@liamsi liamsi Oct 30, 2018

Choose a reason for hiding this comment

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

Oh, I thought sha256 implements the hash.Hash interface which returns []byte. It actually does but not via the "static" methods, only via sha256.New().Sum(bz). Not important though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Could just do sha256.Sum(bz)[:], no?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, theres a weird reason why that doesn't work, I'm forgetting what it was.

const (
TruncatedSize = 20
)

type sha256trunc struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be called AddressHash

Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

LGTM. The constants e.g.MaxHeaderBytes etc need to be updated to fix the tests though (TestMaxHeaderBytes, TestMaxEvidenceBytes, TestMaxVoteBytes).
TestBlockMakePartSetWithEvidence seems to fail because the now marshalled data exceeds 2 "partSize" chunks (?) 🤔

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (develop@a530352). Click here to learn what that means.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             develop    #2732   +/-   ##
==========================================
  Coverage           ?   62.28%           
==========================================
  Files              ?      212           
  Lines              ?    17063           
  Branches           ?        0           
==========================================
  Hits               ?    10628           
  Misses             ?     5563           
  Partials           ?      872
Impacted Files Coverage Δ
p2p/key.go 70.21% <ø> (ø)
crypto/ed25519/ed25519.go 48.43% <0%> (ø)
crypto/crypto.go 0% <0%> (ø)
state/validation.go 65.62% <100%> (ø)
crypto/tmhash/hash.go 72.72% <90%> (ø)

@ebuchman ebuchman merged commit a22c962 into develop Oct 31, 2018
@ebuchman ebuchman deleted the bucky/tmhash branch October 31, 2018 16:42
maxim-levy pushed a commit to maxim-levy/tendermint that referenced this pull request Nov 13, 2018
* 'master' of https://github.com/tendermint/tendermint: (330 commits)
  Release/v0.26.1 (tendermint#2803)
  fix amino overhead computation for Tx (tendermint#2792)
  p2p: re-check after sleeps (tendermint#2664)
  check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787)
  p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797)
  test AutoFile#Size (happy path)
  [autofile/group] do not panic when checking size
  openFile creates a file if not exist => ErrNotExist is not possible
  use our logger in autofile/group
  Add tests for ValidateBasic methods (tendermint#2754)
  [docs] improve organization of ABCI docs & fix links (tendermint#2749)
  p2p: peer-id -> peer_id (tendermint#2771)
  mempool: print postCheck error (tendermint#2762)
  Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756)
  Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758)
  mempool: ErrPreCheck and more log info (tendermint#2724)
  Release/v0.26.0 (tendermint#2726)
  [ADR] [DRAFT] pubsub 2.0 (tendermint#2532)
  validate reactor messages (tendermint#2711)
  TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants