Increase rescan performance of old milestones after IRI restart #1204
Conversation
There was a problem hiding this comment.
I think validateMilestone should be stateless.
it's worse enough that the depth/index is capped by milestoneIndex >= 0x200000 which is only relevant to mainnet but not to other networks w/ theoretically more leaves (+ #1180)
so adding IRRELAVENT inside the function makes it, even more, dependant on state - think of test where the hardcoded testing milestone is below the starting milestone.
-> my proposal is to pull this out of the validate milestone and into a function that checks if the index is valid or needed before calling the validation.
|
i changed the check to be part of the milestone tracker instead |
| @@ -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, | |||
There was a problem hiding this comment.
nice addition of parameter
| @@ -209,8 +208,13 @@ public MilestoneValidity analyzeMilestoneCandidate(TransactionViewModel transact | |||
| transaction.getCurrentIndex() == 0) { | |||
|
|
|||
| int milestoneIndex = milestoneService.getMilestoneIndex(transaction); | |||
| if (milestoneIndex <= snapshotProvider.getInitialSnapshot().getIndex()) { | |||
| return INVALID; | |||
| } | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree w/ @GalRogozinski that returning INVALID may not be the best answer. other than that lgtm.
| @@ -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 { | |||
There was a problem hiding this comment.
👍 on the name change
Description
IRI always scans all potential milestone candidates after a restart. Depending on the database size this can take a really long time (hours in the worst case). If we have a local snapshot we can always immediately discard the transactions prior to the snapshot point because the node doesn't need them anymore. This increases the restart performance of IRI massively (few seconds).
Type of change
Checklist:
Please delete items that are not relevant.