Feature: Add solidification stage to pipeline and only broadcast solid transactions #1646

DyrellC · 2019-10-17T17:48:40Z

Description

Abstract the broadcast queue to allow solidification and networking operations to place transactions in the BroadcastStage. This improves the speed that transactions are broadcast to neighbours to improve synchronisation as well as allow neighbour nodes to receive newly added transactions faster. Improves state of the tangle in higher concentrated TPS environments.

Fixes #1512

Type of change

Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

-High throughput spam mixing correctly with lower throughput spam from neighbouring node

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
New and existing unit tests pass locally with my changes

kwek20

Can you change it so that the BroadcastQueue is obtained form a @Provider, like the other parameters?

Changes: MainInjectionConfiguration

@Singleton
    @Provides
    BroadcastQueue provideBroadcastQueue() {
        return configuration.getBroadcastQueue();
    }

remove lines 123 and 137 from IRI.java
add BroadcastQueue to the Constructor of Iota.java, and remove the configureBroadcastQueue

…into add-Broadcast-queue

…dcast-queue

src/main/java/com/iota/iri/TransactionValidator.java

src/main/java/com/iota/iri/conf/BaseIotaConfig.java

GalRogozinski · 2019-11-04T16:57:58Z

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

@@ -135,7 +143,7 @@ private void addStage(String name, BlockingQueue<ProcessingContext> queue,
                            receivedStageQueue.put(payload.getRight());
                            break;
                        case BROADCAST:
-                            broadcastStageQueue.put(ctx);
+                            broadcastStageQueue.add(ctx);


So if I understand correctly, you still add transactions that are not solid...

@galrogo Just to address this comment and subsequently issue #1512, currently transaction requests are dependent on broadcasting, so if you do not broadcast unsolid transactions, you will never send a transaction request. So it is necessary to pass on transactions before they are solid in the current framework. May be worth discussing a new method of transaction requesting from neighbours to avoid this dependency.

Can we just broadcast random solid tips like before?

I guess moving on to the new messages that were developed for hornet will solve this problem

IIUC, you implemented my suggestion above?

When you say before are you referring to before the refactor? Because as is, after a transaction is placed into the solidifier, the solidify stage will fetch a random solid tip and broadcast it. If the tip is not solid the pipeline goes straight to FINISH instead. As it was before it was:
Transaction -> Pre-Processing -> Hashing/Validation -> Received (store) -> Broadcast (pass transaction on before verification of solidity) -> Finish
But now it would be:
Transaction -> Pre-Processing -> Hashing/Validation -> Received (store) -> Solidify (check if solid, if so pass on to broadcast, if not, place in solidification queue and fetch random tip for broadcasting) -> Broadcast -> Finish

@DyrellC, marking this as point to discuss before merge

never mind, I am good

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

GalRogozinski

Currently the logic nicely moves the transaction nicely through the through the stages. Due to the demand to broadcast only solid transactions maintaining this structure can be a bit hard...

What I believe you should do:

remove the logic from the ReceiveStage that advances the transaction to the BroadcastStage
Make the TransactionValidator contain the ProcessingPipeline as well.
Make sure that each call to tvm.updateSolid is wrapped with a method that also adds to the BroadcastQueue. It might be helpful to move updateSolidTransactions to TransactionValidator as well.

Note that it might be better to move all this solidification/broadcasting code to a new TransactionSolidifier class. But this can be done as a separate PR.

…idifier

GalRogozinski

Looks pretty good!

Can you please add unit test for your new TransactionSolidifier that will go over the public methods?

GalRogozinski · 2019-11-19T09:12:13Z

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

@@ -142,7 +142,8 @@ public ProcessingContext process(ProcessingContext ctx) {
            } catch (Exception e) {
                log.error("error adding reply tx to neighbor's send queue", e);
            }
-            ctx.setNextStage(TransactionProcessingPipeline.Stage.ABORT);
+            ctx.setNextStage(TransactionProcessingPipeline.Stage.BROADCAST);
+            ctx.setPayload(new BroadcastPayload(null, tvm));


Currently not sure why this has changed...

In line 138 you gossip the tx to neighbor

This was to bolster broadcast rate to neighbours to increase the rate that transactionsToRequest are sent out. It was meant to increase solidification speed.

Seems a bit hacky...

I've been thinking about it and I think this should be the correct design:
You need to add a solidification stage and a solidification queue in TransactionProcessingPipeline.
The flow should be like this:
From the ReceivedStage -> if solid (due to quickly solid) -> Broadcast
-> else go to solidification stage -> Broadcast

Also remember to pass neighbor information along so we don't broadcast the solid tx to the neighbor that sent this to us..

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

GalRogozinski · 2019-11-19T09:39:00Z

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

+                TransactionViewModel t = hashIterator.next();
+                broadcastStageQueue.put(new ProcessingContext(new BroadcastPayload(null, t)));
+                toRemove.add(t);


Question:
So now we are sending the tx also to the origin neighbor?

Small peev:
can you change t to tx
I really dislike one letter variables

This refill is what adds solid transactions to the broadcast queue, so here, yes you would send it to all neighbours. I'll change the 1 letter variable though.

Just making sure, now with the new stage, this problem is resolved?

Due to the asynchronous nature of solidification, there is still no neighbour variable to pass through unless we create a mapping, which imo is not worth it to do. So anything that gets updated as solid will be broadcast to all neighbours. But now only transactions that are solid will end up in the broadcast queue. We no longer forward transactions that have just been received.

I think we need to create this mapping
I also don't think we should touch this PR too much, so please create an issue to do this :-)

@DyrellC I am marking this as a point to discuss before merging

We will need to discuss this, because now that the solidify stage does not add the incoming transactions to the solidification queue anymore, we have no entry point to put the origin neighbour into the process. As is, the only place that places transactions into the solidification queue is from the milestone solidifier. This is something that could be introduced in the milestone stage, I mention that in the new issue #1701

src/main/java/com/iota/iri/service/validation/TransactionSolidifier.java

GalRogozinski · 2019-11-19T10:46:40Z

src/main/java/com/iota/iri/service/validation/TransactionValidator.java

+    /**
+     * Exclusively used in the {@link TansactionValidatorTest}
+     */
+    public boolean isNewSolidTxSetsEmpty () {


If so, why did you change the signature?

For whatever reason it was firing off a warning on my ide when it was this way, just changed it back without incident though.

still waiting

DyrellC · 2020-01-15T22:53:48Z

Tests passed, but something happens during the log fetch at the end that leaves it in a failed state despite passing and producing the logs properly. @karimodm has been informed. Hopefully it's nothing big and a quick fix. Same goes for #1614.

…dcast-queue

…into add-Broadcast-queue

…dcast-queue

kwek20

wrong review on only the logging commit

kwek20

trouble!

…dcast-queue

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

…ifier

…dcast-queue

DyrellC and others added 4 commits October 14, 2019 13:02

Add ubiquitous Broadcast Queue

53ea166

Update unit tests with broadcasting queue

be7c132

Remove unneeded log

6593388

Merge branch 'dev' into add-Broadcast-queue

33f193f

kwek20 changed the title ~~Add broadcast queue~~ Feature: Add broadcast queue Oct 30, 2019

kwek20 suggested changes Oct 31, 2019

View reviewed changes

DyrellC added 5 commits November 4, 2019 07:30

Fix broadcast queue injection

ff58495

remove unused function

1a0ea43

Merge branch 'add-Broadcast-queue' of https://github.com/DyrellC/iri …

ad3a203

…into add-Broadcast-queue

Merge branch 'dev' of https://github.com/iotaledger/iri into add-Broa…

2283fb0

…dcast-queue

Clean up imports

24e31a5

GalRogozinski suggested changes Nov 4, 2019

View reviewed changes

GalRogozinski reviewed Nov 4, 2019

View reviewed changes

DyrellC and others added 13 commits November 8, 2019 01:02

Remove BroadcastQueue, add TransactionSolidifier class

03590dd

Add docs

06242eb

Add to solidification queue if update status fails

1ec3794

Fix network injection test

bc5897f

Remove unused import, lower checkSolidity complexity

62a2330

Reduce complexity further

9fc4cba

Comments

ccca691

Change queue type, log error messages differentely

f0c1955

Condense code slightly

fa72acc

Synchronize solidifier queue access, add received transactions to sol…

c3b1417

…idifier

Move solidification queue entry point, reduce interval for scans

1538b36

Broadcast from reply stage to increase transactions to request speed

a34c5e8

Merge branch 'dev' into add-Broadcast-queue

14e784d

GalRogozinski suggested changes Nov 19, 2019

View reviewed changes

DyrellC added 3 commits November 21, 2019 08:29

Clean up PR, add interface

89ec550

Add unit tests

8801ef2

Wrap in try/catch, fix styling

26b9521

Merge branch 'dev' into add-Broadcast-queue

10baada

DyrellC added 8 commits January 16, 2020 09:16

Merge branch 'dev' into add-Broadcast-queue

4327b10

Merge branch 'dev' of https://github.com/iotaledger/iri into add-Broa…

092cfa5

…dcast-queue

Merge branch 'add-Broadcast-queue' of https://github.com/DyrellC/iri …

cc220b0

…into add-Broadcast-queue

Update regression logging

82359a7

Merge branch 'dev' of https://github.com/iotaledger/iri into add-Broa…

7adfdc5

…dcast-queue

Remove double import from merge

d3baeee

Verbose logging for debugging

ffea048

Merge branch 'dev' of https://github.com/iotaledger/iri into add-Broa…

0f29416

…dcast-queue

kwek20 approved these changes Feb 5, 2020

View reviewed changes

kwek20 suggested changes Feb 5, 2020

View reviewed changes

DyrellC added 4 commits February 6, 2020 09:17

Merge branch 'dev' of https://github.com/iotaledger/iri into add-Broa…

0084b48

…dcast-queue

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

4664000

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

Broadcast latest tip even if not solid (will be latest milestone)

cc5e8b8

Correct cpu load issues

d488a1d

DyrellC force-pushed the add-Broadcast-queue branch from 3ca8e7f to d488a1d Compare February 26, 2020 07:02

DyrellC added 2 commits February 26, 2020 00:23

Remove max to propagation cascade

9ad4886

Reintroduce limiter to adding duplicates for solidification

9ab2a7d

GalRogozinski added this to the Doula milestone Mar 3, 2020

DyrellC added 7 commits March 5, 2020 12:55

Merge branch 'dev' of https://github.com/iotaledger/iri into dev

5663d30

Update DagHelper

b2cb269

Update regression logging

31223b3

Move validation to validation service package

ea3bbf8

Merge branch 'update-regression-logging' into merge-transaction-solid…

e4feaf6

…ifier

Merge branch 'dev' of https://github.com/iotaledger/iri into add-Broa…

059e7e4

…dcast-queue

Merge branch 'merge-transaction-solidifier' into add-Broadcast-queue

7528b22

This was referenced Mar 19, 2020

Update regression logging #1803

Merged

Move validation to validation service package #1804

Merged

Add transaction solidifier #1805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add solidification stage to pipeline and only broadcast solid transactions #1646

Feature: Add solidification stage to pipeline and only broadcast solid transactions #1646

DyrellC commented Oct 17, 2019

kwek20 left a comment

GalRogozinski Nov 4, 2019

DyrellC Nov 22, 2019

GalRogozinski Dec 8, 2019

GalRogozinski Dec 8, 2019

DyrellC Dec 10, 2019

GalRogozinski Dec 26, 2019

GalRogozinski Dec 30, 2019

GalRogozinski left a comment

GalRogozinski left a comment

GalRogozinski Nov 19, 2019

GalRogozinski Nov 19, 2019

DyrellC Nov 20, 2019

GalRogozinski Nov 24, 2019

GalRogozinski Nov 19, 2019

DyrellC Nov 20, 2019

GalRogozinski Dec 8, 2019 •

edited

Loading

DyrellC Dec 10, 2019 •

edited

Loading

GalRogozinski Dec 26, 2019

GalRogozinski Dec 26, 2019

DyrellC Dec 30, 2019 •

edited

Loading

GalRogozinski Nov 19, 2019

DyrellC Nov 20, 2019

GalRogozinski Nov 24, 2019

DyrellC commented Jan 15, 2020

kwek20 left a comment •

edited

Loading

kwek20 left a comment

Feature: Add solidification stage to pipeline and only broadcast solid transactions #1646

Are you sure you want to change the base?

Feature: Add solidification stage to pipeline and only broadcast solid transactions #1646

Conversation

DyrellC commented Oct 17, 2019

Description

Type of change

How Has This Been Tested?

Checklist:

kwek20 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

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

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

Choose a reason for hiding this comment

GalRogozinski Dec 8, 2019 • edited Loading

Choose a reason for hiding this comment

DyrellC Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DyrellC Dec 30, 2019 • edited Loading

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 15, 2020

kwek20 left a comment • edited Loading

Choose a reason for hiding this comment

kwek20 left a comment

Choose a reason for hiding this comment

GalRogozinski Dec 8, 2019 •

edited

Loading

DyrellC Dec 10, 2019 •

edited

Loading

DyrellC Dec 30, 2019 •

edited

Loading

kwek20 left a comment •

edited

Loading