Skip to content

Conversation

dmitry-timofeev
Copy link
Contributor

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

Overview

Add flat list proof support.


изображение


See:

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

@dmitry-timofeev dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Oct 7, 2019
@dmitry-timofeev
Copy link
Contributor Author

Currently, only unit tests are implemented, hence WIP status. The unit tests in FlatListProofTest can be reviewed.

@dmitry-timofeev dmitry-timofeev requested a review from slowli October 7, 2019 18:06
@dmitry-timofeev
Copy link
Contributor Author

@slowli Would review please if the most important scenarious are covered?

@vitvakatu
Copy link
Contributor

I like tests so far 👍

78 of 81 tests pass; 3 — fail.
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.

Tests 👍

* If it is valid, the proof contents may be accessed. See {@link CheckedListProof}
* and {@link CheckedMapProof} for available contents description.
*/
// todo: Why do we need to represent invalid proofs?
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we throwing an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flat proof currently does not implement UncheckedProof interface, that's why it may and does throw exceptions. I decided exceptions are easier to implement and test. The question is on the original reasoning to represent them this way.

There is a year-old https://jira.bf.local/browse/ECR-2410 to reconsider.

@dmitry-timofeev dmitry-timofeev removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Oct 10, 2019
@dmitry-timofeev
Copy link
Contributor Author

The implementation is complete. We might consider extracting verification of each type of proof in separate classes to share some data used during verification (e.g., treeHeight, proofEntriesByHeight, etc.)

@coveralls
Copy link

coveralls commented Oct 10, 2019

Coverage Status

Coverage increased (+0.6%) to 86.15% when pulling cb286cb on dmitry-timofeev:flat-list-proofs-ECR-3608 into 89768b2 on exonum:dynamic-services.

A HashMap constructor accepts the initial capacity of the internal
table of hash buckets, not the expected number of elements
(as ArrayList constructor), hence the code used to down-size that
table (given the default 0.75 load factor).

The benchmark with GC profiler confirms the fix:

Profile 1, no fix:

Benchmark                                                       (height)   Mode  Cnt      Score     Error   Units
FlatListProofBenchmark.verify                                         10  thrpt   10     85,289 ±   0,920  ops/ms
FlatListProofBenchmark.verify:·gc.alloc.rate                          10  thrpt   10    931,355 ±  10,365  MB/sec
FlatListProofBenchmark.verify:·gc.alloc.rate.norm                     10  thrpt   10  14320,003 ±   0,001    B/op
FlatListProofBenchmark.verify:·gc.count                               10  thrpt   10    292,000            counts
FlatListProofBenchmark.verify:·gc.time                                10  thrpt   10    159,000                ms

Profile 2, with exp. size:

Benchmark                                                       (height)   Mode  Cnt      Score     Error   Units
FlatListProofBenchmark.verify                                         10  thrpt   10     87,436 ±   0,961  ops/ms
FlatListProofBenchmark.verify:·gc.alloc.rate                          10  thrpt   10    921,130 ±  10,306  MB/sec
FlatListProofBenchmark.verify:·gc.alloc.rate.norm                     10  thrpt   10  13816,003 ±   0,001    B/op
FlatListProofBenchmark.verify:·gc.count                               10  thrpt   10    290,000            counts
FlatListProofBenchmark.verify:·gc.time                                10  thrpt   10    157,000                ms
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.

👍

o — 'virtual' node — not present in the proof, inferred during verification.
Shown mostly to communicate the tree structure of the proof.
See also: https://wiki.bf.local/display/EXN/Flat+list+proofs
Copy link
Contributor

Choose a reason for hiding this comment

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

Link to the internal documentation? It's not a public class though, so it's probably fine

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 will be public soon, but this comment is not a Javadoc, it is an implementation comment, and having the docs available and discoverable to some developers (actually — most 🙃) is better than to none.

I could mark it with [internal] if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

It's good as it is, thanks - indeed it's not a Javadoc.

Push #getHeight to the hashed entry as the element entries
are always at height 0 and their height is neither requested
nor included in the proof originally.
@dmitry-timofeev
Copy link
Contributor Author

Pushed #getHeight down from the interface: cb286cb

@dmitry-timofeev dmitry-timofeev merged commit fad3c49 into exonum:dynamic-services Oct 16, 2019
@dmitry-timofeev dmitry-timofeev deleted the flat-list-proofs-ECR-3608 branch October 16, 2019 11:20
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.

4 participants