-
Notifications
You must be signed in to change notification settings - Fork 715
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
[Core][Qt] Rewind block index to last checkpoint functionality #2820
base: master
Are you sure you want to change the base?
Conversation
// First load options | ||
if(!fJustCheck) | ||
return true; |
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.
Could we automatically detect if a rewind is needed?
If there are checkpoints between the tip and the last (known) checkpoint, we are not on the right chain.
Maybe something like this:
// If there are checkpoints between prevCheckPoint and chainActive.Tip
// This is not the right chain and a rewind to prevCheckPoint is needed
bool bNeedRewind = fJustCheck;
if (Checkpoints::fEnabled){
const MapCheckpoints& checkpoints = *Params().Checkpoints().mapCheckpoints;
for (const auto& i : reverse_iterate(checkpoints)) {
if (i.first > checkPointHeight && i.first <= nHeight) {
bNeedRewind = true;
break;
}
}
}
if(!bNeedRewind)
return true;
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.
If there are checkpoints between the tip and the last (known) checkpoint, we are not on the right chain. and this is right,
but if there are not checkpoints between the tip and the last (known) checkpoint it does not necessarily mean we are on the right chain
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.
I do not see a possibility in automatically detecting a rewind.
In such we have two cases:
1.
There is a checkpoint known and in the future, but we are behind said checkpoint, we would then simply sync to it.
2.
There is not a checkpoint and between the tip and the last known, to which we should be fine.
IE: Checkpoint block is 2150000, we are at block 2150010, if there was any in-between, that user would already have synced those
Checkpoint is block 2150005 and 2150000, while we are at block 2150010, there's no way we would be at 2150010 with a prevCheckpoint of 2150000 not 2150005 and on the same wallet version.
1453d14
to
146e72b
Compare
Add newer checkpoint New checkpoint Add tests Fix whitespace Set correct permissions Adjust test positon and fix num nodes
146e72b
to
48d2841
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.
Successfully tested on mainnet and code looks really good! Just a couple of comments:
@@ -182,12 +182,13 @@ static MapCheckpoints mapCheckpoints = { | |||
{3014000, uint256S("78ad99b7225f73c42238bd7ca841ff700542b92bba75a0ef2ed351caa560f87f")}, //!< PIVX v5.3.0 enforced | |||
{3024000, uint256S("be4bc75afcfb9136924810f7483b2695089a366cc4ee27fd6dc3ecd5396e1f0f")}, //!< Superblock | |||
{3715200, uint256S("a676b9a598c393c82b949c37dd35013aeda55f5d18ab062349db6a8235972aaa")}, //!< Superblock for 5.5.0 mainnet rewards changeover | |||
{3780000, uint256S("2667fa1d552999aca930ced7fd3902ae5721e5c256a607049e3c47a3137a18ee")}, //!< Fri, 10 Mar 2023 rewindblockindex testing |
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.
This checkpoint is useless
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.
I agree! I should have prefaced thats for testing since the last checkpoint was a big further back and would have been a faster sync
}; | ||
|
||
static const CCheckpointData data = { | ||
&mapCheckpoints, | ||
1591401645, // * UNIX timestamp of last checkpoint block | ||
5607713, // * total number of transactions between genesis and last checkpoint | ||
1678401150, // * UNIX timestamp of last checkpoint block |
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.
hence re-update those with last v5.5 checkpoint
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.
Agreed, I will rebase and we just make sure to update more checkpoints more frequently
This pull request supersedes #2298
This feature is a good addition because in cases of late updates, or a short fork a user can start from the latest hardcoded checkpoints instead of re-syncing from scratch.
Cleaned up a little and added the missing functionality that was remaining.
This PR now:
-rewindblockindex
paramRewind Blockchain
inside "Wallet Repair" tabPotential TODO:
Some work was done by PeterL73