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

Remove bit 2 warning. #580

Merged
merged 1 commit into from
May 16, 2019
Merged

Remove bit 2 warning. #580

merged 1 commit into from
May 16, 2019

Conversation

presstab
Copy link
Contributor

closes #563

Copy link
Collaborator

@blondfrogs blondfrogs left a comment

Choose a reason for hiding this comment

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

Code looks fine

@presstab
Copy link
Contributor Author

@blondfrogs forced pushed over what you reviewed. Added iteration over the consensus deployments to manually check if the bit has been defined. It doesn't seem like it serves a good purpose to print a warning of something that your wallet has knowledge of. On the other hand if your wallet does not have knowledge of the bit being signalled, you should be warned.

@blondfrogs
Copy link
Collaborator

Makes sense. I like this change

@presstab presstab added the Tag: Waiting For QA A pull review is waiting for QA to test the pull request label May 10, 2019
@crypt0-fusi0n crypt0-fusi0n self-assigned this May 10, 2019
@crypt0-fusi0n crypt0-fusi0n added QA: In Progress This is being tested by the QA team. KEEP CALM and RELAX. and removed Tag: Waiting For QA A pull review is waiting for QA to test the pull request labels May 10, 2019
@ncrypter90 ncrypter90 added QA: Passed This has passed QA testing and can be merged to master and removed QA: In Progress This is being tested by the QA team. KEEP CALM and RELAX. labels May 11, 2019
@codablock
Copy link

FYI, the real reason for the warning to have appeared is very likely that you have changed nRuleChangeActivationThreshold and nMinerConfirmationWindow without taking the already deployed BIP9 deployments into account. Look at 70de070. This causes things to get messed up now.

The better solution for this would be to maintain these parameters per deployment so that you can vary it when needed. Look at the changes Dash did for DIP1 as an example: dashpay/dash@cd262bf#diff-bc9415451d733499187a3fe59e0fd7e6R34 (the addition of nWindowSize and nThreshold in BIP9Deployment)

@presstab
Copy link
Contributor Author

@codablock awesome reference thanks for linking it. That would be extremely useful, because (as was the case here) sometimes it is necessary to have faster than usual deployments. Will add this to the list of items to get done before we do another deployment.

@presstab presstab merged commit 72408f9 into Veil-Project:master May 16, 2019
presstab added a commit that referenced this pull request May 16, 2019
72408f9 Remove bit warnings if the bit has been defined in params. (presstab)

Pull request description:

  closes #563

Tree-SHA512: 1dc51d0dcfd525b37e9e34cdd0f5f0149dea3523be4c0ef520bedcb5dcc6b86ce3a151b5584800535de06cd672e69faae5027064e9acd01eeaeab4ab7adeca2a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA: Passed This has passed QA testing and can be merged to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of bit 2 warning.
5 participants