refactor milestone signature parameters to be configurable #1322

alon-e · 2019-02-05T12:15:49Z

Description

Refactor of milestone signatures to allow easy configuration of a different Coordinator.
with higher Merkle tree depth.

Fixes # (issue)

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

Run locally on Mainnet and Testnet (testing all new flags)

giving --testnet-coordinator-signature-mode a bad value, like KERM would give the following error:

02/05 14:15:31.993 [main] ERROR com.iota.iri.IRI$IRILauncher - There was a problem parsing commandline arguments: Invalid value for --testnet-coordinator-signature-mode parameter. Allowed values:[CURLP81, CURLP27, KERL]

alon-e · 2019-02-05T12:16:49Z

src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java


-        if (milestoneIndex < 0 || milestoneIndex >= 0x200000) {
+        if (milestoneIndex < 0 || milestoneIndex >= maxMilestoneIndex) {


this will allow higher depths of trees. (and tighter validation for lower trees)

alon-e · 2019-02-05T12:18:10Z

src/main/java/com/iota/iri/conf/TestnetConfig.java

+    }
+
+    @JsonProperty("COORDINATOR_SIGNATURE_MODE")
+    @Parameter(names = "--testnet-coordinator-signature-mode", description = MilestoneConfig.Descriptions.COORDINATOR_SIGNATURE_MODE)


I originally added a converter and validator, a la compass
https://github.com/iotaledger/compass/blob/3aaf05763cded4a229cc23f8e9cd3d53256948db/compass/conf/BaseConfiguration.java#L49

but the default validation is really good (see PR::test)

GalRogozinski · 2019-02-06T11:02:47Z

src/main/java/com/iota/iri/conf/MilestoneConfig.java

+    /**
+     * @return {@value Descriptions#NUMBER_OF_KEYS_IN_A_MILESTONE}
+     */
+    int getNumberOfKeysInMilestone();


Maybe add default value to the description like in #1159?

GalRogozinski · 2019-02-06T11:45:31Z

src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java

-    private ConsensusConfig config;
+    private boolean skipValidation;
+
+    private int maxMilestoneIndex;


Why did you decide to extract those from the config object and not just use the getters like before.
In my opinion this just saves lots of lines of code...

I think the nicer thing to do is to have HashFactory.ADDRESS.create(config.getCoordinator()).
I suppose maxMilestoneIndex = 1 << config.getNumberOfKeysInMilestone(); can also be done as an automatically calculated config in my opinion.

The thing is that if we do this change here, we will have to do the same design in the rest of the code.

So what I am asking for you is either:

Explain why you think it is better to not use config getters. In this case then I insist that at least the fields will be final. According to DDD services should be stateless. I say that at the very least they should be immutable. (The Hash field won't be immutable, as you well know, but we will solve it)

Move everything back to the config and use getters (what I prefer). There is a reason why the setters are not visible

It may be interesting to have a MilestoneService instance that can validate milestones coming from another coo than the one configured (wink, wink ...)

The reason is that these parameters are fetched, constructed and calculated often. I think it's not efficient to do that, but I get the point of not storing state.

Anyways, I'm not sure I'll be available this week to make the change, if you find time to do it great, if not I'll update when I get to it.

I wouldn't expect this to be the bottleneck in any scenario.
Doing a get should be really fast
I would expect that the HotSpots compiler will optimize inefficiencies that occur

karimodm

Ready for configurable!

karimodm · 2019-02-07T18:00:28Z

src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java

-    private ConsensusConfig config;
+    private boolean skipValidation;
+
+    private int maxMilestoneIndex;


It may be interesting to have a MilestoneService instance that can validate milestones coming from another coo than the one configured (wink, wink ...)

karimodm · 2019-02-07T18:04:10Z

src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java

+        coordinatorSignatureMode = config.getCoordinatorSignatureMode();
+        coordinatorSecurityLevel = config.getCoordinatorSecurityLevel();
+        numberOfKeysInMilestone = config.getNumberOfKeysInMilestone();
+        skipValidation = (config.isTestnet() && config.isDontValidateTestnetMilestoneSig());


Avoids very very ugly line

# Conflicts: # src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java

alon-e · 2019-02-19T14:11:03Z

great changes! Approved.

…r#1322)

refactor milestone signature parameters to be configurable

2fbe7e0

alon-e commented Feb 5, 2019

View reviewed changes

alon-e requested review from GalRogozinski and karimodm February 5, 2019 12:18

GalRogozinski suggested changes Feb 6, 2019

View reviewed changes

karimodm approved these changes Feb 7, 2019

View reviewed changes

GalRogozinski added the C-Consensus label Feb 14, 2019

Merge branch 'dev' into alon-e-feat/MilestoneConfigRefactoring

458a5e4

# Conflicts: # src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java

GalRogozinski force-pushed the feat/MilestoneConfigRefactoring branch from bd6b8b5 to 458a5e4 Compare February 18, 2019 10:40

add default values to javadocs

4d8bee0

iotaledger deleted a comment Feb 18, 2019

Use config object to encapsulate parameters and ensure immutability

9135f7a

GalRogozinski force-pushed the feat/MilestoneConfigRefactoring branch from a1cb46f to 78d7d64 Compare February 18, 2019 13:16

Fix type change of getCoordinator

0b2698c

GalRogozinski force-pushed the feat/MilestoneConfigRefactoring branch from 78d7d64 to 0b2698c Compare February 18, 2019 14:08

Merge branch 'dev' into feat/MilestoneConfigRefactoring

a04eaf6

GalRogozinski approved these changes Feb 27, 2019

View reviewed changes

fix conflict

7f07934

GalRogozinski merged commit 9bb3b92 into iotaledger:dev Feb 27, 2019

GalRogozinski deleted the feat/MilestoneConfigRefactoring branch February 27, 2019 13:37

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

jakubcech mentioned this pull request Apr 1, 2019

Change the coordinator address #1394

Closed

1 task

GalRogozinski pushed a commit to GalRogozinski/iri that referenced this pull request Apr 8, 2019

refactor milestone signature parameters to be configurable (iotaledge…

b84c50e

…r#1322)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor milestone signature parameters to be configurable #1322

refactor milestone signature parameters to be configurable #1322

alon-e commented Feb 5, 2019

alon-e Feb 5, 2019

alon-e Feb 5, 2019

GalRogozinski Feb 6, 2019

GalRogozinski Feb 6, 2019

karimodm Feb 7, 2019

alon-e Feb 11, 2019

GalRogozinski Feb 18, 2019 •

edited

Loading

karimodm left a comment

karimodm Feb 7, 2019

karimodm Feb 7, 2019

alon-e commented Feb 19, 2019


		if (milestoneIndex < 0 \|\| milestoneIndex >= 0x200000) {
		if (milestoneIndex < 0 \|\| milestoneIndex >= maxMilestoneIndex) {

refactor milestone signature parameters to be configurable #1322

refactor milestone signature parameters to be configurable #1322

Conversation

alon-e commented Feb 5, 2019

Description

Type of change

How Has This Been Tested?

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 Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

karimodm left a 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 Feb 19, 2019

GalRogozinski Feb 18, 2019 •

edited

Loading