Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Do not process bundled transactions while node is syncing - Closes #2398 #2607

Merged

Conversation

diego-G
Copy link

@diego-G diego-G commented Dec 4, 2018

How to test it?

mocha test/unit/logic/transaction_pool.js

Review checklist

SargeKhan
SargeKhan previously approved these changes Dec 4, 2018
@MaciejBaj MaciejBaj changed the title Do not process bundled transactions while node is syncing - Closes # Do not process bundled transactions while node is syncing - Closes #2398 Dec 4, 2018
@diego-G diego-G requested a review from 4miners December 5, 2018 09:16
logic/transaction_pool.js Show resolved Hide resolved
Copy link
Contributor

@4miners 4miners left a comment

Choose a reason for hiding this comment

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

Don't we need to adjust unit tests?

logic/transaction_pool.js Show resolved Hide resolved
4miners
4miners previously approved these changes Dec 5, 2018
SargeKhan
SargeKhan previously approved these changes Dec 5, 2018
@diego-G diego-G dismissed stale reviews from SargeKhan and 4miners via e8c5435 December 5, 2018 15:02
4miners
4miners previously approved these changes Dec 5, 2018
It was conflicting with other tests after moving the transaction
pool jobs registration to bind function.
@diego-G diego-G force-pushed the 2398-do_not_process_bundled_transactions_while_syncing branch from 9a4d045 to 1eb6c0d Compare December 6, 2018 10:05
@@ -222,7 +222,7 @@ class Round {
}
});
}
return Promise.resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Author

@diego-G diego-G Dec 6, 2018

Choose a reason for hiding this comment

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

@yatki strongly suggested to return null in these kind of cases instead of resolving promises. Test became unstable since we introduce this.

@@ -82,7 +82,7 @@ module.exports = function(configurations, network) {
// to update across all nodes.
setTimeout(() => {
resolve();
}, 5000);
}, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

Network test suite is failing due to the security time to wait for some actions is not enough. I've seen the same problem in propagation scenario.

@MaciejBaj MaciejBaj merged commit 1acde22 into development Dec 7, 2018
@MaciejBaj MaciejBaj deleted the 2398-do_not_process_bundled_transactions_while_syncing branch December 7, 2018 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

During the syncing process, do not process bundled transactions
5 participants