Skip to content

Conversation

dmitry-timofeev
Copy link
Contributor

@dmitry-timofeev dmitry-timofeev commented Oct 28, 2019

Overview

Check that all hashes are SHA-256 hashes (32-byte long).


See: https://jira.bf.local/browse/ECR-3687

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has Javadoc
  • Method preconditions are checked and documented in the Javadoc of the method
  • Changelog is updated if needed (in case of notable or breaking changes)
  • The continuous integration build passes

Check that all hashes are SHA-256 hashes (32-byte long).
@coveralls
Copy link

coveralls commented Oct 28, 2019

Coverage Status

Coverage increased (+0.09%) to 85.954% when pulling 570b76b on dmitry-timofeev:enforce-hash-size-ECR-3687 into 72ea871 on exonum:dynamic-services.

Copy link
Contributor

@MakarovS MakarovS left a comment

Choose a reason for hiding this comment

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

👍

}

// Check proof hashes
checkProofHashes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we check it later and avoid additional traverse through proof? Check this in #verifyEmptyRangeProof and #indexHashedEntriesByHeight? Or is it an unnecessary optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but that would require duplicating the code in both branches. I don't think it will have a significant impact, as the earlier profile showed that hashing takes most of the time. #1163 could tell the numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is literally noise:

(no verification)

Benchmark                      (height)   Mode  Cnt   Score   Error   Units
FlatListProofBenchmark.verify        20  thrpt   10  46,506 ? 0,212  ops/ms

(with verification, flat-list-verify-benchmark-with-hash-checks-ECR-3608)

Benchmark                      (height)   Mode  Cnt   Score   Error   Units
FlatListProofBenchmark.verify        20  thrpt   10  46,407 ? 0,230  ops/ms

@dmitry-timofeev dmitry-timofeev merged commit a20bbf3 into exonum:dynamic-services Oct 29, 2019
@dmitry-timofeev dmitry-timofeev deleted the enforce-hash-size-ECR-3687 branch October 29, 2019 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants