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

Implement difficulty adjustment each block #25

Merged
merged 10 commits into from Oct 17, 2017

Conversation

Projects
None yet
4 participants
@martin-key
Contributor

martin-key commented Oct 13, 2017

Changed ports to 8338 for mainnet and 18338 for testnet
Also decreased the amount of premining to 8000.
The diff adjustment is taken from Zcash.
Changed name of daemon to bgoldd and bgold-cli
Changed data directory

@leto

This comment has been minimized.

Contributor

leto commented Oct 14, 2017

These look like some good changes. I want to mention that the Zcash difficulty algorithm is NOT friendly to small networks. They had serious problems with miners gaming the system, jumping on and off in the very beginning. All those people are just waiting to game your system, be prepared. They will coordinate to jump on and off the network, and "small miners" will have a very hard time in the beginning, until the network grows big enough.

Hush has a lot of data on this issue: MyHush/hush#24

Let us know if we can help.

@martin-key

This comment has been minimized.

Contributor

martin-key commented Oct 14, 2017

@h4x3rotab

Excellent job! The hash retargeting algorithm will be changed to the same as Zcash. And as mentioned above, the current parameters might not be the best. I will also read the provided link and discuss it further.

@@ -14,9 +14,9 @@ AC_CONFIG_HEADERS([src/config/bitcoin-config.h])
AC_CONFIG_AUX_DIR([build-aux])
AC_CONFIG_MACRO_DIR([build-aux/m4])
BITCOIN_DAEMON_NAME=bitcoind
BITCOIN_DAEMON_NAME=bgoldd
BITCOIN_GUI_NAME=bitcoin-qt

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

Do you want to also change bitcoin-qt and bitcoin-tx?

This comment has been minimized.

@leto

leto Oct 15, 2017

Contributor

Just FYI, Zcash has very recently deleted all *-qt code from their tree. To reduce future merge conflicts, you may want to just do the same.

@@ -77,6 +77,9 @@ class CChainParams
const CCheckpointData& Checkpoints() const { return checkpointData; }
const ChainTxData& TxData() const { return chainTxData; }
void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout);

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

Remove too many empty lines.

assert(maxUint/UintToArith256(consensus.powLimit) >= consensus.nPowAveragingWindow);
consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
consensus.nPowTargetTimespan = 10 * 60; // 10 minutes

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

Why not reusing nPowTargetSpacing?

else if (pindexLast->nHeight < params.BTGHeight + params.BTGPremineWindow) {
return nProofOfWorkLimit;
}

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

Too many blank lines

pindexLast.nTime = 1262152739; // Block #32255
pindexLast.nBits = 0x1d00ffff;
BOOST_CHECK_EQUAL(CalculateNextWorkRequired(&pindexLast, nLastRetargetTime, chainParams->GetConsensus()), 0x1d00d86a);
SelectParams(CBaseChainParams::MAIN);

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

I understood these tests are rewrite because of the signature of CalculateNextWorkRequired. I suggest to keep the original function signature of Bitcoin because Zcash was forked from an early version of Bitcoin. So it will be easier to keep in track of the future updates of Bitcion. However it's up to you to decide.

This comment has been minimized.

@leto

leto Oct 15, 2017

Contributor

Zcash has BTC core 0.11.2 frozen into their tree

CBlockIndex *p2 = &blocks[GetRand(10000)];
CBlockIndex *p3 = &blocks[GetRand(10000)];
int64_t tdiff = GetBlockProofEquivalentTime(*p1, *p2, *p3, params);
BOOST_CHECK_EQUAL(tdiff, p1->GetBlockTime() - p2->GetBlockTime());
}
}

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

The current tests can't cover the logic after the fork. At least we should add them later.

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 15, 2017

Contributor

Also there are failing tests. Please make sure the unit tests and reg tests can be passed.

FAIL: test/test_bitcoin
=======================
Running 258 test cases...
test/pow_tests.cpp(27): error: in "pow_tests/get_next_work": check 0x1d011998 == CalculateNextWorkRequired(bnAvg, nThisTime, nLastRetargetTime, params) has failed [486611352 != 486594313]
test/pow_tests.cpp(41): error: in "pow_tests/get_next_work_pow_limit": check 0x1f07ffff == CalculateNextWorkRequired(bnAvg, nThisTime, nLastRetargetTime, params) has failed [520617983 != 486604799]
test/pow_tests.cpp(69): error: in "pow_tests/get_next_work_upper_limit_actual": check 0x1c4a93bb == CalculateNextWorkRequired(bnAvg, nThisTime, nLastRetargetTime, params) has failed [474649531 != 473066834]
@@ -185,10 +196,14 @@ class CTestNetParams : public CChainParams {
consensus.BIP34Hash = uint256S("0x0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8");
consensus.BIP65Height = 581885; // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
consensus.BIP66Height = 330776; // 000000002104c8c45e99a8853285a3b592602a3ccde2b832481da85e9e4ba182
consensus.BTGHeight = 100000000; // Not activated yet.
consensus.BTGHeight = 1220000; // Not activated yet.

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 16, 2017

Contributor

Remove “//Not activated yet” and maybe add the block time as a comment.

consensus.powLimit = uint256S("00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
consensus.nPowTargetTimespan = 14 * 24 * 60 * 60; // two weeks

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 16, 2017

Contributor

I’d rather rename nPowTargetTimespan to nPowTargetTimespanLegacy. And reuse nPowTargetSpacing.

unsigned int nProofOfWorkLimit = UintToArith256(params.powLimit).GetCompact();
int nHeightNext = pindexLast->nHeight + 1;
int diffAdjustmentInterval = 2016; // every 2016 blocks for Bitcoin

This comment has been minimized.

@h4x3rotab

h4x3rotab Oct 16, 2017

Contributor

It’s generally not a good idea to hardcode the interval here. However, feel free to leave a TODO here and let’s move on.

@h4x3rotab

This comment has been minimized.

Contributor

h4x3rotab commented Oct 17, 2017

Let’s wait for the TravisCI results.

@leto leto referenced this pull request Oct 17, 2017

Closed

Change default port number #16

@leto

This comment has been minimized.

Contributor

leto commented Oct 17, 2017

@h4x3rotab @StarbuckBG i need this stuff for my testnet branch, and changes important port numbers, so while it is not perfect, I think we can merge it to master so we can move forward on other things 👍

@h4x3rotab

This comment has been minimized.

Contributor

h4x3rotab commented Oct 17, 2017

@StarbuckBG Please check the TravisCI build result: https://travis-ci.org/BTCGPU/BTCGPU/jobs/288855885

Looks like the regression test are all failing.

@leto

This comment has been minimized.

Contributor

leto commented on 0e6c2d3 Oct 17, 2017

👍

@h4x3rotab h4x3rotab merged commit 5e907e3 into BTCGPU:master Oct 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ingenieriainversa

Hi, have you implemented network magic correctly in chainparams.cpp file?

pchMessageStart[0] = 0x0b;
pchMessageStart[1] = 0x11;
pchMessageStart[2] = 0x09;
pchMessageStart[3] = 0x07;

consensus.powLimit = uint256S("00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
consensus.nPowTargetTimespan = 14 * 24 * 60 * 60; // two weeks
consensus.nPowAveragingWindow = 17;

This comment has been minimized.

@ingenieriainversa

ingenieriainversa Oct 24, 2017

I do not know if it is a good idea but I thought that maybe you could consider changing the value of the block average consensus.nPowAveragingWindow depending on the hashrate of the network, for example using a conditional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment