Sitelet https://github.com/exonum/exonum-java-binding/pull/1156
Skip to content

Add flat list proof support [ECR-3608] - #1156

Merged
dmitry-timofeev merged 24 commits into
exonum:dynamic-servicesfrom
dmitry-timofeev:flat-list-proofs-ECR-3608
Oct 16, 2019
Merged

dmitry-timofeev merged 24 commits into
exonum:dynamic-servicesfrom
dmitry-timofeev:flat-list-proofs-ECR-3608

Conversation

@dmitry-timofeev

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@vitvakatu

Copy link
Copy Markdown
Contributor

I like tests so far 👍

78 of 81 tests pass; 3 — fail.

@MakarovS MakarovS left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

coveralls commented Oct 10, 2019 •

Copy link
Copy Markdown

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

@MakarovS MakarovS left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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