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

some TransactionValidator fixes regarding solidity #913

Merged
merged 28 commits into from Aug 14, 2018

Conversation

Projects
None yet
4 participants
@gjeee
Contributor

gjeee commented Aug 6, 2018

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 #761 and #573. With PR #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 added some commits Aug 6, 2018

Same as previous commit
only different order
Update TipsSolidifier.java
i commented the redundant code and for convenience i added some logging to get an idea of the number of solid/non-solid tips.
in theory this whole thread could be removed (if my reasoning is right of course)
Update TransactionValidator.java
added tx.update(tangle, "solid")
Update TransactionValidator.java
i moved the persistence of the solidity property to db before the approver loop
it now also persists height

@gjeee gjeee changed the title from TransactionValidator is now solely resposible for updating TipsViewModel to TransactionValidator is now solely responsible for updating TipsViewModel Aug 7, 2018

gjeee added some commits Aug 7, 2018

Update TransactionValidator.java
moved updateTipsView(tx); before approver loop
Update TransactionValidator.java
revert some earlier changes
Update TransactionValidator.java
used some wrong objects
Update TransactionValidator.java
well again some wwrong object used
Update TransactionValidator.java
only 3 lines added compared to iotaledger/dev
Update TipsSolidifier.java
i now see the need for this thread (to take care of incidental gaps) so i uncommented the logic again.
now the good thing is that my changes to TransactionValidator make sure there are only a few non-solid tips.....this makes sense since most txs become solid in the forward propagation recursion (as defined in TransactionValidator.propagateTransactions)
@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Aug 11, 2018

Contributor

my latest 2 commits have minimal (but important) changes:

  1. it ensures that the solid property is stored in #TransactionValidator.updateStatus()
  2. if quickSetSolid is true it calls tipsViewModel setSolid(hash). The tips-state is now timely and correctly updated.

on a sidenote: i first thought tipssolidifier could be made redundant but now I see the need for this thread (to take care of incidental gaps) so i undid my initial changes there. now the good thing is that my changes to TransactionValidator make sure there are only a few non-solid tips.....this makes sense since most txs become solid in the forward propagation recursion (as defined in TransactionValidator.propagateTransactions)

Contributor

gjeee commented Aug 11, 2018

my latest 2 commits have minimal (but important) changes:

  1. it ensures that the solid property is stored in #TransactionValidator.updateStatus()
  2. if quickSetSolid is true it calls tipsViewModel setSolid(hash). The tips-state is now timely and correctly updated.

on a sidenote: i first thought tipssolidifier could be made redundant but now I see the need for this thread (to take care of incidental gaps) so i undid my initial changes there. now the good thing is that my changes to TransactionValidator make sure there are only a few non-solid tips.....this makes sense since most txs become solid in the forward propagation recursion (as defined in TransactionValidator.propagateTransactions)

@gjeee gjeee changed the title from TransactionValidator is now solely responsible for updating TipsViewModel to some TransactionValidator fixes regarding solidity Aug 11, 2018

@GalRogozinski

This comment has been minimized.

Show comment
Hide comment
@GalRogozinski

GalRogozinski Aug 12, 2018

Contributor

@karimodm
Could you please try to run your promotion tests with this PR?

Contributor

GalRogozinski commented Aug 12, 2018

@karimodm
Could you please try to run your promotion tests with this PR?

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

@GalRogozinski

Just a little change

@alon-e

nice PR! good catch w/ updating the TipsVM on a solid new tx.

gjeee added some commits Aug 12, 2018

Update TipsSolidifier.java
* log.debug instead of log.info
* every 10 seconds some info rgd number of tips is printed
Update TipsViewModel.java
added solidSize() function
Update TipsSolidifier.java
removed some white lines
Update TipsSolidifier.java
some spaces removed
Update TipsViewModel.java
removed eol
Update TransactionValidator.java
tabs......hopefully it is ok now

gjeee added some commits Aug 12, 2018

@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Aug 12, 2018

Contributor

@alon-e @GalRogozinski i completed adding your feedback! i had some fights with the indentation standard...but now i finally committed the changes cleanly!!!

Contributor

gjeee commented Aug 12, 2018

@alon-e @GalRogozinski i completed adding your feedback! i had some fights with the indentation standard...but now i finally committed the changes cleanly!!!

@karimodm karimodm self-requested a review Aug 13, 2018

@alon-e

alon-e approved these changes Aug 13, 2018

@GalRogozinski

Great,
One last change and once testing the PR is completed we will merge.

gjeee added some commits Aug 14, 2018

Update TipsSolidifier.java
added 2 spaces
@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Aug 14, 2018

Contributor
Contributor

gjeee commented Aug 14, 2018

@GalRogozinski done!

@GalRogozinski

2 more changes, and I think we will be done!

@karimodm

This comment has been minimized.

Show comment
Hide comment
@karimodm

karimodm Aug 14, 2018

Contributor

After some tests comparing this changes with the latest dev I can say that this PR dramatically improves the solid / nonSolid tips ratio. Good for me @gjeee !

Contributor

karimodm commented Aug 14, 2018

After some tests comparing this changes with the latest dev I can say that this PR dramatically improves the solid / nonSolid tips ratio. Good for me @gjeee !

Update TipsSolidifier.java
added log.isDebugEnabled()
@gjeee

This comment has been minimized.

Show comment
Hide comment
@gjeee

gjeee Aug 14, 2018

Contributor

@karimodm the number of non-solid tips can still grow in a malicious environment (see issue #938) but at least this PR tracks and updates the solidity like it is.

Contributor

gjeee commented Aug 14, 2018

@karimodm the number of non-solid tips can still grow in a malicious environment (see issue #938) but at least this PR tracks and updates the solidity like it is.

@karimodm

karimodm approved these changes Aug 14, 2018 edited

Looks good.

@GalRogozinski GalRogozinski merged commit 55c62ba into iotaledger:dev Aug 14, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Aug 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment