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: Node requested old transactions forever #1235

Merged
merged 2 commits into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@hmoog
Copy link
Contributor

hmoog commented Dec 13, 2018

Description

Because of a bug in the checkSolidity function and a problem with the bottom->top solidifier (missing cuckoo filter) the node started requesting very old transactions and never stopped which at some point overwhelmed the requester (and increased the resource consumption).

Type of change

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

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
  • New and existing unit tests pass locally with my changes

hmoog added some commits Dec 13, 2018

Fix: allow the transactionRequester to also send non-solid tips
Allow the transactionRequester to also send non-solid tips - otherwise we might have problems getting the tips solid (if we can not send any requests).
@@ -147,6 +147,9 @@ public void shutdown() {
*/
private TransactionViewModel getTransactionToSendWithRequest() throws Exception {
Hash tip = tipsViewModel.getRandomSolidTipHash();
if (tip == null) {
tip = tipsViewModel.getRandomNonSolidTipHash();
}

This comment has been minimized.

@GalRogozinski

GalRogozinski Dec 13, 2018

Member

Why did you create this change?
You think that due to local snapshots, tips that should be solid won't solidify?

I think usually we don't want to send "spam" tips that approve non-existent transactions.

This comment has been minimized.

@hmoog

hmoog Dec 13, 2018

Contributor

Fix: allow the transactionRequester to also send non-solid tips

Allow the transactionRequester to also send non-solid tips - otherwise we might have problems getting the tips solid (if we can not send any requests).

This comment has been minimized.

@alon-e

alon-e Dec 13, 2018

Member

@hmoog this should not make a difference, as getRandomSolidTipHash will call getRandomNonSolidTipHash if it has no tips
see: #1229

This comment has been minimized.

@hmoog

hmoog Dec 13, 2018

Contributor

Oh, you are right it shouldn't make a difference since getRandomSolidTipHash() has an implementation that doesnt correctly reflects its name. Given the fact that we already have an issue to "fix" this behavior i would anyway keep this change in place to make it still work once getRandomSolidTipHash gets fixed.

if(approovee.getType() == PREFILLED_SLOT) {
// don't solidify from the bottom until cuckoo filters can identify where we deleted -> otherwise we will
// continue requesting old transactions forever
//transactionRequester.requestTransaction(approovee.getHash(), false);

This comment has been minimized.

@alon-e

alon-e Dec 13, 2018

Member

this change was made to prevent false positives - transactions that we think are missing, but have deliberately been deleted.

but it seems this will create false negatives - where actual missing transactions (small holes) will not be requested and thought of as deleted.

This comment has been minimized.

@hmoog

hmoog Dec 13, 2018

Contributor

That is correct @alon-e but they will be requested by the checkSolidity calls from the top anyway. So as long as we don't have a way to determine where to stop the solidification (the cuckoo filters PR prepared and merged), this will break nodes that have local snapshots enabled. If we solidify from the top we stop whenever we reach a solid entry point.

But if we happen to receive a transaction that is "below" a solid entry point - this logic will continue requesting everything below it until we reach the genesis.

Since people are already using LS and people are getting problems with this (see gems comment in slack - we should work around the issue for now and add a correct check + re-enable this request later)

@alon-e

alon-e approved these changes Dec 13, 2018

@@ -436,13 +436,15 @@ private boolean quickSetSolid(final TransactionViewModel transactionViewModel) t
* @throws Exception if we encounter an error while requesting a transaction
*/
private boolean checkApproovee(TransactionViewModel approovee) throws Exception {
if(approovee.getType() == PREFILLED_SLOT) {

This comment has been minimized.

@alon-e

alon-e Dec 13, 2018

Member

this order swap! perfect -- the NULL_HASH also returns PREFILLED_SLOT so up until this fix it would not mark TXs pointing to Genesis as solid.

@GalRogozinski GalRogozinski merged commit df04e1b into iotaledger:dev-localsnapshots Dec 13, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
buildkite/iri-build-jar-prs Build #280 passed and blocked
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