Add no key hashing option to ProofMapIndexProxy [ECR-3779] #1222

MakarovS · 2019-11-12T06:04:38Z

Overview

Add no key hashing option to ProofMapIndexProxy, update tests.

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

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 · 2019-11-12T12:00:33Z

Github shows the modifications weirdly, use git diff master:exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxyIntegrationTest.java HEAD:exonum-java-binding/core/src/test/java/com/exonum/binding/core/storage/indices/BaseProofMapIndexProxyIntegrationTest.java to see the actual differences between the previous tests and the new base tests.

bullet-tooth · 2019-11-12T13:10:45Z

...a-binding/core/src/main/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxy.java

- * and requires that keys are 32-byte long.
+ * <p>This map is implemented as a Merkle-Patricia tree. It does not permit null keys and values.
+ *
+ * <p>This map can be instantiated in two ways - either with keys hashing with methods


with..with...
how about : ...either with keys hashing using methods...

dmitry-timofeev · 2019-11-12T14:44:21Z

...test/java/com/exonum/binding/core/storage/indices/BaseProofMapIndexProxyIntegrationTest.java

+   * [(00…0PK1, V1), (00…0PK2, V2), … (00…0PKi, Vi)].
+   */
+  List<MapEntry<HashCode, String>> createSortedMapEntries() {
+    // Use PROOF_KEYS which are already sorted.


The implementation and documentation are not correct. Nowhere the base class says that getProofKeys are expected to be sorted
and how. The usages of this method must be reviewed to determine if they need sorting.
This method/getProofKeys and documentation must be updated accordingly.

dmitry-timofeev · 2019-11-12T14:45:21Z

...test/java/com/exonum/binding/core/storage/indices/BaseProofMapIndexProxyIntegrationTest.java

+  }
+
+  /**
+   * Creates 257 entries for a ProofMap that, when added to it, will make the underlying


Tests that rely on keys being equal to proof paths make no sense in the base but
non-hashing flavour of the ProofMap where this property holds. In hashing variant
there is no apparent connection between a key bits and the bits in the corresponding proof path.

The tests that set particular key bits to achieve a certain tree structure to exercise the proof validation must be moved to non-hashing variant.

dmitry-timofeev · 2019-11-12T14:46:01Z

...test/java/com/exonum/binding/core/storage/indices/BaseProofMapIndexProxyIntegrationTest.java

+  void getMultiProof_FourEntryMap_DoesNotContain() {
+    runTestWithView(database::createFork, (map) -> {
+      /*
+       This map will have the following structure:


That is incorrect in the base, see below.

dmitry-timofeev · 2019-11-12T14:47:39Z

...a-binding/core/src/main/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxy.java

+ * {@link #newInGroupUnsafe(String, byte[], View, Serializer, Serializer)} or with no key hashing
+ * with methods {@link #newInstanceNoKeyHashing(String, View, Serializer, Serializer)} and
+ * {@link #newInGroupUnsafeNoKeyHashing(String, byte[], View, Serializer, Serializer)}. In case of
+ * no key hashing keys are required to be 32-byte long. Note that the former option is considered


We discussed why it shall be the default option, and the documentation must properly reflect that.

dmitry-timofeev · 2019-11-12T14:48:10Z

...a-binding/core/src/main/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxy.java

+  }
+
+  /**
+   * Creates a ProofMapIndexProxy with no key hashing. Requires that keys are 32-byte long.


Would it be possible to add a short warning with a link to the class-level section?

dmitry-timofeev · 2019-11-12T14:48:56Z

...a-binding/core/src/main/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxy.java

+          ProofMapKeySizeCheckingSerializerDecorator.from(keySerializer);
+      return ProofMapKeyCheckingSerializerDecorator.from(sizeCheckingSerializerDecorator);
+    } else {
+      return ProofMapKeyCheckingSerializerDecorator.from(keySerializer);


Why is this needed at all if it can return just keySerializer, with no extra decorator types?

To check for not null values?

There is one for that already.

com.exonum.binding.common.serialization.CheckingSerializerDecorator

dmitry-timofeev · 2019-11-12T14:49:41Z

...a-binding/core/src/main/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxy.java

   * @param value a storage value to associate with the key
   * @throws IllegalStateException if this map is not valid
-   * @throws IllegalArgumentException if the size of the key is not 32 bytes
+   * @throws IllegalArgumentException if the size of the key is not 32 bytes (in case of a
+   *     no key hashing proof map)


Here and elsewhere 'a no key hashing proof map' does not sound correct.

... a non-key-hashing proof map

... a proof map that does not hash keys

if the size of the key is not 32 bytes and this map uses non-hashed keys ...

@Maria-Nosyk Would you tell us please which works best?

The third variant is absolutely perfect!

Brilliant, thanks!

Is non-key-hashing proof map a bad option though? Third variant is a bit too wordy in some cases.

Yup, the construction you suggest is, first, hard to understand, second, contradicts general requirements to technical documentation. The second variant suggested by @dmitry-timofeev is also acceptable.

dmitry-timofeev · 2019-11-12T14:50:29Z

...est/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxyGroupIntegrationTest.java

@@ -27,6 +27,10 @@

 class ProofMapIndexProxyGroupIntegrationTest extends BaseMapIndexGroupTestable<HashCode> {

+  private static final HashCode PK1 = HashCode.fromString(K1);


Nit: may just use Strings (Kx are already strings).

dmitry-timofeev · 2019-11-12T14:50:55Z

...a/com/exonum/binding/core/storage/indices/ProofMapIndexProxyNoKeyHashingIntegrationTest.java

+
+      putAll(map, entries);
+
+      Iterator<MapEntry<HashCode, String>> entriesIterator = map.entries();


(keys, values, entries) have incomplete documentation of the order of the elements missing the default variant.

The conditional documentation (with the fact that the API users always need to know if hashing happens or not to access the map from other services or verify the proofs it produces) makes me wonder if we shall add two separate types. But that's certainly for a separate issue.

dmitry-timofeev · 2019-11-12T14:54:37Z

...ain/java/com/exonum/binding/core/storage/indices/ProofMapKeyCheckingSerializerDecorator.java

- * A serializer decorator that checks proof map keys for correctness.
- *
- * @see StoragePreconditions#checkProofKey(byte[])
+ * A serializer decorator that checks proof map keys are not null.


Revert the class to its past form.

dmitry-timofeev · 2019-11-12T14:59:59Z

I expect this PR to have non-hashing tests @Disabled (since there is no support of them in native yet); and hashing one — enabled and passing (the appropriate native method requires a not-yet used jboolean flag, probably, prefixed with underscore so that clippy does not complain).

Also, the client code in various schemas must be updated to use the appropriate constructor.

dmitry-timofeev · 2019-11-12T15:02:24Z

...test/java/com/exonum/binding/core/storage/indices/BaseProofMapIndexProxyIntegrationTest.java

+import java.util.stream.Stream;
+import org.junit.jupiter.api.Test;
+
+abstract class BaseProofMapIndexProxyIntegrationTest


This package uses Testable as a suffix to indicate it is not a concrete test but abstract (I don't remember exactly, but I think surefire might also fail to instantiate a non-instantiable 'test' class, as it detects them based on the naming pattern *Test.java).

MakarovS · 2019-11-13T06:29:39Z

.../java/com/exonum/binding/core/storage/indices/BaseProofMapIndexProxyIntegrationTestable.java

-      for (MapEntry<HashCode, String> e : entries) {
-        assertThat(map, provesThatPresent(e.getKey(), e.getValue()));
-      }
+      assertThat(map, provesThatPresent(entries));
    });
  }

  @Test
  void getProof_MultiEntryMapDoesNotContain() {


This test creates 32-byte keys, but it still seems to be appropriate for both proof map cases.

MakarovS · 2019-11-13T06:52:42Z

I expect this PR to have non-hashing tests @disabled (since there is no support of them in native yet); and hashing one — enabled and passing (the appropriate native method requires a not-yet used jboolean flag, probably, prefixed with underscore so that clippy does not complain).

Ok, will disable those, I thought core changes are already merged and we'd be able to integrate this PR with native code, but I see it's still WIP.

dmitry-timofeev · 2019-11-13T12:19:38Z

...a/com/exonum/binding/core/storage/indices/ProofMapIndexProxyNoKeyHashingIntegrationTest.java

+class ProofMapIndexProxyNoKeyHashingIntegrationTest
+    extends BaseProofMapIndexProxyIntegrationTestable {
+
+  static final List<HashCode> PROOF_KEYS = Stream.of(


TEST_KEYS/SORTED_TEST_KEYS

dmitry-timofeev · 2019-11-13T12:20:11Z

...a/com/exonum/binding/core/storage/indices/ProofMapIndexProxyNoKeyHashingIntegrationTest.java

+  private static final HashCode INVALID_PROOF_KEY = HashCode.fromString("1234");
+
+  @Override
+  List<HashCode> getProofKeys() {


Nit: getKeys or getTestKeys

dmitry-timofeev · 2019-11-13T12:20:34Z

...a/com/exonum/binding/core/storage/indices/ProofMapIndexProxyNoKeyHashingIntegrationTest.java

+   * [(00…0PK1, V1), (00…0PK2, V2), … (00…0PKi, Vi)].
+   */
+  List<MapEntry<HashCode, String>> createSortedMapEntries() {
+    // Use PROOF_KEYS which are already sorted.


Use them directly instead of interface method, with appropriate
naming there won't be a need for comment

dmitry-timofeev · 2019-11-14T17:46:14Z

Also, the client code in various schemas must be updated to use the appropriate constructor.

As most of ProofMaps in services and schemas continue using nonHashing, they shall be migrated to that. (The only hashing is probably DispatcherSchema).

Separately we might consider migrating some services to the default hashing variant, e.g., QaService with its two maps 🤔

coveralls · 2019-11-14T22:52:28Z

Coverage decreased (-0.2%) to 85.9% when pulling 3c21a42 on ECR-3779 into d3ff9b6 on master.

MakarovS · 2019-11-15T04:51:15Z

As most of ProofMaps in services and schemas continue using nonHashing, they shall be migrated to that. (The only hashing is probably DispatcherSchema).

Ok, will do that in a separate PR.

dmitry-timofeev · 2019-11-15T12:52:16Z

...a-binding/core/src/main/java/com/exonum/binding/core/storage/indices/ProofMapIndexProxy.java

+  /**
+   * {@inheritDoc}
+   *
+   * The entries are ordered by keys in lexicographical order if this map is a non-key-hashing


... is a <a href="...">non-key-hashing proof map</a>.

dmitry-timofeev · 2019-11-15T13:04:04Z

Ok, will do that in a separate PR.

I don't think it is reasonable: this one changes the specification of newInstance (and it becomes broken according to its spec until the native is done), replacing that with newInstanceNonHashing (which works in accordance with the spec). Why the clients, of which there are 6 total, shall remain on the broken version and complicate the native implementation, which, when done, will render the clients actually broken?

It is 15 seconds search-replace (6 usages in prod code) + 2–5 minutes documenting the non-hashing aspect, where applicable.

MakarovS · 2019-11-16T19:00:27Z

It is 15 seconds search-replace (6 usages in prod code) + 2–5 minutes documenting the non-hashing aspect, where applicable.

Ok, updated those. Not sure what documentation is applicable for those, other than // Use non-hashing proof map, as keys of this map are already 32-byte long hashes, which seems redundant, given Javadoc of the corresponding method.

dmitry-timofeev · 2019-11-18T11:40:12Z

The variant in use must be documented in schemas as they are supposed to provide adequate specification of the service persistent data. Whether or not a proof map hashes its keys affects the clients verifying proofs.

as keys of this map are already 32-byte long hashes

What the API documentation must say is whether or not a hashing variant is used (it doesn't have to explain why). As 32-byte length is not enough to use a non-key-hashing map, as its documentation says, the justification might be either in the API docs or in implementation docs.

dmitry-timofeev · 2019-11-18T11:43:13Z

Whether or not a proof map hashes its keys affects the clients verifying proofs.

If the proofs included the type of map from which they are created, the specification of key hashing won't be needed. I am not sure if it makes sense to include such flag in the proof.

dmitry-timofeev · 2019-11-20T10:55:12Z

@MakarovS please resolve the build.

MakarovS · 2019-11-21T02:51:51Z

@MakarovS please resolve the build.

Fixed.

dmitry-timofeev

Good, thanks!

dmitry-timofeev · 2019-11-21T09:13:53Z

If the ordering is the same, then it might make sense to move the tests of iterators back to the base.

dmitry-timofeev · 2019-11-21T10:22:55Z

https://jira.bf.local/browse/ECR-3871

ECR-3779 Add no key hashing option to ProofMapIndexProxy

e4a3631

MakarovS requested review from bullet-tooth and dmitry-timofeev November 12, 2019 06:04

Merge remote-tracking branch 'remotes/origin/master' into ECR-3779

137e992

bullet-tooth reviewed Nov 12, 2019

View reviewed changes

dmitry-timofeev suggested changes Nov 12, 2019

View reviewed changes

dmitry-timofeev reviewed Nov 12, 2019

View reviewed changes

MakarovS added 2 commits November 13, 2019 01:18

ECR-3779 Review fixes

4048d4c

Merge remote-tracking branch 'remotes/origin/master' into ECR-3779

6cc288a

MakarovS commented Nov 13, 2019

View reviewed changes

dmitry-timofeev reviewed Nov 13, 2019

View reviewed changes

ECR-3779 Review fixes

20e7ac8

MakarovS added 3 commits November 14, 2019 16:41

ECR-3779 Review fixes

8edf1bf

Merge remote-tracking branch 'remotes/origin/master' into ECR-3779

2d22068

ECR-3779 Checkstyle fix

48109ca

dmitry-timofeev reviewed Nov 15, 2019

View reviewed changes

MakarovS added 2 commits November 16, 2019 00:24

ECR-3779 Fix Javadoc and update proof map instantiation in clients

3e215d1

Merge remote-tracking branch 'remotes/origin/master' into ECR-3779

3a8fe9f

MakarovS added 2 commits November 18, 2019 23:39

ECR-3779 Fix Javadocs

95e96ad

Merge remote-tracking branch 'remotes/origin/master' into ECR-3779

5b3e619

dmitry-timofeev approved these changes Nov 19, 2019

View reviewed changes

MakarovS added 2 commits November 20, 2019 21:49

ECR-3779 Checkstyle fix

82bbba8

Merge remote-tracking branch 'origin/master' into ECR-3779

0b00849

Merge branch 'master' into ECR-3779

3c21a42

dmitry-timofeev approved these changes Nov 21, 2019

View reviewed changes

dmitry-timofeev merged commit 4295cf8 into master Nov 21, 2019

dmitry-timofeev deleted the ECR-3779 branch November 21, 2019 09:12

		@@ -27,6 +27,10 @@

		class ProofMapIndexProxyGroupIntegrationTest extends BaseMapIndexGroupTestable<HashCode> {

		private static final HashCode PK1 = HashCode.fromString(K1);


		putAll(map, entries);

		Iterator<MapEntry<HashCode, String>> entriesIterator = map.entries();

Add no key hashing option to ProofMapIndexProxy [ECR-3779] #1222

Add no key hashing option to ProofMapIndexProxy [ECR-3779] #1222

Conversation

MakarovS commented Nov 12, 2019 • edited Loading

Overview

Definition of Done

dmitry-timofeev commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakarovS commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev commented Nov 14, 2019

coveralls commented Nov 14, 2019 • edited Loading

MakarovS commented Nov 15, 2019

Choose a reason for hiding this comment

dmitry-timofeev commented Nov 15, 2019

MakarovS commented Nov 16, 2019

dmitry-timofeev commented Nov 18, 2019 • edited Loading

dmitry-timofeev commented Nov 18, 2019

dmitry-timofeev commented Nov 20, 2019

MakarovS commented Nov 21, 2019

dmitry-timofeev left a comment

Choose a reason for hiding this comment

dmitry-timofeev commented Nov 21, 2019

dmitry-timofeev commented Nov 21, 2019

MakarovS commented Nov 12, 2019 •

edited

Loading

coveralls commented Nov 14, 2019 •

edited

Loading

dmitry-timofeev commented Nov 18, 2019 •

edited

Loading