Make DataWord immutable #1154

zilm13 · 2018-08-13T21:48:35Z

Uses #902 which is a bit outdated

resolve conflicts
few fixes

Should resolve #902 #766 #884 #706
@mkalinin @eugene-shevchenko

…to tonycox-immutableDW

coveralls · 2018-08-13T22:12:12Z

Coverage decreased (-0.2%) to 56.119% when pulling d8bfbef on tonycox-immutableDW into eacfc01 on develop.

mkalinin · 2018-08-14T07:01:23Z

ethereumj-core/src/main/java/org/ethereum/config/CommonConfig.java

@@ -259,8 +259,8 @@ public HeaderStore headerStore() {
                new Serializer<byte[], byte[]>() {
                    public byte[] serialize(byte[] object) {
                        DataWord ret = new DataWord(object);
-                        ret.add(new DataWord(1));
-                        return ret.getLast20Bytes();
+                        DataWord addResult = ret.add(new DataWord(1));


Let's add DataWord.ONE predefined value

mkalinin · 2018-08-14T07:51:26Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-    public static final BigInteger MAX_VALUE = _2_256.subtract(BigInteger.ONE);
-    public static final DataWord ZERO = new DataWord(new byte[32]);      // don't push it in to the stack
-    public static final DataWord ZERO_EMPTY_ARRAY = new DataWord(new byte[0]);      // don't push it in to the stack
+    private static final BigInteger _2_256 = BigInteger.valueOf(2).pow(256);


I think that access modifiers should stay the same

mkalinin · 2018-08-14T07:54:40Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-    public static final DataWord ZERO_EMPTY_ARRAY = new DataWord(new byte[0]);      // don't push it in to the stack
+    private static final BigInteger _2_256 = BigInteger.valueOf(2).pow(256);
+    private static final BigInteger MAX_VALUE = _2_256.subtract(BigInteger.ONE);
+    private static final DataWord ZERO = new DataWord(new byte[32]);      // don't push it in to the stack


ZERO should be represented as an empty array with some constant data, let's use ZERO_EMPTY_ARRAY for that. And since this class became immutable the comment don't push it in to the stack is invalid.

mkalinin · 2018-08-14T07:55:07Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java


-    public DataWord() {
+    public static DataWord zero() {


No need. The only thing we need is DataWord.ZERO

mkalinin · 2018-08-14T07:55:34Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

+    }
+
+    private DataWord() {
+        data = ZERO.getData();


data = ZERO_EMPTY_ARRAY;

mkalinin · 2018-08-14T08:04:36Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

@@ -81,18 +86,26 @@ public DataWord(byte[] data) {
            this.data = ByteUtil.EMPTY_BYTE_ARRAY;


Better use inner constant here.

mkalinin · 2018-08-14T08:05:56Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-        else
-            throw new RuntimeException("Data word can't exceed 32 bytes: " + data);
+        else if (data.length <= 32) {
+            byte[] bytes = ZERO.getData();


To preserve mutability there should be a brand new empty array allocated and then filled with given data. The same thing is with above case

But doing it that way we won't be able to efficiently use this constructor in inner DataWord allocations where DataWord itself cares about immutability. I see two options (i) make this constructor private and use for inner purposes, for outer create a method DataWord.from(byte[] data) with all required checks and copies (ii) do it vise versa

Yep, this looks pretty odd. I see that getData() makes a copy, but new byte[32] suites better here

mkalinin · 2018-08-14T08:15:14Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

+        return Arrays.copyOf(data, data.length);
+    }
+
+    public DataWord insert(int index, byte element) {


This is not needed. We can use program.stackPush(DataWord.ONE) to cover all cases that this method covers

mkalinin · 2018-08-14T08:17:00Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java


-        if (this.isZero()) return;
+        if (this.isZero()) return zero();


return this;

mkalinin · 2018-08-14T08:30:42Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java


        if (word.isZero()) {
-            this.and(ZERO);
-            return;
+            return this.and(ZERO);


return ZERO;

…ntiation with 'of' factory

zilm13 · 2018-08-14T21:55:02Z

Second try!

Nashatyrev · 2018-08-15T20:23:12Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-        else
-            throw new RuntimeException("Data word can't exceed 32 bytes: " + data);
+        else if (data.length <= 32) {
+            byte[] bytes = ZERO.getData();


Yep, this looks pretty odd. I see that getData() makes a copy, but new byte[32] suites better here

Nashatyrev · 2018-08-15T20:26:16Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

    }

    public byte[] getData() {
-        return data;
+        return Arrays.copyOf(data, data.length);
+    }


I would leave this as is, just making sure the returned array is not accidentally modified else where (which normally shouldn't be the case for getData() method). If one really needs a copy I'd suggest add special getDataCopy() method

Nashatyrev · 2018-08-15T20:27:34Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

    }

    public byte[] getNoLeadZeroesData() {
-        return ByteUtil.stripLeadingZeroes(data);
+        return ByteUtil.stripLeadingZeroes(getData());


Again getData() makes absolutely unnecessary copy internally

Nashatyrev · 2018-08-15T20:40:52Z

ethereumj-core/src/main/java/org/ethereum/vm/VM.java

                    if (word1.value().compareTo(word2.value()) == -1) {
-                        word1.and(DataWord.ZERO);
-                        word1.getData()[31] = 1;
+                        program.stackPush(andResult.insert(31, (byte) 1));


Strange code to push 1 constant

It's outdated

Nashatyrev · 2018-08-15T20:41:19Z

ethereumj-core/src/main/java/org/ethereum/vm/VM.java

                    } else {
-                        word1.and(DataWord.ZERO);
+                        program.stackPush(andResult);


Strange code to push 0

It's outdated

Nashatyrev · 2018-08-15T20:42:06Z

ethereumj-core/src/main/java/org/ethereum/vm/VM.java

                    if (word1.sValue().compareTo(word2.sValue()) == -1) {
-                        word1.and(DataWord.ZERO);
-                        word1.getData()[31] = 1;
+                        program.stackPush(andResult.insert(31, (byte) 1));


Nashatyrev · 2018-08-15T20:42:18Z

ethereumj-core/src/main/java/org/ethereum/vm/VM.java

                    } else {
-                        word1.and(DataWord.ZERO);
+                        program.stackPush(andResult);
                    }


And cases below

Nashatyrev · 2018-08-16T09:31:54Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-    public DataWord and(DataWord w2) {
-
+    public DataWord and(DataWord word) {
+        byte[] newData = this.getData();


It's very confusing to use getData() for making an array copy. It better to make it explicitly or add copyData() private method
The similar below.

Nashatyrev · 2018-08-16T09:35:13Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

    }

    public byte[] getData() {
-        return data;
+        return Arrays.copyOf(data, data.length);


I'm still suggesting returning internal array here (mentioning this in method docs). It's a normal practice (e.g. take a look at java.nio.ByteBuffer.array()

mkalinin · 2018-08-17T06:39:26Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

 *
 * @author Roman Mandeleil
 * @since 01.06.2014
 */
-public class DataWord implements Comparable<DataWord> {
+public final class DataWord implements Comparable<DataWord> {


For some reason Github doesn't allow to comment on public boolean isZero() like. But this method lacks a shortcut check for whether this == ZERO or not

mkalinin · 2018-08-17T06:50:32Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

+            return DataWord.ZERO;
+        }
+
+        boolean allExceptLastIsZero = true;


You may use 8 * data.length - ByteUtil.numberOfLeadingZeros(data). And check the residual against 0 or 1

mkalinin · 2018-08-17T06:51:34Z

ethereumj-core/src/main/java/org/ethereum/config/CommonConfig.java

-                        ret.add(new DataWord(1));
-                        return ret.getLast20Bytes();
+                        DataWord ret = DataWord.of(object);
+                        DataWord addResult = ret.add(DataWord.of(1));


Why not use DataWord.ONE here?

mkalinin · 2018-08-17T06:54:58Z

ethereumj-core/src/main/java/org/ethereum/vm/program/Program.java

    }

    public void stackPushZero() {
-        stackPush(new DataWord(0));
+        stackPush(DataWord.of(0));


DataWord.ZERO

mkalinin · 2018-08-17T06:55:09Z

ethereumj-core/src/main/java/org/ethereum/vm/program/Program.java

    }

    public void stackPushOne() {
-        DataWord stackWord = new DataWord(1);
+        DataWord stackWord = DataWord.of(1);


DataWord.ONE

mkalinin · 2018-08-17T06:55:44Z

ethereumj-core/src/test/java/org/ethereum/datasource/RepoNewTest.java

@@ -155,31 +155,31 @@ public void testStorage1() throws Exception {
        RepositoryRoot repo = new RepositoryRoot(stateDb, null);
        byte[] addr1 = decode("aaaa");
        repo.createAccount(addr1);
-        repo.addStorageRow(addr1, new DataWord(1), new DataWord(111));
+        repo.addStorageRow(addr1, DataWord.of(1), DataWord.of(111));


DataWord.ONE

@mkalinin Here you probably a little bit doebalsa :) This is just a test

Ahahaha, maybe just a little bit

…shortcuts where it is possible

eugene-shevchenko

Nice job. It'll help to avoid mystic bugs =)

eugene-shevchenko · 2018-08-17T09:37:30Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-        else
-            throw new RuntimeException("Data word can't exceed 32 bytes: " + data);
+    public static DataWord of(long num) {
+        return of(ByteBuffer.allocate(8).putLong(num).array());


same here Long.BYTES

We've got this already: org.ethereum.util.ByteUtil.longToBytes

eugene-shevchenko · 2018-08-17T09:37:54Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

-    public DataWord(ByteArrayWrapper wrappedData){
-        this(wrappedData.getData());
+    public static DataWord of(int num) {
+        return of(ByteBuffer.allocate(4).putInt(num).array());


It's better use Integer.BYTES constant.

We've got this already: org.ethereum.util.ByteUtil.intToBytes

eugene-shevchenko · 2018-08-17T09:40:30Z

ethereumj-core/src/main/java/org/ethereum/vm/DataWord.java

    public byte[] getData() {
-        return data;
+        return Arrays.copyOf(data, data.length);


Maybe it's better to use copyData() and mark getData() as deprecated.

tonycox and others added 2 commits June 18, 2017 05:25

Made DataWord immutable

b8e6b9d

Merge branch 'immutableDW' of https://github.com/tonycox/ethereumj in…

c6d94bd

…to tonycox-immutableDW

zilm13 requested review from mkalinin and eugene-shevchenko August 13, 2018 21:48

mkalinin added this to the 1.9.0-Constantinople milestone Aug 14, 2018

mkalinin suggested changes Aug 14, 2018

View reviewed changes

Exclude redundant data copy in DataWord while protecting public insta…

55f6413

…ntiation with 'of' factory

Nashatyrev suggested changes Aug 15, 2018

View reviewed changes

zilm13 and others added 2 commits August 16, 2018 04:12

Make Dataword creation cleaner

4a6ce44

Fix for odd DataWord manipulation

25a5803

Nashatyrev reviewed Aug 16, 2018

View reviewed changes

Added copy data method to explicitely show it's purpose

9313415

Nashatyrev approved these changes Aug 16, 2018

View reviewed changes

DataWord final cleanup

49677a6

mkalinin suggested changes Aug 17, 2018

View reviewed changes

zilm13 added 2 commits August 17, 2018 12:21

DataWord simplified plus usage instantiation changed to ZERO and ONE …

6ca160f

…shortcuts where it is possible

DataWord.of(1) changed to DataWord.ONE

85c20f5

mkalinin approved these changes Aug 17, 2018

View reviewed changes

eugene-shevchenko suggested changes Aug 17, 2018

View reviewed changes

Disposing of magic numbers for numeric byte sizes

d8bfbef

eugene-shevchenko approved these changes Aug 17, 2018

View reviewed changes

mkalinin merged commit 5229ee2 into develop Aug 17, 2018

This was referenced Aug 17, 2018

Making DataWord class immutable #766

Closed

Make DataWord immutable #884

Closed

Make DataWord immutable #706

Closed

zilm13 deleted the tonycox-immutableDW branch September 20, 2018 09:11

		@@ -81,18 +86,26 @@ public DataWord(byte[] data) {
		this.data = ByteUtil.EMPTY_BYTE_ARRAY;

Make DataWord immutable #1154

Make DataWord immutable #1154

Conversation

zilm13 commented Aug 13, 2018 • edited Loading

coveralls commented Aug 13, 2018 • edited Loading

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

zilm13 commented Aug 14, 2018

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

Choose a reason for hiding this comment

eugene-shevchenko left a 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

Nashatyrev Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 commented Aug 13, 2018 •

edited

Loading

coveralls commented Aug 13, 2018 •

edited

Loading

Nashatyrev Aug 17, 2018 •

edited

Loading