Skip to content
This repository was archived by the owner on Aug 23, 2020. It is now read-only.

Change: Do not request one transaction at a time when checking for solidity #1311

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Change: Do not request one transaction at a time when checking for solidity #1311

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/iota/iri/TransactionValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public boolean checkSolidity(Hash hash, boolean milestone, int maxProcessedTrans

if (!transactionRequester.isTransactionRequested(hashPointer, milestone)) {
transactionRequester.requestTransaction(hashPointer, milestone);
break;
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if the following suggestion makes sense:
if it's a milestone, then continue, else break.

the reason being:
this mechanism can be used to fill the request queue with arbitrary data faster.

0
| \
0  ?
0
| \
0  ?
0
| \
0  ?
...

in the current code the first ? would be requested and stop (and if this ? does not exist the rest will never be requested, with this change all the historical ?s will be requested (up until the maxProccessedTransactions is reached).

I'm not sure this is such an issue, but my proposal would not change behavior for random transactions but achieve the improvement for syncing nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the current code the first ? would be requested and stop (and if this ? does not exist the rest will never be requested.

Is this true? In the current code, upon the next call of this method, the first ? will already be in the requester queue, and the if (!transactionRequester.isTransactionRequested(hashPointer, milestone)) { will return false: so it will continue to request the next ?.

This being said I don't get your point on continue only if it's a milestone.

Copy link
Contributor

@GalRogozinski GalRogozinski Feb 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Alon's point is that all branches that are not confirmed by a milestone are "arbitrary data" and we don't want to have spend time or resources on them. The requester queue can fill up with sidetangle requests and thus hinder the solidification of "milestone branches".

However, if the TipsSolidifier is not enabled we are not supposed to be solidifying on "arbitrary data", and if it is enabled then maybe it is because the operator cares about "arbitrary data"?

Bottom line, due to the TipsSolidifier activation config I am with @karimodm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point @karimodm, still this will take time to fill (1 tx per scan). I still think that this can be avoided by separating the cases; it will reduce the amount of change.

However, I don't insist on this, if @GalRogozinski thinks it's fine then I'm OK.

Copy link
Contributor Author

@karimodm karimodm Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still this will take time to fill (1 tx per scan)

Doesn't the continue here make the queue fill faster in any of the cases?

@GalRogozinski true on arbitrary data, this change combined with the requester freshness should fend off those scenarios though.

}
} else {
nonAnalyzedTransactions.offer(transaction.getTrunkTransactionHash());
Expand Down