Add transaction solidifier #1805

DyrellC · 2020-03-19T20:43:04Z

Description

This contains the bulk of changes from #1646 and relies on #1804. Transaction solidification should not be contained within the TransactionValidator and depending on the number of transactions required for a walk, using the checkSolidity call within the TransactionValidator, as is done in the MilestoneSolidifier, can take a significant portion of time. In order to mitigate this, the solidification and propagation logic has been placed into its own class running in its own thread independent of the validation logic. Instead of making direct calls to checkConsistency a queue is formed so as to not lock up any other threads, and the solidity of a transaction will first be checked via the view model before it is placed into the queue.

This is a step towards introducing the milestone pipeline refactoring.

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

All corresponding tests pass: Regression, Unit and Synchronisation

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 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-03-25T18:22:13Z

src/main/java/com/iota/iri/network/pipeline/SolidifyStage.java

+    private TransactionSolidifier txSolidifier;
+    private TipsViewModel tipsViewModel;
+    private Tangle tangle;
+    private TransactionViewModel tip;


I don't like this field...
It violates Domain Driven Design principles

I think you can do the same logic w/o this tip field

It was there for the unit test but if you want it removed i'll have to revisit the test to inject a tip into the process.

GalRogozinski · 2020-03-25T18:27:07Z

src/main/java/com/iota/iri/network/pipeline/SolidifyStage.java

+    @VisibleForTesting
+    void injectTip(TransactionViewModel tvm) throws Exception {
+        tip = tvm;
+        tip.updateSolid(true);
+    }


if you do my change you can remove this ugliness

GalRogozinski · 2020-03-25T18:35:41Z

src/main/java/com/iota/iri/network/pipeline/TransactionProcessingPipelineImpl.java

@@ -184,6 +199,7 @@ private void addStage(String name, BlockingQueue<ProcessingContext> queue,
    public void process(Neighbor neighbor, ByteBuffer data) {
        try {
            preProcessStageQueue.put(new ProcessingContext(new PreProcessPayload(neighbor, data)));
+            refillBroadcastQueue();


that's kinda hacky...
It is unexpected that when you start processing data you also do something else

I would have expected that this will purely happen in the solidification stage
Maybe it is because you are in the middle of the design?

GalRogozinski · 2020-03-25T18:47:41Z

src/main/java/com/iota/iri/network/pipeline/TransactionProcessingPipelineImpl.java

+            }
+            txSolidifier.clearFromBroadcastQueue(toRemove);
+        } catch(InterruptedException e){
+            log.info(e.getMessage());


Thread.currentThread().interrupt();

GalRogozinski · 2020-03-25T18:49:23Z

src/main/java/com/iota/iri/service/API.java

+     * @param trytes Transaction data to be stored.
+     * @return {@link com.iota.iri.service.dto.AbstractResponse.Emptyness}
+     * @throws Exception When storing or updating a transaction fails.
+     **/


I know it happened automatically
hopefully we will have time in some sprint to breath and fix all these formatting issues that we have

next time try to refrain

GalRogozinski · 2020-03-25T18:51:10Z

src/main/java/com/iota/iri/service/API.java

@@ -66,33 +67,33 @@
 public class API {


maybe try to re commit this class w/o all the formatting 😅

GalRogozinski · 2020-03-25T19:14:50Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+     * A queue for processing transactions with the {@link #checkSolidity(Hash)} call. Once a transaction has been
+     * marked solid it will be placed into the {@link #transactionsToBroadcast} queue.
+     */
+    private BlockingQueue<Hash> transactionsToSolidify = new ArrayBlockingQueue<>(MAX_SIZE);


I remember I told you to use ArrayBlockingQueue...
We have a bug in networking #1742, that is fixed by doing this GalRogozinski@f65e29b

What happens is that sometiems the queue in networking gets filled in high tps regimes...
So I am rethinking my recommendation

Because before a memory attack IRI just might stop functioning...

I always wondered about that but wasn't sure if i was thinking straight about it. Should I switch this to a LinkedBlockingQueue instead?

GalRogozinski · 2020-03-25T19:20:17Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+     * @param snapshotProvider          For fetching entry points for solidity checks
+     * @param transactionRequester      A requester for missing transactions
+     */
+    public TransactionSolidifierImpl(Tangle tangle, SnapshotProvider snapshotProvider, TransactionRequester transactionRequester,


Maybe have an inner class for the propagation solidifier?
just to separate between responsibilities?

GalRogozinski · 2020-03-26T20:16:19Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+            if(!transactionsToSolidify.contains(hash)) {
+                if (transactionsToSolidify.size() >= MAX_SIZE - 1) {
+                    transactionsToSolidify.remove();
+                }
+
+                transactionsToSolidify.put(hash);


you know I see no reason why to have TransactionSolidifier should run on a separate thread then milestone solidifier

This logic just risks currently more bug entering, and I can't think on why you may improve performance here.
Why don't you have the milestoneSolidifier just call checkSollidity like before...
I thought you were just going to move logic to the transactionSolidifier...

This was logic that was there for preparation for the milestone epic. I can have it run the way you describe for now and we can re-approach the topic when the epic is ready. The motivation here was to create the separation of powers between state checking milestones, and solidifying transactions. In some instances when checkSolidity() is called, it takes a while to run through all the transactions for analysing. This is especially true during larger solidification gaps with a large number of transactions present between milestones. When these calls hang, it slows down the milestoneSolidifer because that thread is dependent on the checkSolidity() call finishing to continue processing the available transactions in the queue. That includes adding newly seen milestones to the queue.

I've seen numerous instances during longer syncs, where there will be no message from the solidifier for an extended period of time. When debugging into the nodes to see why that is, I found that the checkSolidity() call was taking a long time to process through these walks and would finish after processing nearly the maximum number of transactions (50000) while doing so. Sometimes these calls could take anywhere between 5 and 20 seconds to process through. Since the queue for the milestone solidifier holds 20 transactions to be put through this call sequentially, there can be long gaps between the solidifier processing new milestones that may have been received through the requests and gossip. The difference is nearly neglible when doing smaller syncs, but over longer periods of time (say a few days for example) it can start to become more noticeable in the later parts of the synchronisation.

So the logic here was to make sure that the milestoneSolidifier is focused on seeing and processing the milestones with minimal interruption, while the transactionSolidifier would asynchronously be managing the actual solidification process.

I'd put this in a separate PR then
This is not too many lines of code, but still a big important change

GalRogozinski · 2020-03-26T20:19:52Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+        if(maxProcessedTransactions != Integer.MAX_VALUE) {
+            maxProcessedTransactions += analyzedHashes.size();
+        }


I know you copy pasted but this line of code doesn't make sense and you are now rewriting

maybe use Math.addExact and remove the funny if

I guess you want to do this in another PR?

GalRogozinski · 2020-03-26T20:48:51Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+            maxProcessedTransactions += analyzedHashes.size();
+        }
+        boolean solid = true;
+        final Queue<Hash> nonAnalyzedTransactions = new LinkedList<>(Collections.singleton(hash));


Probably an array deque will be faster

I guess you want to do this in another PR?

GalRogozinski · 2020-03-26T21:14:05Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+    void propagateSolidTransactions() {
+        while(!Thread.currentThread().isInterrupted() && solidTransactions.peek() != null) {
+            try {
+                Hash hash = solidTransactions.poll();


I have a theory on the solidification problem...
I think we are trying to solidify before we go through the receive stage

So we fail to getApprovers because they were not received yet stored
And when they come in, the non received transaction is polled out already...

Makes sense?

…lC/iri into add-transaction-solidifier

kwek20 · 2020-03-27T01:16:27Z

@DyrellC added 9 more commits since @GalRogozinski requested review. I will wait for @DyrellC to request review again though a comment

GalRogozinski

Almost there and looking good :-)

GalRogozinski · 2020-03-28T20:51:49Z

src/main/java/com/iota/iri/network/pipeline/SolidifyStage.java

@@ -56,6 +54,8 @@ public ProcessingContext process(ProcessingContext ctx){
            TransactionViewModel tvm = payload.getTransaction();

            if (tvm.isSolid() || txSolidifier.quickSetSolid(tvm)) {
+                // If the transaction is in the solidifier broadcast queue, remove it as it will be broadcast now
+                txSolidifier.clearFromBroadcastQueue(tvm);


Hmm... I think this is a good defense :-)

GalRogozinski · 2020-03-28T20:54:12Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+        if(maxProcessedTransactions != Integer.MAX_VALUE) {
+            maxProcessedTransactions += analyzedHashes.size();
+        }


I guess you want to do this in another PR?

GalRogozinski · 2020-03-28T20:54:43Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+            maxProcessedTransactions += analyzedHashes.size();
+        }
+        boolean solid = true;
+        final Queue<Hash> nonAnalyzedTransactions = new LinkedList<>(Collections.singleton(hash));


I guess you want to do this in another PR?

GalRogozinski · 2020-03-28T20:56:21Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

+     * A queue for processing transactions with the {@link #checkSolidity(Hash)} call. Once a transaction has been
+     * marked solid it will be placed into the {@link #transactionsToBroadcast} queue.
+     */
+    private BlockingQueue<Hash> transactionsToSolidify = new ArrayBlockingQueue<>(MAX_SIZE);


GalRogozinski · 2020-03-28T21:00:24Z

src/main/java/com/iota/iri/service/validation/impl/TransactionSolidifierImpl.java

-                        tipsViewModel.setSolid(h);
+
+
+    public class TransactionPropagator {


pretty sure this can be private

GalRogozinski

LGTM

acha-bill

Looks good to me.
Just the one comment. @DyrellC 😆

acha-bill · 2020-04-02T14:29:40Z

src/main/java/com/iota/iri/network/pipeline/SolidifyStage.java

+    /**
+     * Processes the payload of the {@link ProcessingContext} as a {@link SolidifyPayload}. First the transaction will
+     * be checked for solidity and validity. If the transaction is already solid or can be set solid quickly by the
+     * transaction validator, the transaction is passed to the {@link BroadcastStage}. If not, a random solid tip is


can be set solid quickly by the transaction ~~validator~~ solidifier

Aaah nice catch thought I caught them all. Thanks 👍

DyrellC added 4 commits March 19, 2020 14:29

Move validation to validation service package

6c9096c

Add Transaction Solidifier

029a157

Update Transaction Validator

c72efc3

Update Unit Tests

b6a4e48

DyrellC requested a review from GalRogozinski March 19, 2020 20:43

Gal Rogozinski and others added 2 commits March 25, 2020 19:30

Merge branch 'dev' into add-transaction-solidifier

9ae4f68

Merge branch 'dev' into add-transaction-solidifier

3faf8ec

GalRogozinski suggested changes Mar 25, 2020

View reviewed changes

Remove tip field from solidify stage

7ad6394

GalRogozinski reviewed Mar 26, 2020

View reviewed changes

GalRogozinski requested a review from kwek20 March 26, 2020 21:15

DyrellC added 9 commits March 26, 2020 17:06

Move broadcast queue retreival to solidify stage

bff1915

Undo auto-formatting

0f09a05

Merge branch 'add-transaction-solidifier' of https://github.com/Dyrel…

b735009

…lC/iri into add-transaction-solidifier

More autoformatting

faeabc3

Re-remove refillBroadcast

b96d70b

Move propagation logic to inner class

a21e969

Remove unused imports

3c9414c

Add comment to propagator

d8f3297

Remove unused txSolidifier private field

c55d2fd

GalRogozinski suggested changes Mar 28, 2020

View reviewed changes

DyrellC added 4 commits March 30, 2020 15:11

Remove separate thread logic, check solidity from milestone solidifier

ff0ef35

LinkedHashSet -> ArrayDeque in checkSolidity

fd7e933

Update maxProcessedTransactions value determination

666dadf

Make Transaction Propagator private

f31de91

GalRogozinski mentioned this pull request Apr 1, 2020

reset getInclusion reg test assertion to what it should be #1819

Merged

3 tasks

DyrellC changed the base branch from dev to Dyrell-Milestone-Stage-Draft April 1, 2020 21:32

DyrellC mentioned this pull request Apr 1, 2020

Add transaction solidifier queue #1821

Merged

6 tasks

GalRogozinski approved these changes Apr 2, 2020

View reviewed changes

acha-bill approved these changes Apr 2, 2020

View reviewed changes

Typo correction

5f70a3d

DyrellC changed the base branch from Dyrell-Milestone-Stage-Draft to dev April 2, 2020 16:01

Merge branch 'dev' into add-transaction-solidifier

2c57895

GalRogozinski merged commit 739ea7a into iotaledger:dev Apr 2, 2020

acha-bill mentioned this pull request Apr 6, 2020

Validate bundles as they come in #1810

Open

acha-bill mentioned this pull request Apr 14, 2020

Race and update fixes #1838

Merged

6 tasks

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

Add transaction solidifier #1805

Add transaction solidifier #1805

DyrellC commented Mar 19, 2020

GalRogozinski Mar 25, 2020

DyrellC Mar 25, 2020

GalRogozinski Mar 25, 2020

GalRogozinski Mar 25, 2020

GalRogozinski Mar 25, 2020

GalRogozinski Mar 25, 2020

GalRogozinski Mar 25, 2020

GalRogozinski Mar 25, 2020

DyrellC Mar 26, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 25, 2020

GalRogozinski Mar 26, 2020

DyrellC Mar 26, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 26, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 26, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 26, 2020

kwek20 commented Mar 27, 2020

GalRogozinski left a comment

GalRogozinski Mar 28, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 28, 2020

GalRogozinski Mar 28, 2020

GalRogozinski left a comment

acha-bill left a comment

acha-bill Apr 2, 2020

DyrellC Apr 2, 2020

		tipsViewModel.setSolid(h);


		public class TransactionPropagator {

Add transaction solidifier #1805

Add transaction solidifier #1805

Conversation

DyrellC commented Mar 19, 2020

Description

Type of change

How Has This Been Tested?

Checklist:

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

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

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

kwek20 commented Mar 27, 2020

GalRogozinski left a 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

GalRogozinski left a comment

Choose a reason for hiding this comment

acha-bill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment