Skip to content
This repository has been archived by the owner on Aug 23, 2020. It is now read-only.

Increase rescan performance of old milestones after IRI restart #1204

Merged
merged 9 commits into from Dec 10, 2018
Merged

Increase rescan performance of old milestones after IRI restart #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fd8bc46
Perf: ignore irrelevant (old) milestones
Nov 29, 2018
8cc8b07
Refactor: removed amound of db queries by passing in milestone index
Nov 29, 2018
cb06146
Refactor: refactored the imports
Nov 29, 2018
0af00d8
Refactor: refactor according to codacy
Nov 30, 2018
75d47a7
Refactor: reverted accidental change
Nov 30, 2018
2ea3fb1
Refactor: refactored changes according to comments
Dec 6, 2018
cec8165
Refactor: refactored according to gals proposal
Dec 7, 2018
f21156b
Docs: added a comment
Dec 7, 2018
85db77f
Refactor: changed code according to Gals request
Dec 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/main/java/com/iota/iri/service/milestone/LatestMilestoneTracker.java
View file
Open in desktop
Expand Up @@ -49,21 +49,20 @@ public interface LatestMilestoneTracker {
* milestone and update the internal properties accordingly.<br />
*
* @param transaction the transaction that shall be examined
* @return the result of the analysis translated into the corresponding {@link MilestoneValidity} status
* @return {@code true} if the milestone could be processed and {@code false} if the bundle is not complete, yet
* @throws MilestoneException if anything unexpected happens while trying to analyze the milestone candidate
*/
MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transaction) throws
MilestoneException;
boolean processMilestoneCandidate(TransactionViewModel transaction) throws MilestoneException;

/**
* Does the same as {@link #analyzeMilestoneCandidate(TransactionViewModel)} but automatically retrieves the
* Does the same as {@link #processMilestoneCandidate(TransactionViewModel)} but automatically retrieves the
* transaction belonging to the passed in hash.<br />
*
* @param transactionHash the hash of the transaction that shall be examined
* @return the result of the analysis translated into the corresponding {@link MilestoneValidity} status
* @return {@code true} if the milestone could be processed and {@code false} if the bundle is not complete, yet
* @throws MilestoneException if anything unexpected happens while trying to analyze the milestone candidate
*/
MilestoneValidity analyzeMilestoneCandidate(Hash transactionHash) throws MilestoneException;
boolean processMilestoneCandidate(Hash transactionHash) throws MilestoneException;

/**
* Since the {@link LatestMilestoneTracker} scans all milestone candidates whenever IRI restarts, this flag gives us
Expand All @@ -77,7 +76,7 @@ MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transaction) th
boolean isInitialScanComplete();

/**
* This method starts the background worker that automatically calls {@link #analyzeMilestoneCandidate(Hash)} on all
* This method starts the background worker that automatically calls {@link #processMilestoneCandidate(Hash)} on all
* newly found milestone candidates to update the latest milestone.<br />
*/
void start();
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/iota/iri/service/milestone/MilestoneService.java
View file
Open in desktop
Expand Up @@ -33,13 +33,14 @@ public interface MilestoneService {
* the signature to analyze if the given milestone was really issued by the coordinator.<br />
*
* @param transactionViewModel transaction that shall be analyzed
* @param milestoneIndex milestone index of the transaction (see {@link #getMilestoneIndex(TransactionViewModel)})
* @param mode mode that gets used for the signature verification
* @param securityLevel security level that gets used for the signature verification
* @return validity status of the transaction regarding its role as a milestone
* @throws MilestoneException if anything unexpected goes wrong while validating the milestone transaction
*/
MilestoneValidity validateMilestone(TransactionViewModel transactionViewModel, SpongeFactory.Mode mode,
int securityLevel) throws MilestoneException;
MilestoneValidity validateMilestone(TransactionViewModel transactionViewModel, int milestoneIndex,
SpongeFactory.Mode mode, int securityLevel) throws MilestoneException;

/**
* Updates the milestone index of all transactions that belong to a milestone.<br />
Expand Down
29 changes: 14 additions & 15 deletions src/main/java/com/iota/iri/service/milestone/impl/LatestMilestoneTrackerImpl.java
View file
Open in desktop
Expand Up @@ -11,7 +11,6 @@
import com.iota.iri.service.milestone.MilestoneException;
import com.iota.iri.service.milestone.MilestoneService;
import com.iota.iri.service.milestone.MilestoneSolidifier;
import com.iota.iri.service.milestone.MilestoneValidity;
import com.iota.iri.service.snapshot.Snapshot;
import com.iota.iri.service.snapshot.SnapshotProvider;
import com.iota.iri.storage.Tangle;
Expand All @@ -26,10 +25,6 @@
import java.util.Set;
import java.util.concurrent.TimeUnit;

import static com.iota.iri.service.milestone.MilestoneValidity.INCOMPLETE;
import static com.iota.iri.service.milestone.MilestoneValidity.INVALID;
import static com.iota.iri.service.milestone.MilestoneValidity.VALID;

/**
* Creates a tracker that automatically detects new milestones by incorporating a background worker that periodically
* checks all transactions that are originating from the coordinator address and that exposes the found latest milestone
Expand Down Expand Up @@ -188,9 +183,9 @@ public Hash getLatestMilestoneHash() {
}

@Override
public MilestoneValidity analyzeMilestoneCandidate(Hash transactionHash) throws MilestoneException {
public boolean processMilestoneCandidate(Hash transactionHash) throws MilestoneException {
try {
return analyzeMilestoneCandidate(TransactionViewModel.fromHash(tangle, transactionHash));
return processMilestoneCandidate(TransactionViewModel.fromHash(tangle, transactionHash));
} catch (Exception e) {
throw new MilestoneException("unexpected error while analyzing the transaction " + transactionHash, e);
}
Expand All @@ -203,14 +198,19 @@ public MilestoneValidity analyzeMilestoneCandidate(Hash transactionHash) throws
* {@link MilestoneSolidifier} that takes care of requesting the missing parts of the milestone bundle.<br />
*/
@Override
public MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transaction) throws MilestoneException {
public boolean processMilestoneCandidate(TransactionViewModel transaction) throws MilestoneException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on the name change

try {
if (coordinatorAddress.equals(transaction.getAddressHash()) &&
transaction.getCurrentIndex() == 0) {

int milestoneIndex = milestoneService.getMilestoneIndex(transaction);

switch (milestoneService.validateMilestone(transaction, SpongeFactory.Mode.CURLP27, 1)) {
// if the milestone is older than our ledger start point: we already processed it in the past
if (milestoneIndex <= snapshotProvider.getInitialSnapshot().getIndex()) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Alon's beef was that the validateMilestone depended on the milestone Index. On which he was correct, we don't want other potential callers of this method to be blocked by the index.

He didn't say anything about your IRRELEVANT enum. I think that returning an INVALID on a vaild milestone will be very misleading to anyone in the future (it is at least misleading for me). I would bring back this enum value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I have a better idea... Instead of adding another enum to validity (IRRELEVANT) (which doesn't make much sense), analyzeMilestoneCandidate can return a boolean: true if successfully analyzed, false otherwise.

If the milestoneIndex is below the snapshot then return true and add a little comment: "has already been analyzed if it is below the snapshot" (feel free to change the wording).

I think this makes the most sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree w/ @GalRogozinski that returning INVALID may not be the best answer. other than that lgtm.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i changed it accordingly


switch (milestoneService.validateMilestone(transaction, milestoneIndex, SpongeFactory.Mode.CURLP27, 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try not to ignore codacy.

Even thought the way you did it is less verbose, I think it is still clearer and more readable to have the default case inside the switch brackets. You can also add a little comment:
//We can consider the milestone candidate processed and move on w/o farther action

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is really unnecessary here since the switch is used as a faster / easier to read version of an if then else if then but if you think it helps i can add an empty default with that comment .. :P

case VALID:
if (milestoneIndex > latestMilestoneIndex) {
setLatestMilestone(transaction.getHash(), milestoneIndex);
Expand All @@ -221,22 +221,21 @@ public MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transact
}

transaction.isMilestone(tangle, snapshotProvider.getInitialSnapshot(), true);

return VALID;
break;

case INCOMPLETE:
milestoneSolidifier.add(transaction.getHash(), milestoneIndex);

transaction.isMilestone(tangle, snapshotProvider.getInitialSnapshot(), true);

return INCOMPLETE;
return false;

default:
return INVALID;
// we can consider the milestone candidate processed and move on w/o farther action
}
}

return INVALID;
return true;
} catch (Exception e) {
throw new MilestoneException("unexpected error while analyzing the " + transaction, e);
}
Expand Down Expand Up @@ -350,7 +349,7 @@ private void analyzeMilestoneCandidates() throws MilestoneException {
}

Hash candidateTransactionHash = milestoneCandidatesToAnalyze.pollFirst();
if(analyzeMilestoneCandidate(candidateTransactionHash) == INCOMPLETE) {
if(!processMilestoneCandidate(candidateTransactionHash)) {
seenMilestoneCandidates.remove(candidateTransactionHash);
}
}
Expand Down
5 changes: 2 additions & 3 deletions src/main/java/com/iota/iri/service/milestone/impl/MilestoneServiceImpl.java
View file
Open in desktop
Expand Up @@ -160,10 +160,9 @@ public void resetCorruptedMilestone(int index) throws MilestoneException {
}

@Override
public MilestoneValidity validateMilestone(TransactionViewModel transactionViewModel, SpongeFactory.Mode mode,
int securityLevel) throws MilestoneException {
public MilestoneValidity validateMilestone(TransactionViewModel transactionViewModel, int milestoneIndex,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice addition of parameter

SpongeFactory.Mode mode, int securityLevel) throws MilestoneException {

int milestoneIndex = getMilestoneIndex(transactionViewModel);
if (milestoneIndex < 0 || milestoneIndex >= 0x200000) {
return INVALID;
}
Expand Down