sort milestone candidates before analyzing #1660

achabill · 2019-11-11T13:05:33Z

Description

Sort milestone candidates before analyzing to increase the likely hood of a new milestone being seen early.

Part of #1655

Type of change

Enhancement (a non-breaking change which adds functionality)

Checklist:

My code follows the style guidelines for this project
I have performed a self-review of my own code
I have commented on my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes

DyrellC · 2019-11-18T23:13:43Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+    public static List<TransactionViewModel> loadAsSortedList(Tangle tangle, Indexable hash) throws Exception {
+        Address hashes = (Address)tangle.load(Address.class, hash);
+        return hashes.set.stream()
+                .map(item -> fromHash(tangle,item))


When you map with a fromHash call as you've done here, a tvm will always be created, which invalidates the tvm != null call below when sorting. What happens in this case is that the stream will throw an exception once the sorting hits a transaction that doesn't exist in the db (as is the case when a milestone has been removed from pruning). To fix this issue locally I replaced the mapping with:

.map( item -> { try { if(tangle.exists(Transaction.class, item)) { return fromHash(tangle, item); } else { return null; } }catch(Exception e){ return null; } })

This will make sure that if the transaction does not exist, a tvm will not be created for it. Of course this means that you will either need to not include these transactions in the collection before you analyse it in the LatestMilestoneTrackerImpl or have line 314 include a null check before trying to add to the queue's.
i.e
if (tvm != null && seenMilestoneCandidates.add(tvm)) {

@achabill if you apply a change like this the code should pass. As is however it throws an exception the first time it encounters a pruned milestone. Which is the case in the regression test.

GalRogozinski · 2019-12-02T10:04:48Z

I may be forced to look into #1447 now...
If we implement it, then this PR will become obsolete

GalRogozinski · 2020-01-09T16:37:51Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+     * Constructor for an {@link Address} set controller from an existing {@link Address} set.
+     * The resulting hash list is sorted


This doc is unclear to me. This is not a java constructor...

Maybe write:
"Loads all transaction that mutate a certain address. It sorts them by the attachment timestamp"

GalRogozinski · 2020-01-09T16:39:17Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+     * The resulting hash list is sorted
+     *
+     * @param tangle The tangle reference for the database to find the {@link Address} set in
+     * @param hash The hash identifier for the {@link Address} set that needs to be found


Suggested change

* @param hash The hash identifier for the {@link Address} set that needs to be found

* @param hash The address we are loading the transactions for

GalRogozinski · 2020-01-09T16:40:14Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+     *
+     * @param tangle The tangle reference for the database to find the {@link Address} set in
+     * @param hash The hash identifier for the {@link Address} set that needs to be found
+     * @return The {@link AddressViewModel} controller generated


We are returning a list of transactions (actually transaction controllers unfortunately)

GalRogozinski · 2020-01-09T16:46:29Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+     * @return The {@link AddressViewModel} controller generated
+     * @throws Exception Thrown if the database cannot load an {@link Address} set from the reference {@link Hash}
+     */
+    public static List<TransactionViewModel> loadAsSortedList(Tangle tangle, Indexable hash) throws Exception {


Just note (no need to change):
Usually it is best to use an interface like Indexable in method parameters.

In this case it would probably best to use something more specific like AddressHash because you are not going to be using other subtypes of Indexable in this method.
However at the moment casting will be neccessary to change this...

So this is just a point to think of before we start refactoring...

GalRogozinski · 2020-01-09T16:48:43Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+                if (tangle.exists(Transaction.class, item)) {
+                    return TransactionViewModel.fromHash(tangle, item);
+                }
+                return null;


why return null? Why not do a .filter(item -> (tangle.exists(Transaction.class, item)) so you won't have nulls in your list

GalRogozinski · 2020-01-09T16:49:08Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+                }
+                return null;
+            } catch (Exception e) {
+                return null;


why are you swallowing exceptions? this shouldn't be happening

GalRogozinski · 2020-01-09T16:50:56Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+            } catch (Exception e) {
+                return null;
+            }
+        }).sorted(Comparator.comparing(tvm -> tvm != null ? tvm.getTransaction().attachmentTimestamp : 0))


I guess sorting by attachmentTimestamp is fine

maybe add a comment that this was an arbitrary choice (you also have timestamp which is part of the bundle essence)

DyrellC · 2020-01-13T22:22:42Z

@acha-bill still waiting on you to respond to Gal's review before I can review myself

GalRogozinski · 2020-01-15T08:33:19Z

src/main/java/com/iota/iri/controllers/AddressViewModel.java

+        Address hashes = (Address) tangle.load(Address.class, hash);
+        return hashes.set.stream().filter(hash1 -> {
+            try {
+                return TransactionViewModel.exists(tangle, hash1);
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        }).map(item -> {
+            try {
+                return TransactionViewModel.fromHash(tangle, item);
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        }).sorted(Comparator.comparing(TransactionViewModel::getAttachmentTimestamp))
+                .collect(Collectors.toList());
+    }


I didn't run it, but the following compiles and looks nicer:

public static List<TransactionViewModel> loadAsSortedList(Tangle tangle, Indexable hash) throws Exception { Address hashes = (Address) tangle.load(Address.class, hash); return hashes.set.stream() .filter(ThrowingPredicate.unchecked(hash1 -> TransactionViewModel.exists(tangle, hash1))) .map(ThrowingFunction.unchecked(item -> TransactionViewModel.fromHash(tangle, item))) .sorted(Comparator.comparing(TransactionViewModel::getAttachmentTimestamp)) .collect(Collectors.toList()); }

GalRogozinski

Looks good and test just passed

waiting for @DyrellC to approve before I merge

DyrellC

Looks good 👍

GalRogozinski · 2020-01-16T09:38:37Z

@karimodm
This should now help with solidifying milestones
note that it is merged

…lidification (iotaledger#1660)" This reverts commit b658b88.

…lidification (#1660)" (#1747) This reverts commit b658b88.

…lidification (iotaledger#1660)" (iotaledger#1747) This reverts commit b658b88.

sort milestone candidates before analyzing

aecc83e

DyrellC reviewed Nov 18, 2019

View reviewed changes

don't processess null tvms

9e819d6

acha-bill requested review from DyrellC and GalRogozinski December 1, 2019 23:28

GalRogozinski added the Deffered Will be addressed and merged in later versions. label Dec 17, 2019

GalRogozinski added the T-Discussion label Jan 5, 2020

GalRogozinski mentioned this pull request Jan 6, 2020

Prepare IRI release v.1.9.0 #1703

Open

GalRogozinski added C-Consensus and removed Deffered Will be addressed and merged in later versions. T-Discussion labels Jan 9, 2020

GalRogozinski suggested changes Jan 9, 2020

View reviewed changes

Fixed header comments and refactor loadAsSorted

c19c299

acha-bill requested a review from GalRogozinski January 14, 2020 18:08

GalRogozinski reviewed Jan 15, 2020

View reviewed changes

refactor loadAsSorted

92016a4

acha-bill requested a review from GalRogozinski January 15, 2020 08:42

Merge branch 'dev' into sort_transaction_candidates

95fac6f

GalRogozinski approved these changes Jan 15, 2020

View reviewed changes

DyrellC approved these changes Jan 15, 2020

View reviewed changes

GalRogozinski merged commit b658b88 into iotaledger:dev Jan 16, 2020

GalRogozinski mentioned this pull request Jan 20, 2020

Node failing to solidify milestones correctly #1655

Open

DyrellC added a commit to DyrellC/iri that referenced this pull request Feb 10, 2020

Revert "Fix: sort milestone candidates before analyzing for faster so…

4664000

…lidification (iotaledger#1660)" This reverts commit b658b88.

GalRogozinski pushed a commit that referenced this pull request Feb 11, 2020

Revert "Fix: sort milestone candidates before analyzing for faster so…

8a630e9

…lidification (#1660)" (#1747) This reverts commit b658b88.

kwek20 pushed a commit to kwek20/iri that referenced this pull request Mar 10, 2020

Revert "Fix: sort milestone candidates before analyzing for faster so…

b9f5645

…lidification (iotaledger#1660)" (iotaledger#1747) This reverts commit b658b88.

GalRogozinski mentioned this pull request May 6, 2020

Release 1.8.6 #1851

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sort milestone candidates before analyzing #1660

sort milestone candidates before analyzing #1660

achabill commented Nov 11, 2019

DyrellC Nov 18, 2019

DyrellC Nov 18, 2019

acha-bill Nov 28, 2019

GalRogozinski commented Dec 2, 2019

GalRogozinski Jan 9, 2020

GalRogozinski Jan 9, 2020

GalRogozinski Jan 9, 2020

GalRogozinski Jan 9, 2020

GalRogozinski Jan 9, 2020

GalRogozinski Jan 9, 2020

GalRogozinski Jan 9, 2020

DyrellC commented Jan 13, 2020

GalRogozinski Jan 15, 2020

GalRogozinski left a comment

DyrellC left a comment

GalRogozinski commented Jan 16, 2020

		* Constructor for an {@link Address} set controller from an existing {@link Address} set.
		* The resulting hash list is sorted

	* @param hash The hash identifier for the {@link Address} set that needs to be found
	* @param hash The address we are loading the transactions for

sort milestone candidates before analyzing #1660

sort milestone candidates before analyzing #1660

Conversation

achabill commented Nov 11, 2019

Description

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski commented Dec 2, 2019

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

DyrellC commented Jan 13, 2020

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment

GalRogozinski commented Jan 16, 2020