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 solidity propagation from bottom to top #907

Merged
merged 8 commits into from Aug 6, 2018

Conversation

Projects
None yet
5 participants
@GalRogozinski
Contributor

GalRogozinski commented Aug 5, 2018

Description

There is a propagateSolidTransaction thread that is supposed to cascade solidity from bottom to top.
If a transaction is solid and its approver's other parent is solid then make the approver solid. Then keep on propagating upwards in this manner.
Unfortunately, this didn't happen. The solidified approver didn't reenter the queue so the change was not cascaded.

May help with #899 / #898

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How Has This Been Tested?

The following unit tests have been added

  • testTransactionPropagation
  • testTransactionPropagationFailure

In addition, canaries were run by @DyrellC and @geminoz.

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
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

GalRogozinski added some commits Aug 5, 2018

@GalRogozinski GalRogozinski requested review from paulhandy and alon-e Aug 5, 2018

GalRogozinski added some commits Aug 5, 2018

@alon-e

@GalRogozinski would you say it's fair to add this to the description?

Also, the already solid transaction was added to the queue instead - filling up the queue with useless transactions.
newSolidThread.start();
}
void setMwm(boolean testnet, int mwm) {

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

just a nit, but if(!testnet) MIN_WEIGHT_MAGNITUDE = Math.max(MIN_WEIGHT_MAGNITUDE, 13) would be less magic-number heavy

@th0br0

th0br0 Aug 5, 2018

Contributor

just a nit, but if(!testnet) MIN_WEIGHT_MAGNITUDE = Math.max(MIN_WEIGHT_MAGNITUDE, 13) would be less magic-number heavy

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

I pulled the code to setMwm to make the test work,
The implementation logic is old and not really part of this PR.

But since you commented I will change :-)

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

I pulled the code to setMwm to make the test work,
The implementation logic is old and not really part of this PR.

But since you commented I will change :-)

Show outdated Hide outdated src/main/java/com/iota/iri/TransactionValidator.java Outdated
void propagateSolidTransactions() {
Set<Hash> newSolidHashes = new HashSet<>();
useFirst.set(!useFirst.get());

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

useFirst what?

@th0br0

th0br0 Aug 5, 2018

Contributor

useFirst what?

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

This is part of the old code. I just wanted to solve the current problem without changing too much of it at the moment.

I think much of it should be changed but not atm.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

This is part of the old code. I just wanted to solve the current problem without changing too much of it at the moment.

I think much of it should be changed but not atm.

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

Btw,
This has default visibility for the test code.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

Btw,
This has default visibility for the test code.

Show outdated Hide outdated src/main/java/com/iota/iri/TransactionValidator.java Outdated
Show outdated Hide outdated src/main/java/com/iota/iri/TransactionValidator.java Outdated
};
}
void propagateSolidTransactions() {

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

method should have explicit visibility

@th0br0

th0br0 Aug 5, 2018

Contributor

method should have explicit visibility

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

The reason it doesn't have explicit visibility is because it shouldn't be public yet unfortunately it is used in tests so unfortunately private won't work...
So default package visibility is the best option.

This a common thing we did in my last workplace. You have a better idea?

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

The reason it doesn't have explicit visibility is because it shouldn't be public yet unfortunately it is used in tests so unfortunately private won't work...
So default package visibility is the best option.

This a common thing we did in my last workplace. You have a better idea?

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

Fair enough, ignore this then.

@th0br0

th0br0 Aug 5, 2018

Contributor

Fair enough, ignore this then.

@@ -266,6 +268,11 @@ private boolean checkApproovee(TransactionViewModel approovee) throws Exception
return approovee.isSolid();
}
//for testing
boolean isNewSolidTxSetsEmpty () {

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

method should have explicit visibility? (or did we consciously make the choice to allow this?)

@th0br0

th0br0 Aug 5, 2018

Contributor

method should have explicit visibility? (or did we consciously make the choice to allow this?)

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

ditto

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

ditto

public static TransactionViewModel createTransactionWithTrytes(String trytes) {
String expandedTrytes = expandTrytes(trytes);
byte[] trits = Converter.allocatingTritsFromTrytes(expandedTrytes);
return new TransactionViewModel(trits, Hash.calculate(SpongeFactory.Mode.CURLP81, trits));

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

shouldn't we just add a default constructor to TxVM or a helper method somewhere?

@th0br0

th0br0 Aug 5, 2018

Contributor

shouldn't we just add a default constructor to TxVM or a helper method somewhere?

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

It has this expandTrytes call.
I don't want this as part of default behavior...

@GalRogozinski

GalRogozinski Aug 5, 2018

Contributor

It has this expandTrytes call.
I don't want this as part of default behavior...

@iotaledger iotaledger deleted a comment from codacy-bot Aug 5, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Aug 5, 2018

}
parent = TransactionViewModel.fromHash(tangle, parent.getHash());
Assert.assertTrue("Parent tx was expected to be solid", parent.isSolid());

This comment has been minimized.

@codacy-bot

codacy-bot Aug 5, 2018

parent = TransactionViewModel.fromHash(tangle, parent.getHash());
Assert.assertTrue("Parent tx was expected to be solid", parent.isSolid());
grandParent = TransactionViewModel.fromHash(tangle, grandParent.getHash());
Assert.assertFalse("GrandParent tx was expected to be not solid", grandParent.isSolid());

This comment has been minimized.

@codacy-bot

codacy-bot Aug 5, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Aug 5, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Aug 5, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Aug 5, 2018

parent = TransactionViewModel.fromHash(tangle, parent.getHash());
Assert.assertTrue("Parent tx was expected to be solid", parent.isSolid());
grandParent = TransactionViewModel.fromHash(tangle, grandParent.getHash());
Assert.assertTrue("Grandparent was expected to be solid", grandParent.isSolid());

This comment has been minimized.

@codacy-bot

codacy-bot Aug 5, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Aug 5, 2018

@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Aug 5, 2018

Contributor

well done!!!! i think this will have a major impact on how the tangle behaves (especially the random walks)

Contributor

gjeee commented Aug 5, 2018

well done!!!! i think this will have a major impact on how the tangle behaves (especially the random walks)

}
}
Iterator<Hash> cascadeIterator = newSolidHashes.iterator();
while(cascadeIterator.hasNext() && !shuttingDown.get()) {

This comment has been minimized.

@th0br0

th0br0 Aug 5, 2018

Contributor

Does it make sense to check this here by the way? We're checking in the Runnable anyway

@th0br0

th0br0 Aug 5, 2018

Contributor

Does it make sense to check this here by the way? We're checking in the Runnable anyway

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 6, 2018

Contributor

I think that this will allow for early exit from the loop.
Should it exist? not sure

I just don't want to add more changes for this PR. I want to do sanity tests and then improve it.

@GalRogozinski

GalRogozinski Aug 6, 2018

Contributor

I think that this will allow for early exit from the loop.
Should it exist? not sure

I just don't want to add more changes for this PR. I want to do sanity tests and then improve it.

@alon-e alon-e merged commit 4582700 into iotaledger:dev Aug 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Aug 6, 2018

Contributor

the sleep should be within the while loop i think!

Contributor

gjeee commented Aug 6, 2018

the sleep should be within the while loop i think!

@alon-e

This comment has been minimized.

Show comment
Hide comment
@alon-e

alon-e Aug 6, 2018

Contributor

@gjeee you are right ofc 💯, Thanks!

Contributor

alon-e commented Aug 6, 2018

@gjeee you are right ofc 💯, Thanks!

Thread.sleep(500);
} catch (InterruptedException e) {
e.printStackTrace();
}

This comment has been minimized.

@gjeee

gjeee Aug 6, 2018

Contributor

this try/catch block should be within the while!!!!

@gjeee

gjeee Aug 6, 2018

Contributor

this try/catch block should be within the while!!!!

This comment has been minimized.

@GalRogozinski

GalRogozinski Aug 6, 2018

Contributor

Thanks!
Corrected with #911

@GalRogozinski

GalRogozinski Aug 6, 2018

Contributor

Thanks!
Corrected with #911

alon-e added a commit that referenced this pull request Aug 6, 2018

gjeee added a commit to gjeee/iri that referenced this pull request Aug 6, 2018

TransactionValidator is now solely resposible for updating TipsViewModel
TipsViewModel is responsible for keeping an accurate picture of the solid and non-solid tips. Currently this view is managed within #TransactionValidator.updateStatus() and a separate TipsSolidifier thread (which is not really efficient since it picks one random non-solid tip at a time each 750ms). This leads to a lag in the tips reality. See also issue iotaledger#761 and iotaledger#573. With PR iotaledger#907 in place, we can manage the TipsViewModel fully within TransactionValidator and keep an accurate and up-to-date picture of the current (solid/non-solid) tips.

@gjeee gjeee referenced this pull request Aug 14, 2018

Closed

Unnecessary gaps in branches #573

laumair added a commit to laumair/trinity-wallet that referenced this pull request Oct 13, 2018

Revert (temporarily made) promotion adjustments
To avoid unnecessary reattachments (because of a bug in IRI checkConsistency endpoint), some temporary adjustments were made in iotaledger#368 & iotaledger#371.
Since IRI patched the bug in v1.5.5 (https://github.com/iotaledger/iri/releases/tag/v1.5.5) specifically in iotaledger/iri#907, this reverts the temporary adjustments made to promotion setup.

@laumair laumair referenced this pull request Oct 13, 2018

Merged

Revert (temporarily made) promotion adjustments #495

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment