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

[Consensus] Block version 7: remove accumulators checkpoint #1022

Merged
merged 5 commits into from
Nov 23, 2019

Conversation

random-zebra
Copy link

Since the disabling of zerocoin mints, the accumulator values never change anymore.
Last updated checkpoint was at mainnet block 1686240.
Removing the accumulator checkpoints from block headers saves 256 bits per block (both on chain and on the in-memory indexes).

This PR removes the need for CBlockIndex::nAccumulatorCheckpoint (past 1686240) and enforces block version 7 via hard-fork (after nBlockV7StartHeight currently set at a placeholder value).

Further changes to CBlockIndex (in order to don't store empty uint256) will be addressed in a subsequent PR.

@random-zebra random-zebra added Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes zPiv Protocol-Update Block Generation Mining/Staking related issues Consensus labels Sep 23, 2019
@random-zebra random-zebra added this to the 4.0.0 milestone Sep 23, 2019
@random-zebra random-zebra self-assigned this Sep 23, 2019
@furszy furszy self-requested a review September 27, 2019 14:00
@Warrows Warrows self-requested a review September 27, 2019 14:29
furszy
furszy previously approved these changes Sep 27, 2019
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 83a3736

Fuzzbawls
Fuzzbawls previously approved these changes Sep 27, 2019
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

utACK 83a3736

Copy link

@Warrows Warrows left a comment

Choose a reason for hiding this comment

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

Concept is sound. I just got a few comments.

src/miner.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/zpiv/accumulators.cpp Outdated Show resolved Hide resolved
@random-zebra
Copy link
Author

Fixed pindex != nullptr checks and rebased on master.

@furszy
Copy link

furszy commented Nov 4, 2019

Code wise, it's straightforward and looks good. Just need a rebase.

@random-zebra
Copy link
Author

Rebased.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

This whole zerocoin code will require a big further cleanup in the upcoming months.

Good work on this first step 👍, ACK 7143006

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 7143006

could probably clean up the logic in primitives/block.cpp, but its certainly not a blocking concern.

random-zebra added a commit that referenced this pull request Nov 23, 2019
7143006 [Trivial] Check for pindex != nullptr during loops (random-zebra)
cef5a0e [PoS] Don't look for checkpoints after nBlockLastAccumulatorCheckpoint (random-zebra)
d55e714 [Core] set nBlockLastAccumulatorCheckpoint in chainparams (random-zebra)
9cf6c69 [Core] define Block version 7 removing the accumulator checkpoints (random-zebra)
750d04f [Core] Set placeholders for nBlockV7StartHeight hardfork (random-zebra)

Pull request description:

  Since the disabling of zerocoin mints, the accumulator values never change anymore.
  Last updated checkpoint was at mainnet block 1686240.
  Removing the accumulator checkpoints from block headers saves 256 bits per block (both on chain and on the in-memory indexes).

  This PR removes the need for CBlockIndex::nAccumulatorCheckpoint (past 1686240) and enforces block version 7 via hard-fork (after `nBlockV7StartHeight` currently set at a placeholder value).

  Further changes to CBlockIndex (in order to don't store empty uint256) will be addressed in a subsequent PR.

ACKs for top commit:
  furszy:
    Good work on this first step 👍, ACK 7143006
  Fuzzbawls:
    ACK 7143006

Tree-SHA512: 3d8944b86ffb0c5e968a875977610ee44093f7f22a473450e8ce48358e1e9907b07034d698f833cc13abea2aa9409d6c96a5f4e9e15f677eaf5dafa62418ff6a
@random-zebra random-zebra merged commit 7143006 into PIVX-Project:master Nov 23, 2019
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Dec 14, 2019
random-zebra added a commit to random-zebra/blockbook that referenced this pull request Dec 27, 2019
martinboehm pushed a commit to trezor/blockbook that referenced this pull request Jan 3, 2020
* PIVX: bump to 4.0.0

* PIVX: remove AccCheckpoint in block version 7

ref PIVX-Project/PIVX#1022

* PIVX: Test block v7 in pivxparser_test
bidoudan pushed a commit to stibits-inc/blockbook that referenced this pull request Jul 4, 2022
* PIVX: bump to 4.0.0

* PIVX: remove AccCheckpoint in block version 7

ref PIVX-Project/PIVX#1022

* PIVX: Test block v7 in pivxparser_test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants