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

Refactor hashBoinc for binary claim contexts #1558

Merged

Conversation

@cyrossignol
Copy link
Member

cyrossignol commented Oct 8, 2019

The hashBoinc field of a coinbase transaction stores delimited strings of data that contain the context that augments each generated block. These changes refactor the storage of this data to allow for binary serialization of the claim contexts.

Based on discussions in Slack, this PR moves the storage of claim contexts from the coinbase transaction to the block itself to facilitate submission of much larger superblock data than a transaction allows. This accommodation prepares the protocol for the removal of the team requirement and a greater number of users. The application begins to use the binary serialization format after the next mandatory threshold. This format significantly reduces the size overhead of the claim context and eliminates issues with string-based serialization.

To further reduce the size of claim data, I removed the following legacy fields:

  • last payment time
  • last block hash
  • last PoR block hash
  • research age
  • average magnitude
  • public key

Although it provides an easy-to-access historical log, this data can be reproduced from blockchain data, so it's not necessary to store it in every block. We can also remove the following informational fields from the claim context, but we agreed to keep them in the short-term for explorers:

  • block subsidy (interest/CBR)
  • research subsidy
  • magnitude
  • magnitude unit

The new NN::Claim class replaces MiningCPID and abstracts the related functionality. The changes here continue clean-up work from #1480 by removing the pervasive use of the GlobalCPUMiningCPID state, and RPC methods now produce structured JSON data for claims and superblocks. Claims now limit the size of version and organization fields to 30 and 50 characters for version 11 blocks. This includes a fix for #525 that enables researchers to stake investor-only blocks with no beacon or one that expired.

A subsequent pull request will hook-up the new NN::Superblock class to the main block acceptance and research reward code.

@cyrossignol cyrossignol force-pushed the cyrossignol:superblock-claim branch 3 times, most recently from 10f2ac8 to 4c84b62 Oct 8, 2019
@jamescowens

This comment has been minimized.

Copy link
Member

jamescowens commented Oct 9, 2019

I am requesting all three other active core devs to look at this due to the importance of the changes.

@cyrossignol

This comment has been minimized.

Copy link
Member Author

cyrossignol commented Oct 9, 2019

Thanks, I have a couple more tweaks to squash and push.

@cyrossignol cyrossignol force-pushed the cyrossignol:superblock-claim branch 2 times, most recently from 17c8559 to 6d26605 Oct 9, 2019
@cyrossignol cyrossignol marked this pull request as ready for review Oct 9, 2019
@jamescowens jamescowens added this to the Fern milestone Oct 13, 2019
Copy link
Member

jamescowens left a comment

Wow. This looks good. How much testing have you done on this?

@cyrossignol

This comment has been minimized.

Copy link
Member Author

cyrossignol commented Oct 13, 2019

@jamescowens Sync smoke tests on mainnet and testnet completed without issue, and I staked blocks as a researcher and investor. Real testing starts after v11 switch-over.

@cyrossignol cyrossignol force-pushed the cyrossignol:superblock-claim branch from 6d26605 to d69499e Oct 14, 2019
@cyrossignol

This comment has been minimized.

Copy link
Member Author

cyrossignol commented Oct 14, 2019

Rebased and added check for superblock size.

@cyrossignol

This comment has been minimized.

Copy link
Member Author

cyrossignol commented Oct 16, 2019

Added a check for zero research reward when staking a block--in the same vein as 6d3cbf8, this switches the block to an investor stake so we don't add unnecessary CPID data to the chain. The research age system needs to scan every research reward block at startup, including blocks with zero research reward. By omitting the CPID from the block, the index scan can skip the block at startup without needing to double-check the research reward amount.

@iFoggz
iFoggz approved these changes Oct 16, 2019
Copy link
Member

iFoggz left a comment

After an hour reviewing this code + 2 sandwiches and a tea. I can say there is a lot going on here and this is a big change and a lot of refactoring to accommodate the change. However I did not see anything that pops out that concerns me. I approve to push ahead for more wide scale testing on testnet and see how well this performs for many users. a set bv11 start should be setup and testnet should be upgraded before hand. Good work @cyrossignol, These are changes that need to be done to improve the overall quality of life and expansion of Gridcoin into the future.

src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/miner.cpp Show resolved Hide resolved
src/neuralnet/superblock.h Show resolved Hide resolved
src/txdb-leveldb.cpp Show resolved Hide resolved
@jamescowens

This comment has been minimized.

Copy link
Member

jamescowens commented Oct 17, 2019

As a general comment, tremendous work on this!

@jamescowens

This comment has been minimized.

Copy link
Member

jamescowens commented Oct 20, 2019

Ok. Only one unresolved to address and we can merge this.

cyrossignol added 20 commits Aug 19, 2019
Fixes #525

If a researcher's beacon expired, generate the block as an investor. We
cannot sign a research claim without the beacon key, so this avoids the
issue that prevents a researcher from staking blocks if the beacon does
not exist (it expired or it has yet to be advertised).
This allows generated blocks greater than MAX_BLOCK_SIZE only when those
contain a superblock. The superblock must serialize on the network to at
most Superblock::MAX_SIZE bytes. The rest of the block must hold at most
MAX_BLOCK_SIZE bytes still.
If no pending research subsidy value exists, build an investor claim.
This avoids polluting the block index with non-research reward blocks
that contain CPIDs which increases the effort needed to load research
age context at start-up.
@cyrossignol cyrossignol force-pushed the cyrossignol:superblock-claim branch from 37f8825 to 8af173b Oct 20, 2019
@cyrossignol

This comment has been minimized.

Copy link
Member Author

cyrossignol commented Oct 20, 2019

Rebased to fix an unused argument in ComputeResearchAccrual() added by #1569.

@jamescowens

This comment has been minimized.

Copy link
Member

jamescowens commented Oct 20, 2019

Merging

@jamescowens jamescowens merged commit e88643b into gridcoin-community:development Oct 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.