Skip to content

Replace BIP34: Require height commitment in the generation transactions's locktime instead #3

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.

Sign up for GitHub

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

Open
luke-jr wants to merge 2 commits into
base: 0.14-uasf_backup_hardfork
Choose a base branch
from
Open

Replace BIP34: Require height commitment in the generation transactions's locktime instead #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2413416
chainparams: Define (unset) hardfork time
luke-jr Jun 27, 2017
681b0c1
Replace BIP34: Require height commitment in the generation transactio…
luke-jr Jun 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/chainparams.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ class CMainParams : public CChainParams {
// The best chain should have at least this much work.
consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000003f94d1ad391682fe038bf5");

consensus.HardforkTime = std::numeric_limits<int64_t>::max();

// By default assume that the signatures in ancestors of this block are valid.
consensus.defaultAssumeValid = uint256S("0x00000000000000000013176bf8d7dfeab4e1db31dc93bc311b436e82ab226b90"); //453354

Expand Down Expand Up @@ -203,6 +205,8 @@ class CTestNetParams : public CChainParams {
// The best chain should have at least this much work.
consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000001f057509eba81aed91");

consensus.HardforkTime = std::numeric_limits<int64_t>::max();

// By default assume that the signatures in ancestors of this block are valid.
consensus.defaultAssumeValid = uint256S("0x00000000000128796ee387cf110ccb9d2f36cffaf7f73079c995377c65ac0dcc"); //1079274

Expand Down Expand Up @@ -288,6 +292,8 @@ class CRegTestParams : public CChainParams {
// The best chain should have at least this much work.
consensus.nMinimumChainWork = uint256S("0x00");

consensus.HardforkTime = std::numeric_limits<int64_t>::max();

// By default assume that the signatures in ancestors of this block are valid.
consensus.defaultAssumeValid = uint256S("0x00");

Expand Down
4 changes: 4 additions & 0 deletions src/consensus/params.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ struct Params {
int64_t nPowTargetTimespan;
int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
uint256 nMinimumChainWork;

/** Hardfork parameters */
int64_t HardforkTime;

uint256 defaultAssumeValid;
};
} // namespace Consensus
Expand Down
9 changes: 6 additions & 3 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3021,9 +3021,12 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, const Co
}
}

// Enforce rule that the coinbase starts with serialized block height
if (nHeight >= consensusParams.BIP34Height)
{
if (block.nTime >= consensusParams.HardforkTime) {
Copy link

@jtimon jtimon Aug 29, 2017

Choose a reason for hiding this comment

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

Why not use HardforkHeight instead ?

Copy link
Author

@luke-jr luke-jr Aug 29, 2017

Choose a reason for hiding this comment

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

This is just part of a larger hardfork. The height isn't known at header-validation time.

Copy link

@jtimon jtimon Aug 29, 2017

Choose a reason for hiding this comment

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

how is the height not known at header-validation time? Don't you know the height of your predecessor block?

Copy link
Author

@luke-jr luke-jr Aug 30, 2017

Choose a reason for hiding this comment

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

I'm referring to CheckBlockHeader, not ContextualCheckBlockHeader. This would mean we cannot verify the PoW until we have the block context, which could possibly (?) make a DoS vector. (But maybe some restructuring of the code to ignore all headers without context makes sense?)

Copy link
Author

@luke-jr luke-jr Aug 30, 2017

Choose a reason for hiding this comment

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

In any case, there's little harm to using time here. This is intended to only be deployed after the final SHA2 block is found, which means there is a very well-defined time to specify (in fact, PR #2 requires the time specified is exactly the second after the final SHA2 block). No real benefit to using height.

if (block.vtx[0]->nLockTime != (uint32_t)nHeight - 1) {
return state.DoS(100, false, REJECT_INVALID, "bad-cb-height", false, "block height mismatch in coinbase");
}
} else if (nHeight >= consensusParams.BIP34Height) {
// Enforce rule that the coinbase starts with serialized block height
CScript expect = CScript() << nHeight;
if (block.vtx[0]->vin[0].scriptSig.size() < expect.size() ||
!std::equal(expect.begin(), expect.end(), block.vtx[0]->vin[0].scriptSig.begin())) {
Expand Down