From 8537ecdfc40181249ec37556015a99cfae4b21fd Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Sun, 1 Nov 2015 20:05:16 +0000 Subject: [PATCH 1/2] Revert "Enable policy enforcing GetMedianTimePast as the end point of lock-time constraints" This reverts commit dea8d21fc63e9f442299c97010e4740558f4f037. --- src/policy/policy.h | 2 +- src/test/miner_tests.cpp | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/policy/policy.h b/src/policy/policy.h index f269e8d47..fdc54a70a 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -44,7 +44,7 @@ static const unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VERIFY static const unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS; /** Used as the flags parameter to CheckFinalTx() in non-consensus code */ -static const unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS = LOCKTIME_MEDIAN_TIME_PAST; +static const unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS = 0; bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType); /** diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 827525783..91a3a5738 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -4,7 +4,6 @@ #include "chainparams.h" #include "coins.h" -#include "consensus/consensus.h" #include "consensus/validation.h" #include "main.h" #include "miner.h" @@ -230,7 +229,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.nLockTime = chainActive.Tip()->nHeight+1; hash = tx.GetHash(); mempool.addUnchecked(hash, CTxMemPoolEntry(tx, 11, GetTime(), 111.0, 11)); - BOOST_CHECK(!CheckFinalTx(tx, LOCKTIME_MEDIAN_TIME_PAST)); + BOOST_CHECK(!CheckFinalTx(tx)); // time locked tx2.vin.resize(1); @@ -244,7 +243,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1; hash = tx2.GetHash(); mempool.addUnchecked(hash, CTxMemPoolEntry(tx2, 11, GetTime(), 111.0, 11)); - BOOST_CHECK(!CheckFinalTx(tx2, LOCKTIME_MEDIAN_TIME_PAST)); + BOOST_CHECK(!CheckFinalTx(tx2)); BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); @@ -262,7 +261,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) //BOOST_CHECK(CheckFinalTx(tx2)); BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 2); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3); delete pblocktemplate; chainActive.Tip()->nHeight--; From 40cd32e835092c3158175511da5193193ec54939 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Sun, 1 Nov 2015 20:05:18 +0000 Subject: [PATCH 2/2] Revert "Add rules--presently disabled--for using GetMedianTimePast as endpoint for lock-time calculations" This reverts commit 9d55050773d57c0e12005e524f2e54d9e622c6e2. As noted by Luke-Jr, under some conditions this will accept transactions which are invalid by the network rules. This happens when the current block time is head of the median time past and a transaction's locktime is in the middle. This could be addressed by changing the rule to MAX(this_block_time, MTP+offset) but this solution and the particular offset used deserve some consideration. --- src/consensus/consensus.h | 6 ------ src/main.cpp | 40 +++++---------------------------------- src/main.h | 4 +--- src/miner.cpp | 8 +------- src/policy/policy.h | 3 --- 5 files changed, 7 insertions(+), 54 deletions(-) diff --git a/src/consensus/consensus.h b/src/consensus/consensus.h index 6d6ce7e09..f937844e9 100644 --- a/src/consensus/consensus.h +++ b/src/consensus/consensus.h @@ -13,10 +13,4 @@ static const unsigned int MAX_BLOCK_SIGOPS = MAX_BLOCK_SIZE/50; /** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */ static const int COINBASE_MATURITY = 100; -/** Flags for LockTime() */ -enum { - /* Use GetMedianTimePast() instead of nTime for end point timestamp. */ - LOCKTIME_MEDIAN_TIME_PAST = (1 << 1), -}; - #endif // BITCOIN_CONSENSUS_CONSENSUS_H diff --git a/src/main.cpp b/src/main.cpp index e038fe366..26a22ae6f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -650,35 +650,10 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) return true; } -bool CheckFinalTx(const CTransaction &tx, int flags) +bool CheckFinalTx(const CTransaction &tx) { AssertLockHeld(cs_main); - - // By convention a negative value for flags indicates that the - // current network-enforced consensus rules should be used. In - // a future soft-fork scenario that would mean checking which - // rules would be enforced for the next block and setting the - // appropriate flags. At the present time no soft-forks are - // scheduled, so no flags are set. - flags = std::max(flags, 0); - - // CheckFinalTx() uses chainActive.Height()+1 to evaluate - // nLockTime because when IsFinalTx() is called within - // CBlock::AcceptBlock(), the height of the block *being* - // evaluated is what is used. Thus if we want to know if a - // transaction can be part of the *next* block, we need to call - // IsFinalTx() with one more than chainActive.Height(). - const int nBlockHeight = chainActive.Height() + 1; - - // Timestamps on the other hand don't get any special treatment, - // because we can't know what timestamp the next block will have, - // and there aren't timestamp applications where it matters. - // However this changes once median past time-locks are enforced: - const int64_t nBlockTime = (flags & LOCKTIME_MEDIAN_TIME_PAST) - ? chainActive.Tip()->GetMedianTimePast() - : GetAdjustedTime(); - - return IsFinalTx(tx, nBlockHeight, nBlockTime); + return IsFinalTx(tx, chainActive.Height() + 1, GetAdjustedTime()); } unsigned int GetLegacySigOpCount(const CTransaction& tx) @@ -822,7 +797,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa // Only accept nLockTime-using transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't // be mined yet. - if (!CheckFinalTx(tx, STANDARD_LOCKTIME_VERIFY_FLAGS)) + if (!CheckFinalTx(tx)) return state.DoS(0, false, REJECT_NONSTANDARD, "non-final"); // is it already in the memory pool? @@ -2748,15 +2723,10 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn const Consensus::Params& consensusParams = Params().GetConsensus(); // Check that all transactions are finalized - BOOST_FOREACH(const CTransaction& tx, block.vtx) { - int nLockTimeFlags = 0; - int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) - ? pindexPrev->GetMedianTimePast() - : block.GetBlockTime(); - if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) { + BOOST_FOREACH(const CTransaction& tx, block.vtx) + if (!IsFinalTx(tx, nHeight, block.GetBlockTime())) { return state.DoS(10, error("%s: contains a non-final transaction", __func__), REJECT_INVALID, "bad-txns-nonfinal"); } - } // Enforce block.nVersion=2 rule that the coinbase starts with serialized block height // if 750 of the last 1,000 blocks are version 2 or greater (51/100 if testnet): diff --git a/src/main.h b/src/main.h index 65732d770..202d2c772 100644 --- a/src/main.h +++ b/src/main.h @@ -308,10 +308,8 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime); * Check if transaction will be final in the next block to be created. * * Calls IsFinalTx() with current block height and appropriate block time. - * - * See consensus/consensus.h for flag definitions. */ -bool CheckFinalTx(const CTransaction &tx, int flags = -1); +bool CheckFinalTx(const CTransaction &tx); /** * Closure representing one script verification diff --git a/src/miner.cpp b/src/miner.cpp index 053d9cdbc..42c8bb970 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -148,7 +148,6 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) CBlockIndex* pindexPrev = chainActive.Tip(); const int nHeight = pindexPrev->nHeight + 1; pblock->nTime = GetAdjustedTime(); - const int64_t nMedianTimePast = pindexPrev->GetMedianTimePast(); CCoinsViewCache view(pcoinsTip); // Priority order to process transactions @@ -163,12 +162,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) mi != mempool.mapTx.end(); ++mi) { const CTransaction& tx = mi->GetTx(); - - int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST) - ? nMedianTimePast - : pblock->GetBlockTime(); - - if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, nLockTimeCutoff)) + if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, pblock->nTime)) continue; COrphan* porphan = NULL; diff --git a/src/policy/policy.h b/src/policy/policy.h index fdc54a70a..747c5ce8c 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -43,9 +43,6 @@ static const unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VERIFY /** For convenience, standard but not mandatory verify flags. */ static const unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS; -/** Used as the flags parameter to CheckFinalTx() in non-consensus code */ -static const unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS = 0; - bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType); /** * Check for standard transaction types