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

[DyrellC]

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

[GalRogozinski]

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

[DyrellC]

@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.

[GalRogozinski]

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

[GalRogozinski]

IIUC, you implemented my suggestion above?

[DyrellC]

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

[GalRogozinski]

@DyrellC, marking this as point to discuss before merge

[GalRogozinski]

never mind, I am good

[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:

1. remove the logic from the ReceiveStage that advances the transaction to the BroadcastStage
2. Make the TransactionValidator contain the ProcessingPipeline as well.
3. 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.

[GalRogozinski]

Looks pretty good!

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

[GalRogozinski]

Currently not sure why this has changed...

[GalRogozinski]

In line 138 you gossip the tx to neighbor

[DyrellC]

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

[GalRogozinski]

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..

[GalRogozinski]

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

[DyrellC]

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.

[GalRogozinski]

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

[DyrellC]

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.

[GalRogozinski]

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 :-)

[GalRogozinski]

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

[DyrellC]

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

[GalRogozinski]

If so, why did you change the signature?

[DyrellC]

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

[GalRogozinski]

still waiting

[DyrellC]

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.

[kwek20]

wrong review on only the logging commit

[kwek20]

trouble!