[refer #1132] Change ModifierId representation

[knizhnik]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 95

• 15 of 23 (65.22%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-2.3%) to 81.5%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/main/scala/scorex/util/package.scala	15	23	65.22%

Totals
Change from base Build 83:	-2.3%
Covered Lines:	163
Relevant Lines:	200

---

💛 - Coveralls

[aslesarenko]

Please add tests that checks that new implementation of ModifierId:

1. equals or not equal if and only if the corresponding Base16 strings are equal
2. new modifierOrdering is equivalent with ordering of Base16 strings.

[aslesarenko]

@knizhnik I assume hashBytes is always at least 32 bytes. If it is not, then ModifierId should not be used in a first place.

[knizhnik]

Ok, but there ARE such places. At least in tests.

[knizhnik]

public class ArrayCmp {
	static int myhashfunc(byte[] hashBytes) {
		int len = Math.min(hashBytes.length, 4);
		int hash = 0;
		for (int i = 0; i < len; i++) {
			hash = (hash << 8) | (hashBytes[i] & 0xFF);
		}
		return hash;
	}

	static int yourhashfunc(byte[] hashBytes) {
		return hashBytes[0] << 24 | (hashBytes[1] & 0xFF) << 16 | (hashBytes[2] & 0xFF) << 8 | (hashBytes[3]  & 0xFF);
	}

	public static void main(String[] args)
	{
		byte[] a = new byte[32];
	    long x = 0;
		long start = System.currentTimeMillis();
		for (int i = 0; i < 1000000000; i++) {
			x += myhashfunc(a);
		}
		long stop = System.currentTimeMillis();
		System.out.println("Elapsed time for my hash: "  + (stop - start));
		start = stop;
		for (int i = 0; i < 1000000000; i++) {
			x += myhashfunc(a);
		}
		stop = System.currentTimeMillis();
		System.out.println("Elapsed time your hash: "  + (stop - start));
	}
}

time java ArrayCmp
Elapsed time for my hash: 2789
Elapsed time your hash: 3386

[aslesarenko]

@knizhnik Some issues with this benchmark:

1. Looks like you don't call yourhashfunc
2. initialize a with real hash value
3. need to warm up JIT, use JMH for that, or ScalaMeter at least
4. print accumulated x value

[knizhnik]

Sorry, I was also surprised by this result.
Now yourfunc s ~10x times faster.

[aslesarenko]

Array.utils.compare performs signed byte comparison which is not compatible
with comparison of Base16 strings.
This is why I have to implement comparison myself.
Actually I spent three days trying to understand why all testes are passed by node load is failed.
Sorry, I should mention it in comment.

Yes, this is a good point, 3 days definitely deserve a detailed description.
Please also create the corresponding tests, I mentioned them in the review.

[aslesarenko]

There are places where ModifierId is used for just two-bytes key ("0005").

I think the tests should be fixed. /cc @kushti

[kushti]

Do not see correspondence to content-addressable storage, modifier id is not necessarily derived from the content of the modifier only.

[aslesarenko]

Then this reference can be removed I guess.

[kushti]

if this is unsigned comparison, let's state it clearly in the comments

[knizhnik]

I think the tests should be fixed. /cc @kushti
There is the following extension:

Extension(id: b9e2c5321993b37fb34608d08ae66c516120fed6c381a771e7e96790b64adff5, headerId: 743f7b91da88f5781bd1cdc457e8d2e50931bdc9ec0a3ec759e2b6e9e7c1b29c, fields: Vector(0005 -> 00000064, 0001 -> 001312d0, 0006 -> 000007d0, 0002 -> 00000168, 0007 -> 00000064, 0003 -> 00080000, 007b -> 00000001, 0008 -> 00000064, 0004 -> 000f4240, 007c -> 0000)) 

And here it is checked using ModifierId hash function:

  .validate(exDuplicateKeys, extension.fields.map(kv => bytesToId(kv._1)).distinct.length == extension.fields.length, extension.encodedId)

Is it a valid extension body? If so, should I replace this check not to use bytesToId function?

[kushti]

Please replace bytestoId with just Base16.encode

[kushti]

@aslesarenko @knizhnik let's take LAST 4 bytes, as in case of proof-of-work hash with partial collision may start with zero bits

[aslesarenko]

Yes, good point.