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

Allow minimum-difficulty blocks on testnet #3559

Merged
merged 7 commits into from Oct 6, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Oct 3, 2018

This is a consensus rule change on testnet that will result in a chain split (leaving the stuck chain, as desired).

Reverts #2766 and part of #1338.
Closes #3552.

str4d added some commits Oct 3, 2018

Allow minimum-difficulty blocks on testnet and regtest
A block may be mined with nBits set to the minimum difficulty if its
nTime is set more than six block intervals (15 minutes) after its parent
block.

This is a consensus rule change on testnet that will result in a chain
split (as desired).

@str4d str4d added this to the v2.0.1 milestone Oct 3, 2018

@str4d str4d requested review from bitcartel and daira Oct 3, 2018

@str4d str4d added this to In Review in Zcashd Team Oct 3, 2018

Show resolved Hide resolved src/miner.cpp
@bitcartel
Copy link
Contributor

bitcartel left a comment

Can't mine because the chain has frozen for too long and the node thinks it is in the initial block download state.

./zcash-cli getblocktemplate
error code: -10
error message:
Zcash is downloading blocks...

because:

bool IsInitialBlockDownload()
...
    if (chainActive.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge)) {
        return true;
    }
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 4, 2018

@bitcartel per https://github.com/zcash/zcash/pull/3263/files#r188497384 you can get around this by starting the node once like so:

zcashd -maxtipage=999999
Only enable min-difficulty blocks on testnet from a particular height
The min-difficulty change is a bilateral consensus rule change, and so
must be conditionally enabled in order for the earlier section of the
chain to synchronise.

Technically this could be implemented as a network upgrade, but as this will
never be deployed to mainnet, a targeted fork will suffice.
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 4, 2018

I discovered when trying to synchronise testnet using this branch, that the output of GetNextWorkRequired() is used directly in the consensus rules. If a block takes longer than the 15-minute interval, it is expected to have nBits set to zero. That makes this a bilateral consensus rule change, not simply a constricting change. I think this is fine; I've pushed a commit that makes the "activation" of this on testnet explicit on height.

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Oct 4, 2018

Note that this is a consensus change so should have at least 3 accepting reviews.

@daira
Copy link
Contributor

daira left a comment

The change to the name of nPowAllowMinDifficultyBlocksFromHeight is blocking. Otherwise ut(ACK+cov).

@@ -24,20 +24,14 @@ unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHead
if (pindexLast == NULL)
return nProofOfWorkLimit;

const CBlockIndex* pindexBits = pindexLast;
{

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

It isn't necessary for this to be a block.

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Yes, also braces might help at line 25 and line 34-35 of this method.

@@ -272,7 +272,7 @@ class CTestNetParams : public CChainParams {
consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
consensus.nPowTargetSpacing = 2.5 * 60;
consensus.fPowAllowMinDifficultyBlocks = true;
consensus.nPowAllowMinDifficultyBlocksFromHeight = 299187;

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

I have not checked this height.

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

299187 is what the testnet explorer is reporting as of now, so should be fine.

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Or we set this 299188 since that is what "From" implies.

@@ -726,7 +727,7 @@ void static BitcoinMiner()
// Update nNonce and nTime
pblock->nNonce = ArithToUint256(UintToArith256(pblock->nNonce) + 1);
UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
if (chainparams.GetConsensus().fPowAllowMinDifficultyBlocks)
if (chainparams.GetConsensus().nPowAllowMinDifficultyBlocksFromHeight)

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

It isn't clear how a boost::optional that holds 0 (which is the case for regtest) is coerced to a boolean, unless you pore over the Boost documentation — which isn't actually very clear on this point. Make it more explicitly a check that the optional is set:

if (chainparams.GetConsensus().nPowAllowMinDifficultyBlocksFromHeight != boost::none)

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Since the member starts with 'nPow' a reader might assume the member value is a number, just likenPowMaxAdjustDown and not realize it's actually a boost optional. So perhaps also rename to optPow or optNPow` etc.?

src/pow.cpp Outdated
@@ -25,7 +25,8 @@ unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHead
return nProofOfWorkLimit;

{
if (params.fPowAllowMinDifficultyBlocks)
if (params.nPowAllowMinDifficultyBlocksFromHeight &&

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

Ditto, != boost::none.

@@ -91,7 +93,7 @@ struct Params {
NetworkUpgrade vUpgrades[MAX_NETWORK_UPGRADES];
/** Proof of work parameters */
uint256 powLimit;
bool fPowAllowMinDifficultyBlocks;
boost::optional<uint32_t> nPowAllowMinDifficultyBlocksFromHeight;

This comment has been minimized.

@daira

daira Oct 4, 2018

Contributor

The name of this field has an off-by-one error relative to its semantics. The name would suggest that the block at this height, and subsequent blocks, are allowed to be min-difficulty. In fact only subsequent blocks are allowed to be min-difficulty. Renaming it to nPowAllowMinDifficultyBlocksAfterHeight would resolve this discrepancy.

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

I agree. For example 299187 is the current testnet block height, and what we are saying is we can mine min difficulty blocks after 299187.

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Or set above to 299188.

Resolved by using -maxtipage option

str4d added some commits Oct 4, 2018

Explicitly check the min-difficulty flag against boost::none
It isn't clear how a boost::optional that holds 0 (which is the case for
regtest) is coerced to a boolean, unless you pore over the Boost
documentation. An explicit check is clearer.
@bitcartel
Copy link
Contributor

bitcartel left a comment

Minor nits per @daira's review and mine.

Otherwise,a min difficulty block mined onto testnet looks good:

./zcash-cli getblock 299188

  "bits": "2007ffff",
  "difficulty": 1,

where 0x20 is the length of our target, and 0x07ffff is the prefix of our pow limit, used as the min difficulty target.

@@ -91,7 +93,7 @@ struct Params {
NetworkUpgrade vUpgrades[MAX_NETWORK_UPGRADES];
/** Proof of work parameters */
uint256 powLimit;
bool fPowAllowMinDifficultyBlocks;
boost::optional<uint32_t> nPowAllowMinDifficultyBlocksFromHeight;

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

I agree. For example 299187 is the current testnet block height, and what we are saying is we can mine min difficulty blocks after 299187.

@@ -272,7 +272,7 @@ class CTestNetParams : public CChainParams {
consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
consensus.nPowTargetSpacing = 2.5 * 60;
consensus.fPowAllowMinDifficultyBlocks = true;
consensus.nPowAllowMinDifficultyBlocksFromHeight = 299187;

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Or we set this 299188 since that is what "From" implies.

@@ -91,7 +93,7 @@ struct Params {
NetworkUpgrade vUpgrades[MAX_NETWORK_UPGRADES];
/** Proof of work parameters */
uint256 powLimit;
bool fPowAllowMinDifficultyBlocks;
boost::optional<uint32_t> nPowAllowMinDifficultyBlocksFromHeight;

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Or set above to 299188.

@@ -726,7 +727,7 @@ void static BitcoinMiner()
// Update nNonce and nTime
pblock->nNonce = ArithToUint256(UintToArith256(pblock->nNonce) + 1);
UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
if (chainparams.GetConsensus().fPowAllowMinDifficultyBlocks)
if (chainparams.GetConsensus().nPowAllowMinDifficultyBlocksFromHeight)

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Since the member starts with 'nPow' a reader might assume the member value is a number, just likenPowMaxAdjustDown and not realize it's actually a boost optional. So perhaps also rename to optPow or optNPow` etc.?

@@ -24,20 +24,14 @@ unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHead
if (pindexLast == NULL)
return nProofOfWorkLimit;

const CBlockIndex* pindexBits = pindexLast;
{

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Yes, also braces might help at line 25 and line 34-35 of this method.

@daira

daira approved these changes Oct 4, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov), modulo the fact that I think the MinDifficultyRules test will fail.

Show resolved Hide resolved src/chainparams.cpp
Show resolved Hide resolved src/gtest/test_pow.cpp

@str4d str4d force-pushed the str4d:3552-testnet-min-difficulty-blocks branch from b22929b to 2b47b0d Oct 4, 2018

@daira

daira approved these changes Oct 4, 2018

Copy link
Contributor

daira left a comment

re-ut(ACK+cov) 2b47b0d

@bitcartel
Copy link
Contributor

bitcartel left a comment

ACK. Perhaps squash before merging.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

Hold on.

I switched to the z_shieldcoinbase PR, built it, launched and created some transactions...

Now I see the txs have been mined onto this min difficulty chain?
04d9ae83c29aaed750358d469385028bc0b8b16118ed095fc8f86d78d4dd7790
etc, mined into block 299543

I expected the txs to stay in the mempool and maybe get mined into block 299188 on the main testnet chain.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 4, 2018

We aren't preventing connections between the two chains, so it's possible your node tried to connect to the min-difficulty central node (either in your config or cached in peers.dat), and was able to broadcast the transaction before it blocked the peer for invalid PoW.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

Sorry, I wasn't clearer. My node is (for some reason) on the min diff chain (and I checked the src files and git log to confirm I'm on the Sapling PR) :

./zcash-cli getblockcount
299564
./zcash-cli getchaintips
[
  {
    "height": 299564,
    "hash": "001b2811ff7838268adb0000b27392cce523778b32d2d052f39c5e907b974c06",
    "branchlen": 0,
    "status": "active"
  },
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 4, 2018

Ohh, right. Because this isn't set up as a network upgrade, the logic to check we are on the same chain doesn't exist, and because the last X blocks satisfy the non-min-difficulty rules, chain validation (disconnecting and then optionally reconnecting blocks) wouldn't detect it either. Your node would reject subsequent min-difficulty blocks though.

tl;dr that is expected behaviour for this particular PR, due to the way node state is being maintained across different PRs.

@Eirik0 Eirik0 self-requested a review Oct 5, 2018

@Eirik0

Eirik0 approved these changes Oct 5, 2018

Copy link
Contributor

Eirik0 left a comment

ACK. With this code, I am able to connect to testnet and have received/mined new blocks once again.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 5, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

📌 Commit 2b47b0d has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2018

⌛️ Testing commit 2b47b0d with merge aede10d...

zkbot added a commit that referenced this pull request Oct 5, 2018

Auto merge of #3559 - str4d:3552-testnet-min-difficulty-blocks, r=bit…
…cartel

Allow minimum-difficulty blocks on testnet

This is a consensus rule change on testnet that will result in a chain split (leaving the stuck chain, as desired).

Reverts #2766 and part of #1338.
Closes #3552.

@str4d str4d moved this from In Review to Merge Queue in Zcashd Team Oct 5, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 6, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing aede10d to master...

@zkbot zkbot merged commit 2b47b0d into zcash:master Oct 6, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from Merge Queue to Released (Merged in Master) Oct 6, 2018

@str4d str4d deleted the str4d:3552-testnet-min-difficulty-blocks branch Oct 8, 2018

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