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

Fix: TransactionValidator instance based / TransactionRequester fixed #914

Merged
merged 1 commit into from
Aug 12, 2018
Merged

Fix: TransactionValidator instance based / TransactionRequester fixed #914

Show file tree
Hide file tree
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
10 changes: 5 additions & 5 deletions 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 @@ -76,7 +76,7 @@ public int getMinWeightMagnitude() {
return MIN_WEIGHT_MAGNITUDE;
}

private static boolean hasInvalidTimestamp(TransactionViewModel transactionViewModel) {
private boolean hasInvalidTimestamp(TransactionViewModel transactionViewModel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (transactionViewModel.getAttachmentTimestamp() == 0) {
return transactionViewModel.getTimestamp() < snapshotTimestamp && !Objects.equals(transactionViewModel.getHash(), Hash.NULL_HASH)
|| transactionViewModel.getTimestamp() > (System.currentTimeMillis() / 1000) + MAX_TIMESTAMP_FUTURE;
Expand All @@ -85,7 +85,7 @@ private static boolean hasInvalidTimestamp(TransactionViewModel transactionViewM
|| transactionViewModel.getAttachmentTimestamp() > System.currentTimeMillis() + MAX_TIMESTAMP_FUTURE_MS;
}

public static void runValidation(TransactionViewModel transactionViewModel, final int minWeightMagnitude) {
public void runValidation(TransactionViewModel transactionViewModel, final int minWeightMagnitude) {
transactionViewModel.setMetadata();
transactionViewModel.setAttachmentData();
if(hasInvalidTimestamp(transactionViewModel)) {
Expand All @@ -107,17 +107,17 @@ public static void runValidation(TransactionViewModel transactionViewModel, fina
}
}

public static TransactionViewModel validateTrits(final byte[] trits, int minWeightMagnitude) {
public TransactionViewModel validateTrits(final byte[] trits, int minWeightMagnitude) {
TransactionViewModel transactionViewModel = new TransactionViewModel(trits, Hash.calculate(trits, 0, trits.length, SpongeFactory.create(SpongeFactory.Mode.CURLP81)));
runValidation(transactionViewModel, minWeightMagnitude);
return transactionViewModel;
}

public static TransactionViewModel validateBytes(final byte[] bytes, int minWeightMagnitude) {
public TransactionViewModel validateBytes(final byte[] bytes, int minWeightMagnitude) {
return validateBytes(bytes, minWeightMagnitude, SpongeFactory.create(SpongeFactory.Mode.CURLP81));
}

public static TransactionViewModel validateBytes(final byte[] bytes, int minWeightMagnitude, Sponge curl) {
public TransactionViewModel validateBytes(final byte[] bytes, int minWeightMagnitude, Sponge curl) {
TransactionViewModel transactionViewModel = new TransactionViewModel(bytes, Hash.calculate(bytes, TransactionViewModel.TRINARY_SIZE, curl));
runValidation(transactionViewModel, minWeightMagnitude);
return transactionViewModel;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/iota/iri/network/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ public void preProcessReceivedData(byte[] receivedData, SocketAddress senderAddr
//if not, then validate
receivedTransactionViewModel = new TransactionViewModel(receivedData, Hash.calculate(receivedData, TransactionViewModel.TRINARY_SIZE, SpongeFactory.create(SpongeFactory.Mode.CURLP81)));
receivedTransactionHash = receivedTransactionViewModel.getHash();
TransactionValidator.runValidation(receivedTransactionViewModel, transactionValidator.getMinWeightMagnitude());
transactionValidator.runValidation(receivedTransactionViewModel, transactionValidator.getMinWeightMagnitude());

synchronized (recentSeenBytes) {
recentSeenBytes.put(byteHash, receivedTransactionHash);
Expand Down
49 changes: 24 additions & 25 deletions src/main/java/com/iota/iri/network/TransactionRequester.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ public class TransactionRequester {

public static final int MAX_TX_REQ_QUEUE_SIZE = 10000;

private static volatile long lastTime = System.currentTimeMillis();

private static double P_REMOVE_REQUEST;
private static boolean initialized = false;
private final SecureRandom random = new SecureRandom();
Expand Down Expand Up @@ -84,48 +82,49 @@ private boolean transactionsToRequestIsFull() {


public Hash transactionToRequest(boolean milestone) throws Exception {
final long beginningTime = System.currentTimeMillis();
// determine which set of transactions to operate on
Set<Hash> primarySet = milestone ? milestoneTransactionsToRequest : transactionsToRequest;
Set<Hash> alternativeSet = milestone ? transactionsToRequest : milestoneTransactionsToRequest;
Set<Hash> requestSet = primarySet.size() == 0 ? alternativeSet : primarySet;

// determine the first hash in our set that needs to be processed
Hash hash = null;
Set<Hash> requestSet;
if(milestone) {
requestSet = milestoneTransactionsToRequest;
if(requestSet.size() == 0) {
requestSet = transactionsToRequest;
}
} else {
requestSet = transactionsToRequest;
if(requestSet.size() == 0) {
requestSet = milestoneTransactionsToRequest;
}
}
synchronized (syncObj) {
// repeat while we have transactions that shall be requested
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

while (requestSet.size() != 0) {
// remove the first item in our set for further examination
Iterator<Hash> iterator = requestSet.iterator();
hash = iterator.next();
iterator.remove();

// if we have received the transaction in the mean time ....
if (TransactionViewModel.exists(tangle, hash)) {
// ... dump a log message ...
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is completely extraneous.

log.info("Removed existing tx from request list: " + hash);
messageQ.publish("rtl %s", hash);
} else {
if (!transactionsToRequestIsFull()) {
requestSet.add(hash);
}
break;

// ... and continue to the next element in the set
continue;
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

requestSet.add(hash);

// ... and abort our loop to continue processing with the element we found
break;
}
}

// randomly drop "non-milestone" transactions so we don't keep on asking for non-existent transactions forever
if(random.nextDouble() < P_REMOVE_REQUEST && !requestSet.equals(milestoneTransactionsToRequest)) {
synchronized (syncObj) {
transactionsToRequest.remove(hash);
}
}

long now = System.currentTimeMillis();
if ((now - lastTime) > 10000L) {
lastTime = now;
//log.info("Transactions to request = {}", numberOfTransactionsToRequest() + " / " + TransactionViewModel.getNumberOfStoredTransactions() + " (" + (now - beginningTime) + " ms ). " );
}
// return our result
return hash;
}

Expand Down
8 changes: 4 additions & 4 deletions src/test/java/com/iota/iri/TransactionValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,20 +67,20 @@ public void validateBytes() throws Exception {
Converter.copyTrits(0, trits, 0, trits.length);
byte[] bytes = Converter.allocateBytesForTrits(trits.length);
Converter.bytes(trits, bytes);
TransactionValidator.validateBytes(bytes, MAINNET_MWM);
txValidator.validateBytes(bytes, MAINNET_MWM);
}

@Test
public void validateTrits() {
byte[] trits = getRandomTransactionTrits();
Converter.copyTrits(0, trits, 0, trits.length);
TransactionValidator.validateTrits(trits, MAINNET_MWM);
txValidator.validateTrits(trits, MAINNET_MWM);
}

@Test(expected = RuntimeException.class)
public void validateTritsWithInvalidMetadata() {
byte[] trits = getRandomTransactionTrits();
TransactionValidator.validateTrits(trits, MAINNET_MWM);
txValidator.validateTrits(trits, MAINNET_MWM);
}

@Test
Expand All @@ -89,7 +89,7 @@ public void validateBytesWithNewCurl() throws Exception {
Converter.copyTrits(0, trits, 0, trits.length);
byte[] bytes = Converter.allocateBytesForTrits(trits.length);
Converter.bytes(trits, 0, bytes, 0, trits.length);
TransactionValidator.validateBytes(bytes, txValidator.getMinWeightMagnitude(), SpongeFactory.create(SpongeFactory.Mode.CURLP81));
txValidator.validateBytes(bytes, txValidator.getMinWeightMagnitude(), SpongeFactory.create(SpongeFactory.Mode.CURLP81));
}

@Test
Expand Down