[Refactor] Fix stake age checks for regtest#976
[Refactor] Fix stake age checks for regtest#976random-zebra merged 1 commit intoPIVX-Project:masterfrom
Conversation
random-zebra
force-pushed
the
2019_nStakeMinAge_chainparams
branch
from
ca18333 to
b67becf
Compare
Warrows
left a comment
Warrows
left a comment
There was a problem hiding this comment.
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.
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().somethingstatic 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
random-zebra
force-pushed
the
2019_nStakeMinAge_chainparams
branch
from
b67becf to
2eacdb7
Compare
|
@PIVX-Project/core-developers check this one please, it's pretty straightforward and will clean the #936 commits list for a better review. |
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
Fuzzbawls
added
the
Needs Release Notes
Fuzzbawls
removed
the
Needs Release Notes
5 participants
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
Stakebut not inCheckProofOfStake(the error indicates also the wrong function).This fixes it and moves
nStakeMinAgeto chain params (instead of having it as rogue external variable) and sets it to0for RegTest (for consistency).Also fixes the error messages in kernel.cpp.