Skip to content
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

Merge P2P bugfixes into seednode branch to help node stability issues #3155

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

wiz
Copy link
Contributor

@wiz wiz commented Aug 28, 2019

No description provided.

@wiz wiz requested a review from ripcurlx as a code owner August 28, 2019 07:37
@wiz wiz changed the title Merge 2 patches into seednode branch to help node stability issues Merge P2P bugfixes into seednode branch to help node stability issues Aug 28, 2019
Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

utAck

I am not sure if we should keep adding code and complexity to the project like this. Sure, there is an issue there that needs fixing, hence, utack. However, it seems to me that there is a simpler solution just around the corner.

// We must set the listeners initially and not on onParseBlockChainComplete as activeOrMyUnconfirmedProposals
// is used in voteResults which can be called earlier during sync.
// To avoid unneeded upDateLists calls we delay one render frame so that once the proposalService is complete we
// register out listeners.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// register out listeners.
// register our listeners.

public void start() {
// We must set the listeners initially and not on onParseBlockChainComplete as activeOrMyUnconfirmedProposals
// is used in voteResults which can be called earlier during sync.
// To avoid unneeded upDateLists calls we delay one render frame so that once the proposalService is complete we
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// To avoid unneeded upDateLists calls we delay one render frame so that once the proposalService is complete we
// To avoid unneeded updateLists(); calls, we delay one render frame so that once the proposalService is complete, we

@@ -31,7 +31,7 @@
PROPOSAL,
BLIND_VOTE,
ACK_MSG,
BSQ_BLOCK,
RECEIVE_BSQ_BLOCK,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we should change stuff like that. What I am sure of is that we should add Javadoc to describe what a capability is for and what it does, only so can we make sure devs can understand what these 2 or 3 words mean.

@wiz
Copy link
Contributor Author

wiz commented Aug 28, 2019

@freimair all of these commits were already merged into master in PRs #3148, #3150, and #3151 - this PR is only for the seednode_temporary branch, so if you want to make changes please make a PR that branches off master because this branch will probably go away soon.

@ripcurlx ripcurlx merged commit ab59d56 into bisq-network:seednode_temporaryfix Aug 28, 2019
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

@wiz wiz deleted the seednode_patches branch August 29, 2019 08:24
This was referenced Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants