-
Notifications
You must be signed in to change notification settings - Fork 717
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
[Refactor] Fix stake age checks for regtest #976
[Refactor] Fix stake age checks for regtest #976
Conversation
ca18333
to
b67becf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK. However it would be better to have at least two separate commits, one for real code changes and one for comments, indenting and formatting.
Also, I'm not a native speaker but as far as I know in English there is no space before a :
character. So your changes regarding this matter seems to go in the wrong direction.
Personally I don't like the space between the function and |
Yes, there is no standard convention in the code for that (which is supposed to be just a separator, doesn't have to strictly follow the conventions of the English language for spacing around colons). Anyway this PR is not about code styling. That can be addressed separately. |
Fair enough; especially after peeking at the code and seeing the topic is a can of worms :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, great catch.
Two minor things that can be tackled in another PR without block this one.
-
Would be nice to start moving away from the
Params().something
static calls and have a variable at the beginning of every method that use it, making easy any future BTC back port as they are moving away from that too. -
Would be good to have in ChainParams a
isRegtest()
method and stop doing theParams().NetworkID() != CbaseChainParams::REGTEST
.
Side from that, utACK b67becf
- fix age check in CheckProofOfStake - move nStakeMinAge to chain params - fix error logs in kernel.cpp
b67becf
to
2eacdb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 2eacdb7
@PIVX-Project/core-developers check this one please, it's pretty straightforward and will clean the #936 commits list for a better review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2eacdb7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2eacdb7
2eacdb7 [Refactor] Fix stake age checks for regtest (random-zebra) Pull request description: There is a bug occurring sometimes on regtest where a block is not accepted due to `CheckStakeKernelHash() : min age violation`. This is because the min age requirement for RegTest was changed (in #812) inside `Stake` but not in `CheckProofOfStake` (the error indicates also the wrong function). This fixes it and moves `nStakeMinAge` to chain params (instead of having it as rogue external variable) and sets it to `0` for RegTest (for consistency). Also fixes the error messages in kernel.cpp. ACKs for top commit: furszy: utACK [2eacdb7](2eacdb7) Fuzzbawls: ACK 2eacdb7 Warrows: ACK 2eacdb7 Tree-SHA512: e65d0298ad76ac2c3886507e9d4316d6c00591873f9fb3c59fe856be851d9c7a9ae3dcf9ea01cf7ccb726c4a5c0599dc80cfd427224c2c37d5e72197e79d0eba
There is a bug occurring sometimes on regtest where a block is not accepted due to
CheckStakeKernelHash() : min age violation
.This is because the min age requirement for RegTest was changed (in #812) inside
Stake
but not inCheckProofOfStake
(the error indicates also the wrong function).This fixes it and moves
nStakeMinAge
to chain params (instead of having it as rogue external variable) and sets it to0
for RegTest (for consistency).Also fixes the error messages in kernel.cpp.