Enable validation of milestones with additional hash modes and security levels #1041

alon-e · 2018-10-07T10:21:38Z

Description

Enable validation of milestones with additional hash modes (CURLP27, CURLP81, KERL) and security levels (1, 2, 3)

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

Unit tests added for the different milestones with actually signed milestones from testing network
all combinations tested on a local testnet.

Checklist:

Please delete items that are not relevant.

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

GalRogozinski

Besides the naming convention I want to start treating codacy-bot like a human reviewer.
When it puts a comment either:

Do the change
Comment that the request is out of scope (like for the Npath complexity of an existing method)
Argue that the pmd rule should be removed (I deleted some comments and I shall fix the pmd promptly)

GalRogozinski · 2018-10-08T07:58:12Z

src/test/java/com/iota/iri/MilestoneTrackerTest.java

+    }
+
+    @Test
+    public void milestoneValidationCURLP27() throws Exception {


Stick with test naming conventions. Append ore prepend the word "test" (with proper capitalization).
Reason:
We currently use an old version of the maven surefire plugin that only runs tests that follow convention.

are you sure? afaik this is only for the test file name.

from a local mvn test run:

[INFO] Running com.iota.iri.MilestoneTrackerTest [INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.276 s - in com.iota.iri.MilestoneTrackerTest

from travis: (https://travis-ci.org/iotaledger/iri/jobs/438230685#L995-L999)

[INFO] Running com.iota.iri.MilestoneTrackerTest 10/07 10:53:11.658 [main] INFO c.i.i.s.r.RocksDBPersistenceProvider - Initializing Database Backend... 10/07 10:53:11.717 [main] INFO c.i.i.s.r.RocksDBPersistenceProvider - RocksDB persistence provider initialized. 10/07 10:53:12.396 [main] INFO com.iota.iri.storage.Tangle - Shutting down Tangle Persistence Providers... [INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.742 s - in com.iota.iri.MilestoneTrackerTest

GalRogozinski · 2018-10-08T07:58:43Z

src/test/java/com/iota/iri/MilestoneTrackerTest.java

+    }
+
+    @Test
+    public void milestoneValidationKERL() throws Exception {


GalRogozinski · 2018-10-08T07:59:11Z

src/test/java/com/iota/iri/MilestoneTrackerTest.java

+    }
+
+    @Test
+    public void milestoneValidationCURLP27SecLvl3() throws Exception {


GalRogozinski · 2018-10-08T07:59:45Z

src/test/java/com/iota/iri/MilestoneTrackerTest.java

+    }
+
+    @Test
+    public void milestoneValidationKERLSecLvl3() throws Exception {


GalRogozinski

Mainly style comments

GalRogozinski · 2018-10-08T12:48:47Z

src/main/java/com/iota/iri/MilestoneTracker.java

@@ -198,23 +197,29 @@ private Validity validateMilestone(SpongeFactory.Mode mode, TransactionViewModel
        else {
            for (final List<TransactionViewModel> bundleTransactionViewModels : bundleTransactions) {

-                //if (Arrays.equals(bundleTransactionViewModels.get(0).getHash(),transactionViewModel.getHash())) {
                if (bundleTransactionViewModels.get(0).getHash().equals(transactionViewModel.getHash())) {


Better to have bundleTransactionViewModels.get(0).getHash() be stored as a local variable outside the loop.
It can even be final because the loop opened a new scope.

GalRogozinski · 2018-10-08T12:50:49Z

src/main/java/com/iota/iri/MilestoneTracker.java

                if (bundleTransactionViewModels.get(0).getHash().equals(transactionViewModel.getHash())) {
+                    //the signed transaction - which references the confirmed transactions and contains the Merkle tree siblings.
+                    final TransactionViewModel siblingsTx = bundleTransactionViewModels.get(securityLevel);


if you change the line you can always remove the final

GalRogozinski · 2018-10-08T12:53:54Z

src/main/java/com/iota/iri/MilestoneTracker.java

+                            bb.put(digest);
+                        }
+
+                        final byte[] digests = bb.array();


locally scoped final

GalRogozinski · 2018-10-08T12:54:17Z

src/main/java/com/iota/iri/MilestoneTracker.java

+                        byte[] address = ISS.address(mode, digests);
+
+                        //validate Merkle path
+                        final byte[] merkleRoot = ISS.getMerkleRoot(mode, address,


locally scoped final

GalRogozinski · 2018-10-08T12:55:06Z

src/test/java/com/iota/iri/MilestoneTrackerTest.java

+    }
+
+    @Test
+    public void milestoneValidationCURLP27() throws Exception {


GalRogozinski · 2018-10-08T12:59:24Z

src/main/java/com/iota/iri/MilestoneTracker.java

@@ -4,8 +4,7 @@
 import com.iota.iri.controllers.AddressViewModel;
 import com.iota.iri.controllers.MilestoneViewModel;
 import com.iota.iri.controllers.TransactionViewModel;
-import com.iota.iri.hash.ISS;
-import com.iota.iri.hash.SpongeFactory;
+import com.iota.iri.hash.*;


I looked into it and apparently it is hard for static code analysis tools to handle stars. I will test later if it works better with checkstyle but currently I think it will be the same problem...

GalRogozinski · 2018-10-08T13:04:17Z

src/main/java/com/iota/iri/MilestoneTracker.java

-                                signatureFragmentTrits)),
-                                transactionViewModel2.trits(), 0, index, numOfKeysInMilestone);
+                        for (int i = 0; i < securityLevel; i++) {
+                            ISSInPlace.digest(mode, signedHash, ISS.NUMBER_OF_FRAGMENT_CHUNKS * i, bundleTransactionViewModels.get(i).getSignature(), 0, digest);


Later on checkstyle will fail this becuase the line is too long

GalRogozinski · 2018-10-08T13:09:47Z

src/test/java/com/iota/iri/MilestoneTrackerTest.java

+    @BeforeClass
+    public static void beforeClass() throws Exception {
+
+


While changing this remove all those whitespaces.
I usually don't mind them too much but you went overboard here.

…/milestonesSecLvl # Conflicts: # src/test/java/com/iota/iri/MilestoneTrackerTest.java

expand imports remove blank lines break lines

alon-e added 3 commits October 2, 2018 20:25

enable milestones with different signature modes and security level

419f10e

remove unused args

cacbdf1

extend checks on MilestoneBundleStructure for higher security levels.

eadfb46

alon-e added T-Enhancement C-Consensus E-Refactor Epic - Improving code readability and maintainability labels Oct 7, 2018

alon-e and others added 4 commits October 7, 2018 13:47

enable milestones with different signature modes and security level

82c44e9

remove unused args

dfdd5ef

extend checks on MilestoneBundleStructure for higher security levels.

02e02df

update tests for new TransactionValidator constructor args

bd3c30d

alon-e force-pushed the feat/milestonesSecLvl branch from eadfb46 to bd3c30d Compare October 7, 2018 10:49

iotaledger deleted a comment Oct 8, 2018

GalRogozinski suggested changes Oct 8, 2018

View reviewed changes

GalRogozinski reviewed Oct 8, 2018

View reviewed changes

alon-e added 2 commits October 8, 2018 16:55

Merge remote-tracking branch 'origin/feat/milestonesSecLvl' into feat…

85119b3

…/milestonesSecLvl # Conflicts: # src/test/java/com/iota/iri/MilestoneTrackerTest.java

remove finals

f7af600

expand imports remove blank lines break lines

iotaledger deleted a comment Oct 10, 2018

change visibility to package private

95c220f

GalRogozinski approved these changes Oct 15, 2018

View reviewed changes

GalRogozinski merged commit dcdf28c into iotaledger:dev Oct 15, 2018

alon-e mentioned this pull request Oct 28, 2018

Add bazel buildfiles iotaledger/compass#35

Merged

GalRogozinski mentioned this pull request Dec 22, 2018

Release v1.5.6 #1251

Merged

alon-e mentioned this pull request Mar 17, 2019

ciri: Support Milestones of higher security level and different hashing function iotaledger/entangled#975

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable validation of milestones with additional hash modes and security levels #1041

Enable validation of milestones with additional hash modes and security levels #1041

alon-e commented Oct 7, 2018 •

edited

Loading

GalRogozinski left a comment

GalRogozinski Oct 8, 2018

alon-e Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski left a comment

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

GalRogozinski Oct 8, 2018

		@BeforeClass
		public static void beforeClass() throws Exception {

Enable validation of milestones with additional hash modes and security levels #1041

Enable validation of milestones with additional hash modes and security levels #1041

Conversation

alon-e commented Oct 7, 2018 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

GalRogozinski 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski 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

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

alon-e commented Oct 7, 2018 •

edited

Loading