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 Subsidy Code #33

Merged
merged 12 commits into from
Apr 27, 2021
Merged

Conversation

SmartArray
Copy link

@SmartArray SmartArray commented Apr 12, 2021

Refactor Subsidy Code

Looking at the code in validation.cpp:

digibyte/src/validation.cpp

Lines 1216 to 1244 in 1cb4d6b

CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
{
CAmount nSubsidy = COIN;
LogPrintf("block height for reward is %d\n", nHeight);
if(nHeight < consensusParams.nDiffChangeTarget) {
//this is pre-patch, reward is 8000.
nSubsidy = 8000 * COIN;
if(nHeight < 1440) //1440
{
nSubsidy = 72000 * COIN;
}
else if(nHeight < 5760) //5760
{
nSubsidy = 16000 * COIN;
}
} else {
//patch takes effect after 67,200 blocks solved
nSubsidy = GetDGBSubsidy(nHeight, consensusParams);
}
//make sure the reward is at least 1 DGB
if(nSubsidy < COIN) {
nSubsidy = COIN;
}
return nSubsidy;
}
it is barely documented, has weird indentions and is hard to read.
The original Bitcoin Code-Style Convention was not followed at all.

This PR is supposed to tidy up the subsidy code before introducing future changes (see #20)

Expectations

The altered code must still pass the main_tests unit test with 100%.

Changes

d54c650 merged GetBlockSubsidy and GetDGBSubsidy into one function.
24b4d23 removes dead code that wasn't removed with the previous commit.

How to verify?

  1. Clone using
git clone https://github.com/SmartArray/digibyte.git -b refactor/subsidy && cd digibyte
  1. Prepare the build environment
./autogen.sh && ./configure
  1. Compile the test suite
cd src/test
make -j4
  1. Run it using
./test_digibyte --run_test=main_tests

Expected outcome

$ ./test_digibyte --run_test=main_tests
Running 2 test cases...
*** No errors detected

Added more comments and merged GetDGBSubsidy and GetBlockSubsidy
@SmartArray SmartArray self-assigned this Apr 12, 2021
@SmartArray SmartArray added the refactor Code Refactoring label Apr 12, 2021
@ChillingSilence
Copy link

ACK, beautifully commented too! 👍 Thanks @SmartArray

@SmartArray
Copy link
Author

ACK, beautifully commented too! 👍 Thanks @SmartArray

Thank you @ChillingSilence. Documentation is the biggest intent of this PR.

Also, this PR is a great example of how development can continue on DigiByte Core now, since we have the working unit tests.

Run `make check` every time a code push happens.
gto90
gto90 previously approved these changes Apr 18, 2021
Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

utACK

@gto90 gto90 requested a review from JaredTate April 18, 2021 00:51
@JaredTate
Copy link

Looks good initially. The automated tests are great. But to double check everything 100% was this tested with a running tweaked 7.17.2 node to sync from scratch on main net?

@ChillingSilence
Copy link

Yes that's what the unit tests confirm. They do a full subsidy simulation.
If we don't trust the unit tests then we need a proposal for further amendments to them so that they are considered 'trustworthy'

However yes, it syncs from scratch.

@SmartArray SmartArray dismissed stale reviews from gto90 and ChillingSilence via 55ad95e April 27, 2021 17:26
@SmartArray
Copy link
Author

Thanks @JaredTate for reviewing #40

Thanks to you, this PR is currently being validated by the approved automatic CI workflow and it should yield the build status shortly.

@gto90 gto90 requested review from gto90 and ChillingSilence April 27, 2021 17:56
Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

ACK

Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

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

ACK

@JaredTate JaredTate merged commit c554b8c into DigiByte-Core:develop Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants