New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: TransactionValidator instance based / TransactionRequester fixed #914

Merged
merged 1 commit into from Aug 12, 2018

Conversation

Projects
None yet
4 participants
@hmoog
Contributor

hmoog commented Aug 8, 2018

No description provided.

@th0br0

th0br0 approved these changes Aug 8, 2018

@GalRogozinski

Hey,
Only 2 things I want your reply on :-)

@@ -76,7 +76,7 @@ public int getMinWeightMagnitude() {
return MIN_WEIGHT_MAGNITUDE;
}
private static boolean hasInvalidTimestamp(TransactionViewModel transactionViewModel) {
private boolean hasInvalidTimestamp(TransactionViewModel transactionViewModel) {

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 8, 2018

Contributor

I agree with removing static on all public methods because you don't want to have a global scope TransactionValidator logic. However on private methods I do prefer to keep the static modifier.

This is because of 2 reasons:

  1. I can quickly know that the method doesn't change the instance state by just looking at the signature.
  2. There may be better optimizations that the JIT will do now that the method is marked as static. Today modern JVM are heavily optimized and I am not sure how important this consideration is nowadays. Still the results will be JVM dependent.
@GalRogozinski

GalRogozinski Aug 8, 2018

Contributor

I agree with removing static on all public methods because you don't want to have a global scope TransactionValidator logic. However on private methods I do prefer to keep the static modifier.

This is because of 2 reasons:

  1. I can quickly know that the method doesn't change the instance state by just looking at the signature.
  2. There may be better optimizations that the JIT will do now that the method is marked as static. Today modern JVM are heavily optimized and I am not sure how important this consideration is nowadays. Still the results will be JVM dependent.

This comment has been minimized.

@hmoog

hmoog Aug 8, 2018

Contributor

Atm we use this method to filter old transactions that are still being gossiped about in the network. Since the cut off time is the timestamp of the global snapshot that is not really a big issue in the current environment since it is quite unlikely that somebody has a wrong time set in his node that dates back before the global snapshot.

When we introduce local snapshots and consequently local cutoff times it can however happen (and it does), that we face a transaction with an invalid timestamp that dates back before our cutoff time, even tho it was issued after.

To still be able to consider this transaction valid if we need it for solidification we have to check if the transaction was part of the TransactionRequester. And to be able to access the transactionRequester instance - this method has to be non static.

Regarding 2: I don't think that these kind of micro-optimizations play any role at all - especially if they help us solve a problem that otherwise would break the synced state of a node.

see: iotadevelopment@40df07b

@hmoog

hmoog Aug 8, 2018

Contributor

Atm we use this method to filter old transactions that are still being gossiped about in the network. Since the cut off time is the timestamp of the global snapshot that is not really a big issue in the current environment since it is quite unlikely that somebody has a wrong time set in his node that dates back before the global snapshot.

When we introduce local snapshots and consequently local cutoff times it can however happen (and it does), that we face a transaction with an invalid timestamp that dates back before our cutoff time, even tho it was issued after.

To still be able to consider this transaction valid if we need it for solidification we have to check if the transaction was part of the TransactionRequester. And to be able to access the transactionRequester instance - this method has to be non static.

Regarding 2: I don't think that these kind of micro-optimizations play any role at all - especially if they help us solve a problem that otherwise would break the synced state of a node.

see: iotadevelopment@40df07b

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 9, 2018

Contributor

👍
If you need access to the object than there is no choice.
I still like making private methods static if they don't need the instance.

@GalRogozinski

GalRogozinski Aug 9, 2018

Contributor

👍
If you need access to the object than there is no choice.
I still like making private methods static if they don't need the instance.

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

Even though I am fine with this change, note that the fact that the TransactionValidator has a TransactionRequester instance is a violation of the Single Responsibility Principle, and in my opinion it shouldn't be there. TransactionValidator should ultimately become a stateless Service.

Of course that it is not your fault that IRI is this way and definitely you should be more concerned about getting local snapshots done rather than fixing IRI architecture.
I just wanted to merge now and I looked at the code again and had to share this thought.

@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

Even though I am fine with this change, note that the fact that the TransactionValidator has a TransactionRequester instance is a violation of the Single Responsibility Principle, and in my opinion it shouldn't be there. TransactionValidator should ultimately become a stateless Service.

Of course that it is not your fault that IRI is this way and definitely you should be more concerned about getting local snapshots done rather than fixing IRI architecture.
I just wanted to merge now and I looked at the code again and had to share this thought.

// ... otherwise -> re-add it at the end of the set ...
//
// Note: we always have enough space since we removed the element before

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 8, 2018

Contributor

Are you positive about that?
In requestTransaction method we add without checking if we are full (which may be another bug). But if I call on this method enough times I can exceed capacity and then I won't have enough space even though I just removed a hash.

Anyhows, now you introduced a logic change that should be tested...
The reason it takes time to refactor IRI is becuase we want to add proper tests to the changes and not just replace bugs with other bugs.

I personally want to refactor every single class here but I want to do it correctly.

@GalRogozinski

GalRogozinski Aug 8, 2018

Contributor

Are you positive about that?
In requestTransaction method we add without checking if we are full (which may be another bug). But if I call on this method enough times I can exceed capacity and then I won't have enough space even though I just removed a hash.

Anyhows, now you introduced a logic change that should be tested...
The reason it takes time to refactor IRI is becuase we want to add proper tests to the changes and not just replace bugs with other bugs.

I personally want to refactor every single class here but I want to do it correctly.

This comment has been minimized.

@hmoog

hmoog Aug 8, 2018

Contributor

I am positive about that!

In line 71 (requestTransaction) we DO check if !transactionsToRequestIsFull() if it is a normal transaction. the request set for milestoneTransactions is not limited in size, which makes sense since all of the milestones are relevant for our node.

@hmoog

hmoog Aug 8, 2018

Contributor

I am positive about that!

In line 71 (requestTransaction) we DO check if !transactionsToRequestIsFull() if it is a normal transaction. the request set for milestoneTransactions is not limited in size, which makes sense since all of the milestones are relevant for our node.

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 9, 2018

Contributor

👍

@GalRogozinski

GalRogozinski Aug 9, 2018

Contributor

👍

requestSet.add(hash);
// ... and abort our loop to continue processing with the element we found
break;

This comment has been minimized.

@codacy-bot

codacy-bot Aug 8, 2018

synchronized (syncObj) {
// repeat while we have transactions that shall be requested

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

For next time instead of adding comments you can just refactor the code to be more readable.
For example you could have wrote !requestSize.isEmpty() or if you want a null set to be considered as an empty set (in this specific example it won't ever be null) CollectionUtils.isNotEmpty(requestSet)

@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

For next time instead of adding comments you can just refactor the code to be more readable.
For example you could have wrote !requestSize.isEmpty() or if you want a null set to be considered as an empty set (in this specific example it won't ever be null) CollectionUtils.isNotEmpty(requestSet)

if (TransactionViewModel.exists(tangle, hash)) {
// ... dump a log message ...

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

This comment is completely extraneous.

@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

This comment is completely extraneous.

@GalRogozinski

I think that in general that instead of commenting each line of code we should strive to write more readable code. This will require much more refactoring though.

@GalRogozinski GalRogozinski merged commit a45fa1a into iotaledger:dev Aug 12, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment