Solidify transactions we walk across while doing GTTA #1826

GalRogozinski · 2020-04-05T18:23:02Z

Description

While doing GTTA we will attempt to solidify transactions we walk across.
This will make sure that transactions that we missed out on for whatever reason will solidify

Fixes #1818

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

I ran both 1.8.5 and a canary with this change.
I called GTTA in a loop on both with JMETER

GTTA with this change was still faster compared to 1.8.5.
Throughput: 176.662 ops/minute compared to 71.603 ops/minute

However, profiling showed that solidifying takes 30 % of cpu time...

Checklist:

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 made corresponding changes to the documentation
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 · 2020-04-06T07:46:16Z

src/main/java/com/iota/iri/service/tipselection/impl/AsyncTipSelSolidifier.java

+        ThreadFactory threadFactory = new ThreadFactoryBuilder()
+                .setNameFormat(this.getClass().getSimpleName() + " %d")
+                .build();
+        solidExecutor = Executors.newCachedThreadPool(threadFactory);


Perhaps it needs to be changed to another executor?
Currently we have a limit on the amount of GTTA calls we can make, so this also limits how much solidification we can do

kwek20

What made you chose this dummy solidifier instead of a null check? Probably same speed after optimizing, but just wondering. I would generally not chose the empty class approach.

Theres no issue linked, but besides that it looks good. CPU load increase makes sense due to more tx requested, but i didn´t check locally. Was the network traffic increasing as well?

About the other executor, lets leave it as is until we notice a problem. We shouldn´t solve problems that don´t exist yet.

Maybe we should consider setting the checkSolidity second parameter to something more sensible instead of defaulting to maximum integer, like 10.000 (called in AsyncTipSelSolidifier.java:43)

You may consider this an approval code wise, but as theres some questions im marking it as comment

DyrellC

Overall looks like a good stand in. The solidifier will be easily replaced post 1.9.0, and the implementation of said solidifier will be very simple with this setup (basically just replacing a couple lines). Just a couple comments regarding the checkSolidity call as well as the potential memory leak on an unchecked set size.

DyrellC · 2020-04-07T00:18:28Z

src/main/java/com/iota/iri/service/tipselection/impl/AsyncTipSelSolidifier.java

+    public void solidify(Hash transactionHash) {
+        // Don't try to solidify transactions that are in the process of being solidified
+        //Problem: we are not checking transactions in the past cone here
+        if (transactionToSolidify.add(transactionHash)) {


As you pointed out this has the potential to get DOS attacked if there is enough power behind the spammers being thrown at the node. Does it make sense maybe to have a limit to the maximum number of objects within the set?

When I tested this I counted the number of AsyncSolidifier threads created when I bombed the node with Jmeter spam

I think there are more chances of bombing the CPU out before getting an oom from this

DyrellC · 2020-04-07T00:19:12Z

src/main/java/com/iota/iri/service/tipselection/impl/AsyncTipSelSolidifier.java

+            solidExecutor.submit(() -> {
+                try {
+                    log.debug("attempting to solidify transaction {}", transactionHash);
+                    transactionValidator.checkSolidity(transactionHash);


As Brord pointed out, we probably shouldn't allow this to run up to MAX_INTEGER and limit it to something like 10k analysed transactions

I will use the same solidification limit as the milestone solidifier
If it is too much here, it is too much for milestones

DyrellC · 2020-04-07T00:20:37Z

src/main/java/com/iota/iri/service/tipselection/impl/TipSelectorImpl.java

-import com.iota.iri.service.tipselection.TipSelector;
-import com.iota.iri.service.tipselection.WalkValidator;
-import com.iota.iri.service.tipselection.Walker;
+import com.iota.iri.service.tipselection.*;


Not really a review on this PR specifically, but can we do something about Codacy freaking out about these kinds of imports?

yeah I know...
I need to find time to take care of this, so much to do...

…lidify

GalRogozinski · 2020-04-07T09:52:53Z

@kwek20

What made you chose this dummy solidifier instead of a null check? Probably same speed after optimizing, but just wondering. I would generally not chose the empty class approach.

I am just wired to use polymorphism instead of if-else. In some cases it makes way shorter readable code. In this case, it matters less and maybe the null check is better. The "advantage" here is that now you are ready to inject other imps in other places the WalkValidator is called from. But we probably won't use it. Anyhow, this change will be changed by Dyrell and he can choose another approach

Theres no issue linked

Fixed :-)

acha-bill

LGTM

Gal Rogozinski added 5 commits April 5, 2020 19:04

Add tip sel solidifier

b6b41ca

Add injections

2fee807

Fix tests

067339d

Add solidification call test

46db2f2

add debug log message

b273a32

GalRogozinski added C-Tip Selection Old C-Solidification labels Apr 5, 2020

GalRogozinski requested review from kwek20 and DyrellC April 5, 2020 18:23

GalRogozinski commented Apr 6, 2020

View reviewed changes

kwek20 reviewed Apr 6, 2020

View reviewed changes

DyrellC reviewed Apr 7, 2020

View reviewed changes

Put a limit on the number of transactions we walk upon in order to so…

cd57e6a

…lidify

acha-bill approved these changes Apr 7, 2020

View reviewed changes

GalRogozinski merged commit f975033 into iotaledger:release-v1.9.0 Apr 7, 2020

GalRogozinski deleted the solidify-gtta branch April 7, 2020 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solidify transactions we walk across while doing GTTA #1826

Solidify transactions we walk across while doing GTTA #1826

GalRogozinski commented Apr 5, 2020 •

edited

Loading

GalRogozinski Apr 6, 2020 •

edited

Loading

kwek20 left a comment

DyrellC left a comment

DyrellC Apr 7, 2020

GalRogozinski Apr 7, 2020

DyrellC Apr 7, 2020

GalRogozinski Apr 7, 2020

DyrellC Apr 7, 2020

GalRogozinski Apr 7, 2020

GalRogozinski commented Apr 7, 2020 •

edited

Loading

acha-bill left a comment

Solidify transactions we walk across while doing GTTA #1826

Solidify transactions we walk across while doing GTTA #1826

Conversation

GalRogozinski commented Apr 5, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

GalRogozinski Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

kwek20 left a comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment

DyrellC Apr 7, 2020

Choose a reason for hiding this comment

GalRogozinski Apr 7, 2020

Choose a reason for hiding this comment

DyrellC Apr 7, 2020

Choose a reason for hiding this comment

GalRogozinski Apr 7, 2020

Choose a reason for hiding this comment

DyrellC Apr 7, 2020

Choose a reason for hiding this comment

GalRogozinski Apr 7, 2020

Choose a reason for hiding this comment

GalRogozinski commented Apr 7, 2020 • edited Loading

acha-bill left a comment

Choose a reason for hiding this comment

GalRogozinski commented Apr 5, 2020 •

edited

Loading

GalRogozinski Apr 6, 2020 •

edited

Loading

GalRogozinski commented Apr 7, 2020 •

edited

Loading